[libvirt] [PATCHv3 1/3] util: netlink: Introduce virNetlinkNewLink helper

[Shi Lei]

On 2018-09-11 at 01:17, John Ferlan wrote:
>[...]
>
>>>
>>> I would say:
>>>
>>>  * Returns 0 on success, -1 on error. Additionally, if the @error is
>>>  * non-zero, then the failure occurred during virNetlinkCommand, but
>>>  * no error message generated leaving it up to the caller to handle
>>>  * the condition.
>>
>> "is generated" I guess?
>>
>> Anyway, I agree.
>>
>
>right... fingers don't always comply with mind ;-)
>
>
>>>
>
>[...]
>
>>>>> +    if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) &&
>>>>> +         extra_args &&
>>>>> +         extra_args->macvlan_mode &&
>>>>> +         *extra_args->macvlan_mode > 0) {
>>>
>>> Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
>>>
>>>>> +        if (!(infodata = nla_nest_start(nl_msg, IFLA_INFO_DATA)))
>>>>> +            goto buffer_too_small;
>>>>> +
>>>>> +        if (nla_put_u32(nl_msg, IFLA_MACVLAN_MODE, *extra_args->macvlan_mode) < 0)
>>>>
>>>> here too...
>>>>
>>>>> +            goto buffer_too_small;
>>>>> +
>>>>> +        nla_nest_end(nl_msg, infodata);
>>>>> +    }
>>>>> +
>>>>> +    nla_nest_end(nl_msg, linkinfo);
>>>>> +
>>>>> +    if (extra_args) {
>>>>> +        if (extra_args->ifindex &&
>>>>> +            nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
>>>>
>>>> and here as well....
>>>>
>>>
>>> Similarly @ifindex doesn't seem to need to be a "const int *"
>>
>> Are you referring to the const correctness or the fact it's a pointer? If it's
>> the former, then I don't see a problem, if it's the latter, then I simply
>> wanted a deterministic way of telling that an argument is set, in case values
>> 0, -1, etc. had some meaning.
>>
>
>latter that it's a pointer.  The way it looks to me is that the value
>could be changed in the function, but I understand the point of 0 (not
>supplied) vs. NULL...
>
>So fair enough reason to keep as const int *...
>
>John 

Thank you for your comments, John.    :-)

Shi Lei