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

[Dwight Engen]

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. 

>  	subkey = strstr(key, token);
>  
>  	if (!subkey) {
> @@ -1499,8 +1506,9 @@ static int config_mount(const char *key, const
> char *value, return config_mount_auto(key, value, lxc_conf);
>  	}
>  
> -	if (!strlen(subkey))
> -		return -1;
> +	/* At this point we definately have key = lxc.mount.entry */
> +	if (!value || strlen(value) == 0)
> +		return lxc_clear_mount_entries(lxc_conf);
>  
>  	mntlist = malloc(sizeof(*mntlist));
>  	if (!mntlist)