On 09/11/2018 02:29 AM, Erik Skultety wrote:
> On Mon, Sep 10, 2018 at 01:17:43PM -0400, 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 *...
>
> Okay, so do you have any other comments or shall I proceed with adding the
> function commentary you suggested in your response + removing the
> ATTRIBUTE_NONNULLs and merging the patch set with my proposed adjustment diffs?
>
No other comments from me.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
