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

[Neal H Walfield]

> 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...

You are right.  However, as I do not have a lot of access to the net
(once a week), this makes this very hard.

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

Yes, I have already assigned my work on the Hurd to the FSF;  I assume
that that should cover it.  Let me know if I need to do more.

> >         * 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.

Sure.

> >         * 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?

Of course, better to be consistent.  I was using the Hurd naming
conventions and did not look at what you had already done.

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

I modeled my changes against the libc model.

> > * 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?

Having taken a bunch of English literature and writing courses, I can
assure you that in formal writing, contractions are a Bad Thing.  Having
said that Stevens uses them.  Either way, there are a bunch of places
that they were misused (i.e. ``it's'' is ``it is'' not possessive).

> > 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.

I do not know much about automake, if you have a better idea, let me
know.

> > * 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.

I have been informed that the correct thing is to just define
_LARGEFILE64_SOURCE.

> > * 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?

Not sectors, records.  On the Hurd, there is no guarantee about the
record size of a store.

> > * 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.

Again, I do not know what the Right Thing is;  we need an automake guru.

> > * 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?

It will not generate shared libraries due to external dependencies
on other shared libraries.

> > * 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...

It overlaps other tables;  the Open BSD partition table overlays the
entire disk.

> 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.

Maybe you can provide suffixes, i.e. M for megabytes, G for giga.
Then if the use uses suffixes, it will round, otherwise, it will not.

> > * 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!

Ok, what is the difference between mkpartfs?

> > * 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?

In bash:
        $ for i in *; do
        > echo $i
        > done
        ...
        $ <UP>for i in *; do echo $i; done

In mkparted:
        (parted) mkpart
        Partition type?  primary/logical? log
        File system type? ext2? 
        Start? 4.001
        End? 5
        (parted) <UP>5

I expect parted to print:

        (parted) <UP>mkpart logical 4.001 5

> 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.

2.2.17 under Linux.  As with the Hurd, the CVS code from two weeks ago.

> > * 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

This is a Hurd bug.

> 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.

What ever you think is best.  I prefer the glibc style.

Cheers.

pgp6PNhpTBYz5.pgp
Description: PGP signature