Re: [PATCH] diskpart

[Roland McGrath]

I have a variety of things to say about the work you've done here.  The
first thing is that I'm very glad to see that you have taken on a project
like this and gotten nominally working.  This is a fine way for you and
others who read your code to get a good understanding of how to write Hurd
components.

Next, some small issues with the code.  These are mostly general points,
not specific to this particular filesystem you've written. 

* The Hurd is built with -D_GNU_SOURCE=1, so you never need to 
  define _GNU_SOURCE in source files and we always leave it out.

* You can't use zero as an st_ino value.  st_ino values are also d_fileno
  (aka d_ino) values, and a dirent with d_fileno==0 means a deleted entry
  that is skipped by the reader.  There are probably other obscure ways in
  which the zero st_ino value will bite something.  It is traditional to
  use 2 for the root of the filesystem, though the only thing that really
  matters is not to ever use zero for any node.

* Never do this:
              disk_node = realloc (disk_node,
                                   sizeof (struct netnode *) * allocated);
              if (! disk_node)
                return ENOMEM;
  When realloc fails, it leaves the old allocation intact, and now you have
  leaked it (as well as screwing up the `allocated' count).  You always
  have to do something like this:
              void *new = realloc (disk_node,
                                   sizeof (struct netnode *) *
                                   (allocated * 2));
              if (! new)
                return ENOMEM;
              allocated *= 2;
              disk_node = new;
  Note that I also changed it to double the size on reallocations; this is
  a good practice for this kind of thing--it reduces the number of
  reallocations in a good way.

* Using fshelp_touch at startup not appropriate.  You should propagate the
  times from the underlying node.

* I'm not sure this is robust:
        entry = strtol (name, &end, 10);
        if (*end != '\0')
  I think END might be null, or never set (I don't recall exactly).
  I'd do this:
        char *end = 0;
        entry = strtol (name, &end, 10);
        if (end == 0 || *end != '\0')
        
* The get_dirents function needs to return . and .. entries.
  POSIX doesn't strictly require it, but it is expected Hurd behavior.
  You always need to round d_reclen to a multiple of 8.

* store_return does not consume the store, so your
  netfs_file_get_storage_info function leaks the store.
  Call store_free after store_return.

* Since get_dirents returns information that never changes, you can just
  set up a whole buffer of dirents at startup and always return it.


Now, some comments about the small behaviors of the filesystem hooks.

* netfs_check_open_permissions ought to do an fshelp_access check.
  In fact, you ought to make it refuse if O_READ or O_WRITE is set;
  then it will be impossible to reach the netfs_attempt_{read,write}
  hooks and you can make those call abort or use assert.

* For most of the stubs, I would return EROFS rather than EOPNOTSUPP.
  netfs_attempt_readlink should return EINVAL according to readlink(2).
  
* Don't hard-code f_bsize as 512.  Use the underlying device record size.
  Set f_blocks from the device size.  You might want to just have a global
  struct statfs initialized at startup and just do "*st = partition_statfs".

* Most of the node operations (chfoo) you should pass through to the
  underlying node when they are on the root node (like trivfs does).
  Start by using io_stat on the underlying node to set flags, mode&~S_IFMT,
  nlink, uid, gid, author.
  That said, I'd have netfs_validate_stat reflect the node stats (mode,
  uid, etc) of the root node on each leaf node.

* Don't use S_IFREG for the leaf nodes.  Use S_IFCHR or S_IFBLK.

* Always set st_blksize; if you have nothing good to use, use vm_page_size.

* Set st_blocks, st_size in the leaf nodes.  
  You have the information right there.


Now, the big structural issues:

* Dude, just use libstore!  There is no reason any ordinary Hurd program
  ought to have to do direct device access.  Just use libstore to open the
  device.  Let it parse the arguments for you, see storeio.c for how.  As
  well as making your code simpler, this lets you operate on regular files
  or whatever else you want instead of just Mach devices.  That should be
  dandy for testing.

* I'm not sure I want to have libdiskpart as a first-class library in the
  Hurd package. Maybe that code should just live in the translator.  Or, if
  the code is really shared with grub, it should be made its own package
  and the source not live in the hurd at all.

* I'd like to see partition handling that is a bit more generic.  e.g.,
  look for partition tables within partitions and make hierachical
  pseudo-directories, etc.

* You should probably have a netfs_runtime_argp that understands --update
  to re-read the partition table.

* The whole notion of producing pseudo-nodes that don't do i/o is kind of
  bogus.  It's true that you can make reasonable things by making nodes
  that use storeio with a partition pseudo-node as the store.  But that is
  a bit convoluted.  The pseudo-nodes ought to behave like storeio nodes.
  You could do this by incorporating code like storeio has directly, or you
  could do this by automagically starting storeio translators when someone
  opens a pseudo-node for i/o (i.e. O_READ or O_WRITE is set).

  The easy way to go about this is just define netfs_get_translator to
  return "/hurd/storeio" and set S_IPTRANS in nn_stat for leaf nodes.
  Then netfs will take care of starting the active translators for you on
  any lookup.  You don't want to do that if you want to avoid using storeio
  for non-i/o opens.  In that case you'd really have to diddle with netfs.

  For this to work we need to modify storeio so that it will operate on its
  underlying node.  store.h:store_argp_params needs an option to say that
  having no args is ok, and then storeio can see when it has no stores on
  its command line and instead use store_create on the underlying node port.

* A feature: why not have an entry in the virtual directory that refers to
  the entire underlying store?  You could also make i/o operations work on
  the root node, so that it is both a device file and a directory; that
  seems kind of natural if it's /dev/sd0 and you get /dev/sd0/1, etc.
  OTOH, it would be way easier to make "partition 0" the whole device and
  leave it at that.

And finally, the most deep and important question for any program: the name.
I think we should call it partitionfs.