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

[Julio Faracco]

Em sáb, 10 de nov de 2018 às 11:17, John Ferlan <jferlan at redhat.com> escreveu:
>
>
>
> 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.

No problems, John. You can go ahead with the changes.
I forgot too add VIR_STRDUP after checking the property.
It was causing the test error.

>
> 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.

Yes, I agree too. But only config files that don't have netowork settings.
Version 3.X and higher have another syntax to configure network too.
And it was not implemented yet. I'm planning to propose this feature
in the future.

>
> [...]