Re: [PATCH 1/3] Add the ``--mount'' command line option

[olafBuddenhagen]

Hi,

On Tue, Jul 14, 2009 at 12:27:02PM +0300, Sergiu Ivanov wrote:

> +  /* Add the --mount option to the result.  */
> +  if (mountee_argz)
> +    {
> +      /* The string describing the value of the ``--mount'' option.  */
> +      char * opt = NULL;

The comment is confusing: this does not describe only the value, but the
whole option string...

Also note that "buf" is used for the very same thing in another place in
the same function, so probably better to use the same here for
consistency -- and I don't think it's really necessary to comment it at
all.

> +
> +      /* The mountee command line converted to a 0-terminated string
> +      form.  */
> +      char * mountee_cl = malloc (mountee_argz_len);

I'd stress that it is converted *back* to a normal string...

BTW, does unionfs use this style (malloc right in the variable
definition) in other places? It's discouraged by GCS...

> +      if (!mountee_cl)
> +     return ENOMEM;
> +
> +      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)
> +     return ENOMEM;

Again for consistency, please handle the error like in the rest of the
function.

> +
> +      err = argz_add (argz, argz_len, opt);
> +    }
> +
>    ulfs_iterate
>      {
>        if (! err)

You forgot to free the temporary strings...

-antrik-