Re: [PATCH 1/4] Add the ``--no-mount'' option.

[Sergiu Ivanov]

Hello,

On Wed, Jul 29, 2009 at 09:27:10AM +0200, olafBuddenhagen@gmx.net wrote:
> On Fri, Jul 17, 2009 at 01:56:57PM +0300, Sergiu Ivanov wrote:
>
> > diff --git a/mount.c b/mount.c
> > index 7045423..4d12cd6 100644
> > --- a/mount.c
> > +++ b/mount.c
> > @@ -40,6 +40,10 @@ mach_port_t mountee_port;
> >  
> >  int mountee_started = 0;
> >  
> > +/* Shows the mode in which the current instance of unionmount
> > +   operates (transparent/non-transparent).  */
> > +int transparent_mount = 1;
> 
> I think it would be clearer to default to "0" and set it on --mount...
> 
> But that's not terribly important really :-)

I set it to 1 because in this case argp_parse_common_options requires
only the addition of the lines for handling the OPT_NOMOUNT option,
which resumes to assigning 0 to transparent_mount.  Setting this
variable to 0 initially will require adding one more line and several
line shuffles.

As usual, I'm optimizing the most minor (and unimportant) bits :-)
 
> > +      char * opt_name = (transparent_mount ? OPT_LONG (OPT_LONG_MOUNT)
> > +                    : OPT_LONG (OPT_LONG_NOMOUNT));
> 
> Don't mix declarations and statements. While C99 allows this, and gcc
> supported it even before, it's not very good coding style IMHO. I
> haven't seen it in other Hurd code.
> 
> (There is a single instance of "for (int i=..." in rpctrace; but even
> that is questionable -- and it's not quite the same thing anyways...)

OK, thank you for explanation.  I had never used this style before and
wanted to ``try'' it out.
 
> I don't remember whether GCS says something on that?

I skimmed the ``Making the Best Use of C'' section and didn't notice
anything (though a more attentive perusal might reveal something).
Anyways, I've never seen variables initialized at declaration in the
Hurd, so I won't do like that.
 
> Also, I wonder whether the macro would break if you do:
> 
>    opt_name = OPT_LONG(transparent_mount ? OPT_LONG_MOUNT : OPT_LONG_NOMOUNT)

I had tried this before posting the patch; it breaks.

Take a look at the definition of OPT_LONG:

#define OPT_LONG(o) "--" o
 
> > +      if (asprintf (&opt, "%s=\"%s\"", opt_name, mountee_cl) == -1)
> >     return ENOMEM;
> >  
> >        err = argz_add (argz, argz_len, opt);
> > diff --git a/options.c b/options.c
> > index e2e5dcd..49d8701 100644
> > --- a/options.c
> > +++ b/options.c
> > @@ -56,9 +56,12 @@ static const struct argp_option argp_common_options[] =
> >        "send debugging messages to stderr" },
> >      { OPT_LONG_CACHE_SIZE, OPT_CACHE_SIZE, "SIZE", 0,
> >        "specify the maximum number of nodes in the cache" },
> > -    { OPT_LONG_MOUNT, OPT_MOUNT, "MOUNTEE", 0,
> > +    { OPT_LONG_NOMOUNT, OPT_NOMOUNT, "MOUNTEE", 0,
> >        "use MOUNTEE as translator command line, start the translator,"
> >        "and add its filesystem"},
> > +    { OPT_LONG_MOUNT, OPT_MOUNT, "MOUNTEE", 0,
> > +      "like --no-mount, but make it appear as if MOUNTEE had been mounted "
> > +      "directly"},
> 
> [...]
> BTW, I'm not really convinced that explaining --mount in terms of
> --no-mount is really the clearest approach... But I can't think of
> anything good either, so I'll leave it at it :-)

Fine :-) I'll keep in mind this nifty trick of explaining an option in
terms of another one ;-)

Regards,
scolobb