[libvirt] [PATCHv3 0/3] Add virNetlinkNewLink helper for simplifying virNetDev*Create
v2 here: https://www.redhat.com/archives/libvir-list/2018-September/msg00131.html Diff from V2: This series remove all unrelated changes. Those changes would be standalone patches. Shi Lei (3): util: netlink: Introduce virNetlinkNewLink helper util: netlink: Add wrapper macros to make virNetlinkNewLink more readable util: netlink: Using virNetlinkNewLink to simplify virNetDev*Create src/libvirt_private.syms | 1 + src/util/virnetdevbridge.c | 73 +++--------------------- src/util/virnetdevmacvlan.c | 110 +++++------------------------------- src/util/virnetlink.c | 104 ++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 43 ++++++++++++++ 5 files changed, 172 insertions(+), 159 deletions(-) -- 2.17.1
This patch introduces 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 | 117 +++++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 13 +++++ 3 files changed, 131 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0fc5314..a0d229a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2446,6 +2446,7 @@ virNetlinkEventServiceStop; virNetlinkEventServiceStopAll; virNetlinkGetErrorCode; virNetlinkGetNeighbor; +virNetlinkNewLink; virNetlinkShutdown; virNetlinkStartup; diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 8f06446..d53cc73 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -488,6 +488,123 @@ virNetlinkDumpLink(const char *ifname, int ifindex, } +/** + * virNetlinkNewLink: + * + * @ifname: name of the link + * @type: the type of the device, i.e. "bridge", "macvtap", "macvlan" + * @extra_args: the extra args for creating the netlink interface + * @error: netlink error code + * + * A generic wrapper to create a network link. + * + * Returns 0 on success, < 0 on fatal error. + */ +int +virNetlinkNewLink(const char *ifname, + const char *type, + virNetlinkNewLinkDataPtr extra_args, + 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 (!ifname || !type) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("neither of name and type can be NULL")); + return -EINVAL; + } + + nl_msg = nlmsg_alloc_simple(RTM_NEWLINK, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); + if (!nl_msg) { + virReportOOMError(); + return -ENOMEM; + } + + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 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")) && + 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, *extra_args->macvlan_mode) < 0) + goto buffer_too_small; + + nla_nest_end(nl_msg, infodata); + } + + nla_nest_end(nl_msg, linkinfo); + + if (extra_args) { + if (extra_args->ifindex && + nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0) + goto buffer_too_small; + + if (extra_args->mac && + nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac) < 0) + goto buffer_too_small; + } + + 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 -EBADMSG; + + buffer_too_small: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + return -ENOMEM; +} + + /** * virNetlinkDelLink: * diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 1e1e616..09bab08 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 int *ifindex; /* The index for the 'link' device */ + 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, + virNetlinkNewLinkDataPtr data, + int *error); + typedef int (*virNetlinkDelLinkFallback)(const char *ifname); int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback); -- 2.17.1
On Fri, Sep 07, 2018 at 03:17:24PM +0800, Shi Lei wrote:
This patch introduces 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 | 117 +++++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 13 +++++ 3 files changed, 131 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0fc5314..a0d229a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2446,6 +2446,7 @@ virNetlinkEventServiceStop; virNetlinkEventServiceStopAll; virNetlinkGetErrorCode; virNetlinkGetNeighbor; +virNetlinkNewLink; virNetlinkShutdown; virNetlinkStartup;
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 8f06446..d53cc73 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -488,6 +488,123 @@ virNetlinkDumpLink(const char *ifname, int ifindex, }
+/** + * virNetlinkNewLink: + * + * @ifname: name of the link + * @type: the type of the device, i.e. "bridge", "macvtap", "macvlan" + * @extra_args: the extra args for creating the netlink interface + * @error: netlink error code + * + * A generic wrapper to create a network link. + * + * Returns 0 on success, < 0 on fatal error.
-1 on error, no need for errno, we mostly use that only for system errors and these will most likely (most often) be internal errors, additionally, you were not consistent with the returns, sometimes you returned a genuine errno and sometimes -1, it should either be errno at all times or -1.
+ */ +int +virNetlinkNewLink(const char *ifname, + const char *type, + virNetlinkNewLinkDataPtr extra_args, + 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;
maybe worth a VIR_DEBUG too.
+ + if (!ifname || !type) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("neither of name and type can be NULL"));
_("both interface name and type must be non-NULL"); OR _("either iterface name or type is missing")
+ return -EINVAL;
return -1;
+ } + + nl_msg = nlmsg_alloc_simple(RTM_NEWLINK, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); + if (!nl_msg) { + virReportOOMError(); + return -ENOMEM; + } + + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) + goto buffer_too_small; + + if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
A tiny nitpick ^here...since we settled down on having NETLINK_MSG_PUT wrapper macro only, then I think we should not use nla_put_string, even though we're going to replace it in the next patch, simply for consistency, IOW the replacement should be nla_put for NETLINK_MSG_PUT.
+ 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)
same as above...
+ goto buffer_too_small; + + 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, *extra_args->macvlan_mode) < 0)
here too...
+ goto buffer_too_small; + + nla_nest_end(nl_msg, infodata); + } + + nla_nest_end(nl_msg, linkinfo); + + if (extra_args) { + if (extra_args->ifindex && + nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
and here as well....
+ goto buffer_too_small; + + if (extra_args->mac && + nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac) < 0) + goto buffer_too_small; + } + + 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 -EBADMSG;
return -1
+ + buffer_too_small: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + return -ENOMEM;
return -1
+} + + /** * virNetlinkDelLink: * diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 1e1e616..09bab08 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 int *ifindex; /* The index for the 'link' device */ + 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, + virNetlinkNewLinkDataPtr data, + int *error);
I'd suggest using also ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNUL(2);
+ typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
Would you agree with squashing the following in before merging? (this was btw failing on mingw, so I added a symbol for that too) Erik diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index bc377b5921..ed2db27799 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -416,78 +416,23 @@ 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; - 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, &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); - } + if (error == -EOPNOTSUPP) { + /* fallback to ioctl if netlink doesn't support creating bridges */ + return virNetDevBridgeCreateWithIoctl(brname); + } # endif - - virReportSystemError(-err->error, - _("error creating bridge interface %s"), + if (error < 0) + virReportSystemError(-error, _("error creating bridge interface %s"), brname); - return -1; - } - break; - 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 2035b1f021..a66ab59952 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -307,113 +307,33 @@ 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: + data.ifindex = &ifindex; + if (virNetlinkNewLink(ifname, type, &data, &error) < 0) { + char macstr[VIR_MAC_STRING_BUFLEN]; + if (error == -EEXIST) *retry = 1; - goto cleanup; - - default: - virReportSystemError(-err->error, + else if (error < 0) + 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; } /**
On 09/10/2018 10:38 AM, Erik Skultety wrote:
On Fri, Sep 07, 2018 at 03:17:24PM +0800, Shi Lei wrote:
This patch introduces 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 | 117 +++++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 13 +++++ 3 files changed, 131 insertions(+)
I was looking too while Erik posted this... I have many of the same comments, but a couple more...
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0fc5314..a0d229a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2446,6 +2446,7 @@ virNetlinkEventServiceStop; virNetlinkEventServiceStopAll; virNetlinkGetErrorCode; virNetlinkGetNeighbor; +virNetlinkNewLink; virNetlinkShutdown; virNetlinkStartup;
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 8f06446..d53cc73 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -488,6 +488,123 @@ virNetlinkDumpLink(const char *ifname, int ifindex, }
+/** + * virNetlinkNewLink: + * + * @ifname: name of the link + * @type: the type of the device, i.e. "bridge", "macvtap", "macvlan" + * @extra_args: the extra args for creating the netlink interface + * @error: netlink error code + * + * A generic wrapper to create a network link. + * + * Returns 0 on success, < 0 on fatal error.
-1 on error, no need for errno, we mostly use that only for system errors and these will most likely (most often) be internal errors, additionally, you were not consistent with the returns, sometimes you returned a genuine errno and sometimes -1, it should either be errno at all times or -1.
I would say: * Returns 0 on success, -1 on error. Additionally, if the @error is * non-zero, then the failure occurred during virNetlinkCommand, but * no error message generated leaving it up to the caller to handle * the condition.
+ */ +int +virNetlinkNewLink(const char *ifname, + const char *type, + virNetlinkNewLinkDataPtr extra_args, + 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;
maybe worth a VIR_DEBUG too.
+ + if (!ifname || !type) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("neither of name and type can be NULL"));
_("both interface name and type must be non-NULL");
OR
_("either iterface name or type is missing")
+ return -EINVAL;
return -1;
+ } + + nl_msg = nlmsg_alloc_simple(RTM_NEWLINK, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); + if (!nl_msg) { + virReportOOMError(); + return -ENOMEM; + } + + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) + goto buffer_too_small; + + if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
A tiny nitpick ^here...since we settled down on having NETLINK_MSG_PUT wrapper macro only, then I think we should not use nla_put_string, even though we're going to replace it in the next patch, simply for consistency, IOW the replacement should be nla_put for NETLINK_MSG_PUT.
+ 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)
same as above...
+ goto buffer_too_small; + + if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && + extra_args && + extra_args->macvlan_mode && + *extra_args->macvlan_mode > 0) {
Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
+ if (!(infodata = nla_nest_start(nl_msg, IFLA_INFO_DATA))) + goto buffer_too_small; + + if (nla_put_u32(nl_msg, IFLA_MACVLAN_MODE, *extra_args->macvlan_mode) < 0)
here too...
+ goto buffer_too_small; + + nla_nest_end(nl_msg, infodata); + } + + nla_nest_end(nl_msg, linkinfo); + + if (extra_args) { + if (extra_args->ifindex && + nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
and here as well....
Similarly @ifindex doesn't seem to need to be a "const int *"
+ goto buffer_too_small; + + if (extra_args->mac && + nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac) < 0) + goto buffer_too_small; + } + + 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 -EBADMSG;
return -1
+ + buffer_too_small: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + return -ENOMEM;
return -1
+} + + /** * virNetlinkDelLink: * diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 1e1e616..09bab08 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 int *ifindex; /* The index for the 'link' device */ + 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, + virNetlinkNewLinkDataPtr data, + int *error);
I'd suggest using also ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNUL(2);
FWIW: Those ONLY help if NULL is passed, but do not help if say ifname or type were NULL. Furthermore, they cause a coverity error because the values are being checked. John
+ typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
Would you agree with squashing the following in before merging? (this was btw failing on mingw, so I added a symbol for that too)
Erik
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index bc377b5921..ed2db27799 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -416,78 +416,23 @@ 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;
- 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, &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); - } + if (error == -EOPNOTSUPP) { + /* fallback to ioctl if netlink doesn't support creating bridges */ + return virNetDevBridgeCreateWithIoctl(brname); + } # endif - - virReportSystemError(-err->error, - _("error creating bridge interface %s"), + if (error < 0) + virReportSystemError(-error, _("error creating bridge interface %s"), brname); - return -1; - } - break;
- 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 2035b1f021..a66ab59952 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -307,113 +307,33 @@ 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: + data.ifindex = &ifindex; + if (virNetlinkNewLink(ifname, type, &data, &error) < 0) { + char macstr[VIR_MAC_STRING_BUFLEN]; + if (error == -EEXIST) *retry = 1; - goto cleanup; - - default: - virReportSystemError(-err->error, + else if (error < 0) + 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; }
/**
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 10, 2018 at 11:03:04AM -0400, John Ferlan wrote:
On 09/10/2018 10:38 AM, Erik Skultety wrote:
On Fri, Sep 07, 2018 at 03:17:24PM +0800, Shi Lei wrote:
This patch introduces 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 | 117 +++++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 13 +++++ 3 files changed, 131 insertions(+)
I was looking too while Erik posted this... I have many of the same comments, but a couple more...
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0fc5314..a0d229a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2446,6 +2446,7 @@ virNetlinkEventServiceStop; virNetlinkEventServiceStopAll; virNetlinkGetErrorCode; virNetlinkGetNeighbor; +virNetlinkNewLink; virNetlinkShutdown; virNetlinkStartup;
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 8f06446..d53cc73 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -488,6 +488,123 @@ virNetlinkDumpLink(const char *ifname, int ifindex, }
+/** + * virNetlinkNewLink: + * + * @ifname: name of the link + * @type: the type of the device, i.e. "bridge", "macvtap", "macvlan" + * @extra_args: the extra args for creating the netlink interface + * @error: netlink error code + * + * A generic wrapper to create a network link. + * + * Returns 0 on success, < 0 on fatal error.
-1 on error, no need for errno, we mostly use that only for system errors and these will most likely (most often) be internal errors, additionally, you were not consistent with the returns, sometimes you returned a genuine errno and sometimes -1, it should either be errno at all times or -1.
I would say:
* Returns 0 on success, -1 on error. Additionally, if the @error is * non-zero, then the failure occurred during virNetlinkCommand, but * no error message generated leaving it up to the caller to handle * the condition.
"is generated" I guess? Anyway, I agree.
+ */ +int +virNetlinkNewLink(const char *ifname, + const char *type, + virNetlinkNewLinkDataPtr extra_args, + 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;
maybe worth a VIR_DEBUG too.
+ + if (!ifname || !type) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("neither of name and type can be NULL"));
_("both interface name and type must be non-NULL");
OR
_("either iterface name or type is missing")
+ return -EINVAL;
return -1;
+ } + + nl_msg = nlmsg_alloc_simple(RTM_NEWLINK, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); + if (!nl_msg) { + virReportOOMError(); + return -ENOMEM; + } + + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) + goto buffer_too_small; + + if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
A tiny nitpick ^here...since we settled down on having NETLINK_MSG_PUT wrapper macro only, then I think we should not use nla_put_string, even though we're going to replace it in the next patch, simply for consistency, IOW the replacement should be nla_put for NETLINK_MSG_PUT.
+ 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)
same as above...
+ goto buffer_too_small; + + if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && + extra_args && + extra_args->macvlan_mode && + *extra_args->macvlan_mode > 0) {
Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
+ if (!(infodata = nla_nest_start(nl_msg, IFLA_INFO_DATA))) + goto buffer_too_small; + + if (nla_put_u32(nl_msg, IFLA_MACVLAN_MODE, *extra_args->macvlan_mode) < 0)
here too...
+ goto buffer_too_small; + + nla_nest_end(nl_msg, infodata); + } + + nla_nest_end(nl_msg, linkinfo); + + if (extra_args) { + if (extra_args->ifindex && + nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
and here as well....
Similarly @ifindex doesn't seem to need to be a "const int *"
Are you referring to the const correctness or the fact it's a pointer? If it's the former, then I don't see a problem, if it's the latter, then I simply wanted a deterministic way of telling that an argument is set, in case values 0, -1, etc. had some meaning.
+ goto buffer_too_small; + + if (extra_args->mac && + nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac) < 0) + goto buffer_too_small; + } + + 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 -EBADMSG;
return -1
+ + buffer_too_small: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + return -ENOMEM;
return -1
+} + + /** * virNetlinkDelLink: * diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 1e1e616..09bab08 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 int *ifindex; /* The index for the 'link' device */ + 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, + virNetlinkNewLinkDataPtr data, + int *error);
I'd suggest using also ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNUL(2);
FWIW: Those ONLY help if NULL is passed, but do not help if say ifname
Right, for everything else, we have the NULL check at the beginning.
or type were NULL. Furthermore, they cause a coverity error because the values are being checked.
Huh, I didn't know ^that, note taken, thanks. Erik
John
+ typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
Would you agree with squashing the following in before merging? (this was btw failing on mingw, so I added a symbol for that too)
Erik
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index bc377b5921..ed2db27799 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -416,78 +416,23 @@ 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;
- 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, &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); - } + if (error == -EOPNOTSUPP) { + /* fallback to ioctl if netlink doesn't support creating bridges */ + return virNetDevBridgeCreateWithIoctl(brname); + } # endif - - virReportSystemError(-err->error, - _("error creating bridge interface %s"), + if (error < 0) + virReportSystemError(-error, _("error creating bridge interface %s"), brname); - return -1; - } - break;
- 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 2035b1f021..a66ab59952 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -307,113 +307,33 @@ 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: + data.ifindex = &ifindex; + if (virNetlinkNewLink(ifname, type, &data, &error) < 0) { + char macstr[VIR_MAC_STRING_BUFLEN]; + if (error == -EEXIST) *retry = 1; - goto cleanup; - - default: - virReportSystemError(-err->error, + else if (error < 0) + 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; }
/**
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[...]
I would say:
* Returns 0 on success, -1 on error. Additionally, if the @error is * non-zero, then the failure occurred during virNetlinkCommand, but * no error message generated leaving it up to the caller to handle * the condition.
"is generated" I guess?
Anyway, I agree.
right... fingers don't always comply with mind ;-)
[...]
+ if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && + extra_args && + extra_args->macvlan_mode && + *extra_args->macvlan_mode > 0) {
Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
+ if (!(infodata = nla_nest_start(nl_msg, IFLA_INFO_DATA))) + goto buffer_too_small; + + if (nla_put_u32(nl_msg, IFLA_MACVLAN_MODE, *extra_args->macvlan_mode) < 0)
here too...
+ goto buffer_too_small; + + nla_nest_end(nl_msg, infodata); + } + + nla_nest_end(nl_msg, linkinfo); + + if (extra_args) { + if (extra_args->ifindex && + nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
and here as well....
Similarly @ifindex doesn't seem to need to be a "const int *"
Are you referring to the const correctness or the fact it's a pointer? If it's the former, then I don't see a problem, if it's the latter, then I simply wanted a deterministic way of telling that an argument is set, in case values 0, -1, etc. had some meaning.
latter that it's a pointer. The way it looks to me is that the value could be changed in the function, but I understand the point of 0 (not supplied) vs. NULL... So fair enough reason to keep as const int *... John
On 2018-09-11 at 01:17, John Ferlan wrote:
[...]
I would say:
* Returns 0 on success, -1 on error. Additionally, if the @error is * non-zero, then the failure occurred during virNetlinkCommand, but * no error message generated leaving it up to the caller to handle * the condition.
"is generated" I guess?
Anyway, I agree.
right... fingers don't always comply with mind ;-)
[...]
+ if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && + extra_args && + extra_args->macvlan_mode && + *extra_args->macvlan_mode > 0) {
Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
+ if (!(infodata = nla_nest_start(nl_msg, IFLA_INFO_DATA))) + goto buffer_too_small; + + if (nla_put_u32(nl_msg, IFLA_MACVLAN_MODE, *extra_args->macvlan_mode) < 0)
here too...
+ goto buffer_too_small; + + nla_nest_end(nl_msg, infodata); + } + + nla_nest_end(nl_msg, linkinfo); + + if (extra_args) { + if (extra_args->ifindex && + nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
and here as well....
Similarly @ifindex doesn't seem to need to be a "const int *"
Are you referring to the const correctness or the fact it's a pointer? If it's the former, then I don't see a problem, if it's the latter, then I simply wanted a deterministic way of telling that an argument is set, in case values 0, -1, etc. had some meaning.
latter that it's a pointer. The way it looks to me is that the value could be changed in the function, but I understand the point of 0 (not supplied) vs. NULL...
So fair enough reason to keep as const int *...
John
Thank you for your comments, John. :-) Shi Lei
On Mon, Sep 10, 2018 at 01:17:43PM -0400, John Ferlan wrote:
[...]
I would say:
* Returns 0 on success, -1 on error. Additionally, if the @error is * non-zero, then the failure occurred during virNetlinkCommand, but * no error message generated leaving it up to the caller to handle * the condition.
"is generated" I guess?
Anyway, I agree.
right... fingers don't always comply with mind ;-)
[...]
+ if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && + extra_args && + extra_args->macvlan_mode && + *extra_args->macvlan_mode > 0) {
Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
+ if (!(infodata = nla_nest_start(nl_msg, IFLA_INFO_DATA))) + goto buffer_too_small; + + if (nla_put_u32(nl_msg, IFLA_MACVLAN_MODE, *extra_args->macvlan_mode) < 0)
here too...
+ goto buffer_too_small; + + nla_nest_end(nl_msg, infodata); + } + + nla_nest_end(nl_msg, linkinfo); + + if (extra_args) { + if (extra_args->ifindex && + nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
and here as well....
Similarly @ifindex doesn't seem to need to be a "const int *"
Are you referring to the const correctness or the fact it's a pointer? If it's the former, then I don't see a problem, if it's the latter, then I simply wanted a deterministic way of telling that an argument is set, in case values 0, -1, etc. had some meaning.
latter that it's a pointer. The way it looks to me is that the value could be changed in the function, but I understand the point of 0 (not supplied) vs. NULL...
So fair enough reason to keep as const int *...
Okay, so do you have any other comments or shall I proceed with adding the function commentary you suggested in your response + removing the ATTRIBUTE_NONNULLs and merging the patch set with my proposed adjustment diffs? Erik
On 09/11/2018 02:29 AM, Erik Skultety wrote:
On Mon, Sep 10, 2018 at 01:17:43PM -0400, John Ferlan wrote:
[...]
I would say:
* Returns 0 on success, -1 on error. Additionally, if the @error is * non-zero, then the failure occurred during virNetlinkCommand, but * no error message generated leaving it up to the caller to handle * the condition.
"is generated" I guess?
Anyway, I agree.
right... fingers don't always comply with mind ;-)
[...]
+ if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && + extra_args && + extra_args->macvlan_mode && + *extra_args->macvlan_mode > 0) {
Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
+ if (!(infodata = nla_nest_start(nl_msg, IFLA_INFO_DATA))) + goto buffer_too_small; + + if (nla_put_u32(nl_msg, IFLA_MACVLAN_MODE, *extra_args->macvlan_mode) < 0)
here too...
+ goto buffer_too_small; + + nla_nest_end(nl_msg, infodata); + } + + nla_nest_end(nl_msg, linkinfo); + + if (extra_args) { + if (extra_args->ifindex && + nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
and here as well....
Similarly @ifindex doesn't seem to need to be a "const int *"
Are you referring to the const correctness or the fact it's a pointer? If it's the former, then I don't see a problem, if it's the latter, then I simply wanted a deterministic way of telling that an argument is set, in case values 0, -1, etc. had some meaning.
latter that it's a pointer. The way it looks to me is that the value could be changed in the function, but I understand the point of 0 (not supplied) vs. NULL...
So fair enough reason to keep as const int *...
Okay, so do you have any other comments or shall I proceed with adding the function commentary you suggested in your response + removing the ATTRIBUTE_NONNULLs and merging the patch set with my proposed adjustment diffs?
No other comments from me. John
On 2018-09-10 at 22:38, Erik Skultety wrote:
On Fri, Sep 07, 2018 at 03:17:24PM +0800, Shi Lei wrote:
This patch introduces 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 | 117 +++++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 13 +++++ 3 files changed, 131 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0fc5314..a0d229a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2446,6 +2446,7 @@ virNetlinkEventServiceStop; virNetlinkEventServiceStopAll; virNetlinkGetErrorCode; virNetlinkGetNeighbor; +virNetlinkNewLink; virNetlinkShutdown; virNetlinkStartup;
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 8f06446..d53cc73 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -488,6 +488,123 @@ virNetlinkDumpLink(const char *ifname, int ifindex, }
+/** + * virNetlinkNewLink: + * + * @ifname: name of the link + * @type: the type of the device, i.e. "bridge", "macvtap", "macvlan" + * @extra_args: the extra args for creating the netlink interface + * @error: netlink error code + * + * A generic wrapper to create a network link. + * + * Returns 0 on success, < 0 on fatal error.
-1 on error, no need for errno, we mostly use that only for system errors and these will most likely (most often) be internal errors, additionally, you were not consistent with the returns, sometimes you returned a genuine errno and sometimes -1, it should either be errno at all times or -1.
Okay.
+ */ +int +virNetlinkNewLink(const char *ifname, + const char *type, + virNetlinkNewLinkDataPtr extra_args, + 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;
maybe worth a VIR_DEBUG too.
Yes.
+ + if (!ifname || !type) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("neither of name and type can be NULL"));
_("both interface name and type must be non-NULL");
OR
_("either iterface name or type is missing")
Okay. But ^ might be interface :-)
+ return -EINVAL;
return -1;
+ } + + nl_msg = nlmsg_alloc_simple(RTM_NEWLINK, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); + if (!nl_msg) { + virReportOOMError(); + return -ENOMEM; + } + + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) + goto buffer_too_small; + + if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
A tiny nitpick ^here...since we settled down on having NETLINK_MSG_PUT wrapper macro only, then I think we should not use nla_put_string, even though we're going to replace it in the next patch, simply for consistency, IOW the replacement should be nla_put for NETLINK_MSG_PUT.
Yes.
+ 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)
same as above...
Okay.
+ goto buffer_too_small; + + 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, *extra_args->macvlan_mode) < 0)
here too...
Okay.
+ goto buffer_too_small; + + nla_nest_end(nl_msg, infodata); + } + + nla_nest_end(nl_msg, linkinfo); + + if (extra_args) { + if (extra_args->ifindex && + nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
and here as well....
Okay.
+ goto buffer_too_small; + + if (extra_args->mac && + nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac) < 0) + goto buffer_too_small; + } + + 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 -EBADMSG;
return -1
Okay.
+ + buffer_too_small: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + return -ENOMEM;
return -1
Okay.
+} + + /** * virNetlinkDelLink: * diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 1e1e616..09bab08 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 int *ifindex; /* The index for the 'link' device */ + 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, + virNetlinkNewLinkDataPtr data, + int *error);
I'd suggest using also ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNUL(2);
Okay.
+ typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
Would you agree with squashing the following in before merging? (this was btw failing on mingw, so I added a symbol for that too)
Erik
Okay. I agree. Thanks, Shi Lei
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index bc377b5921..ed2db27799 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -416,78 +416,23 @@ 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;
- 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, &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); - } + if (error == -EOPNOTSUPP) { + /* fallback to ioctl if netlink doesn't support creating bridges */ + return virNetDevBridgeCreateWithIoctl(brname); + } # endif - - virReportSystemError(-err->error, - _("error creating bridge interface %s"), + if (error < 0) + virReportSystemError(-error, _("error creating bridge interface %s"), brname); - return -1; - } - break;
- 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 2035b1f021..a66ab59952 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -307,113 +307,33 @@ 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: + data.ifindex = &ifindex; + if (virNetlinkNewLink(ifname, type, &data, &error) < 0) { + char macstr[VIR_MAC_STRING_BUFLEN]; + if (error == -EEXIST) *retry = 1; - goto cleanup; - - default: - virReportSystemError(-err->error, + else if (error < 0) + 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; }
/**
This patch adds wrapper macros around nla_nest_[start|end] and nla_put which can make virNetlinkNewLink more readable. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetlink.c | 43 +++++++++++++++---------------------------- src/util/virnetlink.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 28 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index d53cc73..99ad003 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -529,41 +529,33 @@ virNetlinkNewLink(const char *ifname, return -ENOMEM; } - 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", + _("out of memory: nlmsg_append")); + return -ENOMEM; + } - if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname)+1), ifname); - if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO))) - goto buffer_too_small; + NETLINK_MSG_NEST_START(nl_msg, linkinfo, IFLA_LINKINFO); - if (type && nla_put_string(nl_msg, IFLA_INFO_KIND, type) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_INFO_KIND, (strlen(type)+1), type); 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, *extra_args->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(nl_msg, IFLA_MACVLAN_MODE, + sizeof(uint32_t), extra_args->macvlan_mode); + NETLINK_MSG_NEST_END(nl_msg, infodata); } - nla_nest_end(nl_msg, linkinfo); + NETLINK_MSG_NEST_END(nl_msg, linkinfo); if (extra_args) { - if (extra_args->ifindex && - nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0) - goto buffer_too_small; - - if (extra_args->mac && - nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_LINK, sizeof(uint32_t), extra_args->ifindex); + NETLINK_MSG_PUT(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac); } if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 0) @@ -597,11 +589,6 @@ virNetlinkNewLink(const char *ifname, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return -EBADMSG; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - return -ENOMEM; } diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 09bab08..6f59d6e 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -48,6 +48,36 @@ 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", \ + _("out of memory: nla_nest_start [" #attrtype "]")); \ + return -ENOMEM; \ + } \ +} while(0) + +# define NETLINK_MSG_NEST_END(msg, container) \ +do { nla_nest_end(msg, container); } while(0) + +/* + * @data may be like &foo sometimes, therefore making compilers complain + * about the check below, so let's use an intermediary pointer type + * "the address of [...] will always evaluate as 'true' [-Werror=address]" + */ +# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ +do { \ + const void *dataptr = data; \ + if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("out of memory: nla_put [" #attrtype "]")); \ + return -ENOMEM; \ + } \ +} while(0) + + int virNetlinkStartup(void); void virNetlinkShutdown(void); -- 2.17.1
On Fri, Sep 07, 2018 at 03:17:25PM +0800, Shi Lei wrote:
This patch adds wrapper macros around nla_nest_[start|end] and nla_put which can make virNetlinkNewLink more readable.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetlink.c | 43 +++++++++++++++---------------------------- src/util/virnetlink.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 28 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index d53cc73..99ad003 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -529,41 +529,33 @@ virNetlinkNewLink(const char *ifname, return -ENOMEM; }
- 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", + _("out of memory: nlmsg_append"));
Let's drop this hunk, see my comment at the end of the mail.
+ return -ENOMEM;
return -1;
+ }
- if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname)+1), ifname);
- if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO))) - goto buffer_too_small; + NETLINK_MSG_NEST_START(nl_msg, linkinfo, IFLA_LINKINFO);
- if (type && nla_put_string(nl_msg, IFLA_INFO_KIND, type) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_INFO_KIND, (strlen(type)+1), type);
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, *extra_args->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(nl_msg, IFLA_MACVLAN_MODE, + sizeof(uint32_t), extra_args->macvlan_mode); + NETLINK_MSG_NEST_END(nl_msg, infodata); }
- nla_nest_end(nl_msg, linkinfo); + NETLINK_MSG_NEST_END(nl_msg, linkinfo);
if (extra_args) { - if (extra_args->ifindex && - nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0) - goto buffer_too_small; - - if (extra_args->mac && - nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_LINK, sizeof(uint32_t), extra_args->ifindex);
^needs to be line wrapped...
+ NETLINK_MSG_PUT(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac); }
if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 0) @@ -597,11 +589,6 @@ virNetlinkNewLink(const char *ifname, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return -EBADMSG; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - return -ENOMEM; }
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 09bab08..6f59d6e 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -48,6 +48,36 @@ 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", \ + _("out of memory: nla_nest_start [" #attrtype "]")); \
So I checked and buffer_too_small label is very common among util/virnet*.c modules and that's where we want to use these macros, so let's simply use goto buffer_too_small and we can then figure out, how to optimize that, i.e. have a single place where we define the error, so that we wouldn't have to repeat _("allocated netlink buffer too small").
+ return -ENOMEM; \
return -1
+ } \ +} while(0) + +# define NETLINK_MSG_NEST_END(msg, container) \ +do { nla_nest_end(msg, container); } while(0) + +/* + * @data may be like &foo sometimes, therefore making compilers complain + * about the check below, so let's use an intermediary pointer type + * "the address of [...] will always evaluate as 'true' [-Werror=address]"
we need to use an intermediary pointer to @data as compilers may sometimes complain about @data not being a pointer type: "error: the address of 'foo' will always evaluate as 'true' [-Werror=address]"
+ */ +# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ +do { \ + const void *dataptr = data; \ + if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("out of memory: nla_put [" #attrtype "]")); \
see my comment a few paragraphs above...
+ return -ENOMEM; \
return -1; Erik I'm suggesting to squash the following (let me know if you agree): diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 4e63edcfe5..eb03df4bda 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -537,7 +537,6 @@ virNetlinkNewLink(const char *ifname, NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname); NETLINK_MSG_NEST_START(nl_msg, linkinfo, IFLA_LINKINFO); - NETLINK_MSG_PUT(nl_msg, IFLA_INFO_KIND, (strlen(type) + 1), type); if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && @@ -553,7 +552,8 @@ virNetlinkNewLink(const char *ifname, NETLINK_MSG_NEST_END(nl_msg, linkinfo); if (extra_args) { - NETLINK_MSG_PUT(nl_msg, IFLA_LINK, sizeof(uint32_t), extra_args->ifindex); + NETLINK_MSG_PUT(nl_msg, IFLA_LINK, + sizeof(uint32_t), extra_args->ifindex); NETLINK_MSG_PUT(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac); } @@ -588,6 +588,11 @@ 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; } diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 8bc33d0cd3..552a3a8fe1 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -48,33 +48,26 @@ 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", \ - _("out of memory: nla_nest_start [" #attrtype "]")); \ - return -ENOMEM; \ - } \ + if (!container) \ + goto buffer_too_small; \ } while(0) # define NETLINK_MSG_NEST_END(msg, container) \ do { nla_nest_end(msg, container); } while(0) /* - * @data may be like &foo sometimes, therefore making compilers complain - * about the check below, so let's use an intermediary pointer type - * "the address of [...] will always evaluate as 'true' [-Werror=address]" + * we need to use an intermediary pointer to @data as compilers may sometimes + * complain about @data not being a pointer type: + * error: the address of 'foo' will always evaluate as 'true' [-Werror=address] */ # define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ do { \ const void *dataptr = data; \ - if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) { \ - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ - _("out of memory: nla_put [" #attrtype "]")); \ - return -ENOMEM; \ - } \ + if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) \ + goto buffer_too_small; \ } while(0)
On 2018-09-10 at 22:39, Erik Skultety wrote:
On Fri, Sep 07, 2018 at 03:17:25PM +0800, Shi Lei wrote:
This patch adds wrapper macros around nla_nest_[start|end] and nla_put which can make virNetlinkNewLink more readable.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetlink.c | 43 +++++++++++++++---------------------------- src/util/virnetlink.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 28 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index d53cc73..99ad003 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -529,41 +529,33 @@ virNetlinkNewLink(const char *ifname, return -ENOMEM; }
- 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", + _("out of memory: nlmsg_append"));
Let's drop this hunk, see my comment at the end of the mail.
Okay. I have read it.
+ return -ENOMEM;
return -1;
Okay.
+ }
- if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname)+1), ifname);
- if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO))) - goto buffer_too_small; + NETLINK_MSG_NEST_START(nl_msg, linkinfo, IFLA_LINKINFO);
- if (type && nla_put_string(nl_msg, IFLA_INFO_KIND, type) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_INFO_KIND, (strlen(type)+1), type);
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, *extra_args->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(nl_msg, IFLA_MACVLAN_MODE, + sizeof(uint32_t), extra_args->macvlan_mode); + NETLINK_MSG_NEST_END(nl_msg, infodata); }
- nla_nest_end(nl_msg, linkinfo); + NETLINK_MSG_NEST_END(nl_msg, linkinfo);
if (extra_args) { - if (extra_args->ifindex && - nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0) - goto buffer_too_small; - - if (extra_args->mac && - nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_LINK, sizeof(uint32_t), extra_args->ifindex);
^needs to be line wrapped...
Yes.
+ NETLINK_MSG_PUT(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac); }
if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 0) @@ -597,11 +589,6 @@ virNetlinkNewLink(const char *ifname, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return -EBADMSG; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - return -ENOMEM; }
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 09bab08..6f59d6e 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -48,6 +48,36 @@ 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", \ + _("out of memory: nla_nest_start [" #attrtype "]")); \
So I checked and buffer_too_small label is very common among util/virnet*.c modules and that's where we want to use these macros, so let's simply use goto buffer_too_small and we can then figure out, how to optimize that, i.e. have a single place where we define the error, so that we wouldn't have to repeat _("allocated netlink buffer too small").
Okay.
+ return -ENOMEM; \
return -1
Okay.
+ } \ +} while(0) + +# define NETLINK_MSG_NEST_END(msg, container) \ +do { nla_nest_end(msg, container); } while(0) + +/* + * @data may be like &foo sometimes, therefore making compilers complain + * about the check below, so let's use an intermediary pointer type + * "the address of [...] will always evaluate as 'true' [-Werror=address]"
we need to use an intermediary pointer to @data as compilers may sometimes complain about @data not being a pointer type: "error: the address of 'foo' will always evaluate as 'true' [-Werror=address]"
Okay.
+ */ +# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ +do { \ + const void *dataptr = data; \ + if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("out of memory: nla_put [" #attrtype "]")); \
see my comment a few paragraphs above...
Okay.
+ return -ENOMEM; \
return -1;
Okay.
Erik
I'm suggesting to squash the following (let me know if you agree):
Okay. I agree.
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 4e63edcfe5..eb03df4bda 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -537,7 +537,6 @@ virNetlinkNewLink(const char *ifname, NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname);
NETLINK_MSG_NEST_START(nl_msg, linkinfo, IFLA_LINKINFO); - NETLINK_MSG_PUT(nl_msg, IFLA_INFO_KIND, (strlen(type) + 1), type);
if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && @@ -553,7 +552,8 @@ virNetlinkNewLink(const char *ifname, NETLINK_MSG_NEST_END(nl_msg, linkinfo);
if (extra_args) { - NETLINK_MSG_PUT(nl_msg, IFLA_LINK, sizeof(uint32_t), extra_args->ifindex); + NETLINK_MSG_PUT(nl_msg, IFLA_LINK, + sizeof(uint32_t), extra_args->ifindex); NETLINK_MSG_PUT(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac); }
@@ -588,6 +588,11 @@ 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; }
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 8bc33d0cd3..552a3a8fe1 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -48,33 +48,26 @@ 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", \ - _("out of memory: nla_nest_start [" #attrtype "]")); \ - return -ENOMEM; \ - } \ + if (!container) \ + goto buffer_too_small; \ } while(0)
# define NETLINK_MSG_NEST_END(msg, container) \ do { nla_nest_end(msg, container); } while(0)
/* - * @data may be like &foo sometimes, therefore making compilers complain - * about the check below, so let's use an intermediary pointer type - * "the address of [...] will always evaluate as 'true' [-Werror=address]" + * we need to use an intermediary pointer to @data as compilers may sometimes + * complain about @data not being a pointer type: + * error: the address of 'foo' will always evaluate as 'true' [-Werror=address] */ # define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ do { \ const void *dataptr = data; \ - if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) { \ - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ - _("out of memory: nla_put [" #attrtype "]")); \ - return -ENOMEM; \ - } \ + if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) \ + goto buffer_too_small; \ } while(0)
This patch simplifies virNetDevBridgeCreate and virNetDevMacVLanCreate by using virNetlinkNewLink. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetdevbridge.c | 73 +++--------------------- src/util/virnetdevmacvlan.c | 110 +++++------------------------------- 2 files changed, 24 insertions(+), 159 deletions(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index bc377b5..ed2db27 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -416,78 +416,23 @@ 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; - - 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; + int error = 0; - if (err->error < 0) { + if (virNetlinkNewLink(brname, "bridge", NULL, &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); - } + if (error == -EOPNOTSUPP) { + /* fallback to ioctl if netlink doesn't support creating bridges */ + return virNetDevBridgeCreateWithIoctl(brname); + } # endif - - virReportSystemError(-err->error, - _("error creating bridge interface %s"), + if (error < 0) + virReportSystemError(-error, _("error creating bridge interface %s"), brname); - return -1; - } - break; - 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..a66ab59 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -307,113 +307,33 @@ 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: + data.ifindex = &ifindex; + if (virNetlinkNewLink(ifname, type, &data, &error) < 0) { + char macstr[VIR_MAC_STRING_BUFLEN]; + if (error == -EEXIST) *retry = 1; - goto cleanup; - - default: - virReportSystemError(-err->error, + else if (error < 0) + 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
On Fri, Sep 07, 2018 at 03:17:26PM +0800, Shi Lei wrote:
This patch simplifies virNetDevBridgeCreate and virNetDevMacVLanCreate by using virNetlinkNewLink.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> ---
I'll make a tiny cosmetic change to the commit message before merging. Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 2018-09-10 at 22:39, Erik Skultety wrote:
On Fri, Sep 07, 2018 at 03:17:26PM +0800, Shi Lei wrote:
This patch simplifies virNetDevBridgeCreate and virNetDevMacVLanCreate by using virNetlinkNewLink.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> ---
I'll make a tiny cosmetic change to the commit message before merging. Reviewed-by: Erik Skultety <eskultet@redhat.com>
Thanks, Shi Lei
On Fri, Sep 07, 2018 at 03:17:23PM +0800, Shi Lei wrote:
v2 here: https://www.redhat.com/archives/libvir-list/2018-September/msg00131.html
Diff from V2: This series remove all unrelated changes. Those changes would be standalone patches.
Shi Lei (3): util: netlink: Introduce virNetlinkNewLink helper util: netlink: Add wrapper macros to make virNetlinkNewLink more readable util: netlink: Using virNetlinkNewLink to simplify virNetDev*Create
src/libvirt_private.syms | 1 + src/util/virnetdevbridge.c | 73 +++--------------------- src/util/virnetdevmacvlan.c | 110 +++++------------------------------- src/util/virnetlink.c | 104 ++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 43 ++++++++++++++ 5 files changed, 172 insertions(+), 159 deletions(-)
So I applied the changes as discussed and merged. Regards, Erik
On 2018-09-12 at 15:28, Erik Skultety wrote:
On Fri, Sep 07, 2018 at 03:17:23PM +0800, Shi Lei wrote:
v2 here: https://www.redhat.com/archives/libvir-list/2018-September/msg00131.html
Diff from V2: This series remove all unrelated changes. Those changes would be standalone patches.
Shi Lei (3): util: netlink: Introduce virNetlinkNewLink helper util: netlink: Add wrapper macros to make virNetlinkNewLink more readable util: netlink: Using virNetlinkNewLink to simplify virNetDev*Create
src/libvirt_private.syms | 1 + src/util/virnetdevbridge.c | 73 +++--------------------- src/util/virnetdevmacvlan.c | 110 +++++------------------------------- src/util/virnetlink.c | 104 ++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 43 ++++++++++++++ 5 files changed, 172 insertions(+), 159 deletions(-)
So I applied the changes as discussed and merged.
Regards, Erik
Okay. I have seen that when I git pull a moment ago. Thanks :-) Shi Lei
participants (3)
-
Erik Skultety -
John Ferlan -
Shi Lei