Hello,
On Wed, Jul 29, 2009 at 09:27:10AM +0200, olafbuddenha...@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
