Re: [libvirt] [PATCH 2/4] virtnetdev: Add support functions for mac and portprofile associations on a hostdev

On 03/04/2012 10:15 PM, Roopa Prabhu wrote: > From: Roopa Prabhu <roprabhu(a)cisco.com&gt; > > This patch adds the following: > - functions to set and get vf configs > - Functions to replace and store vf configs (Only mac address is handled > today. > But the functions can be easily extended for vlans and other vf configs) > - function to dump link dev info (This is moved from virnetdevvportprofile.c) > > Signed-off-by: Roopa Prabhu <roprabhu(a)cisco.com&gt; > --- > src/util/virnetdev.c | 531 > ++++++++++++++++++++++++++++++++++++++++++++++++++ > src/util/virnetdev.h | 19 ++ > 2 files changed, 549 insertions(+), 1 deletions(-) (BTW, I never thought about doing it this way before, but I'm glad you added the function here in a separate patch from the patch that removes it from virnetdevvportprofile.c - that makes it easy to open the two patches side-by-side and verify that it really is moving the same code (well, mostly).) > > > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c > index 9d76d47..25f2155 100644 > --- a/src/util/virnetdev.c > +++ b/src/util/virnetdev.c > @@ -1127,8 +1127,497 @@ virNetDevGetPhysicalFunction(const char *ifname, char > **pfname) > > return ret; > } > -#else /* !__linux__ */ The functions here that use libnl need to be inside of #if defined(__linux__) && defined(HAVE_LIBNL) since there are linux platforms that don't have libnl, or don't have the proper LIBNL (RHEL5, in particular, still has libnl-1.0)

> > +static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = { > + [IFLA_VF_MAC] = { .type = NLA_UNSPEC, > + .maxlen = sizeof(struct ifla_vf_mac) }, > + [IFLA_VF_VLAN] = { .type = NLA_UNSPEC, > + .maxlen = sizeof(struct ifla_vf_vlan) }, > +}; > + > +/** > + * virNetDevLinkDump: > + * > + * @ifname: The name of the interface; only use if ifindex < 0 > + * @ifindex: The interface index; may be < 0 if ifname is given > + * @nltarget_kernel: whether to send the message to the kernel or another > + * process > + * @nlattr: pointer to a pointer of netlink attributes that will contain > + * the results > + * @recvbuf: Pointer to the buffer holding the returned netlink response > + * message; free it, once not needed anymore > + * @getPidFunc: Pointer to a function that will be invoked if the kernel > + * is not the target of the netlink message but it is to be > + * sent to another process. > + * > + * Get information about an interface given its name or index. > + * > + * Returns 0 on success, -1 on fatal error. > + */ > +int > +virNetDevLinkDump(const char *ifname, int ifindex, > + bool nltarget_kernel, struct nlattr **tb, > + unsigned char **recvbuf, > + uint32_t (*getPidFunc)(void)) > +{ > + int rc = 0; > + struct nlmsghdr *resp; > + struct nlmsgerr *err; > + struct ifinfomsg ifinfo = { > + .ifi_family = AF_UNSPEC, > + .ifi_index = ifindex > + }; > + unsigned int recvbuflen; > + uint32_t pid = 0; > + struct nl_msg *nl_msg; > + > + *recvbuf = NULL; > + > + if (ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0) > + return -1; > + > + ifinfo.ifi_index = ifindex; > + > + nl_msg = nlmsg_alloc_simple(RTM_GETLINK, NLM_F_REQUEST); > + if (!nl_msg) { > + virReportOOMError(); > + return -1; > + } > + > + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) > + goto buffer_too_small; > + > + if (ifindex < 0 && ifname) { > + if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) > + goto buffer_too_small; > + } Is this bit necessary any more? You've added code above that converts the ifname into an ifindex, and we've already returned if it wasn't successful.

> + > + if (!nltarget_kernel) { > + pid = getPidFunc(); > + if (pid == 0) { > + rc = -1; > + goto cleanup; > + } > + } > + > + if (virNetlinkCommand(nl_msg, recvbuf, &recvbuflen, pid) < 0) { > + rc = -1; > + goto cleanup; > + } > + > + if (recvbuflen < NLMSG_LENGTH(0) || *recvbuf == NULL) > + goto malformed_resp; > + > + resp = (struct nlmsghdr *)*recvbuf; > + > + 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) { > + virReportSystemError(-err->error, > + _("error dumping %s (%d) interface"), > + ifname, ifindex); > + rc = -1; > + } > + break; > + > + case GENL_ID_CTRL: > + case NLMSG_DONE: > + rc = nlmsg_parse(resp, sizeof(struct ifinfomsg), > + tb, IFLA_MAX, NULL); > + if (rc < 0) > + goto malformed_resp; > + break; > + > + default: > + goto malformed_resp; > + } > + > + if (rc != 0) > + VIR_FREE(*recvbuf); > + > +cleanup: > + nlmsg_free(nl_msg); > + > + return rc; > + > +malformed_resp: > + nlmsg_free(nl_msg); > + > + virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("malformed netlink response message")); > + VIR_FREE(*recvbuf); > + return -1; > + > +buffer_too_small: > + nlmsg_free(nl_msg); > + > + virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("allocated netlink buffer is too small")); > + return -1; > +} This is mostly just movement of existing code, so it's not appropriate to make the changes here, but this function should be refactored to 1) initialize rc = -1, then combine the three different return paths so that malformed_resp and buffer_too_small just log a message and goto cleanup. cleanup will then have cleanup: if (rc < 0) VIR_FREE(*recvbuf); nlmsg_free(nl_msg); return rc; Again, it's better to keep the code as it is in this patch (for this series even), and we can just send a separate patch for the cleanup later.

> + > +static int > +virNetDevSetVfConfig(const char *ifname, int ifindex, int vf, > + bool nltarget_kernel, const unsigned char *macaddr, > + int vlanid, uint32_t (*getPidFunc)(void)) > +{ > + int rc = -1; > + struct nlmsghdr *resp; > + struct nlmsgerr *err; > + unsigned char *recvbuf = NULL; > + unsigned int recvbuflen = 0; > + uint32_t pid = 0; > + struct nl_msg *nl_msg; > + struct ifinfomsg ifinfo = { > + .ifi_family = AF_UNSPEC, > + .ifi_index = ifindex > + }; > + > + nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST); > + if (!nl_msg) { > + virReportOOMError(); > + return rc; > + } > + > + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) > + goto buffer_too_small; > + > + if (ifname && > + nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) > + goto buffer_too_small; > + > + if (macaddr || vlanid >= 0) { > + struct nlattr *vfinfolist, *vfinfo; > + > + if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST))) > + goto buffer_too_small; > + > + if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO))) > + goto buffer_too_small; > + > + if (macaddr) { > + struct ifla_vf_mac ifla_vf_mac = { > + .vf = vf, > + .mac = { 0, }, Doesn't hurt anything, but it's also not necessary to initialize mac address is it? (since the next statement is a memcpy to set it)

> + }; > + > + memcpy(ifla_vf_mac.mac, macaddr, 6); This should use VIR_MAC_BUFLEN instead of 6.

> + > + if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), > + &ifla_vf_mac) < 0) > + goto buffer_too_small; > + } > + > + if (vlanid >= 0) { > + struct ifla_vf_vlan ifla_vf_vlan = { > + .vf = vf, > + .vlan = vlanid, > + .qos = 0, > + }; > + > + if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), > + &ifla_vf_vlan) < 0) > + goto buffer_too_small; > + } > + > + nla_nest_end(nl_msg, vfinfo); > + nla_nest_end(nl_msg, vfinfolist); > + } > + > + if (!nltarget_kernel) { > + pid = getPidFunc(); > + if (pid == 0) { > + rc = -1; > + goto err_exit; > + } > + } > + > + if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, pid) < 0) > + goto err_exit; > + > + if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL) > + goto malformed_resp; > + > + resp = (struct nlmsghdr *)recvbuf; > + > + 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) { > + virReportSystemError(-err->error, > + _("error during set vf mac of ifindex %d"), > + ifindex); It's also possible that this error would be the result of failing to set the vlan tag. We should probably turn "mac" into "%s", and have it set to one of "mac address", "vlanid", or "mac address / vlanid" according to what was passed in.

> + goto err_exit; > + } > + break; > + > + case NLMSG_DONE: > + break; > + > + default: > + goto malformed_resp; > + } > + > + rc = 0; > + > +err_exit: I prefer the label "cleanup" for return paths that are also used for successful returns. > + nlmsg_free(nl_msg); > + > + VIR_FREE(recvbuf); > + > + return rc; > + > +malformed_resp: > + nlmsg_free(nl_msg); > + > + virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("malformed netlink response message")); > + VIR_FREE(recvbuf); > + return rc; > + > +buffer_too_small: > + nlmsg_free(nl_msg); > + > + virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("allocated netlink buffer is too small")); > + return rc; > +} These two last labels can be reduced to: malformed_resp: virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); goto cleanup; buffer_too_small: virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", _("allocated netlink buffer is too small")); goto cleanup; That way if additional resource cleanup is added in the future, it only needs to be added in one place.

> + > +static int > +virNetDevParseVfConfig(struct nlattr **tb, int32_t vf, unsigned char *mac, > + int *vlanid) > +{ > + const char *msg = NULL; > + int rc = -1; > + > + if (tb[IFLA_VFINFO_LIST]) { > + struct ifla_vf_mac *vf_mac; > + struct ifla_vf_vlan *vf_vlan; > + struct nlattr *tb_vf_info = {NULL, }; > + struct nlattr *tb_vf[IFLA_VF_MAX+1]; > + int found = 0; > + int rem; > + > + nla_for_each_nested(tb_vf_info, tb[IFLA_VFINFO_LIST], rem) { > + if (nla_type(tb_vf_info) != IFLA_VF_INFO) > + continue; > + > + if (nla_parse_nested(tb_vf, IFLA_VF_MAX, tb_vf_info, > + ifla_vf_policy)) { > + msg = _("error parsing IFLA_VF_INFO"); > + goto err_exit; > + } > + > + if (tb[IFLA_VF_MAC]) { > + vf_mac = RTA_DATA(tb_vf[IFLA_VF_MAC]); > + if (vf_mac && vf_mac->vf == vf) { > + memcpy(mac, vf_mac->mac, VIR_MAC_BUFLEN); > + found = 1; > + } > + } > + > + if (tb[IFLA_VF_VLAN]) { > + vf_vlan = RTA_DATA(tb_vf[IFLA_VF_VLAN]); > + if (vf_vlan && vf_vlan->vf == vf) { > + *vlanid = vf_vlan->vlan; > + found = 1; > + } > + } > + if (found) { > + rc = 0; > + break; > + } > + } > + } > + > +err_exit: cleanup:, not err_exit: > + if (msg) > + virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", msg); Rather than doing this, just call virNetDevError at the point the error is encountered. Makes it easier to backtrack from error log to the actual source of the problem (since the log message will have a line number).

> + > + return rc; > +} > + > +static int > +virNetDevGetVfConfig(const char *ifname, int vf, unsigned char *mac, > + int *vlanid) > +{ > + int rc = -1; > + unsigned char *recvbuf = NULL; > + struct nlattr *tb[IFLA_MAX + 1] = {NULL, }; > + int ifindex = -1; > + > + rc = virNetDevLinkDump(ifname, ifindex, true, tb, &recvbuf, NULL); > + if (rc < 0) > + return rc; > + > + rc = virNetDevParseVfConfig(tb, vf, mac, vlanid); > + > + VIR_FREE(recvbuf); > + > + return rc; > +} > + > +static int > +virNetDevReplaceVfConfig(const char *pflinkdev, int vf, > + const unsigned char *macaddress, > + int vlanid, > + const char *stateDir) > +{ > + unsigned char oldmac[6]; > + int oldvlanid = -1; > + char *path = NULL; > + char macstr[VIR_MAC_STRING_BUFLEN]; > + int ifindex = -1; > + > + if (virNetDevGetVfConfig(pflinkdev, vf, oldmac, &oldvlanid) < 0) > + return -1; > + > + if (virAsprintf(&path, "%s/%s_vf%d", > + stateDir, pflinkdev, vf) < 0) { > + virReportOOMError(); > + return -1; > + } > + > + virMacAddrFormat(oldmac, macstr); > + if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY) < 0) { > + virReportSystemError(errno, _("Unable to preserve mac for pf = %s, > vf = %d"), pflinkdev, vf); break this into multiple lines to fit within 80 columns.

> + VIR_FREE(path); > + return -1; > + } > + > + VIR_FREE(path); > + > + return virNetDevSetVfConfig(pflinkdev, ifindex, vf, true, > + macaddress, vlanid, NULL); > +} > + > +static int > +virNetDevRestoreVfConfig(const char *pflinkdev, int vf, > + const char *stateDir) > +{ > + int rc = -1; > + char *macstr = NULL; > + char *path = NULL; > + unsigned char oldmac[6]; > + int vlanid = -1; > + int ifindex = -1; > + > + if (virAsprintf(&path, "%s/%s_vf%d", > + stateDir, pflinkdev, vf) < 0) { > + virReportOOMError(); > + return rc; > + } > + > + if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN, &macstr) < 0) { > + VIR_FREE(path); > + return rc; This could just goto cleanup instead of freeing path itself.

> + } > + > + if (virMacAddrParse(macstr, &oldmac[0]) != 0) { > + virNetDevError(VIR_ERR_INTERNAL_ERROR, > + _("Cannot parse MAC address from '%s'"), > + macstr); > + goto cleanup; > + } > + > + /*reset mac and remove file-ignore results*/ > + rc = virNetDevSetVfConfig(pflinkdev, ifindex, vf, true, > + oldmac, vlanid, NULL); > + ignore_value(unlink(path)); > + > +cleanup: > + VIR_FREE(path); > + VIR_FREE(macstr); > + > + return rc; > +} > + > +/** > + * virNetDevReplaceNetConfig: > + * @linkdev: name of the interface > + * @vf: vf index if linkdev is a pf > + * @macaddress: new MAC address for interface > + * @vlanid: new vlanid > + * @stateDir: directory to store old net config > + * > + * Returns 0 on success, -1 on failure > + * > + */ > +int > +virNetDevReplaceNetConfig(char *linkdev, int vf, > + const unsigned char *macaddress, int vlanid, > + char *stateDir) > +{ > + if (vf == -1) > + return virNetDevReplaceMacAddress(linkdev, macaddress, stateDir); > + else > + return virNetDevReplaceVfConfig(linkdev, vf, macaddress, vlanid, > + stateDir); > +} > + > +/** > + * virNetDevRestoreNetConfig: > + * @linkdev: name of the interface > + * @vf: vf index if linkdev is a pf > + * @stateDir: directory containing old net config > + * > + * Returns 0 on success, -errno on failure. > + * > + */ > +int > +virNetDevRestoreNetConfig(char *linkdev, int vf, char *stateDir) > +{ > + if (vf == -1) > + return virNetDevRestoreMacAddress(linkdev, stateDir); > + else > + return virNetDevRestoreVfConfig(linkdev, vf, stateDir); > +} > + > +/** > + * virNetDevGetVirtualFunctionInfo: > + * @vfname: name of the virtual function interface > + * @pfname: name of the physical function > + * @vf: vf index > + * > + * Returns 0 on success, -errno on failure. > + * > + */ > +int > +virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname, > + int *vf) > +{ > + char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL; > + int ret = -1; You should initialize *pfname = NULL;

> + > + if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) > + return ret; > + > + if (virNetDevSysfsDeviceFile(&pf_sysfs_path, *pfname, "device") < 0) { > + VIR_FREE(*pfname); > + return ret; > + } > + > + if (virNetDevSysfsDeviceFile(&vf_sysfs_path, vfname, "device") < 0) { > + VIR_FREE(*pfname); > + VIR_FREE(pf_sysfs_path); > + return ret; > + } > + > + ret = pciGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf); error handling can be consolidated by putting a cleanup label here, with: cleanup: if (ret < 0) VIR_FREE(*pfname); > + > + VIR_FREE(vf_sysfs_path); > + VIR_FREE(pf_sysfs_path); > + > + return ret; Then both of the failure cases above can directly goto cleanup without needing to free anything themselves.

> +} > +#else /* !__linux__ */ > int > virNetDevGetVirtualFunctions(const char *pfname ATTRIBUTE_UNUSED, > char ***vfname ATTRIBUTE_UNUSED, > @@ -1165,4 +1654,44 @@ virNetDevGetPhysicalFunction(const char *ifname > ATTRIBUTE_UNUSED, > _("Unable to get physical function status on this > platform")); > return -1; > } > + > +int > +virNetDevLinkDump(const char *ifname, int ifindex, > + bool nltarget_kernel, struct nlattr **tb, > + unsigned char **recvbuf, > + uint32_t (*getPidFunc)(void)) Every arg needs ATTRIBUTE_UNUSED > +{ > + virReportSystemError(ENOSYS, "%s", > + _("Unable to dump link info on this platform")); > + return -1; > +} > + > +int > +virNetDevReplaceNetConfig(char *linkdev, int vf, > + const unsigned char *macaddress, int vlanid, > + char *stateDir) Again, you need multiple ATTRIBUTE_UNUSEDs (also for the stub functions below)

> +{ > + virReportSystemError(ENOSYS, "%s", > + _("Unable to replace net config on this > platform")); > + return -1; > + > +} > + > +int > +virNetDevRestoreNetConfig(char *linkdev, int vf, char *stateDir) > +{ > + virReportSystemError(ENOSYS, "%s", > + _("Unable to restore net config on this > platform")); > + return -1; > +} > + > +int > +virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname, > + int *vf) > +{ > + virReportSystemError(ENOSYS, "%s", > + _("Unable to get virtual function info on this > platform")); > + return -1; > +} > + > #endif /* !__linux__ */ > diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h > index 7845e25..d6b78c3 100644 > --- a/src/util/virnetdev.h > +++ b/src/util/virnetdev.h > @@ -24,6 +24,7 @@ > # define __VIR_NETDEV_H__ > > # include "virsocketaddr.h" > +# include "virnetlink.h" > > int virNetDevExists(const char *brname) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; > @@ -105,4 +106,22 @@ int virNetDevGetVirtualFunctions(const char *pfname, > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) > ATTRIBUTE_RETURN_CHECK; > > +int virNetDevLinkDump(const char *ifname, int ifindex, > + bool nltarget_kernel, struct nlattr **tb, > + unsigned char **recvbuf, > + uint32_t (*getPidFunc)(void)) > + ATTRIBUTE_RETURN_CHECK; > + > +int virNetDevReplaceNetConfig(char *linkdev, int vf, > + const unsigned char *macaddress, int vlanid, > + char *stateDir) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5); > + > +int virNetDevRestoreNetConfig(char *linkdev, int vf, char *stateDir) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); > + > +int virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname, > + int *vf) > + ATTRIBUTE_NONNULL(1); > + > #endif /* __VIR_NETDEV_H__ */ > >