[libvirt] [PATCH v3] lxc: Include support to lxc version 3.0 or higher.

[John Ferlan]

On 11/9/18 12:30 PM, Julio Faracco wrote:
> This patch introduce the new settings for LXC 3.0 or higher. The older
> versions keep the compatibility to deprecated settings for LXC, but
> after release 3.0, the compatibility was removed. This commit adds the
> support to the refactored settings.
> 
> v1-v2: Michal's suggestions to handle differences between versions.
> v2-v3: Adding suggestions from Pino and John too.

These type of comments would go below the --- below so that they're not
part of commit history...

> 
> Signed-off-by: Julio Faracco <jcfaracco at gmail.com>
> ---
>  src/lxc/lxc_native.c | 45 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index cbdec723dd..d3ba12bb0e 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c

[...]

> @@ -724,12 +734,13 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr value, void *data)
>      char type;
>      unsigned long start, target, count;
>  
> -    if (STRNEQ(name, "lxc.id_map") || !value->str)
> +    /* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */
> +    if (STRNEQ(name, "lxc.idmap") || STRNEQ(name, "lxc.id_map") || !value->str)
>          return 0;

This one caused lxcconf2xmltest to fail and needs to change to:

    /* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */
    if (STRNEQ(name, "lxc.idmap") || !value->str) {
        if (!value->str || STRNEQ(name, "lxc.id_map"))
            return 0;
    }

The failure occurred because of the STRNEQ OR not being true (silly me
on first pass not running the tests too ;-))

>  
>      if (sscanf(value->str, "%c %lu %lu %lu", &type,
>                 &target, &start, &count) != 4) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid lxc.id_map: '%s'"),
> +        virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid: '%s'"),

Do you mind if I alter this to:

        virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid %s: '%s'"),
                       name, value->str);

That way the conf name string is provided like it was before


>                         value->str);
>          return -1;
>      }
> @@ -1041,19 +1052,27 @@ lxcParseConfigString(const char *config,
>      if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0)
>          goto error;
>  
> -    if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0 ||
> -        VIR_STRDUP(vmdef->name, value) < 0)
> -        goto error;
> +    if (virConfGetValueString(properties, "lxc.uts.name", &value) <= 0) {
> +        virResetLastError();
> +
> +        /* Check for pre LXC 3.0 legacy key */
> +        if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0)
> +            goto error;
> +    }
> +

I think in this case the @value needs to be restored... Previous if the
GetValueString OR the VIR_STRDUP of the value was < 0, we go to error.
Although I'm not quite sure how @value would be NULL so as to cause the
subsequent line to be executed...  In any case, copying @value needs to
be done, so add:

    if (VIR_STRDUP(vmdef->name, value) < 0)
        goto error;

Which I can add if you agree.

With those changes,

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

As a follow-up maybe adding/altering/updating the tests/lxcconf2xmldata
to include both pre and post 3.0 type data would be a good thing.

[...]