[libvirt] [PATCHv4 0/2] Add virNetlinkNewLink helper for simplifying virNetDev*Create
v3 here: https://www.redhat.com/archives/libvir-list/2018-September/msg00233.html Diff from v3: This series make changes according to the comments from Erik and John. Shi Lei (2): util: netlink: Introduce virNetlinkNewLink helper to simplify virNetDev*Create util: netlink: Add wrapper macros to make virNetlinkNewLink more readable src/libvirt_private.syms | 1 + src/util/virnetdevbridge.c | 73 +++--------------------- src/util/virnetdevmacvlan.c | 110 +++++------------------------------ src/util/virnetlink.c | 111 ++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 36 ++++++++++++ 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. The helper simplifies virNetDevBridgeCreate and virNetDevMacVLanCreate by using virNetlinkNewLink. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/libvirt_private.syms | 1 + src/util/virnetdevbridge.c | 73 +++------------------ src/util/virnetdevmacvlan.c | 110 +++++--------------------------- src/util/virnetlink.c | 124 ++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 13 ++++ 5 files changed, 162 insertions(+), 159 deletions(-) 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/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; } /** diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 8f06446..815bbee 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -488,6 +488,130 @@ 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, -1 on error. Additionally, if the @error is + * non-zero, then the failure occurred during virNetlinkCommand, but + * no error message is 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; + + if (!ifname || !type) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("either interface name or type is missing")); + return -1; + } + + VIR_DEBUG("Create network link ifname=%s, type=%s", ifname, type); + + 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 (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 (type && nla_put(nl_msg, IFLA_INFO_KIND, strlen(type)+1, 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(nl_msg, IFLA_MACVLAN_MODE, sizeof(uint32_t), + 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(nl_msg, IFLA_LINK, sizeof(uint32_t), + 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 -1; + + buffer_too_small: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + 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); + typedef int (*virNetlinkDelLinkFallback)(const char *ifname); int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback); -- 2.17.1
On Tue, Sep 11, 2018 at 04:23:56PM +0800, Shi Lei wrote:
This patch introduces virNetlinkNewLink helper which wraps the common libnl/netlink code to create a new link. The helper simplifies virNetDevBridgeCreate and virNetDevMacVLanCreate by using virNetlinkNewLink.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> ---
For some reason this patch merged with (originally) patch 3. I already have the suggested changes in the previous revision applied on my local branch, but I'd like to wait for John if he doesn't have further comments, if not, then I'll proceed with v3 and the suggested changes. Erik
On 2018-09-11 at 17:16, Erik Skultety wrote:
On Tue, Sep 11, 2018 at 04:23:56PM +0800, Shi Lei wrote:
This patch introduces virNetlinkNewLink helper which wraps the common libnl/netlink code to create a new link. The helper simplifies virNetDevBridgeCreate and virNetDevMacVLanCreate by using virNetlinkNewLink.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> ---
For some reason this patch merged with (originally) patch 3. I already have the suggested changes in the previous revision applied on my local branch, but I'd like to wait for John if he doesn't have further comments, if not, then I'll proceed with v3 and the suggested changes.
Erik
Okay. Thanks. Shi Lei
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 | 35 +++++++++++------------------------ src/util/virnetlink.h | 23 +++++++++++++++++++++++ 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 815bbee..b424b1b 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -537,40 +537,27 @@ virNetlinkNewLink(const char *ifname, 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 (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO))) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname); - if (type && nla_put(nl_msg, IFLA_INFO_KIND, strlen(type)+1, type) < 0) - goto buffer_too_small; + 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")) && 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(nl_msg, IFLA_MACVLAN_MODE, sizeof(uint32_t), - 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(nl_msg, IFLA_LINK, sizeof(uint32_t), - 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) diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 09bab08..fd140f0 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -48,6 +48,29 @@ struct nlmsghdr; # endif /* __linux__ */ +# define NETLINK_MSG_NEST_START(msg, container, attrtype) \ +do { \ + container = nla_nest_start(msg, attrtype); \ + if (!container) \ + goto buffer_too_small; \ +} while(0) + +# define NETLINK_MSG_NEST_END(msg, container) \ +do { nla_nest_end(msg, container); } while(0) + +/* + * 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) \ + goto buffer_too_small; \ +} while(0) + + int virNetlinkStartup(void); void virNetlinkShutdown(void); -- 2.17.1
participants (2)
-
Erik Skultety -
Shi Lei