8:26 a.m.
On Wed, Sep 05, 2018 at 04:36:27PM +0800, Shi Lei wrote:
the subject should read:
util: netlink: Introduce virNetlinkNewLink helper
This patch introduces virNetlinkNewLink which wraps newlink code
using libnl.
... virNetlinkNewLink helper which wraps the common libnl/netlink code to
create a new link.
Signed-off-by: Shi Lei <shi_lei(a)massclouds.com>
---
src/libvirt_private.syms | 1 +
src/util/virnetlink.c | 120 +++++++++++++++++++++++++++++++++++++--
src/util/virnetlink.h | 13 +++++
3 files changed, 129 insertions(+), 5 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e2340d2..77c9b9e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2447,6 +2447,7 @@ virNetlinkEventServiceStop;
virNetlinkEventServiceStopAll;
virNetlinkGetErrorCode;
virNetlinkGetNeighbor;
+virNetlinkNewLink;
virNetlinkShutdown;
virNetlinkStartup;
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 8f06446..32aa62d 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -420,10 +420,8 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
goto buffer_too_small;
- if (ifname) {
- if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
- goto buffer_too_small;
- }
+ if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
+ goto buffer_too_small;
unrelated to this patch...this kind of change should be a separate patch
somewhere at the beginning of the series...
# ifdef RTEXT_FILTER_VF
/* if this filter exists in the kernel's netlink implementation,
@@ -488,6 +486,118 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
}
+/**
+ * virNetlinkNewLink:
+ *
+ * @ifname: Name of the link
+ * @type: The type of device, i.e., "bridge", "macvtap",
"macvlan"
s/Name/name
s/The/the
s/of device/of the device/
s/i.e.,/i.e.
+ * @ifindex: The index for the 'link' device
I think ^this one should be part of the extra args, e.g. bridge doesn't use it
+ * @data: The extra args for creating the netlink interface
actually extra_args is a better name for the argument :)
+ * @error: for retrieving error code
s/for retrieving error code/netlink error code
+ *
+ * Create a network "link" (aka interface aka device) with the given
+ * args. This works for many different types of network devices,
+ * including macvtap and bridges.
A generic wrapper to create a network link.
+ *
+ * Returns 0 on success, -1 on fatal error.
+ */
+int
+virNetlinkNewLink(const char *ifname,
+ const char *type,
+ const int *ifindex,
+ virNetlinkNewLinkDataPtr data,
+ int *error)
+{
+ struct nlmsgerr *err;
+ struct nlattr *linkinfo = NULL;
+ struct nlattr *infodata = NULL;
+ unsigned int buflen;
+ struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
+ VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
+ VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
+
+ *error = 0;
+
+ if (!data)
+ return -1;
Extra args should not be required, BridgeCreate doesn't make use of them. That
said, I think it's reasonable to make @ifname and @type mandatory and fail
whenever we don't get either of them:
if (!ifname && !type)
virReportError(...);
+
+ nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
+ NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
+ if (!nl_msg) {
+ virReportOOMError();
+ return -1;
+ }
+
+ if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
+ goto buffer_too_small;
+
+ if (ifindex && nla_put_u32(nl_msg, IFLA_LINK, *ifindex) < 0)
+ goto buffer_too_small;
+
+ if (data->mac && nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN,
data->mac) < 0)
+ goto buffer_too_small;
The order of the data in the msg payload doesn't matter, right? If that's true,
we could move everything related to @extra_args and *not* related to nested
containers right before actually calling into virNetlinkCommand. If the order
does actually matter, then you can disregard this comment.
+
+ if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
+ goto buffer_too_small;
+
+ if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
+ goto buffer_too_small;
+
+ if (type && nla_put_string(nl_msg, IFLA_INFO_KIND, type) < 0)
+ goto buffer_too_small;
+
+ if ((STREQ(type, "macvtap") || STREQ(type, "macvlan"))
&&
+ data->macvlan_mode && *data->macvlan_mode > 0) {
this check would get a bit uglier though:
if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) &&
extra_args &&
extra_args->macvlan_mode &&
*extra_args->macvlan_mode > 0)
+ if (!(infodata = nla_nest_start(nl_msg, IFLA_INFO_DATA)))
+ goto buffer_too_small;
+
+ if (nla_put_u32(nl_msg, IFLA_MACVLAN_MODE, *data->macvlan_mode) < 0)
+ goto buffer_too_small;
+
+ nla_nest_end(nl_msg, infodata);
+ }
+
+ nla_nest_end(nl_msg, linkinfo);
here you'd have:
if (extra_args) {
...
}
+
+ if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) <
0)
+ return -1;
+
+ if (buflen < NLMSG_LENGTH(0) || resp == NULL)
+ goto malformed_resp;
+
+ switch (resp->nlmsg_type) {
+ case NLMSG_ERROR:
+ err = (struct nlmsgerr *)NLMSG_DATA(resp);
+ if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
+ goto malformed_resp;
+
+ if (err->error < 0) {
+ *error = err->error;
+ return -1;
+ }
+ break;
+
+ case NLMSG_DONE:
+ break;
+
+ default:
+ goto malformed_resp;
+ }
+
+ return 0;
+
+ malformed_resp:
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("malformed netlink response message"));
+ return -1;
+
+ buffer_too_small:
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("allocated netlink buffer is too small"));
+ return -1;
+}
+
+
/**
* virNetlinkDelLink:
*
@@ -522,7 +632,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback
fallback)
if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
goto buffer_too_small;
- if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
+ if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
goto buffer_too_small;
...unrelated to this patch...
if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
index 1e1e616..8163a81 100644
--- a/src/util/virnetlink.h
+++ b/src/util/virnetlink.h
@@ -65,6 +65,19 @@ int virNetlinkDumpCommand(struct nl_msg *nl_msg,
unsigned int protocol, unsigned int groups,
void *opaque);
+typedef struct _virNetlinkNewLinkData virNetlinkNewLinkData;
+typedef virNetlinkNewLinkData *virNetlinkNewLinkDataPtr;
+struct _virNetlinkNewLinkData {
+ const virMacAddr *mac; /* The MAC address of the device */
+ const uint32_t *macvlan_mode; /* The mode of macvlan */
this would contain const int *ifindex too...
+};
+
+int virNetlinkNewLink(const char *ifname,
+ const char *type,
+ const int *ifindex,
+ virNetlinkNewLinkDataPtr data,
+ int *error);
+
typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
The patch looks good, except for the few nitpicks I mentioned.
Erik