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

[olafBuddenhagen]

Hi,

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 :-)

> +
>  /* Starts the mountee (given by `argz` and `argz_len`), sets it on
>     node `np` and opens a port `port` to with `flags`.  `port` is not
>     modified when an error occurs.  */
> diff --git a/mount.h b/mount.h
> index 2dd1900..d01b234 100644
> --- a/mount.h
> +++ b/mount.h
> @@ -40,6 +40,10 @@ extern mach_port_t mountee_port;
>  
>  extern int mountee_started;
>  
> +/* Shows the mode in which the current instance of unionmount
> +   operates (transparent/non-transparent).  */
> +extern int transparent_mount;
> +
>  /* Starts the mountee (given by `argz` and `argz_len`), sets it on
>     node `np` and opens a port `port` to with `flags`.  `port` is not
>     modified when an error occurs.  */
> diff --git a/netfs.c b/netfs.c
> index 5549365..fc2572f 100644
> --- a/netfs.c
> +++ b/netfs.c
> @@ -50,10 +50,10 @@ netfs_append_args (char **argz, size_t *argz_len)
>  {
>    error_t err = 0;
>  
> -  /* Add the --mount option to the result.  */
> +  /* Add the --mount or --no-mount option to the result.  */
>    if (mountee_argz)
>      {
> -      /* The string describing the value of the ``--mount'' option.  */
> +      /* The string describing the value of the option.  */
>        char * opt = NULL;
>  
>        /* The mountee command line converted to a 0-terminated string
> @@ -65,7 +65,9 @@ netfs_append_args (char **argz, size_t *argz_len)
>        memcpy (mountee_cl, mountee_argz, mountee_argz_len);
>        argz_stringify (mountee_cl, mountee_argz_len, ' ');
>  
> -      if (asprintf (&opt, "%s=\"%s\"", OPT_LONG (OPT_LONG_MOUNT), 
> mountee_cl) == -1)
> +      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...)

I don't remember whether GCS says something on that?

Also, I wonder whether the macro would break if you do:

   opt_name = OPT_LONG(transparent_mount ? OPT_LONG_MOUNT : OPT_LONG_NOMOUNT)

> +      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"},

"mounted direcly" is not very descriptive. You should say something like
"... appear as if MOUNTEE was set on the underlying node directly" or
so.

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 :-)

-antrik-