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

Erik Skultety eskultet at redhat.com
Tue Sep 11 06:29:45 UTC 2018


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?

Erik




More information about the libvir-list mailing list