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 *...