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@xxxxxxxxxxxxxx>
>> ---
>> 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;
> }
>
> /**
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
