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

[Sergiu Ivanov]

Hello,

On Fri, Jul 10, 2009 at 04:17:23AM +0200, olafBuddenhagen@gmx.net wrote:
> 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?

Nope, the compiler tells nothing.  The local definition overrides the
global one and it only complains when I try to call 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?
> 
> Yeah, I already considered suggesting it myself.

I will create a patch for unionfs shortly.
 
> > > > +  /*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 ;-)

I see :-)
 
> 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 :-(

I see your point.  Indeed, alloca is widely used in Hurd code; these
bits of code are actually nothing more than a copy-and-paste from
settrans, IIRC.
 
> > > > +  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.

I added the corresponding bit to the comment.  IMHO, it is a good
thing when the function does not clobber its out parameters on errors.

Regards,
scolobb