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

[Carl Fredrik Hammar]

Hi,

This time around I'll review your patch more thoroughly.

On Thu, Jun 11, 2009 at 09:10:24PM +0300, Sergiu Ivanov wrote:
> >From d0f0f5c41d9046aec765a7264914c19642adead9 Mon Sep 17 00:00:00 2001
> From: Sergiu Ivanov <unlimitedscolobb@gmail.com>
> Date: Thu, 11 Jun 2009 15:22:24 +0300
> Subject: [PATCH] Add the ``--mount'' command line option.
> 
> * Makefile: Add unionmount.c to the list of compiled
> object files.
> Update copyright information.
> 
> * options.h (OPT_MOUNT): Add the definition.
> (OPT_LONG_MOUNT): Likewise.
> Update copyright information.
> 
> * options.c (argp_common_options): Add option ``--mount''
> to the array of options.
> (argp_parse_common_options): Add the code for handling option
> ``--mount''.
> Update copyright information.
> ---
>  Makefile     |    8 ++++++--
>  options.c    |   16 +++++++++++++++-
>  options.h    |    7 ++++++-
>  unionmount.c |   28 ++++++++++++++++++++++++++++
>  unionmount.h |   31 +++++++++++++++++++++++++++++++
>  5 files changed, 86 insertions(+), 4 deletions(-)
>  create mode 100644 unionmount.c
>  create mode 100644 unionmount.h
> 
> diff --git a/Makefile b/Makefile
> index b180072..e65f29b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,6 +1,10 @@
>  # Hurd unionfs
> -# Copyright (C) 2001, 2002, 2003, 2005 Free Software Foundation, Inc.
> +# Copyright (C) 2001, 2002, 2003, 2005, 2009 Free Software Foundation,
> +# Inc.

Break the line after the years instead, like so:

# Copyright (C) 2001, 2002, 2003, 2005, 2009
#   Free Software Foundation, Inc.

As is done in other parts of the Hurd.

> +#
>  # Written by Jeroen Dekkers <jeroen@dekkers.cx>.
> +#
> +# Adapted to unionmount by Sergiu Ivanov <unlimitedscolobb@gmail.com>

This sentence is missing a period.

>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -25,7 +29,7 @@ CFLAGS += -Wall -g -O2 -D_FILE_OFFSET_BITS=64 -std=gnu99 \
>  LDFLAGS += -lnetfs -lfshelp -liohelp -lthreads \
>             -lports -lihash -lshouldbeinlibc
>  OBJS = main.o node.o lnode.o ulfs.o ncache.o netfs.o \
> -       lib.o options.o pattern.o stow.o update.o
> +       lib.o options.o pattern.o stow.o update.o unionmount.o
>  
>  MIGCOMSFLAGS = -prefix stow_
>  fs_notify-MIGSFLAGS = -imacros ./stow-mutations.h
> diff --git a/options.c b/options.c
> index ef29a02..e2d8521 100644
> --- a/options.c
> +++ b/options.c
> @@ -1,7 +1,10 @@
>  /* Hurd unionfs
> -   Copyright (C) 2001, 2002, 2005 Free Software Foundation, Inc.
> +   Copyright (C) 2001, 2002, 2005, 2009 Free Software Foundation, Inc.
> +
>     Written by Moritz Schulte <moritz@duesseldorf.ccc.de>.
>  
> +   Adapted to unionmount by Sergiu Ivanov <unlimitedscolobb@gmail.com>

Again missing period.

>     This program is free software; you can redistribute it and/or
>     modify it under the terms of the GNU General Public License as
>     published by the Free Software Foundation; either version 2 of the
> @@ -23,6 +26,7 @@
>  
>  #include <argp.h>
>  #include <error.h>
> +#include <argz.h>
>  
>  #include "options.h"
>  #include "ulfs.h"
> @@ -33,6 +37,7 @@
>  #include "pattern.h"
>  #include "stow.h"
>  #include "update.h"
> +#include "unionmount.h"
>  
>  /* This variable is set to a non-zero value after parsing of the
>     startup options.  Whenever the argument parser is later called to
> @@ -51,6 +56,8 @@ 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,
> +      "start MOUNTEE and add the filesystem it publishes" },

"start the translator MOUNTEE and add it's filesystem" would be clearer
IMHO.

>      { 0, 0, 0, 0, "Runtime options:", 1 },
>      { OPT_LONG_STOW, OPT_STOW, "STOWDIR", 0,
>        "stow given directory", 1},
> @@ -124,6 +131,13 @@ argp_parse_common_options (int key, char *arg, struct 
> argp_state *state)
>        ulfs_match = 0;
>        break;
>  
> +    case OPT_MOUNT:
> +      /*TODO: Improve the mountee command line parsing mechanism. */

Please begin a comments with a space.  Also use two spaces after each
sentence, even if it's at the end of a comment.  For instance:

/* TODO: Improve the mountee command line parsing mechanism.  */

As is done in the rest of unionfs and the Hurd.

> +      err = argz_create_sep (arg, ' ', &mountee_argz, &mountee_argz_len);
> +      if (err)
> +     error (EXIT_FAILURE, err, "argz_create_sep");
> +      break;
> +
>      case OPT_UNDERLYING:     /* --underlying  */
>      case ARGP_KEY_ARG:
>  
> diff --git a/options.h b/options.h
> index eb74ce6..126759c 100644
> --- a/options.h
> +++ b/options.h
> @@ -1,7 +1,10 @@
>  /* Hurd unionfs
> -   Copyright (C) 2001, 2002 Free Software Foundation, Inc.
> +   Copyright (C) 2001, 2002, 2009 Free Software Foundation, Inc.
> +
>     Written by Moritz Schulte <moritz@duesseldorf.ccc.de>.
>  
> +   Adapted to unionmount by Sergiu Ivanov <unlimitedscolobb@gmail.com>

Again add a period.

>     This program is free software; you can redistribute it and/or
>     modify it under the terms of the GNU General Public License as
>     published by the Free Software Foundation; either version 2 of the
> @@ -29,6 +32,7 @@
>  #define OPT_PATTERN    'm'
>  #define OPT_PRIORITY   'p'
>  #define OPT_STOW       's'
> +#define OPT_MOUNT      't'
>  
>  /* The long options.  */
>  #define OPT_LONG_UNDERLYING "underlying"
> @@ -40,6 +44,7 @@
>  #define OPT_LONG_PATTERN    "match"
>  #define OPT_LONG_PRIORITY   "priority"
>  #define OPT_LONG_STOW       "stow"
> +#define OPT_LONG_MOUNT           "mount"

Here you're using a tab where the other strings are lined up with spaces.

> diff --git a/unionmount.c b/unionmount.c
> new file mode 100644
> index 0000000..e4aa043
> --- /dev/null
> +++ b/unionmount.c

Given that this is to implement the --mount option, I think mount.c
would be a better name.  The context of unionfs establishes that this
means unioning.

> @@ -0,0 +1,28 @@
> +/* Hurd unionmount
> +   The core of unionmount functionality.

Again the purpose of the file isn't really to implement unionmount,
but to implement the --mount option.

> +   Copyright (C) 2009 Free Software Foundation, Inc.
> +
> +   Written by Sergiu Ivanov <unlimitedscolobb@gmail.com>.

Again add a period.  Oh, wait it's already there!  ;-)

> +   This program is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU General Public License version
> +   2 as published by the Free Software Foundation.
> +
> +   This program is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> +   USA.  */
> +
> +#define _GNU_SOURCE 1
> +
> +#include "unionmount.h"
> +
> +/*The command line for starting the mountee. */

Again, missing spaces.

> +char * mountee_argz;
> +size_t mountee_argz_len;

> diff --git a/unionmount.h b/unionmount.h
> new file mode 100644
> index 0000000..a3f7588
> --- /dev/null
> +++ b/unionmount.h

Same as for unionmount.c.

> @@ -0,0 +1,31 @@
> +/* Hurd unionmount
> +   General information and properties for unionmount/unionfs.

Same as for unionmount.c comment.

> +   Copyright (C) 2009 Free Software Foundation, Inc.
> +
> +   Written by Sergiu Ivanov <unlimitedscolobb@gmail.com>.
> +
> +   This program is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU General Public License version
> +   2 as published by the Free Software Foundation.
> +
> +   This program is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> +   USA.  */
> +
> +#ifndef INCLUDED_UNIONMOUNT_H
> +#define INCLUDED_UNIONMOUNT_H

Conventionally this should be UNIONMOUNT_H, but given that other unionfs
also does this, perhaps it is best to be consistent.  Also don't forget
to change it to reflect the name of the file, if you do rename it.

> +#include <unistd.h>
> +
> +/*The command line for starting the mountee. */

Missing spaces.

> +extern char * mountee_argz;
> +extern size_t mountee_argz_len;
> +
> +#endif /*INCLUDED_UNIONMOUNT_H*/

Missing spaces, and since it's the end of a ifndef and not a ifdef,
should be:

#endif /* not INCLUDED_UNIONMOUNT_H */

(According to the GNU coding standards.)

Regards,
  Fredrik