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

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