Hello,
On Fri, Jul 10, 2009 at 04:17:23AM +0200, olafbuddenha...@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, 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; .
>
> 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
