3:36 a.m.
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
3:36 a.m.
New subject: [libvirt] [PATCHv2 1/4] Introduce virNetlinkNewLink
This patch introduces virNetlinkNewLink which wraps newlink code
using libnl.
Signed-off-by: Shi Lei <shi_lei(a)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
8:26 a.m.
New subject: [libvirt] [PATCHv2 1/4] Introduce virNetlinkNewLink
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(a)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
11:21 p.m.
New subject: [libvirt] [PATCHv2 1/4] Introduce virNetlinkNewLink
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(a)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
3:36 a.m.
New subject: [libvirt] [PATCHv2 2/4] Add wrapper macros around nla_nest*/nla_put* to make code more readable
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(a)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
10:01 a.m.
New subject: [libvirt] [PATCHv2 2/4] Add wrapper macros around nla_nest*/nla_put* to make code more readable
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(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
3:09 a.m.
New subject: [libvirt] [PATCHv2 2/4] Add wrapper macros around nla_nest*/nla_put* to make code more readable
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(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
8:27 a.m.
New subject: [libvirt] [PATCHv2 2/4] Add wrapper macros around nla_nest*/nla_put* to make code more readable
...
>
>> + 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(a)redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:49 p.m.
New subject: [libvirt] [PATCHv2 2/4] Add wrapper macros around nla_nest*/nla_put* to make code more readable
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(a)redhat.com
> >> https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
3:36 a.m.
New subject: [libvirt] [PATCHv2 3/4] Using virNetlinkNewLink to simplify virNetDev*Create
This patch simplifies virNetDevBridgeCreate and virNetDevMacVLanCreate
by using virNetlinkNewLink.
Signed-off-by: Shi Lei <shi_lei(a)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
3:36 a.m.
New subject: [libvirt] [PATCHv2 4/4] Remove virNetDevPutExtraHeader and replace with nlmsg_append
This patch replaces virNetDevPutExtraHeader with nlmsg_append directly.
Signed-off-by: Shi Lei <shi_lei(a)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
10:02 a.m.
New subject: [libvirt] [PATCHv2 4/4] Remove virNetDevPutExtraHeader and replace with nlmsg_append
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
12:48 a.m.
New subject: [libvirt] [PATCHv2 4/4] Remove virNetDevPutExtraHeader and replace with nlmsg_append
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
2269
days inactive
2271
days old
12 comments
2 participants
participants (2)
-
Erik Skultety -
Shi Lei