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

On 3/5/12 10:23 AM, "Laine Stump" <laine(a)laine.org&gt; wrote: > 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) > I was hoping someone will point out what #defines to use here. So thanks. I will add HAVE_LIBNL. We also need it for IFLA_VF_MAC and VLAN. They were under HAVE_VIRTPORT before. Can you suggest what I can do here ?

>> >> +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. > > Ok yes I will remove it. I added the virNetDevGetIndex at the end because for some reason rhel kernel allowed a ifindex in setlink but not getlink (And this is a deviation from the upstream kernel). I will fix it. >> + >> + 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. > Ok thanks. I will do that. >> + >> +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) > This again was some code I moved out of virnetdevvportprofile.c. Did not change anything.

>> + }; >> + >> + memcpy(ifla_vf_mac.mac, macaddr, 6); > This should use VIR_MAC_BUFLEN instead of 6. > Same here. But I will fix it.

>> + >> + 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. > > Yes I don't remember why I added only mac here. Will fix it. >> + 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. > > This was also previous code. I will make the changes.

>> + >> +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). > Ok...i will fix it. I took this style from existing virNetDevVPortProfileGetStatus

>> + >> + 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. > Will do >> + 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. > Will fix it. >> + } >> + >> + 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; > Ok. >> + >> + 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. > > Will fix it. >> +} >> +#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) > Ok. >> +{ >> + 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__ */ >> >> Thanks Laine. I will make the required changes and push a v3.