bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/3] Add the mountee to the list of merged filesystems.


From: olafBuddenhagen
Subject: Re: [PATCH 3/3] Add the mountee to the list of merged filesystems.
Date: Fri, 30 Oct 2009 10:13:09 +0100
User-agent: Mutt/1.5.19 (2009-01-05)

Hi,

On Mon, Aug 17, 2009 at 08:55:37PM +0300, Sergiu Ivanov wrote:
> On Sun, Aug 16, 2009 at 07:56:03PM +0200, olafBuddenhagen@gmx.net wrote:
> > On Mon, Aug 03, 2009 at 08:42:27PM +0300, Sergiu Ivanov wrote:

> > > +  /* A path equal to "" will mean that the current ULFS entry is the
> > > +     mountee port.  */
> > > +  ulfs_register ("", 0, 0);
> > 
> > This comment would actually be more appropriate near the definition of
> > the actual data structure and/or the function filling it in...
> > 
> > Of course, it doesn't hurt to mention it here *in addition* to that :-)
> 
> I've added the corresponding comment to ulfs_register, but I didn't
> add anything to variable or structure declarations, because I'm not
> sure whether it would be suitable to describe the convention in the
> comment to the declaration of struct ulfs or in the comment to the
> declaration of ulfs_chain.

The latter I'd say -- it's not really a property of the ulfs structure
itself, but rather a special entry in the list...

> Also, in ulfs.h, both are near the declaration of ulfs_register, so it
> seems to me that it's sufficient to describe the convention in the
> comment to ulfs_register only.

Perhaps. Though generally, properly documenting data structures is more
important than documenting functions... So I'd rather do it the other
way round :-)

> > I actually wonder whether the patches are really split up in the
> > most useful manner... But I'd rather leave it as is now.
> 
> I'm asking out of pure interest: what different way of splitting the
> functionality across patches do you envision?

Quite frankly, I don't know :-)

> diff --git a/mount.c b/mount.c
[...]
> @@ -535,8 +539,13 @@ node_init_root (node_t *node)
>         break;
>  
>        if (ulfs->path)
> -     node_ulfs->port = file_name_lookup (ulfs->path,
> -                                         O_READ | O_DIRECTORY, 0);
> +     {
> +     if (!ulfs->path[0])

You forgot to indent the contents of the block I think?...

> diff --git a/ulfs.c b/ulfs.c
[...]
> @@ -212,14 +216,16 @@ ulfs_for_each_under_priv (char *path_under,
>    return err;
>  }
>  
> -/* Register a new underlying filesystem.  */
> +/* Register a new underlying filesystem.  A null path refers to the
> +   underlying filesystem; a path equal to an empty string refers to
> +   the filesystem of the mountee.  */

This comment is very confusing, as "underlying filesystem" is used in
two different meanings side by side... Please try to reword it.

It's very unfortunate that the original unionfs often refers to the
constituents of the union as "underlying filesystems" -- perhaps it
would be useful to change this globally...

Aside from these formalities, this patch looks fine to me :-)

-antrik-




reply via email to

[Prev in Thread] Current Thread [Next in Thread]