Re: [PATCH 2/3] Add the code for starting up the mountee

[Sergiu Ivanov]

Hello,

On Sat, Jul 18, 2009 at 06:33:11AM +0200, olafBuddenhagen@gmx.net wrote:
> On Thu, Jul 16, 2009 at 01:04:06PM +0300, Sergiu Ivanov wrote:
> > +/* The node the mountee is sitting on.  */
> > +node_t * unionmount_proxy;
> > +
> > +mach_port_t mountee_port;
> 
> This is the control port of the mountee I assume? Perhaps it would be
> useful to mention it, either in a comment, or even in the variable name
> itself. (i.e. "mountee_control" or "mountee_ctrl")

Nope, it's the port to the root of the mountee.  I realize now that
this name is indeed not suggestive, so I changed it to mountee_root,
as you pointed out in one of your reviews.  Also, I added a comment to
avoid ambiguity.
 
> > +/* Starts the mountee (given by `argz` and `argz_len`), sets it on
> > +   node `np` and opens a port `port` to with `flags`.  `port` is not
> > +   modified when an error occurs.  */
> > +error_t
> > +start_mountee (node_t * np, char * argz, size_t argz_len, int flags,
> > +          mach_port_t * port)
> 
>  [...]
> And I still don't like "np".

I looked through unionfs again and I can confirm that it uses ``np''
for ``node pointer'' everywhere.  Should I break the convention?  I do
agree that this isn't a very intuitive name, but I'm not sure what to
choose: a better name or consistency.
 
> > +{
> > +  error_t err;
> > +  mach_port_t underlying_port;
> > +
> > +  /* The intermediate container for the port to the root of the
> > +     mountee.  */
> > +  mach_port_t res_port;
> 
> The name is still not very descriptive. Something like "mountee_root"
> would be. You don't need the comment at all in this case, as the name
> says it all...
> 
> As I said in the other mail, I'm still not convinced that it's actually
> useful to use a temporary variable here at all... But again, it's not
> really important -- I wouldn't complain if the patch was perfect
> otherwise :-)

I thought of it a little more and decided to drop the temporary
variable.  I already have the global variable mountee_root and I would
like to avoid choosing a different suitable name for a
not-really-useful local variable.
 
> > +
> > +  /* An unauthenticated port to the root of the translator, which
> > +     plays the role of the directory containing the underlying node of
> > +     the mountee. This one is used in fsys_getroot as the dotdot
> > +     parameter, so it is not really important what we put here because
> > +     the dotdot parameter is used mostly with symlinks.  */
> > +  mach_port_t unauth_dir;
> 
> Actually, dotdot can be used by all kinds of relative lookups AIUI; it
> has nothing to do with symlinks...

My conclusion about dotdot and symlinks derives from what I can see in
netfs_S_fsys_get_root (hurd/libnetfs/fsys-getroot.c:110): dotdot
appears to be used in a meaningful way when the translator is a
symlink.  In all other cases dotdot is either deallocated or fed to
fshelp_fetch_root, which ultimately calls fsys_getroot on another
translator.
 
> I don't see though why unionfs would ever do relative lookups on any of
> the merged filesystems -- so your conclusion that this doesn't actually
> play a role is probably correct; only your explanation is wrong :-)

Changing the explanation is much easier than changing the code ;-)
 
> However, I don't think it's helpful to misuse the unionfs root node for
> that. I tend to think that it would be best just to create a dummy port.
> (No RPCs should ever be invoked on it anyways, right?)
> 
> Or maybe even just pass MACH_PORT_NULL, if that's possible?...

Frankly, I was thought of passing MACH_PORT_NULL, but some obscure
thoughts of impropriety hindered me.  I tried passing MACH_PORT_NULL
as dotdot and it works pretty well.  Note that the documentation for
mach_port_deallocate says it's okay to deallocate dead names (and I'd
say MACH_PORT_NULL is not much worse than a dead name).

> > +/* Sets up a proxy node, sets the translator on it, and registers the
> > +   filesystem published by the translator in the list of merged
> > +   filesystems.  */
> > +error_t
> > +setup_unionmount (void)
> > +{
> > +  error_t err = 0;
> > +
> > +  /* The proxy node on which the mountee will be sitting must be able
> > +     to forward some of the RPCs coming from the mountee to the
> > +     underlying filesystem.  That is why we create this proxy node as
> > +     a clone of the root node: the mountee will feel as if there is no
> > +     unionfs under itself.  */
> > +  unionmount_proxy = netfs_make_node (netfs_root_node->nn);
> 
> It's confusing to call it "proxy", when we aren't doing any explicit
> proxying...
> 
> (In fact I don't think that any RPCs are actually forwarded this way at
> all?)

Of course, there is no *explicit* forwarding, but, as I remark in the
new comment to this line, most of the RPCs work out of the box,
because the netnode contained in the node to which I attach the
mountee is the same as in netfs_root_node.  For example, the
translator I test unionmount on io_stats its underlying node.  Since
the underlying node is actually a unionfs node, netfs_validate_stat is
invoked and this function processes the node in a usual way, fetching
valid stat information.
 
> Indeed, the comment is fundamentally wrong: the whole point of using the
> unionfs root node here is so that the mountee *does* see the unionfs
> under it -- while in the transparent case, we will (probably) use the
> underlying node of unionfs instead, so that the mountee really doesn't
> see unionfs.

Yeah, sure.  I wanted to mean something like proxying, but apparently
ran into formulation problems.
 
> > +  if (!unionmount_proxy)
> > +    return ENOMEM;
> > +
> > +  /* Set the mountee on the proxy node.
> > +     Note that the O_READ flag does not actually limit access to the
> > +     mountee's filesystem considerably.  Whenever a client looks up a
> > +     node which is not a directory, unionfs will give off a port to
> > +     the node itself, withouth proxying it.  Proxying happens only for
> > +     directory nodes.  */
> > +  err = start_mountee (unionmount_proxy, mountee_argz,
> > +                  mountee_argz_len, O_READ, &mountee_port);
> > +  if (err)
> > +    return err;
> > +
> > +  mountee_started = 1;
> > +
> > +  return err;
> > +}                          /* setup_unionmount */
> 
> "err" must always be 0 here, so it's probably clearer to just return 0.
> 
> Alternatively, you could make the "mounte_started" assignment
> conditional on !err, instead of returning early. This is often more
> elegant; the Hurd code uses this approach in many places.

I'll return 0, if you don't mind, because later patches introduce
actions in between mountee_started and return err, so rebasing would
imply introducing more if statements, which I would be happy to avoid.
 
> (Same applies to startup_mountee(), BTW -- it *might* be more elegant to
> handle it this way... Though this is pretty case-specific; and I guess
> this is also a matter of taste to some extent at least.)

Generally, I prefer to avoid such ``elegant'' style.  I agree that it
looks aesthetically nice, but, when using this style, I often ran into
the necessity of building a structure of nested if statements, which
at a definite moment became too sophisticated to look nice.  I admit
that I could have applied a little bit more effort and split the
tree-like if statements, but it's a hard task for me to understand why
I should apply more effort to maintain an ``elegant'' construction, if
an absolutely equivalent though less elegant structure keeps me out of
trouble :-)

Also, I believe gcc optimizes all these statements anyways, so both
styles most probably end up in the same machine code.

Regards,
scolobb