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

[olafBuddenhagen]

Hi,

On Sun, Jul 05, 2009 at 07:10:48PM +0300, Sergiu Ivanov wrote:
> 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; .

Right... This is rather ugly indeed. I wonder, doesn't the compiler warn
about this?

> 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?

Yeah, I already considered suggesting it myself.

> > > +/*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.

Well, this already tells me that it's not used consistently anyways --
so there is really no reason not to use a better name in new code :-)

> > > +  /*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?

Well, in this specific case, the variables are only used in a
subfunction -- so you wouldn't really break any tradition by moving them
there...

In general, I'd say this is not a point where consistency is important.

> > > +  /*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?

Well, it's not really portable for one... But that doesn't really matter
for the Hurd :-)

When used instead of local variables, I can't really give any objective
reasons -- the effect is exactly the same, it just somehow doesn't feel
right to me... I suggest you ignore my rambling ;-)

If used instead of malloc() OTOH, the fact that it doesn't handle errors
is a serious drawback. The size of the UID array is usually very small;
but there is really nothing preventing it from getting arbitrarily large
-- stack allocation is not very nice in this case... But then, other
Hurd code seems to do that too :-(

> > > +  /*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?

In general, the existing code is the authority. From what I've seen so
far, this is handled very consistently in all existing Hurd code, and
unionfs in fact has many examples.

> > > +  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.

You are right. However, if it is really important that "port" doesn't
get clobbered when an error occurs, this should be explicitely
documented in the function comment. If callers don't rely on this
behaviour OTOH, I wouldn't bother.

-antrik-