On 2018-09-05 at 21:26, Erik Skultety wrote:
>On Wed, Sep 05, 2018 at 04:36:27PM +0800, Shi Lei wrote:
>
>the subject should read:
>
>util: netlink: Introduce virNetlinkNewLink helper
>
>> This patch introduces virNetlinkNewLink which wraps newlink code
>> using libnl.
>
>... virNetlinkNewLink helper which wraps the common libnl/netlink code to
>create a new link.
Okay.
>
>>
>> Signed-off-by: Shi Lei <shi_lei@xxxxxxxxxxxxxx>
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virnetlink.c | 120 +++++++++++++++++++++++++++++++++++++--
>> src/util/virnetlink.h | 13 +++++
>> 3 files changed, 129 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index e2340d2..77c9b9e 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2447,6 +2447,7 @@ virNetlinkEventServiceStop;
>> virNetlinkEventServiceStopAll;
>> virNetlinkGetErrorCode;
>> virNetlinkGetNeighbor;
>> +virNetlinkNewLink;
>> virNetlinkShutdown;
>> virNetlinkStartup;
>>
>> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
>> index 8f06446..32aa62d 100644
>> --- a/src/util/virnetlink.c
>> +++ b/src/util/virnetlink.c
>> @@ -420,10 +420,8 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
>> if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
>> goto buffer_too_small;
>>
>> - if (ifname) {
>> - if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
>> - goto buffer_too_small;
>> - }
>> + if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
>> + goto buffer_too_small;
>
>unrelated to this patch...this kind of change should be a separate patch
>somewhere at the beginning of the series...
Okay.
>
>>
>> # ifdef RTEXT_FILTER_VF
>> /* if this filter exists in the kernel's netlink implementation,
>> @@ -488,6 +486,118 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
>> }
>>
>>
>> +/**
>> + * virNetlinkNewLink:
>> + *
>> + * @ifname: Name of the link
>> + * @type: The type of device, i.e., "bridge", "macvtap", "macvlan"
>
>s/Name/name
>s/The/the
>s/of device/of the device/
>s/i.e.,/i.e.
Okay.
>
>> + * @ifindex: The index for the 'link' device
>
>I think ^this one should be part of the extra args, e.g. bridge doesn't use it
Okay.
>
>> + * @data: The extra args for creating the netlink interface
>
>actually extra_args is a better name for the argument :)
Yes. The extra_args is more clear.
>
>> + * @error: for retrieving error code
>
>s/for retrieving error code/netlink error code
Okay.
>
>> + *
>> + * Create a network "link" (aka interface aka device) with the given
>> + * args. This works for many different types of network devices,
>> + * including macvtap and bridges.
>
>A generic wrapper to create a network link.
Okay.
>
>> + *
>> + * Returns 0 on success, -1 on fatal error.
>> + */
>> +int
>> +virNetlinkNewLink(const char *ifname,
>> + const char *type,
>> + const int *ifindex,
>> + virNetlinkNewLinkDataPtr data,
>> + int *error)
>> +{
>> + struct nlmsgerr *err;
>> + struct nlattr *linkinfo = NULL;
>> + struct nlattr *infodata = NULL;
>> + unsigned int buflen;
>> + struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
>> + VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
>> + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
>> +
>> + *error = 0;
>> +
>> + if (!data)
>> + return -1;
>
>Extra args should not be required, BridgeCreate doesn't make use of them. That
>said, I think it's reasonable to make @ifname and @type mandatory and fail
>whenever we don't get either of them:
>
>if (!ifname && !type)
> virReportError(...);
Okay. I see.
>
>> +
>> + nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
>> + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
>> + if (!nl_msg) {
>> + virReportOOMError();
>> + return -1;
>> + }
>> +
>> + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
>> + goto buffer_too_small;
>> +
>> + if (ifindex && nla_put_u32(nl_msg, IFLA_LINK, *ifindex) < 0)
>> + goto buffer_too_small;
>> +
>> + if (data->mac && nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, data->mac) < 0)
>> + goto buffer_too_small;
>
>The order of the data in the msg payload doesn't matter, right? If that's true,
>we could move everything related to @extra_args and *not* related to nested
>containers right before actually calling into virNetlinkCommand. If the order
>does actually matter, then you can disregard this comment.
Right. I look through the rtnl_newlink function which uses nlmsg_parse which uses nla_parse.
In nla_parse function, we can see that the order of the netlink attributes does *not* matter.
So I could do as you said.
>
>> +
>> + if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
>> + goto buffer_too_small;
>> +
>> + if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
>> + goto buffer_too_small;
>> +
>> + if (type && nla_put_string(nl_msg, IFLA_INFO_KIND, type) < 0)
>> + goto buffer_too_small;
>> +
>> + if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) &&
>> + data->macvlan_mode && *data->macvlan_mode > 0) {
>
>this check would get a bit uglier though:
>if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) &&
> extra_args &&
> extra_args->macvlan_mode &&
> *extra_args->macvlan_mode > 0)
Okay.
>
>> + if (!(infodata = nla_nest_start(nl_msg, IFLA_INFO_DATA)))
>> + goto buffer_too_small;
>> +
>> + if (nla_put_u32(nl_msg, IFLA_MACVLAN_MODE, *data->macvlan_mode) < 0)
>> + goto buffer_too_small;
>> +
>> + nla_nest_end(nl_msg, infodata);
>> + }
>> +
>> + nla_nest_end(nl_msg, linkinfo);
>
>here you'd have:
>
>if (extra_args) {
>...
>}
Okay. I see.
>
>> +
>> + if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 0)
>> + return -1;
>> +
>> + if (buflen < NLMSG_LENGTH(0) || resp == NULL)
>> + goto malformed_resp;
>> +
>> + switch (resp->nlmsg_type) {
>> + case NLMSG_ERROR:
>> + err = (struct nlmsgerr *)NLMSG_DATA(resp);
>> + if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
>> + goto malformed_resp;
>> +
>> + if (err->error < 0) {
>> + *error = err->error;
>> + return -1;
>> + }
>> + break;
>> +
>> + case NLMSG_DONE:
>> + break;
>> +
>> + default:
>> + goto malformed_resp;
>> + }
>> +
>> + return 0;
>> +
>> + malformed_resp:
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("malformed netlink response message"));
>> + return -1;
>> +
>> + buffer_too_small:
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("allocated netlink buffer is too small"));
>> + return -1;
>> +}
>> +
>> +
>> /**
>> * virNetlinkDelLink:
>> *
>> @@ -522,7 +632,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
>> if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
>> goto buffer_too_small;
>>
>> - if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
>> + if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
>> goto buffer_too_small;
>
>...unrelated to this patch...
Okay.
>
>>
>> if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
>> diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
>> index 1e1e616..8163a81 100644
>> --- a/src/util/virnetlink.h
>> +++ b/src/util/virnetlink.h
>> @@ -65,6 +65,19 @@ int virNetlinkDumpCommand(struct nl_msg *nl_msg,
>> unsigned int protocol, unsigned int groups,
>> void *opaque);
>>
>> +typedef struct _virNetlinkNewLinkData virNetlinkNewLinkData;
>> +typedef virNetlinkNewLinkData *virNetlinkNewLinkDataPtr;
>> +struct _virNetlinkNewLinkData {
>> + const virMacAddr *mac; /* The MAC address of the device */
>> + const uint32_t *macvlan_mode; /* The mode of macvlan */
>
>this would contain const int *ifindex too...
Okay.
>
>> +};
>> +
>> +int virNetlinkNewLink(const char *ifname,
>> + const char *type,
>> + const int *ifindex,
>> + virNetlinkNewLinkDataPtr data,
>> + int *error);
>> +
>> typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
>>
>> int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
>
>The patch looks good, except for the few nitpicks I mentioned.
>
>Erik
Thanks for your comments!
Shi Lei
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
