Re: [PATCH] Operating system independence; Hurd Port.

[Andrew Clausen]

Hi Neal,

Neal H Walfield wrote:
> 
> This patch makes Parted much more portable by breaking code that is
> dependent on the operating system into operating system specific
> directories of which a single set is chosen at compile time.  As a
> proof of concept, Parted was ported to the Hurd.

Wow!

This is a very big patch/change.  I've been "playing" with 1.5.x
also (after 1.5.1-pre1), mainly with file system stuff.  So, I
think we should fork for the time being.  When the file system
stuff is ready, I'll merge that in to the main branch.
Fortunately, there should be no conflicts, since my work is on
a different layer :-)

In future, I think it would be better to let me / the maintainer
know when you intend to write a non-trivial patch.  I would
have preferred that this be done in lots of small patches, but
don't worry now...

BTW, everything ok with assigning the copyright to the FSF?

In this email, I've skimmed over the issues a bit for now,
so I can get an answer to you quickly :-)  More mail coming!

>         * libparted/include/parted/device.h (_PedDevice):  fd field removed.
>         void *po added -- operating system dependent peropen hook.

Not sure about the name... os_specific would be better, IMHO.

>         * libparted/device/device-add.c: New file.  Function formerly from
>         libparted/device.c.

In the rest of libparted, it has been device_add.c.  I think
we should be consistent... do you think we should change everything
to device-add.c?

Also, this seems to be a very low-level to split source files
up at.  I'll think about this some more, later...

>         * parted/parted.c (do_print): Print out free space if it is more
>         then a few blocks.

Interesting.

> * doc/USER: Improve language.  Remove contractions.

Why is removing contractions a good thing?  I prefer to be
fairly informal, because it is easier to read (for native
English speakers).  It's hard to know for non-native speakers...
I have learned a bit of Portuguese, and I would certainly
find informal PT easier to understand, because it's what I
use when talking to friends.  But, this might be unusual.
Difficult issue.  What's your motivation?

> Compiling
> ---------
> 
> To compile (on GNU/Linux or GNU/Hurd), you need to do several things.
> Patch parted-1.5.1-pre1.  Run:
> 
>   $ autconf && automake && automake libparted/device/gnu && automake
>   libparted/device/linux

Why do we need the extra automake stuff?  I think the method
you used to select the directory to use is broken... but
perhaps automake is broken.  For example: we want to be able
to use "make dist", even if we configured to use Hurd.

> * On the Hurd, off_t is 32 bits, however, 64 bit offsets are supported
>   through the store_offset_t type which is aliased to off64_t.  At the
>   moment, I have been going the following hack in config.h:
> 
>   #include <sys/types.h>
>   #define off_t off64_t
> 
>   This needs to be fixed.

Yep.

> * The Hurd implementation of device_{read,write} happily support files of
>   record sizes other than 512 bytes.  So far, 512 and 1 byte records
>   have been tested; greater than 512 bytes needs to be tested (use
>   tmpfs).

Why do we need other than 512 byte sectors?

> * When rebuilding the automake files, it is necessary to run automake
>   manually on the libparted/device/{gnu,linux} directories:
> 
>   # automake && automake libparted/device/gnu && automake \
>   > libparted/device/linux
> 
>   as the directories are mentioned via a variable in configure.in.

Yes, this is bad.
 
> * Because there are library dependencies of libparted on libstore and
>   libshouldbeinlibc, libtool refuses to build shared libraries.  We really
>   need them on the Hurd, how do we work around this?

I don't understand the problem.  libparted needs to link against
libstore and libshouldbeinlibc, right?  Why is this a problem?
What does libtool say?
 
> * autoconf claims that we cannot cross compile;  What do we need to do?

We probably need to provide defaults, etc.
 
> * <asm/page.h> does not need to be included on the Hurd.  How can this
>   be autoconf'ed?

This is a weird one... we need to be able to call getpagesize().
What do we need for Hurd?

> * I have broken the translations -- all messages that point to
>   libparted/device.h, libparted/llseek.{c,h} are uncommented and
>   I have fixed the language used in several spots.

OK

> Bugs and Feature Requests
> -------------------------
> 
> * If there are three primary partitions and not an extended partition
>   and you attempt to make an extended partition, you are not prompted
>   for a type, i.e.:
> 
>   (parted) mkpart
>   Partition type? ex
>   File system type? ext2?
>   Start? 3.001
>   End? 9.999
> 
>   You found a bug in ...
> 
>   Assertion (dos_data != NULL) at
>   ../../../parted-1.5.1-pre1/libparted/fs_ext2/interface.c:291 in function
>   _ext2_set_system () failed.
> 
>   Typing in ``extended'' when prompted for the partition type works correctly.

Thanks, I'll look into this...
 
> * Cfdisk only extends extended partitions as far as it needs (i.e. the
>   end of the last partition).  When Parted encounters this, it will
>   not resize the extended partitions as it claims it overlaps other
>   partitions.  Of course it over laps other partitions!  Also, when trying
>   to create a partition in the ``free space'' Parted says that the
>   space overlaps another partition (not very informative).

Weird... I'll also take a look!

> * On that note, I am happy that Parted is very conservative when writing
>   partition table, however, it is far to strict when reading them.  For
>   instance, I had to use Cfdisk to ``fix'' my partition table (Parted failed
>   to read my Open BSD partition table correctly).  Since we want to use
>   libparted to help boot strap the Hurd, this needs to be relaxed a lot.

What did libparted complain about?  The BSD code doesn't complain
about much...
 
> * When prompting for arguments, Parted should suggest a default (e.g. in
>   brackets).

Yep.  This is all a new feature, and hasn't been polished yet.
I'm accepting patches, of course, but I'll get around to it
eventually.  The hooks are already there...
 
> * It would be very nice if, like Cfdisk, Parted could offer nice defaults
>   for numbers, e.g. when creating a partition, find the first free space
>   and use that as a default or print the start of all of the starts of
>   free space.

Yep.  Already on TODO (well, mentally anyway...)

> * When entering the start and end of a partition, Parted should be liberal
>   within a few blocks, that is to say, if a partition ends at 3.000 and
>   I want to make a second that starts at 3, I should not have to type in
>   3.001.  The same should hold if the partition ends at 2.901 and I enter
>   3 as the start of a new partition -- there is should be no tiny free
>   space.  Or, if that is unsatisfactory, prompt if the user would like
>   it to happen:
> 
>     You entered 3.000 as the start of a new partition, If I do this, there
>     will be 0.089 MB of free space between this and the nearest partition,
>     would you like to start the partition at 2.902? [yes] no?

Hmmm.  Automatically rounding is bad because people might want
to reconstruct partition tables (eg: if they delete a partition,
but don't damage the file system).  Displaying this message is
going to be annoying.  It is likely to popup every time!

I think the default start/end thing will handle most cases.
 
> * When using the mkpart command, it is not obvious that the filesystem type
>   is optional:  I have hit enter several times thinking that ext2 was the
>   default.  Maybe provide none as an option?

The fs type isn't optional... and ext2 is the default!

> * The file system type argument to mkpart is not optional (try
>   ``mkpart logical 2 5'')!

Yes, this correct.  The file system type is needed for partition
IDs.  Yes, this is broken (for things like LVM).  Will think
more later!
 
> * When Parted reads an argument that it prompted for, it adds an entry to
>   the history buffer rather then appending to the command line (like bash
>   does).

I don't understand what you're saying.  Perhaps you should give
an example?

> * When typing ``foo bar'' at a Parted prompt (i.e. an error), Parted prints
>   the help screen for each argument (in this example twice).

OK, so do_help should command_line_flush()

> * linux_swap_check is throughly broken -- Linux says bad argument when
>   unmounting and the Hurd says that it is paging to a raw partition
>   (normally says that it found a linux swap signature).  Basically,
>   why is it writing anything?

It checks for bad sectors, and writes out the new list.  Also,
it always creates the new version of the linux swap format,
because the old version causes lots of problems (particularly
for Sun disk labels).

What version of Linux are you using?  Perhaps it doesn't recognise
the new version.  Likewise with Hurd.
 
> * linux_swap_check takes a _long_ time, how about a progress meter?

Yeah, progress meters would be nice... already on TODO.
 
> * ped_device_write takes a const buffer.  Logically, this is fine, however,
>   this generates warnings when passing the buffer to functions such as those
>   (store_write) used to write the buffer to disk.

Well, hack around it I guess.  i.e. typecast
 
> * All of the functions in libpart/device/priv.h should be named
>   privately, which is to say that they should start with an underscore
>   and not have public prefixes.

FWICT, most of priv.h shouldn't exist.  I don't like having
lots of 20 line source files.  For helper functions like
_do_close(), we should probably come up with a better
abstraction.

Thanks!
Andrew Clausen