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

[Thomas Schwinge]

Hello!

On Thu, Jun 18, 2009 at 11:10:19PM +0300, Sergiu Ivanov wrote:
> On Wed, Jun 17, 2009 at 01:43:42AM +0200, Thomas Schwinge wrote:
> > So, instead of your version...
> > 
> >     [...]
> >     (argp_parse_common_options): Add the code for handling option
> >     ``--mount''.
> >     [...]
> >
> > ... I would have written this commit message as follows:
> >
> >     [...]
> >     (argp_parse_common_options): Handle it.
> >     [...]
> 
> I took the liberty of making this changelog entry a bit longer
> (``Handle the new option''). The reason is that it took me some time
> to realize what ``it'' refers to. I hope this deviation isn't that
> critical...

Sure, that is fine.


> > On Mon, Jun 15, 2009 at 10:39:22PM +0300, Sergiu Ivanov wrote:
> > > --- a/options.c
> > > +++ b/options.c
> > > @@ -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.  */
> > > +      err = argz_create_sep (arg, ' ', &mountee_argz, &mountee_argz_len);
> > 
> > If I understand it correctly, indeed: doing it this way, you loose the
> > ability to pass strings containing ``quoted space characters'', i.e. ones
> > that do not separate arguments.  Of couse, for simple use cases it
> > doesn't matter too much.
> 
> In this patch series I do pursue simple use-cases :-)

OK then.

> > There must be other applications that have the same problem --
> > unfortunately it's too late for me to investigate and see how they solve
> > this quoting / argument splitting problem.
> 
> I'll try to make an investigation of my own, shall we choose the
> continue the ``--mount'' idea, but I'll definitely be happy to have
> your help in that situation :-)

OK, I'll tell you as soon as I happen to find something usable or at
least interesting.


> > > diff --git a/unionmount.c b/unionmount.c
> > > new file mode 100644
> > > index 0000000..2b3041f
> > > --- /dev/null
> > > +++ b/unionmount.c
> > 
> > > +#define _GNU_SOURCE 1
> > 
> > That should usually be done in the Makefile / build system.
> 
> All .c files in unionfs define _GNU_SOURCE explicitly (though, true,
> they do only #define _GNU_SOURCE /*(no 1)*/, and I've changed my new
> patch accordingly). I chose to do similarly in unionmount.c, because
> moving the definition of _GNU_SOURCE into Makefile (if I understand
> you correctly) will mean changing a lot stuff loosely connected to
> unionmount matters. I guess this transition has its place in a
> separate clean-up patch.

Absolutely right, and sorry that I didn't check the other unionfs files
before complaining -- that's one major problem with reviewing patches out
of their context.

Yes, that's totally a separate change then, and you are correct to first
do it like it's done in the other unionfs files.  So, this change is
first to be done in the unionfs repository, and then that commit is to be
merged into your unionmount branch, at the same time extending the merge
to cover the unionmount.c file (needing the exactly same change) that is
present in your branch, but not in the master one.  I think this is
exactly how we're doing to deal with this issue -- sometime later, as
this is not really that important right now.  And then you'll see how
non-trivial (but not too difficult either) merging is done.


Regards,
 Thomas

signature.asc
Description: Digital signature