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

[Serge Hallyn]

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

-serge