[libvirt] [PATCHv2 0/4] Add virNetlinkNewLink for simplifying virNetDev*Create
Patch v1 here: https://www.redhat.com/archives/libvir-list/2018-August/msg01413.html Shi Lei (4): 1/4: Introduce virNetlinkNewLink (Since v1) - Remove callback and just use special-case to handle macvlan in virNetlinkNewLink - Add struct virNetlinkNewLinkData to pack some arguments of virNetlinkNewLink 2/4: Add wrapper macros around nla_nest*/nla_put* to make code more readable (New patch) - Add wrapper macros according to the code written by Erik - Apply these macros to virNetlinkNewLink and virNetDevSetVfConfig and virNetDevVPortProfileOpSetLink 3/4: Using virNetlinkNewLink to simplify virNetDev*Create (Since v1) - Remove the callback for macvlan in virnetdevmacvlan.c 4/4: Remove virNetDevPutExtraHeader and replace with nlmsg_append (New patch) src/libvirt_private.syms | 1 + src/util/virnetdev.c | 98 ++++++++--------------- src/util/virnetdevbridge.c | 78 ++++--------------- src/util/virnetdevmacvlan.c | 109 ++++---------------------- src/util/virnetdevvportprofile.c | 117 +++++++++------------------- src/util/virnetlink.c | 130 +++++++++++++++++++++++++------ src/util/virnetlink.h | 65 ++++++++++++++++ 7 files changed, 270 insertions(+), 328 deletions(-) -- 2.17.1
This patch introduces virNetlinkNewLink which wraps newlink code using libnl. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- 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; # 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" + * @ifindex: The index for the 'link' device + * @data: The extra args for creating the netlink interface + * @error: for retrieving error code + * + * 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. + * + * 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; + + 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; + + 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) { + 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); + + 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; 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 */ +}; + +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); -- 2.17.1
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.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- 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...
# 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.
+ * @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
+ * @data: The extra args for creating the netlink interface
actually extra_args is a better name for the argument :)
+ * @error: for retrieving error code
s/for retrieving error code/netlink error code
+ * + * 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.
+ * + * 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(...);
+ + 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.
+ + 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)
+ 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) { ... }
+ + 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...
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...
+}; + +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
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@massclouds.com> --- 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
This patch adds wrapper macros around nla_nest*/nla_put* which apply to virNetlinkNewLink and virNetDevSetVfConfig and virNetDevVPortProfileOpSetLink. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetdev.c | 54 ++++++-------- src/util/virnetdevvportprofile.c | 117 ++++++++++--------------------- src/util/virnetlink.c | 74 +++++++------------ src/util/virnetlink.h | 52 ++++++++++++++ 4 files changed, 134 insertions(+), 163 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 8eac419..892a147 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1657,11 +1657,11 @@ virNetDevSetVfConfig(const char *ifname, int vf, { int rc = -1; char macstr[VIR_MAC_STRING_BUFLEN]; - struct nlmsghdr *resp = NULL; struct nlmsgerr *err; unsigned int recvbuflen = 0; - struct nl_msg *nl_msg; struct nlattr *vfinfolist, *vfinfo; + VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL; + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL; struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC, .ifi_index = -1, @@ -1676,47 +1676,41 @@ virNetDevSetVfConfig(const char *ifname, int vf, return rc; } - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; - - if (ifname && - nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) - goto buffer_too_small; - + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); + return rc; + } - if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST))) - goto buffer_too_small; + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname); - if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO))) - goto buffer_too_small; + NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST); + NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO); if (macaddr) { struct ifla_vf_mac ifla_vf_mac = { - .vf = vf, - .mac = { 0, }, + .vf = vf, + .mac = { 0, }, }; virMacAddrGetRaw(macaddr, ifla_vf_mac.mac); - if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC, + sizeof(ifla_vf_mac), &ifla_vf_mac); } if (vlanid >= 0) { struct ifla_vf_vlan ifla_vf_vlan = { - .vf = vf, - .vlan = vlanid, - .qos = 0, + .vf = vf, + .vlan = vlanid, + .qos = 0, }; - if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), - &ifla_vf_vlan) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN, + sizeof(ifla_vf_vlan), &ifla_vf_vlan); } - nla_nest_end(nl_msg, vfinfo); - nla_nest_end(nl_msg, vfinfolist); + NETLINK_MSG_NEST_END(nl_msg, vfinfo); + NETLINK_MSG_NEST_END(nl_msg, vfinfolist); if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) @@ -1767,20 +1761,12 @@ virNetDevSetVfConfig(const char *ifname, int vf, ifname, vf, macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)", vlanid, rc < 0 ? "Fail" : "Success"); - - nlmsg_free(nl_msg); - VIR_FREE(resp); return rc; malformed_resp: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); goto cleanup; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - goto cleanup; } static int diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index 3ebf757..8574571 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -641,8 +641,6 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, int32_t vf, uint8_t op) { - int rc = -1; - struct nlmsghdr *resp = NULL; struct nlmsgerr *err; struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC, @@ -651,12 +649,14 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, unsigned int recvbuflen = 0; int src_pid = 0; uint32_t dst_pid = 0; - struct nl_msg *nl_msg; struct nlattr *vfports = NULL, *vfport; char macStr[VIR_MAC_STRING_BUFLEN]; char hostUUIDStr[VIR_UUID_STRING_BUFLEN]; char instanceUUIDStr[VIR_UUID_STRING_BUFLEN]; const char *opName; + int vfport_type = IFLA_PORT_SELF; + VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL; + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL; switch (op) { case PORT_REQUEST_PREASSOCIATE: @@ -691,24 +691,21 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST); if (!nl_msg) { virReportOOMError(); - return rc; + return -1; } - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); + return -1; + } - if (ifname && - nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname); if (macaddr || vlanid >= 0) { struct nlattr *vfinfolist, *vfinfo; - if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST))) - goto buffer_too_small; - - if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO))) - goto buffer_too_small; + NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST); + NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO); if (macaddr) { struct ifla_vf_mac ifla_vf_mac = { @@ -718,9 +715,8 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, virMacAddrGetRaw(macaddr, ifla_vf_mac.mac); - if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC, + sizeof(ifla_vf_mac), &ifla_vf_mac); } if (vlanid >= 0) { @@ -730,77 +726,49 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, .qos = 0, }; - if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), - &ifla_vf_vlan) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN, + sizeof(ifla_vf_vlan), &ifla_vf_vlan); } - nla_nest_end(nl_msg, vfinfo); - nla_nest_end(nl_msg, vfinfolist); - } - - if (vf == PORT_SELF_VF && nltarget_kernel) { - if (!(vfport = nla_nest_start(nl_msg, IFLA_PORT_SELF))) - goto buffer_too_small; - } else { - if (!(vfports = nla_nest_start(nl_msg, IFLA_VF_PORTS))) - goto buffer_too_small; - - /* begin nesting vfports */ - if (!(vfport = nla_nest_start(nl_msg, IFLA_VF_PORT))) - goto buffer_too_small; - } - - if (profileId) { - if (nla_put(nl_msg, IFLA_PORT_PROFILE, strlen(profileId) + 1, - profileId) < 0) - goto buffer_too_small; - } - - if (portVsi) { - if (nla_put(nl_msg, IFLA_PORT_VSI_TYPE, sizeof(*portVsi), - portVsi) < 0) - goto buffer_too_small; + NETLINK_MSG_NEST_END(nl_msg, vfinfo); + NETLINK_MSG_NEST_END(nl_msg, vfinfolist); } - if (instanceId) { - if (nla_put(nl_msg, IFLA_PORT_INSTANCE_UUID, VIR_UUID_BUFLEN, - instanceId) < 0) - goto buffer_too_small; + if (vf != PORT_SELF_VF || !nltarget_kernel) { + NETLINK_MSG_NEST_START(nl_msg, vfports, IFLA_VF_PORTS); + /* begin nesting of vfports */ + vfport_type = IFLA_VF_PORT; } + /* begin nesting of vfport */ + NETLINK_MSG_NEST_START(nl_msg, vfport, vfport_type); - if (hostUUID) { - if (nla_put(nl_msg, IFLA_PORT_HOST_UUID, VIR_UUID_BUFLEN, - hostUUID) < 0) - goto buffer_too_small; - } + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_PORT_PROFILE, profileId); + NETLINK_MSG_PUT(nl_msg, IFLA_PORT_VSI_TYPE, sizeof(*portVsi), portVsi); + NETLINK_MSG_PUT(nl_msg, IFLA_PORT_INSTANCE_UUID, VIR_UUID_BUFLEN, instanceId); + NETLINK_MSG_PUT(nl_msg, IFLA_PORT_HOST_UUID, VIR_UUID_BUFLEN, hostUUID); - if (vf != PORT_SELF_VF) { - if (nla_put(nl_msg, IFLA_PORT_VF, sizeof(vf), &vf) < 0) - goto buffer_too_small; - } + if (vf != PORT_SELF_VF) + NETLINK_MSG_PUT(nl_msg, IFLA_PORT_VF, sizeof(vf), &vf); - if (nla_put(nl_msg, IFLA_PORT_REQUEST, sizeof(op), &op) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_PORT_REQUEST, sizeof(op), &op); /* end nesting of vport */ - nla_nest_end(nl_msg, vfport); - + NETLINK_MSG_NEST_END(nl_msg, vfport); if (vfports) { /* end nesting of vfports */ - nla_nest_end(nl_msg, vfports); + NETLINK_MSG_NEST_END(nl_msg, vfports); } if (!nltarget_kernel) { if ((src_pid = virNetlinkEventServiceLocalPid(NETLINK_ROUTE)) < 0) - goto cleanup; + return -1; if ((dst_pid = virNetDevVPortProfileGetLldpadPid()) == 0) - goto cleanup; + return -1; } if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, src_pid, dst_pid, NETLINK_ROUTE, 0) < 0) - goto cleanup; + return -1; if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) goto malformed_resp; @@ -815,7 +783,7 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, virReportSystemError(-err->error, _("error during virtual port configuration of ifindex %d"), ifindex); - goto cleanup; + return -1; } break; @@ -826,21 +794,12 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, goto malformed_resp; } - rc = 0; - cleanup: - nlmsg_free(nl_msg); - VIR_FREE(resp); - return rc; + return 0; malformed_resp: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); - goto cleanup; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - goto cleanup; + return -1; } diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 32aa62d..4d19ac9 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -417,11 +417,12 @@ virNetlinkDumpLink(const char *ifname, int ifindex, return -1; } - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); + return -1; + } - if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname); # ifdef RTEXT_FILTER_VF /* if this filter exists in the kernel's netlink implementation, @@ -430,11 +431,7 @@ virNetlinkDumpLink(const char *ifname, int ifindex, */ { uint32_t ifla_ext_mask = RTEXT_FILTER_VF; - - if (nla_put(nl_msg, IFLA_EXT_MASK, - sizeof(ifla_ext_mask), &ifla_ext_mask) < 0) { - goto buffer_too_small; - } + NETLINK_MSG_PUT_U32PTR(nl_msg, IFLA_EXT_MASK, &ifla_ext_mask); } # endif @@ -478,11 +475,6 @@ virNetlinkDumpLink(const char *ifname, int ifindex, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return rc; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - return rc; } @@ -528,36 +520,27 @@ virNetlinkNewLink(const char *ifname, 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 (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); + return -1; + } - if (data->mac && nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, data->mac) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT_U32PTR(nl_msg, IFLA_LINK, ifindex); + NETLINK_MSG_PUT(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, data->mac); + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname); - if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0) - goto buffer_too_small; + NETLINK_MSG_NEST_START(nl_msg, linkinfo, IFLA_LINKINFO); - 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; + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_INFO_KIND, type); if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && data->macvlan_mode && *data->macvlan_mode > 0) { - 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); + NETLINK_MSG_NEST_START(nl_msg, infodata, IFLA_INFO_DATA); + NETLINK_MSG_PUT_U32PTR(nl_msg, IFLA_MACVLAN_MODE, data->macvlan_mode); + NETLINK_MSG_NEST_END(nl_msg, infodata); } - nla_nest_end(nl_msg, linkinfo); + NETLINK_MSG_NEST_END(nl_msg, linkinfo); if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 0) return -1; @@ -590,11 +573,6 @@ virNetlinkNewLink(const char *ifname, 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; } @@ -629,11 +607,12 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback) return -1; } - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); + return -1; + } - if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname); if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) { @@ -673,11 +652,6 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback) 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; } /** diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 8163a81..2e64570 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -48,6 +48,58 @@ struct nlmsghdr; # endif /* __linux__ */ + +# define NETLINK_MSG_NEST_START(msg, container, attrtype) \ +do { \ + container = nla_nest_start(msg, attrtype); \ + if (!container) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("nla_nest_start error [" #attrtype "]")); \ + return -1; \ + } \ +} while(0) + +# define NETLINK_MSG_NEST_END(msg, container) \ +do { nla_nest_end(msg, container); } while(0) + +/* + * When data is an rvalue, compiler will report error as below: + * "the address of [...] will always evaluate as 'true' [-Werror=address]" + * Add a dummy(as an lvalue) to solve it. + */ +# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ +do { \ + const void *dummy = data; \ + if (dummy && nla_put(msg, attrtype, datalen, data) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("nla_put error [" #attrtype "]")); \ + return -1; \ + } \ +} while(0) + +# define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \ +do { \ + const void *dummy = str; \ + if (dummy && nla_put_string(msg, attrtype, str) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("nla_put_string error [" #attrtype "]: '%s'"), \ + str); \ + return -1; \ + } \ +} while(0) + +# define NETLINK_MSG_PUT_U32PTR(msg, attrtype, ptr) \ +do { \ + const void *dummy = ptr; \ + if (dummy && nla_put_u32(msg, attrtype, *ptr) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("nla_put_u32 error [" #attrtype "]: '%u'"), \ + *ptr); \ + return -1; \ + } \ +} while(0) + + int virNetlinkStartup(void); void virNetlinkShutdown(void); -- 2.17.1
On Wed, Sep 05, 2018 at 04:36:28PM +0800, Shi Lei wrote:
This patch adds wrapper macros around nla_nest*/nla_put* which apply to virNetlinkNewLink and virNetDevSetVfConfig and virNetDevVPortProfileOpSetLink.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetdev.c | 54 ++++++-------- src/util/virnetdevvportprofile.c | 117 ++++++++++--------------------- src/util/virnetlink.c | 74 +++++++------------ src/util/virnetlink.h | 52 ++++++++++++++ 4 files changed, 134 insertions(+), 163 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 8eac419..892a147 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1657,11 +1657,11 @@ virNetDevSetVfConfig(const char *ifname, int vf, { int rc = -1; char macstr[VIR_MAC_STRING_BUFLEN]; - struct nlmsghdr *resp = NULL; struct nlmsgerr *err; unsigned int recvbuflen = 0; - struct nl_msg *nl_msg; struct nlattr *vfinfolist, *vfinfo; + VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL; + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
changes related to AUTO{PTR,FREE} should completely separate, even from the series itself.
struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC, .ifi_index = -1, @@ -1676,47 +1676,41 @@ virNetDevSetVfConfig(const char *ifname, int vf, return rc; }
- if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; - - if (ifname && - nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
as I mentioned in my reply to the previous patch, I'd first replace ocurrences like ^these with nla_put_string for consistency and also making reviewer's life easier.
- goto buffer_too_small; - + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); + return rc; + }
- if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST))) - goto buffer_too_small; + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname);
- if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO))) - goto buffer_too_small; + NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST); + NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO);
if (macaddr) { struct ifla_vf_mac ifla_vf_mac = { - .vf = vf, - .mac = { 0, }, + .vf = vf, + .mac = { 0, }, };
virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
- if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC, + sizeof(ifla_vf_mac), &ifla_vf_mac); }
if (vlanid >= 0) { struct ifla_vf_vlan ifla_vf_vlan = { - .vf = vf, - .vlan = vlanid, - .qos = 0, + .vf = vf, + .vlan = vlanid, + .qos = 0, };
- if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), - &ifla_vf_vlan) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN, + sizeof(ifla_vf_vlan), &ifla_vf_vlan); }
- nla_nest_end(nl_msg, vfinfo); - nla_nest_end(nl_msg, vfinfolist); + NETLINK_MSG_NEST_END(nl_msg, vfinfo); + NETLINK_MSG_NEST_END(nl_msg, vfinfolist);
if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) @@ -1767,20 +1761,12 @@ virNetDevSetVfConfig(const char *ifname, int vf, ifname, vf, macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)", vlanid, rc < 0 ? "Fail" : "Success"); - - nlmsg_free(nl_msg); - VIR_FREE(resp); return rc;
malformed_resp: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); goto cleanup; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - goto cleanup; }
static int diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index 3ebf757..8574571 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -641,8 +641,6 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, int32_t vf, uint8_t op) { - int rc = -1; - struct nlmsghdr *resp = NULL; struct nlmsgerr *err; struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC, @@ -651,12 +649,14 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, unsigned int recvbuflen = 0; int src_pid = 0; uint32_t dst_pid = 0; - struct nl_msg *nl_msg; struct nlattr *vfports = NULL, *vfport; char macStr[VIR_MAC_STRING_BUFLEN]; char hostUUIDStr[VIR_UUID_STRING_BUFLEN]; char instanceUUIDStr[VIR_UUID_STRING_BUFLEN]; const char *opName; + int vfport_type = IFLA_PORT_SELF; + VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL; + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
again the VIR_AUTO{PTR,FREE} stuff...
switch (op) { case PORT_REQUEST_PREASSOCIATE: @@ -691,24 +691,21 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST); if (!nl_msg) { virReportOOMError(); - return rc; + return -1; }
- if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); + return -1; + }
- if (ifname && - nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname);
if (macaddr || vlanid >= 0) { struct nlattr *vfinfolist, *vfinfo;
- if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST))) - goto buffer_too_small; - - if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO))) - goto buffer_too_small; + NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST); + NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO);
if (macaddr) { struct ifla_vf_mac ifla_vf_mac = { @@ -718,9 +715,8 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
- if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC, + sizeof(ifla_vf_mac), &ifla_vf_mac); }
if (vlanid >= 0) { @@ -730,77 +726,49 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, .qos = 0, };
- if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), - &ifla_vf_vlan) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN, + sizeof(ifla_vf_vlan), &ifla_vf_vlan); }
- nla_nest_end(nl_msg, vfinfo); - nla_nest_end(nl_msg, vfinfolist); - } - - if (vf == PORT_SELF_VF && nltarget_kernel) { - if (!(vfport = nla_nest_start(nl_msg, IFLA_PORT_SELF))) - goto buffer_too_small; - } else { - if (!(vfports = nla_nest_start(nl_msg, IFLA_VF_PORTS))) - goto buffer_too_small; - - /* begin nesting vfports */ - if (!(vfport = nla_nest_start(nl_msg, IFLA_VF_PORT))) - goto buffer_too_small; - }
at this point it got quite difficult to keep track of all the replacements, so I'd suggest making the replacements one at a time, first one macro, then another, etc. ...
/** diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 8163a81..2e64570 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -48,6 +48,58 @@ struct nlmsghdr;
# endif /* __linux__ */
+ +# define NETLINK_MSG_NEST_START(msg, container, attrtype) \ +do { \ + container = nla_nest_start(msg, attrtype); \ + if (!container) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("nla_nest_start error [" #attrtype "]")); \ + return -1; \ + } \ +} while(0) + +# define NETLINK_MSG_NEST_END(msg, container) \ +do { nla_nest_end(msg, container); } while(0) + +/* + * When data is an rvalue, compiler will report error as below: + * "the address of [...] will always evaluate as 'true' [-Werror=address]" + * Add a dummy(as an lvalue) to solve it.
Okay, let's try rewording this in a more compact manner below...
+ */ +# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ +do { \
how about: /* @data may not always be a pointer type, therefore making compilers complain * about the check below, so let's use an intermediary pointer type */
+ const void *dummy = data; \
let's rename @dummy to dataptr to make it more explicit
+ if (dummy && nla_put(msg, attrtype, datalen, data) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("nla_put error [" #attrtype "]")); \ + return -1; \ + } \ +} while(0) + +# define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \ +do { \ + const void *dummy = str; \ + if (dummy && nla_put_string(msg, attrtype, str) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("nla_put_string error [" #attrtype "]: '%s'"), \ + str); \
I don't think that changing the error is necessary, all of the variants essentially call nla_put and that one either fails with -EINVAL (not our case, since we always provide a valid datalen) or -ENOMEM, so the original error stating that the buffer wasn't large enough is sufficient.
+ return -1; \ + } \ +} while(0)
I'm not entirely sold on these MSG_PUT_X macros, but I don't have a reasonable argument against either, so let's leave them in. Alternatively, we could have #define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \ NETLINK_MSG_PUT(msg, attrtype, strlen(str) + 1, str) and that would make things more clean, afterall that is what all the nla_put_foo will do anyway... Erik
+ +# define NETLINK_MSG_PUT_U32PTR(msg, attrtype, ptr) \ +do { \ + const void *dummy = ptr; \ + if (dummy && nla_put_u32(msg, attrtype, *ptr) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("nla_put_u32 error [" #attrtype "]: '%u'"), \ + *ptr); \ + return -1; \ + } \ +} while(0) + + int virNetlinkStartup(void); void virNetlinkShutdown(void);
-- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 2018-09-05 at 23:01, Erik Skultety wrote:
On Wed, Sep 05, 2018 at 04:36:28PM +0800, Shi Lei wrote:
This patch adds wrapper macros around nla_nest*/nla_put* which apply to virNetlinkNewLink and virNetDevSetVfConfig and virNetDevVPortProfileOpSetLink.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetdev.c | 54 ++++++-------- src/util/virnetdevvportprofile.c | 117 ++++++++++--------------------- src/util/virnetlink.c | 74 +++++++------------ src/util/virnetlink.h | 52 ++++++++++++++ 4 files changed, 134 insertions(+), 163 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 8eac419..892a147 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1657,11 +1657,11 @@ virNetDevSetVfConfig(const char *ifname, int vf, { int rc = -1; char macstr[VIR_MAC_STRING_BUFLEN]; - struct nlmsghdr *resp = NULL; struct nlmsgerr *err; unsigned int recvbuflen = 0; - struct nl_msg *nl_msg; struct nlattr *vfinfolist, *vfinfo; + VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL; + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
changes related to AUTO{PTR,FREE} should completely separate, even from the series itself.
Okay.
struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC, .ifi_index = -1, @@ -1676,47 +1676,41 @@ virNetDevSetVfConfig(const char *ifname, int vf, return rc; }
- if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; - - if (ifname && - nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
as I mentioned in my reply to the previous patch, I'd first replace ocurrences like ^these with nla_put_string for consistency and also making reviewer's life easier.
Okay. Since the changes of virNetDevSetVfConfig and virNetDevVPortProfileOpSetLink are unrelated with this series, I'll take them out and post them as another series.
- goto buffer_too_small; - + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); + return rc; + }
- if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST))) - goto buffer_too_small; + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname);
- if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO))) - goto buffer_too_small; + NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST); + NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO);
if (macaddr) { struct ifla_vf_mac ifla_vf_mac = { - .vf = vf, - .mac = { 0, }, + .vf = vf, + .mac = { 0, }, };
virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
- if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC, + sizeof(ifla_vf_mac), &ifla_vf_mac); }
if (vlanid >= 0) { struct ifla_vf_vlan ifla_vf_vlan = { - .vf = vf, - .vlan = vlanid, - .qos = 0, + .vf = vf, + .vlan = vlanid, + .qos = 0, };
- if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), - &ifla_vf_vlan) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN, + sizeof(ifla_vf_vlan), &ifla_vf_vlan); }
- nla_nest_end(nl_msg, vfinfo); - nla_nest_end(nl_msg, vfinfolist); + NETLINK_MSG_NEST_END(nl_msg, vfinfo); + NETLINK_MSG_NEST_END(nl_msg, vfinfolist);
if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) @@ -1767,20 +1761,12 @@ virNetDevSetVfConfig(const char *ifname, int vf, ifname, vf, macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)", vlanid, rc < 0 ? "Fail" : "Success"); - - nlmsg_free(nl_msg); - VIR_FREE(resp); return rc;
malformed_resp: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); goto cleanup; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - goto cleanup; }
static int diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index 3ebf757..8574571 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -641,8 +641,6 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, int32_t vf, uint8_t op) { - int rc = -1; - struct nlmsghdr *resp = NULL; struct nlmsgerr *err; struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC, @@ -651,12 +649,14 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, unsigned int recvbuflen = 0; int src_pid = 0; uint32_t dst_pid = 0; - struct nl_msg *nl_msg; struct nlattr *vfports = NULL, *vfport; char macStr[VIR_MAC_STRING_BUFLEN]; char hostUUIDStr[VIR_UUID_STRING_BUFLEN]; char instanceUUIDStr[VIR_UUID_STRING_BUFLEN]; const char *opName; + int vfport_type = IFLA_PORT_SELF; + VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL; + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
again the VIR_AUTO{PTR,FREE} stuff...
Okay.
switch (op) { case PORT_REQUEST_PREASSOCIATE: @@ -691,24 +691,21 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST); if (!nl_msg) { virReportOOMError(); - return rc; + return -1; }
- if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); + return -1; + }
- if (ifname && - nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname);
if (macaddr || vlanid >= 0) { struct nlattr *vfinfolist, *vfinfo;
- if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST))) - goto buffer_too_small; - - if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO))) - goto buffer_too_small; + NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST); + NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO);
if (macaddr) { struct ifla_vf_mac ifla_vf_mac = { @@ -718,9 +715,8 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
- if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC, + sizeof(ifla_vf_mac), &ifla_vf_mac); }
if (vlanid >= 0) { @@ -730,77 +726,49 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, .qos = 0, };
- if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), - &ifla_vf_vlan) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN, + sizeof(ifla_vf_vlan), &ifla_vf_vlan); }
- nla_nest_end(nl_msg, vfinfo); - nla_nest_end(nl_msg, vfinfolist); - } - - if (vf == PORT_SELF_VF && nltarget_kernel) { - if (!(vfport = nla_nest_start(nl_msg, IFLA_PORT_SELF))) - goto buffer_too_small; - } else { - if (!(vfports = nla_nest_start(nl_msg, IFLA_VF_PORTS))) - goto buffer_too_small; - - /* begin nesting vfports */ - if (!(vfport = nla_nest_start(nl_msg, IFLA_VF_PORT))) - goto buffer_too_small; - }
at this point it got quite difficult to keep track of all the replacements, so I'd suggest making the replacements one at a time, first one macro, then another, etc.
Sorry for the trouble. As I say above, I'll take those unrelated code out of this series, except for virNetlinkNewLink.
...
/** diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 8163a81..2e64570 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -48,6 +48,58 @@ struct nlmsghdr;
# endif /* __linux__ */
+ +# define NETLINK_MSG_NEST_START(msg, container, attrtype) \ +do { \ + container = nla_nest_start(msg, attrtype); \ + if (!container) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("nla_nest_start error [" #attrtype "]")); \ + return -1; \ + } \ +} while(0) + +# define NETLINK_MSG_NEST_END(msg, container) \ +do { nla_nest_end(msg, container); } while(0) + +/* + * When data is an rvalue, compiler will report error as below: + * "the address of [...] will always evaluate as 'true' [-Werror=address]" + * Add a dummy(as an lvalue) to solve it.
Okay, let's try rewording this in a more compact manner below...
+ */ +# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ +do { \
how about: /* @data may not always be a pointer type, therefore making compilers complain * about the check below, so let's use an intermediary pointer type */
Okay. It's clear.
+ const void *dummy = data; \
let's rename @dummy to dataptr to make it more explicit
Okay.
+ if (dummy && nla_put(msg, attrtype, datalen, data) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("nla_put error [" #attrtype "]")); \ + return -1; \ + } \ +} while(0) + +# define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \ +do { \ + const void *dummy = str; \ + if (dummy && nla_put_string(msg, attrtype, str) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("nla_put_string error [" #attrtype "]: '%s'"), \ + str); \
I don't think that changing the error is necessary, all of the variants essentially call nla_put and that one either fails with -EINVAL (not our case, since we always provide a valid datalen) or -ENOMEM, so the original error stating that the buffer wasn't large enough is sufficient.
Okay. Then I think that the code could be like below: if (dummy && nla_put_string(msg, attrtype, str) < 0) { \ virReportError(VIR_ERR_NO_MEMORY, \ _("nla_put_string error [" #attrtype "]: '%s'"), \ str); \ return -ENOMEM; \ } \ But we should ensure that the return-value of those functions which call nla_put* would accord with their declaration. I find that many of the functions in util/virnetdev.c and util/virnetdev.c always return -1 when they fail, whatever the error. For virNetlinkDumpLink/virNetlinkDelLink, their notes include stuff like below: /* Returns 0 on success, -1 on fatal error. */ So I would do some work on these declaration...
+ return -1; \ + } \ +} while(0)
I'm not entirely sold on these MSG_PUT_X macros, but I don't have a reasonable argument against either, so let's leave them in. Alternatively, we could have
#define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \ NETLINK_MSG_PUT(msg, attrtype, strlen(str) + 1, str)
and that would make things more clean, afterall that is what all the nla_put_foo will do anyway...
Erik
I understand. Most of the nla_put_foo actually use nla_put simply in current implementation. But I have some other ideas on this point: The authors of the libnl provide many nla_put_foo/nla_get_foo functions for various types (e.g. s8/u8/s16/u16/.../string/msecs/...). Perhaps they are more than convenient helper-functions. I suppose they might want users to use them as far as possible if the users know the attribute type actually, because they would get more information about this attribute payload. There's a chance that in future they could do some extra work in nla_put_foo/nla_get_foo according to the exact type, such as type-check or optimization ... So I think it's useful to provide these NETLINK_MSG_PUT_[type] which just wrap nla_put_[type]. Thanks for your patience. Have a nice day! :-) Shi Lei
+ +# define NETLINK_MSG_PUT_U32PTR(msg, attrtype, ptr) \ +do { \ + const void *dummy = ptr; \ + if (dummy && nla_put_u32(msg, attrtype, *ptr) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("nla_put_u32 error [" #attrtype "]: '%u'"), \ + *ptr); \ + return -1; \ + } \ +} while(0) + + int virNetlinkStartup(void); void virNetlinkShutdown(void);
-- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
...
+ if (dummy && nla_put(msg, attrtype, datalen, data) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("nla_put error [" #attrtype "]")); \ + return -1; \ + } \ +} while(0) + +# define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \ +do { \ + const void *dummy = str; \ + if (dummy && nla_put_string(msg, attrtype, str) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("nla_put_string error [" #attrtype "]: '%s'"), \ + str); \
I don't think that changing the error is necessary, all of the variants essentially call nla_put and that one either fails with -EINVAL (not our case, since we always provide a valid datalen) or -ENOMEM, so the original error stating that the buffer wasn't large enough is sufficient.
Okay. Then I think that the code could be like below:
if (dummy && nla_put_string(msg, attrtype, str) < 0) { \ virReportError(VIR_ERR_NO_MEMORY, \ _("nla_put_string error [" #attrtype "]: '%s'"), \ str); \ return -ENOMEM; \
VIR_ERR_NO_MEMORY is for allocation errors, this is not an allocation error, it's simply that didn't allocate a large enough msg to fit all the payload. In fact, this would return: "out of memory: nla_put_string error..." but the daemon would continue running happily. This really is an internal error. I would also like to drop the per-variant specific error messages to avoid any redundancy. One thing that would help when debugging for sure would be to add a VIR_DEBUG at the beginning of virNetlinkNewLink in the previous patch, that would IMHO help more.
} \
But we should ensure that the return-value of those functions which call nla_put* would accord with their declaration. I find that many of the functions in util/virnetdev.c and util/virnetdev.c always return -1 when they fail, whatever the error. For virNetlinkDumpLink/virNetlinkDelLink, their notes include stuff like below: /* Returns 0 on success, -1 on fatal error. */
So I would do some work on these declaration...
+ return -1; \ + } \ +} while(0)
I'm not entirely sold on these MSG_PUT_X macros, but I don't have a reasonable argument against either, so let's leave them in. Alternatively, we could have
#define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \ NETLINK_MSG_PUT(msg, attrtype, strlen(str) + 1, str)
and that would make things more clean, afterall that is what all the nla_put_foo will do anyway...
Erik
I understand. Most of the nla_put_foo actually use nla_put simply in current implementation.
But I have some other ideas on this point: The authors of the libnl provide many nla_put_foo/nla_get_foo functions for various types (e.g. s8/u8/s16/u16/.../string/msecs/...). Perhaps they are more than convenient helper-functions. I suppose they might want users to use them as far as possible if the users know the attribute type actually, because they would get more information about this attribute payload. There's a chance that in future they could do some extra work in nla_put_foo/nla_get_foo according to the exact type, such as type-check or optimization ...
So I think it's useful to provide these NETLINK_MSG_PUT_[type] which just wrap nla_put_[type].
Well, I truly don't want to end up having a NETLINK_MSG_PUT_[type] macro for every possible int length and signedness there is in libnl, I'd like to believe that NETLINK_MSG_PUT_INT with explicit size requirement should be enough, besides, users calling into these nla_put_uintX_t variants already know the correct size. Another thing is, that whatever these variants do, should be possible with plain nla_put with a little overhead if necessary unless libnl explicitly deprecates/discourages the usage of it. The reason why I'm trying to keep the number of macros we'll introduce reasonably small, ideally linking one to another is to prevent redundancy (the error code path) as well as preserve maintainability, otherwise this might just eventually explode into our faces. Erik
Thanks for your patience. Have a nice day! :-)
Shi Lei
+ +# define NETLINK_MSG_PUT_U32PTR(msg, attrtype, ptr) \ +do { \ + const void *dummy = ptr; \ + if (dummy && nla_put_u32(msg, attrtype, *ptr) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("nla_put_u32 error [" #attrtype "]: '%u'"), \ + *ptr); \ + return -1; \ + } \ +} while(0) + + int virNetlinkStartup(void); void virNetlinkShutdown(void);
-- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 2018-09-06 at 21:27, Erik Skultety wrote:
...
+ if (dummy && nla_put(msg, attrtype, datalen, data) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("nla_put error [" #attrtype "]")); \ + return -1; \ + } \ +} while(0) + +# define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \ +do { \ + const void *dummy = str; \ + if (dummy && nla_put_string(msg, attrtype, str) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("nla_put_string error [" #attrtype "]: '%s'"), \ + str); \
I don't think that changing the error is necessary, all of the variants essentially call nla_put and that one either fails with -EINVAL (not our case, since we always provide a valid datalen) or -ENOMEM, so the original error stating that the buffer wasn't large enough is sufficient.
Okay. Then I think that the code could be like below:
if (dummy && nla_put_string(msg, attrtype, str) < 0) { \ virReportError(VIR_ERR_NO_MEMORY, \ _("nla_put_string error [" #attrtype "]: '%s'"), \ str); \ return -ENOMEM; \
VIR_ERR_NO_MEMORY is for allocation errors, this is not an allocation error, it's simply that didn't allocate a large enough msg to fit all the payload. In fact, this would return: "out of memory: nla_put_string error..." but the daemon would continue running happily. This really is an internal error. I would also like to drop the per-variant specific error messages to avoid any redundancy. One thing that would help when debugging for sure would be to add a VIR_DEBUG at the beginning of virNetlinkNewLink in the previous patch, that would IMHO help more.
Okay. I see.
} \
But we should ensure that the return-value of those functions which call nla_put* would accord with their declaration. I find that many of the functions in util/virnetdev.c and util/virnetdev.c always return -1 when they fail, whatever the error. For virNetlinkDumpLink/virNetlinkDelLink, their notes include stuff like below: /* Returns 0 on success, -1 on fatal error. */
So I would do some work on these declaration...
+ return -1; \ + } \ +} while(0)
I'm not entirely sold on these MSG_PUT_X macros, but I don't have a reasonable argument against either, so let's leave them in. Alternatively, we could have
#define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \ NETLINK_MSG_PUT(msg, attrtype, strlen(str) + 1, str)
and that would make things more clean, afterall that is what all the nla_put_foo will do anyway...
Erik
I understand. Most of the nla_put_foo actually use nla_put simply in current implementation.
But I have some other ideas on this point: The authors of the libnl provide many nla_put_foo/nla_get_foo functions for various types (e.g. s8/u8/s16/u16/.../string/msecs/...). Perhaps they are more than convenient helper-functions. I suppose they might want users to use them as far as possible if the users know the attribute type actually, because they would get more information about this attribute payload. There's a chance that in future they could do some extra work in nla_put_foo/nla_get_foo according to the exact type, such as type-check or optimization ...
So I think it's useful to provide these NETLINK_MSG_PUT_[type] which just wrap nla_put_[type].
Well, I truly don't want to end up having a NETLINK_MSG_PUT_[type] macro for every possible int length and signedness there is in libnl, I'd like to believe that NETLINK_MSG_PUT_INT with explicit size requirement should be enough, besides, users calling into these nla_put_uintX_t variants already know the correct size. Another thing is, that whatever these variants do, should be possible with plain nla_put with a little overhead if necessary unless libnl explicitly deprecates/discourages the usage of it. The reason why I'm trying to keep the number of macros we'll introduce reasonably small, ideally linking one to another is to prevent redundancy (the error code path) as well as preserve maintainability, otherwise this might just eventually explode into our faces.
Okay. If there's no need to consider the things I mentioned, then I would like to return to the macro you written in v1. Then the only macro NETLINK_MSG_PUT with explicit size would deal with all types (includes string uint int ...). Yes, it's clear enough. Shi Lei
Erik
Thanks for your patience. Have a nice day! :-)
Shi Lei
+ +# define NETLINK_MSG_PUT_U32PTR(msg, attrtype, ptr) \ +do { \ + const void *dummy = ptr; \ + if (dummy && nla_put_u32(msg, attrtype, *ptr) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("nla_put_u32 error [" #attrtype "]: '%u'"), \ + *ptr); \ + return -1; \ + } \ +} while(0) + + int virNetlinkStartup(void); void virNetlinkShutdown(void);
-- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
This patch simplifies virNetDevBridgeCreate and virNetDevMacVLanCreate by using virNetlinkNewLink. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetdevbridge.c | 78 +++++--------------------- src/util/virnetdevmacvlan.c | 109 +++++------------------------------- 2 files changed, 27 insertions(+), 160 deletions(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index bc377b5..beef345 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -416,78 +416,26 @@ int virNetDevBridgeCreate(const char *brname) { /* use a netlink RTM_NEWLINK message to create the bridge */ - const char *type = "bridge"; - struct nlmsgerr *err; - struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC }; - unsigned int recvbuflen; - struct nlattr *linkinfo; - VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL; - VIR_AUTOFREE(struct nlmsghdr *) resp = NULL; + int error = 0; + virNetlinkNewLinkData data = { + .macvlan_mode = NULL, + .mac = NULL, + }; - 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 (nla_put(nl_msg, IFLA_IFNAME, strlen(brname)+1, brname) < 0) - goto buffer_too_small; - if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO))) - goto buffer_too_small; - if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0) - goto buffer_too_small; - nla_nest_end(nl_msg, linkinfo); - - if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, - NETLINK_ROUTE, 0) < 0) { - return -1; - } - - if (recvbuflen < 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) { + if (virNetlinkNewLink(brname, "bridge", NULL, &data, &error) < 0) { # if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR) - if (err->error == -EOPNOTSUPP) { - /* fallback to ioctl if netlink doesn't support creating - * bridges - */ - return virNetDevBridgeCreateWithIoctl(brname); - } -# endif - - virReportSystemError(-err->error, - _("error creating bridge interface %s"), - brname); - return -1; + if (error == -EOPNOTSUPP) { + /* fallback to ioctl if netlink doesn't support creating bridges */ + return virNetDevBridgeCreateWithIoctl(brname); } - break; +# endif + virReportSystemError(-error, _("error creating bridge interface %s"), + brname); - case NLMSG_DONE: - break; - default: - goto malformed_resp; + return -1; } 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; } diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 2035b1f..1372a1a 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -307,113 +307,32 @@ virNetDevMacVLanCreate(const char *ifname, uint32_t macvlan_mode, int *retry) { - int rc = -1; - struct nlmsgerr *err; - struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC }; - int ifindex; - unsigned int recvbuflen; - struct nl_msg *nl_msg; - struct nlattr *linkinfo, *info_data; - char macstr[VIR_MAC_STRING_BUFLEN]; - VIR_AUTOFREE(struct nlmsghdr *) resp = NULL; - - if (virNetDevGetIndex(srcdev, &ifindex) < 0) - return -1; + int error = 0; + int ifindex = 0; + virNetlinkNewLinkData data = { + .macvlan_mode = &macvlan_mode, + .mac = macaddress, + }; *retry = 0; - nl_msg = nlmsg_alloc_simple(RTM_NEWLINK, - NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); - if (!nl_msg) { - virReportOOMError(); + if (virNetDevGetIndex(srcdev, &ifindex) < 0) return -1; - } - - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; - - if (nla_put_u32(nl_msg, IFLA_LINK, ifindex) < 0) - goto buffer_too_small; - - if (nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, macaddress) < 0) - goto buffer_too_small; - - if (ifname && - nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) - goto buffer_too_small; - - if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO))) - goto buffer_too_small; - - if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0) - goto buffer_too_small; - - if (macvlan_mode > 0) { - if (!(info_data = nla_nest_start(nl_msg, IFLA_INFO_DATA))) - goto buffer_too_small; - - if (nla_put(nl_msg, IFLA_MACVLAN_MODE, sizeof(macvlan_mode), - &macvlan_mode) < 0) - goto buffer_too_small; - - nla_nest_end(nl_msg, info_data); - } - - nla_nest_end(nl_msg, linkinfo); - if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, - NETLINK_ROUTE, 0) < 0) { - goto cleanup; - } - - if (recvbuflen < 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; - - switch (err->error) { - - case 0: - break; - - case -EEXIST: + if (virNetlinkNewLink(ifname, type, &ifindex, &data, &error) < 0) { + char macstr[VIR_MAC_STRING_BUFLEN]; + if (error == -EEXIST) *retry = 1; - goto cleanup; - - default: - virReportSystemError(-err->error, + else + virReportSystemError(-error, _("error creating %s interface %s@%s (%s)"), type, ifname, srcdev, virMacAddrFormat(macaddress, macstr)); - goto cleanup; - } - break; - - case NLMSG_DONE: - break; - default: - goto malformed_resp; + return -1; } - rc = 0; - cleanup: - nlmsg_free(nl_msg); - return rc; - - malformed_resp: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed netlink response message")); - goto cleanup; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - goto cleanup; + return 0; } /** -- 2.17.1
This patch replaces virNetDevPutExtraHeader with nlmsg_append directly. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetdev.c | 44 ++++++++++++-------------------------------- 1 file changed, 12 insertions(+), 32 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 892a147..a7bc8e0 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3155,28 +3155,6 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, # if HAVE_DECL_DEVLINK_CMD_ESWITCH_GET -/** - * virNetDevPutExtraHeader - * reserve and prepare room for an extra header - * This function sets to zero the room that is required to put the extra - * header after the initial Netlink header. This function also increases - * the nlmsg_len field. - * - * @nlh: pointer to Netlink header - * @size: size of the extra header that we want to put - * - * Returns pointer to the start of the extended header - */ -static void * -virNetDevPutExtraHeader(struct nlmsghdr *nlh, - size_t size) -{ - char *ptr = (char *)nlh + nlh->nlmsg_len; - size_t len = NLMSG_ALIGN(size); - nlh->nlmsg_len += len; - return ptr; -} - /** * virNetDevGetFamilyId: @@ -3191,7 +3169,11 @@ virNetDevGetFamilyId(const char *family_name) { struct nl_msg *nl_msg = NULL; struct nlmsghdr *resp = NULL; - struct genlmsghdr* gmsgh = NULL; + struct genlmsghdr gmsgh = { + .cmd = CTRL_CMD_GETFAMILY, + .version = DEVLINK_GENL_VERSION, + .reserved = 0, + }; struct nlattr *tb[CTRL_ATTR_MAX + 1] = {NULL, }; unsigned int recvbuflen; uint32_t family_id = 0; @@ -3202,12 +3184,9 @@ virNetDevGetFamilyId(const char *family_name) goto cleanup; } - if (!(gmsgh = virNetDevPutExtraHeader(nlmsg_hdr(nl_msg), sizeof(struct genlmsghdr)))) + if (nlmsg_append(nl_msg, &gmsgh, sizeof(gmsgh), NLMSG_ALIGNTO) < 0) goto cleanup; - gmsgh->cmd = CTRL_CMD_GETFAMILY; - gmsgh->version = DEVLINK_GENL_VERSION; - if (nla_put_string(nl_msg, CTRL_ATTR_FAMILY_NAME, family_name) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("allocated netlink buffer is too small")); @@ -3254,7 +3233,11 @@ virNetDevSwitchdevFeature(const char *ifname, unsigned int recvbuflen; struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {NULL, }; virPCIDevicePtr pci_device_ptr = NULL; - struct genlmsghdr* gmsgh = NULL; + struct genlmsghdr gmsgh = { + .cmd = DEVLINK_CMD_ESWITCH_GET, + .version = DEVLINK_GENL_VERSION, + .reserved = 0, + }; const char *pci_name; char *pfname = NULL; int is_vf = -1; @@ -3284,12 +3267,9 @@ virNetDevSwitchdevFeature(const char *ifname, goto cleanup; } - if (!(gmsgh = virNetDevPutExtraHeader(nlmsg_hdr(nl_msg), sizeof(struct genlmsghdr)))) + if (nlmsg_append(nl_msg, &gmsgh, sizeof(gmsgh), NLMSG_ALIGNTO) < 0) goto cleanup; - gmsgh->cmd = DEVLINK_CMD_ESWITCH_GET; - gmsgh->version = DEVLINK_GENL_VERSION; - pci_name = virPCIDeviceGetName(pci_device_ptr); if (nla_put(nl_msg, DEVLINK_ATTR_BUS_NAME, strlen("pci")+1, "pci") < 0 || -- 2.17.1
On Wed, Sep 05, 2018 at 04:36:30PM +0800, Shi Lei wrote:
This patch replaces virNetDevPutExtraHeader with nlmsg_append directly.
This patch is unrelated to the series and thus should become a standalone patch. Erik
On 2018-09-05 at 23:02, Erik Skultety wrote:
On Wed, Sep 05, 2018 at 04:36:30PM +0800, Shi Lei wrote:
This patch replaces virNetDevPutExtraHeader with nlmsg_append directly.
This patch is unrelated to the series and thus should become a standalone patch.
Erik
Okay. I take it out of the series. Thanks, Shi Lei
participants (2)
-
Erik Skultety -
Shi Lei