12:06 p.m.
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(a)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;
}
/**