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

[John Ferlan]

On 11/11/18 12:46 PM, Julio Faracco wrote:
> 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.
> 
>>
>> [...]

Since you have access to the V3.0 environment, perhaps it's best that
you update the patch based on my comments and also add the test *.config
files using the v3 syntax.

John