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

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?