Re: [libvirt] [PATCHv3 1/3] util: netlink: Introduce virNetlinkNewLink helper

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(a)massclouds.com&gt; >> --- >> 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list >