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