[lxc-devel] [PATCH 1/1] fix lxc.mount.auto clearing

[Dwight Engen]

On Thu, 9 Oct 2014 22:50:46 +0000
Serge Hallyn <serge.hallyn at ubuntu.com> wrote:

> Quoting Dwight Engen (dwight.engen at oracle.com):
> > On Thu, 9 Oct 2014 16:01:15 +0000
> > Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> > 
> > > the way config_mount was structured, sending 'lxc.mount.auto = '
> > > ended up actually clearing all lxc.mount.entrys.  Fix that by
> > > moving the check for an empty value to after the subkey checks.
> > > Then, actually do the clearing of auto_mounts in
> > > config_mount_auto.
> > > 
> > > The 'strlen(subkey)' check being removed was bogus - the subkey
> > > either known to be 'lxc.mount.entry', else subkey would have been
> > > NULL (and forced a return in the block above).
> > > 
> > > This would have been clearer if the config_mount() and helper
> > > fns were structured like the rest of confile.c.  It's tempting
> > > to switch it over, but there are subtleties in there so it's
> > > not something to do without a lot of thought and testing.
> > > 
> > > Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
> > 
> > Comment below,
> > 
> > Acked-by: Dwight Engen <dwight.engen at oracle.com>
> > 
> > > ---
> > >  src/lxc/confile.c | 22 +++++++++++++++-------
> > >  1 file changed, 15 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/lxc/confile.c b/src/lxc/confile.c
> > > index 0722a37..09eb94d 100644
> > > --- a/src/lxc/confile.c
> > > +++ b/src/lxc/confile.c
> > > @@ -1403,6 +1403,8 @@ out:
> > >  static int config_fstab(const char *key, const char *value,
> > >  			struct lxc_conf *lxc_conf)
> > >  {
> > > +	if (!value || strlen(value) == 0)
> > > +		return -1;
> > >  	return config_path_item(&lxc_conf->fstab, value);
> > >  }
> > >  
> > > @@ -1434,8 +1436,10 @@ static int config_mount_auto(const char
> > > *key, const char *value, int i;
> > >  	int ret = -1;
> > >  
> > > -	if (!strlen(value))
> > > -		return -1;
> > > +	if (!value || strlen(value) == 0) {
> > > +		lxc_conf->auto_mounts = 0;
> > > +		return 0;
> > > +	}
> > >  
> > >  	autos = strdup(value);
> > >  	if (!autos) {
> > > @@ -1469,6 +1473,12 @@ static int config_mount_auto(const char
> > > *key, const char *value, return ret;
> > >  }
> > >  
> > > +/*
> > > + * TODO
> > > + * This fn is handling lxc.mount, lxc.mount.entry, and
> > > lxc.mount.auto.
> > > + * It should probably be split into 3 separate functions indexed
> > > by
> > > + * the config[] entries at top.
> > > + */
> > >  static int config_mount(const char *key, const char *value,
> > >  			struct lxc_conf *lxc_conf)
> > >  {
> > > @@ -1479,9 +1489,6 @@ static int config_mount(const char *key,
> > > const char *value, char *mntelem;
> > >  	struct lxc_list *mntlist;
> > >  
> > > -	if (!value || strlen(value) == 0)
> > > -		return lxc_clear_mount_entries(lxc_conf);
> > > -
> > 
> > It seems that just plain "lxc.mount = " would've also cleared the
> > list, and now only "lxc.mount.entry = " will. I think the new
> > behavior for "lxc.mount = " will be to clear lxc_conf->fstab via
> > config_path_item() so that seems correct to me. I'd consider the old
> > behavior a bug so I like the change, but just thought I'd point out
> > that its a difference. 
> 
> Yeah, thanks, that might be worth maintaining.  The simplest fix would
> be to do this check before the return -1 at line 1501.  If you wanted
> to to post such a patch I'd ack it :)

I actually like the way you have it now, it fixes the inconsistent
behavior and makes it so lxc_conf->fstab can be cleared. I think it can
be argued that the old behavior was just buggy, so I'd recommend we
keep with what you've done.

> -serge
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel