On Mon, Sep 10, 2018 at 11:03:04AM -0400, John Ferlan wrote:
>
>
> 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@xxxxxxxxxxxxxx>
> >> ---
> >> 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.
"is generated" I guess?
Anyway, I agree.
>
> >> + */
> >> +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 *"
Are you referring to the const correctness or the fact it's a pointer? If it's
the former, then I don't see a problem, if it's the latter, then I simply
wanted a deterministic way of telling that an argument is set, in case values
0, -1, etc. had some meaning.
>
> >> + 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
Right, for everything else, we have the NULL check at the beginning.
> or type were NULL. Furthermore, they cause a coverity error because the
> values are being checked.
Huh, I didn't know ^that, note taken, thanks.
Erik
>
> 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@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
