On 11/11/18 12:46 PM, Julio Faracco wrote:
> Em sáb, 10 de nov de 2018 às 11:17, John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxx>
>>
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
