Re: [PATCH 2/3] Start the mountee in a lazy fashion.

[Sergiu Ivanov]

Hello,

On Sun, Aug 16, 2009 at 07:42:30PM +0200, olafBuddenhagen@gmx.net wrote:
> On Mon, Aug 03, 2009 at 08:41:55PM +0300, Sergiu Ivanov wrote:
> 
> > Subject: [PATCH 2/3] Start the mountee in a lazy fashion.
> 
> This title line is not very helpful. The important point is that this
> patch adds the code for starting the mountee.

AIUI, this title line does say that some code for starting the mountee
is added.  Or do I understand something wrong?

> Further explanation about how this happens, would normally go into
> the body of the commit message -- but as we know, GNU changelog
> policy is stuck in the 1980s and forbids this... :-(

Indeed; I like the commit message style you suggested last summer for
my nsmux github repository so much better.
 
> The fact that you submitted various variants of this patch with
> different titles, is very confusing -- I actually failed to review some
> versions, because I got quite mixed up as to which are newer variants of
> this patch, and which are different things...

I changed the title of this patch because the very first one -- ``Add
the code for starting up the mountee'' -- is too verbose (almost every
patch *adds* some code).  Nevertheless, I think I never did anything
similar to my other patches, so I can say I'm sorry for this one and
add that this won't happen again.

Also, things might have got mixed up because I frequently posted
responses to E-mails and the corresponding patches separately.  I'll
try to always submit these together to avoid further confusion.

I won't submit patch with corrections you mention in this E-mail right
away, because the corrections are mainly about changing some comments
or strings and I think it will be harder for you to review the changes
if I post the whole patch again.
 
> > * mount.c (mountee_node): New variable.
> > (mountee_root): Likewise.
> > (mountee_started): Likewise.
> > (start_mountee): New function (based on node_set_translator
> > in nsmux).
> > (setup_unionmount): New function.
> > * (mountee_root): New variable.
> 
> I think you forgot the file name here...

Sure :-( Corrected.
 
> > +    /* Create a port to `np`.  */
> 
> This comment is really why I dislike "np" so much...
> 
> If you insist on using a meaningless variable name, please at least let
> the comment say what this is actually about :-)

I changed it to: ``Create a port to node on which the mountee should
sit (np).''
 
> > +  /* Obtain the port to the root of the newly-set translator.  */
> > +  err = fsys_getroot (mountee_control, MACH_PORT_NULL, 
> > MACH_MSG_TYPE_COPY_SEND,
> > +                 uids, nuids, gids, ngids, flags, &retry_port,
> > +                 retry_name, port);
> > +  if (err)
> > +    return err;
> > +
> > +  return 0;
> 
> The last four lines are silly... Just return err and be done with it.

Oh yeah :-( I changed it to just ``return err;'', as you suggested.
 
> I think this actually hints that the other style of error handling I
> suggested would indeed be more appropriate here... :-)

Indeed; I think I'll have to follow this style more often.
 
> > +}                          /* start_mountee */
> > +
> > +/* 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)
> 
> The comment is wrong I think: You do not actually register the mountee
> yet; this is done only with the next patch...

Ah, sure :-( I changed it to: ``Sets up a proxy node and sets the
translator on it.''
 
> > +{
> > +  error_t err = 0;
> > +
> > +  /* The mountee will be sitting on this node.  This node is based on
> > +     the netnode of the root node, so most RPCs on this node can be
> > +     automatically carried out correctly.  */
> 
> I guess it would be helpful to explicitely mention the word "clone"
> somewhere. Also, the comment should explain *why* it is necessary to
> clone, instead of just using the original node...

Changed to: ``The mountee will be sitting on this node.  This node is
based on the netnode of the root node (it is essentially a clone of
the root node), so most RPCs on this node can be automatically carried
out correctly.  Note the we cannot set the mountee on the root node
directly, because in this case the mountee's filesystem will obscure
the filesystem published by unionfs.''
 
> > +  mountee_node = netfs_make_node (netfs_root_node->nn);
> > +  if (!mountee_node)
> > +    return ENOMEM;
> > +
> > +  /* Set the mountee on the new 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.  */
> 
> Why are you passing O_READ, anyways?...

The flags which I pass to start_mountee are used in opening the port
to the root node of the mountee.  (I'm sure you've noticed this; I'm
just re-stating it to avoid ambiguities).  Inside unionfs, this port
is used for lookups *only*, so O_READ should be sufficient for any
internal unionfs needs.  Ports to files themselves are not proxied by
unionfs (as the comment reads), so the flags passed here don't
influence that case.

Also, unionfs itself uses O_READ when opening directory nodes, too
(well, it actually uses O_READ | O_NOTRANS, but that's unapplicable in
our case).
 
> > diff --git a/netfs.c b/netfs.c
> > index 8d2a4ec..f8d5e3c 100644
> > --- a/netfs.c
> > +++ b/netfs.c
> > @@ -199,6 +199,13 @@ netfs_validate_stat (struct node *np, struct iouser 
> > *cred)
> >      }
> >    else 
> >      {
> > +      if (!mountee_started)
> > +   {
> > +     err = setup_unionmount ();
> > +     if (err)
> > +       error (EXIT_FAILURE, err, "failed to setup the mountee");
> 
> When used as verb, "set up" remain seperate words.

OK, thank you.  I'll keep this in mind.

Regards,
scolobb