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, olafbuddenha...@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
