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

[Sergiu Ivanov]

Hello,

To avoid redundant text, I'll state beforehand that everything I
didn't comment on I had corrected in the patches I had sent in several
minutes before.

One remark on the comments in the unionmount_start_mountee function:
this code I borrowed from nsmux with little change, which means that
the comments there are those from nsmux.  And you know what my comment
style was last summer :-)

On Fri, Jul 03, 2009 at 08:07:01PM +0200, olafBuddenhagen@gmx.net wrote:
> > +   /*This is actually the end of initialization, so if something
> > +     goes bad here we are rightful to die. And, of course,
> > +     unionmount makes no sense if the mountee is not running. */
> > +     errno = error;
> > +     perror ("failed to setup the mountee");
> > +     exit (EXIT_FAILURE);
> 
> Just use the error() function.

I didn't use error function here, because in this context error was
already defined like error_t error; .  Although it's not already
required in the corrected patch, I'll ask: what to do in this case?
Shall I first create a clean-up patch to rename the variable?
 
> > +/*Starts the mountee (given by `argz` and `argz_len`), sets it on node
> > +  `np` and opens a port `port` to with `flags`. */
> > +error_t
> > +unionmount_start_mountee (struct protid * diruser, node_t * np, char * 
> > argz,
> > +                     size_t argz_len, int flags, mach_port_t * port)
> > +{
> 
> "np" is not a very self-explaining variable name... Unless there is some
> convention that makes it worthwhile to stick to it, better use a more
> verbose name.

``np'' (``node pointer'') is often used in unionfs.  Sometimes one can
also see ``node'', too, but I like ``np'' better, because it stresses
that the variable is a pointer.
 
> > +  /*An unauthenticated port to the directory containing `np` */
> > +  mach_port_t unauth_dir;
> 
> Err... The mountee is set on an internal node, not attached to the real
> filesystem. What is the "directory containing np" in this case?...

This is the comment which remained from nsmux...
 
> > +  /*A copy of the user information, supplied in `user` */
> > +  struct iouser * user;
> > +
> > +  /*A protid for the supplied node */
> > +  struct protid * newpi;
> 
> These variables are used only in open_node() I believe?
> 
> Always define variables as close to the code actually using them as
> possible.

I use this strategy whenever I write my own programs, but in unionfs
all functions define their locals at the beginning, just as done here.
Should I keep the tradition or drop it and define variables near to
the code block using them?
 
> (I actually even tend to create extra code blocks only to be able to
> define certain variables in a more local scope...)

Oh yeah, I sometimes do the same :-)
 
> > +  /*Allocate some memory for the UIDs on the stack */
> > +  uids = alloca (nuids * sizeof (uid_t));
> 
> Hm, alloca() always seems a bit dirty... Though other parts of the Hurd
> seem to use it as well, so I guess it's fine.

Could you please explain why alloca is dirty?  Because it uses the
stack frame?  Or because it is hardware dependent (as the man page
says)?
 
> > +  /*Opens the port on which to set the new translator */
> > +  error_t
> > +    open_port
> > +    (int flags, mach_port_t * underlying,
> > +     mach_msg_type_name_t * underlying_type, task_t task, void *cookie)
> 
> AFAIK open_port should not be indented, and the parameter list should
> start on the same line.

I read in the GCS that emacs should be considered as an expert in GCS
indentation, and it indents things like this.  Which authority should
I comply with?
 
> > +  err = fsys_getroot
> > +    (active_control, unauth_dir, MACH_MSG_TYPE_COPY_SEND,
> > +     uids, nuids, gids, ngids, flags, &retry_port, retry_name, &p);
> > +  if (err)
> > +    return err;
> > +
> > +  /*Return the port */
> > +  *port = p;
> 
> [...]
> Also, why not just pass "port" to fsys_getroot() directly?

I met this style (using intermediate variables) quite often in unionfs
and my understanding is that keeping to such style you don't clobber
the parameters.  That is, should fsys_getroot fail, port will remain
unchanged.

Regards,
scolobb