[libvirt] [PATCH 0/4] Support mac and port profile for <interface type='hostdev'>
v2: changes include: - feedback from stefan for 802.1Qbg. Code now prints an error if virtualport is specified for 802.1Qbg on an interface of type hostdev - feedback from laine for non-sriov devices. Interface type hostdev for non-sriov devices is not supported. v1: https://www.redhat.com/archives/libvir-list/2012-March/msg00015.html This patch series is based on laines patches to support <interface type='hostdev'>. https://www.redhat.com/archives/libvir-list/2012-February/msg01126.html It support to set mac and port profile on an interface of type hostdev. * If virtualport is specified, the existing virtual port functions are called to set mac, vlan and port profile. * If virtualport is not specified and device is a sriov virtual function, - mac is set using IFLA_VF_MAC * If virtualport is not specified and device is a non-sriov virtual function, - mac is set using existing SIOCGIFHWADDR (This requires that the netdev be present on the host before starting the VM) This series implements the below : 01/4 pci: Add two new pci util pciDeviceGetVirtualFunctionInfo and pciConfigAddressToSysfsFile 02/4 virtnetdev: Add support functions for mac and portprofile associations on a hostdev 03/4 virnetdevvportprofile: Changes to support portprofiles for hostdevs 04/4 qemu_hostdev: Add support to install port profile and mac address on hostdev Stefan Berger is CC'ed for 802.1Qbg changes in patch 03/4. Current code for 802.1Qbg uses macvtap ifname. And for network interfaces with type=hostdev a macvtap ifname does not exist. This patch just adds a null check for ifname in 802.1Qbg port profile handling code.
From: Roopa Prabhu <roprabhu@cisco.com> pciDeviceGetVirtualFunctionInfo returns pf netdevice name and virtual function index for a given vf. This is just a wrapper around existing functions to return vf's pf and vf_index with one api call pciConfigAddressToSysfsfile returns the sysfile pci device link from a 'struct pci_config_address' Signed-off-by: Roopa Prabhu <roprabhu@cisco.com> --- src/util/pci.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/pci.h | 7 +++++++ 2 files changed, 62 insertions(+), 0 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index c660e8d..d8c136e 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -2081,6 +2081,20 @@ pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link) return 0; } +int +pciConfigAddressToSysfsFile(struct pci_config_address *dev, + char **pci_sysfs_device_link) +{ + if (virAsprintf(pci_sysfs_device_link, + PCI_SYSFS "devices/%04x:%02x:%02x.%x", dev->domain, + dev->bus, dev->slot, dev->function) < 0) { + virReportOOMError(); + return -1; + } + + return 0; +} + /* * Returns the network device name of a pci device */ @@ -2123,6 +2137,38 @@ out: return ret; } + +int +pciDeviceGetVirtualFunctionInfo(const char *vf_sysfs_device_path, + char **pfname, int *vf_index) +{ + struct pci_config_address *pf_config_address = NULL; + char *pf_sysfs_device_path = NULL; + int ret = -1; + + if (pciGetPhysicalFunction(vf_sysfs_device_path, &pf_config_address) < 0) + return ret; + + if (pciConfigAddressToSysfsFile(pf_config_address, + &pf_sysfs_device_path) < 0) { + + VIR_FREE(pf_config_address); + return ret; + } + + if (pciGetVirtualFunctionIndex(pf_sysfs_device_path, vf_sysfs_device_path, + vf_index) < 0) + goto cleanup; + + ret = pciDeviceNetName(pf_sysfs_device_path, pfname); + +cleanup: + VIR_FREE(pf_config_address); + VIR_FREE(pf_sysfs_device_path); + + return ret; +} + #else int pciGetPhysicalFunction(const char *vf_sysfs_path ATTRIBUTE_UNUSED, @@ -2170,4 +2216,13 @@ pciDeviceNetName(char *device_link_sysfs_path ATTRIBUTE_UNUSED, "supported on non-linux platforms")); return -1; } + +int +pciDeviceGetVirtualFunctionInfo(const char *vf_sysfs_device_path, + char **pfname, int *vf_index) +{ + pciReportError(VIR_ERR_INTERNAL_ERROR, _("pciDeviceGetVirtualFunctionInfo " + "is not supported on non-linux platforms")); + return -1; +} #endif /* __linux__ */ diff --git a/src/util/pci.h b/src/util/pci.h index 25b5b66..b71bb12 100644 --- a/src/util/pci.h +++ b/src/util/pci.h @@ -111,6 +111,9 @@ int pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link, const char *vf_sysfs_device_link, int *vf_index); +int pciConfigAddressToSysfsFile(struct pci_config_address *dev, + char **pci_sysfs_device_link); + int pciDeviceNetName(char *device_link_sysfs_path, char **netname); int pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link) @@ -122,4 +125,8 @@ int pciGetDeviceAddrString(unsigned domain, unsigned function, char **pciConfigAddr) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; + +int pciDeviceGetVirtualFunctionInfo(const char *vf_sysfs_device_path, + char **pfname, int *vf_index); + #endif /* __VIR_PCI_H__ */
On 03/04/2012 10:15 PM, Roopa Prabhu wrote:
From: Roopa Prabhu <roprabhu@cisco.com>
pciDeviceGetVirtualFunctionInfo returns pf netdevice name and virtual function index for a given vf. This is just a wrapper around existing functions to return vf's pf and vf_index with one api call
pciConfigAddressToSysfsfile returns the sysfile pci device link from a 'struct pci_config_address'
Signed-off-by: Roopa Prabhu <roprabhu@cisco.com> --- src/util/pci.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/pci.h | 7 +++++++ 2 files changed, 62 insertions(+), 0 deletions(-)
ACK.
From: Roopa Prabhu <roprabhu@cisco.com> 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@cisco.com> --- src/util/virnetdev.c | 531 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 19 ++ 2 files changed, 549 insertions(+), 1 deletions(-) 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__ */ +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; + } + + 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; +} + +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, }, + }; + + memcpy(ifla_vf_mac.mac, macaddr, 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); + goto err_exit; + } + break; + + case NLMSG_DONE: + break; + + default: + goto malformed_resp; + } + + rc = 0; + +err_exit: + 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; +} + +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: + if (msg) + virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", msg); + + 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); + 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; + } + + 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; + + 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); + + VIR_FREE(vf_sysfs_path); + VIR_FREE(pf_sysfs_path); + + return ret; +} +#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)) +{ + 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) +{ + 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__ */
On 03/04/2012 10:15 PM, Roopa Prabhu wrote:
From: Roopa Prabhu <roprabhu@cisco.com>
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@cisco.com> --- 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__ */
On 3/5/12 10:23 AM, "Laine Stump" <laine@laine.org> wrote:
On 03/04/2012 10:15 PM, Roopa Prabhu wrote:
From: Roopa Prabhu <roprabhu@cisco.com>
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@cisco.com> --- 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.
On 3/5/12 11:50 AM, "Roopa Prabhu" <roprabhu@cisco.com> wrote:
On 3/5/12 10:23 AM, "Laine Stump" <laine@laine.org> wrote:
On 03/04/2012 10:15 PM, Roopa Prabhu wrote:
From: Roopa Prabhu <roprabhu@cisco.com>
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@cisco.com> --- 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 ?
Correction: They were under WITH_VIRUALPORT (which has a configure option and a check for IFLA_VF_PORT_MAX). We will need a check for IFLA_VF_MAX here. I think I will try putting this under defined(IFLA_VF_MAX). This should cover IFLA_VF_MAC and IFLA_VF_VLAN I think. If you have any other suggestions let me know. Thanks, Roopa
On 03/05/2012 02:50 PM, Roopa Prabhu wrote:
On 3/5/12 10:23 AM, "Laine Stump" <laine@laine.org> wrote:
From: Roopa Prabhu <roprabhu@cisco.com>
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@cisco.com> --- 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
On 03/04/2012 10:15 PM, Roopa Prabhu wrote: 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 ?
I would put it inside HAVE_LIBNL, because that's the library you need to compile it.
+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.
Okay, leave it then.
+ }; + + memcpy(ifla_vf_mac.mac, macaddr, 6); This should use VIR_MAC_BUFLEN instead of 6.
Same here. But I will fix it.
Sorry, I didn't notice that. That ones simple enough we should just fix it now as part of the movement.
+ + 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.
Ah, wasn't paying close enough attention. Disregard involved fixups like that to existing code that was just moved, i.e. if the potential for regression is anything > 0.
+ +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
Okay. If it's new code (including copied), then fix it. Moved code, leave it as is.
+ + 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.
From: Roopa Prabhu <roprabhu@cisco.com> This patch includes the following changes - removes some netlink functions which are now available in virnetdev.c - Adds a vf argument to all port profile functions For 802.1Qbh devices, the port profile calls can use a vf argument if passed by the caller. If the vf argument is -1 it will try to derive the vf if the device passed is a virtual function. For 802.1Qbg devices, This patch introduces a null check for the device argument because during port profile assignment on a hostdev, this argument can be null. Stefan CC'ed for comments Signed-off-by: Roopa Prabhu <roprabhu@cisco.com> --- src/qemu/qemu_migration.c | 2 src/util/virnetdevmacvlan.c | 6 + src/util/virnetdevvportprofile.c | 203 +++++--------------------------------- src/util/virnetdevvportprofile.h | 8 + 4 files changed, 42 insertions(+), 177 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 7df2d4f..b80ed69 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2605,6 +2605,7 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { virDomainNetGetActualVirtPortProfile(net), net->mac, virDomainNetGetActualDirectDev(net), + -1, def->uuid, VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) < 0) goto err_exit; @@ -2622,6 +2623,7 @@ err_exit: virDomainNetGetActualVirtPortProfile(net), net->mac, virDomainNetGetActualDirectDev(net), + -1, VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH)); } } diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 25d0846..498a2a0 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -486,6 +486,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, uint32_t macvtapMode; const char *cr_ifname; int ret; + int vf = -1; macvtapMode = modeMap[mode]; @@ -547,6 +548,7 @@ create_name: virtPortProfile, macaddress, linkdev, + vf, vmuuid, vmOp) < 0) { rc = -1; goto link_del_exit; @@ -597,6 +599,7 @@ disassociate_exit: virtPortProfile, macaddress, linkdev, + vf, vmOp)); link_del_exit: @@ -624,6 +627,8 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, char *stateDir) { int ret = 0; + int vf = -1; + if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) { ignore_value(virNetDevRestoreMacAddress(linkdev, stateDir)); } @@ -633,6 +638,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, virtPortProfile, macaddr, linkdev, + vf, VIR_NETDEV_VPORT_PROFILE_OP_DESTROY) < 0) ret = -1; if (virNetDevMacVLanDelete(ifname) < 0) diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index 67fd7bc..8d9e906 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -126,11 +126,6 @@ static struct nla_policy ifla_port_policy[IFLA_PORT_MAX + 1] = { [IFLA_PORT_RESPONSE] = { .type = NLA_U16 }, }; -static struct nla_policy ifla_policy[IFLA_MAX + 1] = -{ - [IFLA_VF_PORTS] = { .type = NLA_NESTED }, -}; - static uint32_t virNetDevVPortProfileGetLldpadPid(void) { @@ -164,126 +159,6 @@ virNetDevVPortProfileGetLldpadPid(void) { return pid; } - -/** - * virNetDevVPortProfileLinkDump: - * - * @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. - */ -static int -virNetDevVPortProfileLinkDump(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; - - 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; - } - - 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: - if (nlmsg_parse(resp, sizeof(struct ifinfomsg), - tb, IFLA_MAX, ifla_policy)) { - 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; -} - /** * virNetDevVPortProfileGetStatus: * @@ -607,7 +482,7 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int return -1; while (!end && i <= nthParent) { - rc = virNetDevVPortProfileLinkDump(ifname, ifindex, true, tb, &recvbuf, NULL); + rc = virNetDevLinkDump(ifname, ifindex, true, tb, &recvbuf, NULL); if (rc < 0) break; @@ -676,8 +551,8 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, } while (--repeats >= 0) { - rc = virNetDevVPortProfileLinkDump(NULL, ifindex, nltarget_kernel, tb, - &recvbuf, virNetDevVPortProfileGetLldpadPid); + rc = virNetDevLinkDump(NULL, ifindex, nltarget_kernel, tb, + &recvbuf, virNetDevVPortProfileGetLldpadPid); if (rc < 0) goto err_exit; @@ -750,6 +625,7 @@ virNetDevVPortProfileGetPhysdevAndVlan(const char *ifname, int *root_ifindex, ch static int virNetDevVPortProfileOp8021Qbg(const char *ifname, const unsigned char *macaddr, + int vf, const virNetDevVPortProfilePtr virtPort, enum virNetDevVPortProfileLinkOp virtPortOp) { @@ -763,7 +639,11 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname, int vlanid; int physdev_ifindex = 0; char physdev_ifname[IFNAMSIZ] = { 0, }; - int vf = PORT_SELF_VF; + + if (!ifname) + return -1; + + vf = PORT_SELF_VF; if (virNetDevVPortProfileGetPhysdevAndVlan(ifname, &physdev_ifindex, physdev_ifname, &vlanid) < 0) { @@ -811,46 +691,11 @@ err_exit: return rc; } - -static int -virNetDevVPortProfileGetPhysfnDev(const char *linkdev, - int32_t *vf, - char **physfndev) -{ - int rc = -1; - - if (virNetDevIsVirtualFunction(linkdev) == 1) { - /* if linkdev is SR-IOV VF, then set vf = VF index */ - /* and set linkdev = PF device */ - - rc = virNetDevGetPhysicalFunction(linkdev, physfndev); - if (!rc) - rc = virNetDevGetVirtualFunctionIndex(*physfndev, linkdev, vf); - } else { - - /* Not SR-IOV VF: physfndev is linkdev and VF index - * refers to linkdev self - */ - - *vf = PORT_SELF_VF; - *physfndev = strdup(linkdev); - if (!*physfndev) { - virReportOOMError(); - goto err_exit; - } - rc = 0; - } - -err_exit: - - return rc; -} - - /* Returns 0 on success, -1 on general failure, and -2 on timeout */ static int virNetDevVPortProfileOp8021Qbh(const char *ifname, const unsigned char *macaddr, + int32_t vf, const virNetDevVPortProfilePtr virtPort, const unsigned char *vm_uuid, enum virNetDevVPortProfileLinkOp virtPortOp) @@ -858,14 +703,20 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, int rc = 0; char *physfndev = NULL; unsigned char hostuuid[VIR_UUID_BUFLEN]; - int32_t vf; bool nltarget_kernel = true; int ifindex; int vlanid = -1; - rc = virNetDevVPortProfileGetPhysfnDev(ifname, &vf, &physfndev); - if (rc < 0) - goto err_exit; + if (vf == -1 && virNetDevIsVirtualFunction(ifname) == 1) { + if (virNetDevGetVirtualFunctionInfo(ifname, &physfndev, &vf) < 0) + goto err_exit; + } else { + physfndev = strdup(ifname); + if (!physfndev) { + virReportOOMError(); + goto err_exit; + } + } rc = virNetDevGetIndex(physfndev, &ifindex); if (rc < 0) @@ -953,13 +804,14 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macvtap_macaddr, const char *linkdev, + int vf, const unsigned char *vmuuid, enum virNetDevVPortProfileOp vmOp) { int rc = 0; VIR_DEBUG("Associating port profile '%p' on link device '%s'", - virtPort, macvtap_ifname); + virtPort, (macvtap_ifname ? macvtap_ifname : linkdev)); VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, virNetDevVPortProfileOpTypeToString(vmOp)); @@ -974,14 +826,14 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname, case VIR_NETDEV_VPORT_PROFILE_8021QBG: rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, - virtPort, + vf, virtPort, (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) ? VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE : VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE); break; case VIR_NETDEV_VPORT_PROFILE_8021QBH: - rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, + rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, vf, virtPort, vmuuid, (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) ? VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR @@ -1014,6 +866,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macvtap_macaddr, const char *linkdev, + int vf, enum virNetDevVPortProfileOp vmOp) { int rc = 0; @@ -1033,7 +886,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, break; case VIR_NETDEV_VPORT_PROFILE_8021QBG: - rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, + rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, vf, virtPort, VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); break; @@ -1043,7 +896,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) break; ignore_value(virNetDevSetOnline(linkdev, false)); - rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, + rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, vf, virtPort, NULL, VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); break; @@ -1057,6 +910,7 @@ int virNetDevVPortProfileAssociate(const char *macvtap_ifname ATTRIBUTE_UNUSED, const virNetDevVPortProfilePtr virtPort ATTRIBUTE_UNUSED, const unsigned char *macvtap_macaddr ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, + int vf, const unsigned char *vmuuid ATTRIBUTE_UNUSED, enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED) { @@ -1069,6 +923,7 @@ int virNetDevVPortProfileDisassociate(const char *macvtap_ifname ATTRIBUTE_UNUSE const virNetDevVPortProfilePtr virtPort ATTRIBUTE_UNUSED, const unsigned char *macvtap_macaddr ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, + int vf, enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", diff --git a/src/util/virnetdevvportprofile.h b/src/util/virnetdevvportprofile.h index ec2d06d..5c5dcf1 100644 --- a/src/util/virnetdevvportprofile.h +++ b/src/util/virnetdevvportprofile.h @@ -85,17 +85,19 @@ int virNetDevVPortProfileAssociate(const char *ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macaddr, const char *linkdev, + int vf, const unsigned char *vmuuid, enum virNetDevVPortProfileOp vmOp) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) - ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_RETURN_CHECK; int virNetDevVPortProfileDisassociate(const char *ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macaddr, const char *linkdev, + int vf, enum virNetDevVPortProfileOp vmOp) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK;
I encountered two conflicts when I rebased this patch to upstream. Noted in the comments. On 03/04/2012 10:15 PM, Roopa Prabhu wrote:
From: Roopa Prabhu <roprabhu@cisco.com>
This patch includes the following changes - removes some netlink functions which are now available in virnetdev.c - Adds a vf argument to all port profile functions
For 802.1Qbh devices, the port profile calls can use a vf argument if passed by the caller. If the vf argument is -1 it will try to derive the vf if the device passed is a virtual function.
For 802.1Qbg devices, This patch introduces a null check for the device argument because during port profile assignment on a hostdev, this argument can be null. Stefan CC'ed for comments
Signed-off-by: Roopa Prabhu <roprabhu@cisco.com> --- src/qemu/qemu_migration.c | 2 src/util/virnetdevmacvlan.c | 6 + src/util/virnetdevvportprofile.c | 203 +++++--------------------------------- src/util/virnetdevvportprofile.h | 8 + 4 files changed, 42 insertions(+), 177 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 7df2d4f..b80ed69 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2605,6 +2605,7 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { virDomainNetGetActualVirtPortProfile(net), net->mac, virDomainNetGetActualDirectDev(net), + -1, def->uuid, VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) < 0) goto err_exit; @@ -2622,6 +2623,7 @@ err_exit: virDomainNetGetActualVirtPortProfile(net), net->mac, virDomainNetGetActualDirectDev(net), + -1, VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH)); } } diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 25d0846..498a2a0 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -486,6 +486,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, uint32_t macvtapMode; const char *cr_ifname; int ret; + int vf = -1;
macvtapMode = modeMap[mode];
@@ -547,6 +548,7 @@ create_name: virtPortProfile, macaddress, linkdev, + vf, vmuuid, vmOp) < 0) { rc = -1; goto link_del_exit; @@ -597,6 +599,7 @@ disassociate_exit: virtPortProfile, macaddress, linkdev, + vf, vmOp));
link_del_exit: @@ -624,6 +627,8 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, char *stateDir) { int ret = 0; + int vf = -1; + if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) { ignore_value(virNetDevRestoreMacAddress(linkdev, stateDir)); } @@ -633,6 +638,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, virtPortProfile, macaddr, linkdev, + vf, VIR_NETDEV_VPORT_PROFILE_OP_DESTROY) < 0) ret = -1; if (virNetDevMacVLanDelete(ifname) < 0) diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index 67fd7bc..8d9e906 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -126,11 +126,6 @@ static struct nla_policy ifla_port_policy[IFLA_PORT_MAX + 1] = { [IFLA_PORT_RESPONSE] = { .type = NLA_U16 }, }; -static struct nla_policy ifla_policy[IFLA_MAX + 1] = -{ - [IFLA_VF_PORTS] = { .type = NLA_NESTED }, -}; -
static uint32_t virNetDevVPortProfileGetLldpadPid(void) { @@ -164,126 +159,6 @@ virNetDevVPortProfileGetLldpadPid(void) { return pid; }
- -/** - * virNetDevVPortProfileLinkDump: - * - * @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. - */ -static int -virNetDevVPortProfileLinkDump(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; - - 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; - } - - 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: - if (nlmsg_parse(resp, sizeof(struct ifinfomsg), - tb, IFLA_MAX, ifla_policy)) { - 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; -} - /** * virNetDevVPortProfileGetStatus: * @@ -607,7 +482,7 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int return -1;
while (!end && i <= nthParent) { - rc = virNetDevVPortProfileLinkDump(ifname, ifindex, true, tb, &recvbuf, NULL); + rc = virNetDevLinkDump(ifname, ifindex, true, tb, &recvbuf, NULL); if (rc < 0) break;
@@ -676,8 +551,8 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, }
while (--repeats >= 0) { - rc = virNetDevVPortProfileLinkDump(NULL, ifindex, nltarget_kernel, tb, - &recvbuf, virNetDevVPortProfileGetLldpadPid); + rc = virNetDevLinkDump(NULL, ifindex, nltarget_kernel, tb, + &recvbuf, virNetDevVPortProfileGetLldpadPid); if (rc < 0) goto err_exit;
@@ -750,6 +625,7 @@ virNetDevVPortProfileGetPhysdevAndVlan(const char *ifname, int *root_ifindex, ch static int virNetDevVPortProfileOp8021Qbg(const char *ifname, const unsigned char *macaddr, + int vf, const virNetDevVPortProfilePtr virtPort, enum virNetDevVPortProfileLinkOp virtPortOp) { @@ -763,7 +639,11 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname, int vlanid; int physdev_ifindex = 0; char physdev_ifname[IFNAMSIZ] = { 0, }; - int vf = PORT_SELF_VF; + + if (!ifname) + return -1; + + vf = PORT_SELF_VF;
if (virNetDevVPortProfileGetPhysdevAndVlan(ifname, &physdev_ifindex, physdev_ifname, &vlanid) < 0) { @@ -811,46 +691,11 @@ err_exit: return rc; }
- -static int -virNetDevVPortProfileGetPhysfnDev(const char *linkdev, - int32_t *vf, - char **physfndev) -{ - int rc = -1; - - if (virNetDevIsVirtualFunction(linkdev) == 1) { - /* if linkdev is SR-IOV VF, then set vf = VF index */ - /* and set linkdev = PF device */ - - rc = virNetDevGetPhysicalFunction(linkdev, physfndev); - if (!rc) - rc = virNetDevGetVirtualFunctionIndex(*physfndev, linkdev, vf); - } else { - - /* Not SR-IOV VF: physfndev is linkdev and VF index - * refers to linkdev self - */ - - *vf = PORT_SELF_VF; - *physfndev = strdup(linkdev); - if (!*physfndev) { - virReportOOMError(); - goto err_exit; - } - rc = 0; - } - -err_exit: - - return rc; -} - - /* Returns 0 on success, -1 on general failure, and -2 on timeout */ static int virNetDevVPortProfileOp8021Qbh(const char *ifname, const unsigned char *macaddr, + int32_t vf, const virNetDevVPortProfilePtr virtPort, const unsigned char *vm_uuid, enum virNetDevVPortProfileLinkOp virtPortOp) @@ -858,14 +703,20 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, int rc = 0; char *physfndev = NULL; unsigned char hostuuid[VIR_UUID_BUFLEN]; - int32_t vf; bool nltarget_kernel = true; int ifindex; int vlanid = -1;
- rc = virNetDevVPortProfileGetPhysfnDev(ifname, &vf, &physfndev); - if (rc < 0) - goto err_exit; + if (vf == -1 && virNetDevIsVirtualFunction(ifname) == 1) {
It's kind of bothersome that virNetDevIsVirtualFunction() can return an error, but we're ignoring it here. Perhaps it should go something like this: bool is_vf = false; if (vf == -1) { int isvf_ret = virNetDevIstVirtualFunction(ifname) if (isvf_ret == -1) goto cleanup; // yep - change its name :-) is_vf = !!isvf_ret; } if (is_vf) { blah blah blah
+ if (virNetDevGetVirtualFunctionInfo(ifname, &physfndev, &vf) < 0) + goto err_exit; + } else { + physfndev = strdup(ifname); + if (!physfndev) { + virReportOOMError(); + goto err_exit; + } + }
rc = virNetDevGetIndex(physfndev, &ifindex); if (rc < 0) @@ -953,13 +804,14 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macvtap_macaddr, const char *linkdev, + int vf, const unsigned char *vmuuid, enum virNetDevVPortProfileOp vmOp) { int rc = 0;
VIR_DEBUG("Associating port profile '%p' on link device '%s'", - virtPort, macvtap_ifname); + virtPort, (macvtap_ifname ? macvtap_ifname : linkdev));
VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, virNetDevVPortProfileOpTypeToString(vmOp));
@@ -974,14 +826,14 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname,
case VIR_NETDEV_VPORT_PROFILE_8021QBG: rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, - virtPort, + vf, virtPort, (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) ? VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE : VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE); break;
case VIR_NETDEV_VPORT_PROFILE_8021QBH: - rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, + rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, vf, virtPort, vmuuid, (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) ? VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR @@ -1014,6 +866,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macvtap_macaddr, const char *linkdev, + int vf, enum virNetDevVPortProfileOp vmOp) { int rc = 0; @@ -1033,7 +886,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, break;
case VIR_NETDEV_VPORT_PROFILE_8021QBG: - rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, + rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, vf, virtPort, VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); break; @@ -1043,7 +896,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) break; ignore_value(virNetDevSetOnline(linkdev, false)); - rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, + rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, vf, virtPort, NULL, VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); break; @@ -1057,6 +910,7 @@ int virNetDevVPortProfileAssociate(const char *macvtap_ifname ATTRIBUTE_UNUSED, const virNetDevVPortProfilePtr virtPort ATTRIBUTE_UNUSED, const unsigned char *macvtap_macaddr ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, + int vf, const unsigned char *vmuuid ATTRIBUTE_UNUSED, enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED) { @@ -1069,6 +923,7 @@ int virNetDevVPortProfileDisassociate(const char *macvtap_ifname ATTRIBUTE_UNUSE const virNetDevVPortProfilePtr virtPort ATTRIBUTE_UNUSED, const unsigned char *macvtap_macaddr ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, + int vf, enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", diff --git a/src/util/virnetdevvportprofile.h b/src/util/virnetdevvportprofile.h index ec2d06d..5c5dcf1 100644 --- a/src/util/virnetdevvportprofile.h +++ b/src/util/virnetdevvportprofile.h @@ -85,17 +85,19 @@ int virNetDevVPortProfileAssociate(const char *ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macaddr, const char *linkdev, + int vf, const unsigned char *vmuuid, enum virNetDevVPortProfileOp vmOp) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) - ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_RETURN_CHECK;
This function (virNetDevVPortProfileAssociate()) has a new final argument "setlink_only". It was properly merged in most places, but the change to ATTRIBUTE macros here caused a conflict. There was a call to that function failed to merge, and that of course didn't get the new arg. Unfortunately, I did "git commit --amend" in my local tree, so I no longer have the info about *which* call to the function, but it leap right out at you :-)
int virNetDevVPortProfileDisassociate(const char *ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macaddr, const char *linkdev, + int vf, enum virNetDevVPortProfileOp vmOp) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK;
Otherwise this looks fine.
On 3/5/12 11:16 AM, "Laine Stump" <laine@laine.org> wrote:
I encountered two conflicts when I rebased this patch to upstream. Noted in the comments.
On 03/04/2012 10:15 PM, Roopa Prabhu wrote:
From: Roopa Prabhu <roprabhu@cisco.com>
This patch includes the following changes - removes some netlink functions which are now available in virnetdev.c - Adds a vf argument to all port profile functions
For 802.1Qbh devices, the port profile calls can use a vf argument if passed by the caller. If the vf argument is -1 it will try to derive the vf if the device passed is a virtual function.
For 802.1Qbg devices, This patch introduces a null check for the device argument because during port profile assignment on a hostdev, this argument can be null. Stefan CC'ed for comments
Signed-off-by: Roopa Prabhu <roprabhu@cisco.com> --- src/qemu/qemu_migration.c | 2 src/util/virnetdevmacvlan.c | 6 + src/util/virnetdevvportprofile.c | 203 +++++--------------------------------- src/util/virnetdevvportprofile.h | 8 + 4 files changed, 42 insertions(+), 177 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 7df2d4f..b80ed69 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2605,6 +2605,7 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) {
virDomainNetGetActualVirtPortProfile(net), net->mac,
virDomainNetGetActualDirectDev(net), + -1, def->uuid,
VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) < 0) goto err_exit; @@ -2622,6 +2623,7 @@ err_exit:
virDomainNetGetActualVirtPortProfile(net), net->mac,
virDomainNetGetActualDirectDev(net), + -1,
VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH)); } } diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 25d0846..498a2a0 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -486,6 +486,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, uint32_t macvtapMode; const char *cr_ifname; int ret; + int vf = -1;
macvtapMode = modeMap[mode];
@@ -547,6 +548,7 @@ create_name: virtPortProfile, macaddress, linkdev, + vf, vmuuid, vmOp) < 0) { rc = -1; goto link_del_exit; @@ -597,6 +599,7 @@ disassociate_exit: virtPortProfile, macaddress, linkdev, + vf, vmOp));
link_del_exit: @@ -624,6 +627,8 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, char *stateDir) { int ret = 0; + int vf = -1; + if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) { ignore_value(virNetDevRestoreMacAddress(linkdev, stateDir)); } @@ -633,6 +638,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, virtPortProfile, macaddr, linkdev, + vf,
VIR_NETDEV_VPORT_PROFILE_OP_DESTROY) < 0) ret = -1; if (virNetDevMacVLanDelete(ifname) < 0) diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index 67fd7bc..8d9e906 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -126,11 +126,6 @@ static struct nla_policy ifla_port_policy[IFLA_PORT_MAX + 1] = { [IFLA_PORT_RESPONSE] = { .type = NLA_U16 }, }; -static struct nla_policy ifla_policy[IFLA_MAX + 1] = -{ - [IFLA_VF_PORTS] = { .type = NLA_NESTED }, -}; -
static uint32_t virNetDevVPortProfileGetLldpadPid(void) { @@ -164,126 +159,6 @@ virNetDevVPortProfileGetLldpadPid(void) { return pid; }
- -/** - * virNetDevVPortProfileLinkDump: - * - * @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. - */ -static int -virNetDevVPortProfileLinkDump(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; - - 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; - } - - 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: - if (nlmsg_parse(resp, sizeof(struct ifinfomsg), - tb, IFLA_MAX, ifla_policy)) { - 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; -} - /** * virNetDevVPortProfileGetStatus: * @@ -607,7 +482,7 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int return -1;
while (!end && i <= nthParent) { - rc = virNetDevVPortProfileLinkDump(ifname, ifindex, true, tb, &recvbuf, NULL); + rc = virNetDevLinkDump(ifname, ifindex, true, tb, &recvbuf, NULL); if (rc < 0) break;
@@ -676,8 +551,8 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, }
while (--repeats >= 0) { - rc = virNetDevVPortProfileLinkDump(NULL, ifindex, nltarget_kernel, tb, - &recvbuf, virNetDevVPortProfileGetLldpadPid); + rc = virNetDevLinkDump(NULL, ifindex, nltarget_kernel, tb, + &recvbuf, virNetDevVPortProfileGetLldpadPid); if (rc < 0) goto err_exit;
@@ -750,6 +625,7 @@ virNetDevVPortProfileGetPhysdevAndVlan(const char *ifname, int *root_ifindex, ch static int virNetDevVPortProfileOp8021Qbg(const char *ifname, const unsigned char *macaddr, + int vf, const virNetDevVPortProfilePtr virtPort, enum virNetDevVPortProfileLinkOp virtPortOp) { @@ -763,7 +639,11 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname, int vlanid; int physdev_ifindex = 0; char physdev_ifname[IFNAMSIZ] = { 0, }; - int vf = PORT_SELF_VF; + + if (!ifname) + return -1; + + vf = PORT_SELF_VF;
if (virNetDevVPortProfileGetPhysdevAndVlan(ifname, &physdev_ifindex, physdev_ifname, &vlanid) < 0) { @@ -811,46 +691,11 @@ err_exit: return rc; }
- -static int -virNetDevVPortProfileGetPhysfnDev(const char *linkdev, - int32_t *vf, - char **physfndev) -{ - int rc = -1; - - if (virNetDevIsVirtualFunction(linkdev) == 1) { - /* if linkdev is SR-IOV VF, then set vf = VF index */ - /* and set linkdev = PF device */ - - rc = virNetDevGetPhysicalFunction(linkdev, physfndev); - if (!rc) - rc = virNetDevGetVirtualFunctionIndex(*physfndev, linkdev, vf); - } else { - - /* Not SR-IOV VF: physfndev is linkdev and VF index - * refers to linkdev self - */ - - *vf = PORT_SELF_VF; - *physfndev = strdup(linkdev); - if (!*physfndev) { - virReportOOMError(); - goto err_exit; - } - rc = 0; - } - -err_exit: - - return rc; -} - - /* Returns 0 on success, -1 on general failure, and -2 on timeout */ static int virNetDevVPortProfileOp8021Qbh(const char *ifname, const unsigned char *macaddr, + int32_t vf, const virNetDevVPortProfilePtr virtPort, const unsigned char *vm_uuid, enum virNetDevVPortProfileLinkOp virtPortOp) @@ -858,14 +703,20 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, int rc = 0; char *physfndev = NULL; unsigned char hostuuid[VIR_UUID_BUFLEN]; - int32_t vf; bool nltarget_kernel = true; int ifindex; int vlanid = -1;
- rc = virNetDevVPortProfileGetPhysfnDev(ifname, &vf, &physfndev); - if (rc < 0) - goto err_exit; + if (vf == -1 && virNetDevIsVirtualFunction(ifname) == 1) {
It's kind of bothersome that virNetDevIsVirtualFunction() can return an error, but we're ignoring it here.
Perhaps it should go something like this:
bool is_vf = false;
if (vf == -1) { int isvf_ret = virNetDevIstVirtualFunction(ifname)
if (isvf_ret == -1) goto cleanup; // yep - change its name :-) is_vf = !!isvf_ret; } if (is_vf) { blah blah blah
Ok will fix it.
+ if (virNetDevGetVirtualFunctionInfo(ifname, &physfndev, &vf) < 0) + goto err_exit; + } else { + physfndev = strdup(ifname); + if (!physfndev) { + virReportOOMError(); + goto err_exit; + } + }
rc = virNetDevGetIndex(physfndev, &ifindex); if (rc < 0) @@ -953,13 +804,14 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macvtap_macaddr, const char *linkdev, + int vf, const unsigned char *vmuuid, enum virNetDevVPortProfileOp vmOp) { int rc = 0;
VIR_DEBUG("Associating port profile '%p' on link device '%s'", - virtPort, macvtap_ifname); + virtPort, (macvtap_ifname ? macvtap_ifname : linkdev));
VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, virNetDevVPortProfileOpTypeToString(vmOp));
@@ -974,14 +826,14 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname,
case VIR_NETDEV_VPORT_PROFILE_8021QBG: rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, - virtPort, + vf, virtPort, (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) ? VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE : VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE); break;
case VIR_NETDEV_VPORT_PROFILE_8021QBH: - rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, + rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, vf, virtPort, vmuuid, (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) ? VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR @@ -1014,6 +866,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macvtap_macaddr, const char *linkdev, + int vf, enum virNetDevVPortProfileOp vmOp) { int rc = 0; @@ -1033,7 +886,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, break;
case VIR_NETDEV_VPORT_PROFILE_8021QBG: - rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, + rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, macvtap_macaddr, vf, virtPort,
VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); break; @@ -1043,7 +896,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) break; ignore_value(virNetDevSetOnline(linkdev, false)); - rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, + rc = virNetDevVPortProfileOp8021Qbh(linkdev, macvtap_macaddr, vf, virtPort, NULL,
VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); break; @@ -1057,6 +910,7 @@ int virNetDevVPortProfileAssociate(const char *macvtap_ifname ATTRIBUTE_UNUSED, const virNetDevVPortProfilePtr virtPort ATTRIBUTE_UNUSED, const unsigned char *macvtap_macaddr ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, + int vf, const unsigned char *vmuuid ATTRIBUTE_UNUSED, enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED) { @@ -1069,6 +923,7 @@ int virNetDevVPortProfileDisassociate(const char *macvtap_ifname ATTRIBUTE_UNUSE const virNetDevVPortProfilePtr virtPort ATTRIBUTE_UNUSED, const unsigned char *macvtap_macaddr ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, + int vf, enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", diff --git a/src/util/virnetdevvportprofile.h b/src/util/virnetdevvportprofile.h index ec2d06d..5c5dcf1 100644 --- a/src/util/virnetdevvportprofile.h +++ b/src/util/virnetdevvportprofile.h @@ -85,17 +85,19 @@ int virNetDevVPortProfileAssociate(const char *ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macaddr, const char *linkdev, + int vf, const unsigned char *vmuuid, enum virNetDevVPortProfileOp vmOp) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) - ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_RETURN_CHECK;
This function (virNetDevVPortProfileAssociate()) has a new final argument "setlink_only". It was properly merged in most places, but the change to ATTRIBUTE macros here caused a conflict. There was a call to that function failed to merge, and that of course didn't get the new arg. Unfortunately, I did "git commit --amend" in my local tree, so I no longer have the info about *which* call to the function, but it leap right out at you :-)
:)
int virNetDevVPortProfileDisassociate(const char *ifname, const virNetDevVPortProfilePtr virtPort, const unsigned char *macaddr, const char *linkdev, + int vf, enum virNetDevVPortProfileOp vmOp) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK;
Otherwise this looks fine.
Ok thanks laine.
From: Roopa Prabhu <roprabhu@cisco.com> These changes are applied only if the hostdev has a parent net device. If the parent netdevice has virtual port information, the original virtualport associate functions are called (these set and restore both mac and port profile on an interface). Else, only mac address is set on the device using other methods depending on if its a sriov device or not. Changes also include hotplug pci devices Signed-off-by: Roopa Prabhu <roprabhu@cisco.com> --- src/qemu/qemu_hostdev.c | 237 +++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_hostdev.h | 8 +- src/qemu/qemu_hotplug.c | 11 ++ 3 files changed, 243 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index b3cad8e..62fe467 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -29,6 +29,13 @@ #include "memory.h" #include "pci.h" #include "hostusb.h" +#include "virnetdev.h" + +VIR_ENUM_IMPL(virNetDevVPort, VIR_NETDEV_VPORT_PROFILE_LAST, + "none", + "802.1Qbg", + "802.1Qbh", + "openvswitch") static pciDeviceList * qemuGetPciHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs) @@ -156,19 +163,179 @@ int qemuUpdateActivePciHostdevs(struct qemud_driver *driver, return 0; } +static int +qemuDomainHostdevPciSysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path) +{ + struct pci_config_address config_address; + + config_address.domain = hostdev->source.subsys.u.pci.domain; + config_address.bus = hostdev->source.subsys.u.pci.bus; + config_address.slot = hostdev->source.subsys.u.pci.slot; + config_address.function = hostdev->source.subsys.u.pci.function; + + return pciConfigAddressToSysfsFile(&config_address, sysfs_path); +} + +int +qemuDomainHostdevIsVirtualFunction(virDomainHostdevDefPtr hostdev) +{ + char *sysfs_path = NULL; + int ret = -1; + + if (qemuDomainHostdevPciSysfsPath(hostdev, &sysfs_path) < 0) + return ret; + + ret = pciDeviceIsVirtualFunction(sysfs_path); + + VIR_FREE(sysfs_path); + + return ret; +} + +static int +qemuDomainHostdevNetDevice(virDomainHostdevDefPtr hostdev, char **linkdev, + int *vf) +{ + int ret = -1; + char *sysfs_path = NULL; + + if (qemuDomainHostdevPciSysfsPath(hostdev, &sysfs_path) < 0) + return ret; + + if (pciDeviceIsVirtualFunction(sysfs_path) == 1) { + if (pciDeviceGetVirtualFunctionInfo(sysfs_path, linkdev, + vf) < 0) + goto cleanup; + } else { + if (pciDeviceNetName(sysfs_path, linkdev) < 0) + goto cleanup; + *vf = -1; + } + + ret = 0; + +cleanup: + VIR_FREE(sysfs_path); + + return ret; +} + +static int +qemuDomainHostdevNetConfigVirtPortProfile(const char *linkdev, int vf, + virNetDevVPortProfilePtr virtPort, + const unsigned char *macaddr, + const unsigned char *uuid, + int associate) +{ + int ret = -1; + + if (!virtPort) + return ret; + + switch(virtPort->virtPortType) { + case VIR_NETDEV_VPORT_PROFILE_NONE: + case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: + case VIR_NETDEV_VPORT_PROFILE_8021QBG: + case VIR_NETDEV_VPORT_PROFILE_LAST: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("virtualport type %s is " + "currently not supported on interfaces of type " + "hostdev"), + virNetDevVPortTypeToString(virtPort->virtPortType)); + break; + + case VIR_NETDEV_VPORT_PROFILE_8021QBH: + if (associate) + ret = virNetDevVPortProfileAssociate(NULL, virtPort, macaddr, + linkdev, vf, uuid, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE); + else + ret = virNetDevVPortProfileDisassociate(NULL, virtPort, + macaddr, linkdev, vf, + VIR_NETDEV_VPORT_PROFILE_OP_DESTROY); + break; + } + + return ret; +} + +int +qemuDomainHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, + const unsigned char *uuid, + char *stateDir) +{ + char *linkdev = NULL; + virNetDevVPortProfilePtr virtPort; + int ret = -1; + int vf = -1; + int vlanid = -1; + int port_profile_associate = 1; + + if (qemuDomainHostdevNetDevice(hostdev, &linkdev, &vf) < 0) + return ret; + + virtPort = virDomainNetGetActualVirtPortProfile( + hostdev->parent.data.net); + if (virtPort) + ret = qemuDomainHostdevNetConfigVirtPortProfile(linkdev, vf, + virtPort, + hostdev->parent.data.net->mac, + uuid, + port_profile_associate); + else + /* Set only mac */ + ret = virNetDevReplaceNetConfig(linkdev, vf, + hostdev->parent.data.net->mac, vlanid, + stateDir); + + VIR_FREE(linkdev); + + return ret; +} + +int +qemuDomainHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, + char *stateDir) +{ + char *linkdev = NULL; + virNetDevVPortProfilePtr virtPort; + int ret = -1; + int vf = -1; + int port_profile_associate = 0; + + if (qemuDomainHostdevNetDevice(hostdev, &linkdev, &vf) < 0) + return ret; + + virtPort = virDomainNetGetActualVirtPortProfile( + hostdev->parent.data.net); + if (virtPort) + ret = qemuDomainHostdevNetConfigVirtPortProfile(linkdev, vf, + virtPort, + hostdev->parent.data.net->mac, + NULL, + port_profile_associate); + else + ret = virNetDevRestoreNetConfig(linkdev, vf, stateDir); + + VIR_FREE(linkdev); + + return ret; +} + int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, const char *name, + const unsigned char *uuid, virDomainHostdevDefPtr *hostdevs, int nhostdevs) { pciDeviceList *pcidevs; + int last_processed_hostdev_vf = -1; int i; int ret = -1; if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs))) return -1; - /* We have to use 7 loops here. *All* devices must + /* We have to use 9 loops here. *All* devices must * be detached before we reset any of them, because * in some cases you have to reset the whole PCI, * which impacts all devices on it. Also, all devices @@ -225,7 +392,29 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, goto reattachdevs; } - /* Loop 4: Now mark all the devices as active */ + /* Loop 4: For SRIOV network devices, Now that we have detached the + * the network device, set the netdev config */ + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + continue; + if (hostdev->parent.data.net && + qemuDomainHostdevIsVirtualFunction(hostdev) == 1) { + if (qemuDomainHostdevNetConfigReplace(hostdev, uuid, + driver->stateDir) < 0) + goto resetvfnetconfig; + } else { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Interface type hostdev is only supported " + "with sriov VF's")); + goto resetvfnetconfig; + } + last_processed_hostdev_vf = i; + } + + /* Loop 5: Now mark all the devices as active */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); if (pciDeviceListAdd(driver->activePciHostdevs, dev) < 0) { @@ -234,13 +423,13 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, } } - /* Loop 5: Now remove the devices from inactive list. */ + /* Loop 6: Now remove the devices from inactive list. */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); pciDeviceListDel(driver->inactivePciHostdevs, dev); } - /* Loop 6: Now set the used_by_domain of the device in + /* Loop 7: Now set the used_by_domain of the device in * driver->activePciHostdevs as domain name. */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { @@ -252,7 +441,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, pciDeviceSetUsedBy(activeDev, name); } - /* Loop 7: Now set the original states for hostdev def */ + /* Loop 8: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { pciDevice *dev; pciDevice *pcidev; @@ -284,7 +473,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, pciFreeDevice(dev); } - /* Loop 8: Now steal all the devices from pcidevs */ + /* Loop 9: Now steal all the devices from pcidevs */ while (pciDeviceListCount(pcidevs) > 0) { pciDevice *dev = pciDeviceListGet(pcidevs, 0); pciDeviceListSteal(pcidevs, dev); @@ -302,6 +491,14 @@ inactivedevs: pciDeviceListSteal(driver->activePciHostdevs, dev); } +resetvfnetconfig: + for (i = 0; i < last_processed_hostdev_vf; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + if (hostdev->parent.data.net && + qemuDomainHostdevIsVirtualFunction(hostdev) == 1) + qemuDomainHostdevNetConfigRestore(hostdev, driver->stateDir); + } + reattachdevs: for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); @@ -317,10 +514,10 @@ static int qemuPrepareHostPCIDevices(struct qemud_driver *driver, virDomainDefPtr def) { - return qemuPrepareHostdevPCIDevices(driver, def->name, def->hostdevs, def->nhostdevs); + return qemuPrepareHostdevPCIDevices(driver, def->name, def->uuid, + def->hostdevs, def->nhostdevs); } - int qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, const char *name, @@ -494,8 +691,10 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, return; } - /* Again 3 loops; mark all devices as inactive before reset - * them and reset all the devices before re-attach */ + /* Again 4 loops; mark all devices as inactive before reset + * them and reset all the devices before re-attach. + * Attach mac and port profile parameters to devices + */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); pciDevice *activeDev = NULL; @@ -514,6 +713,21 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, pciDeviceListSteal(driver->activePciHostdevs, dev); } + /* + * For SRIOV net host devices, unset mac and port profile before + * reset and reattach device + */ + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + continue; + if (hostdev->parent.data.net && + qemuDomainHostdevIsVirtualFunction(hostdev) == 1) + qemuDomainHostdevNetConfigRestore(hostdev, driver->stateDir); + } + for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); if (pciResetDevice(dev, driver->activePciHostdevs, @@ -540,5 +754,6 @@ void qemuDomainReAttachHostDevices(struct qemud_driver *driver, if (!def->nhostdevs) return; - qemuDomainReAttachHostdevDevices(driver, def->name, def->hostdevs, def->nhostdevs); + qemuDomainReAttachHostdevDevices(driver, def->name, def->hostdevs, + def->nhostdevs); } diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index d852f5b..173e4f4 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -31,6 +31,7 @@ int qemuUpdateActivePciHostdevs(struct qemud_driver *driver, virDomainDefPtr def); int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, const char *name, + const unsigned char *uuid, virDomainHostdevDefPtr *hostdevs, int nhostdevs); int qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, @@ -46,6 +47,11 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, int nhostdevs); void qemuDomainReAttachHostDevices(struct qemud_driver *driver, virDomainDefPtr def); - +int qemuDomainHostdevIsVirtualFunction(virDomainHostdevDefPtr hostdev); +int qemuDomainHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, + const unsigned char *uuid, + char *stateDir); +int qemuDomainHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, + char *stateDir); #endif /* __QEMU_HOSTDEV_H__ */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 50563c5..e36cf97 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -927,7 +927,8 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, return -1; } - if (qemuPrepareHostdevPCIDevices(driver, vm->def->name, &hostdev, 1) < 0) + if (qemuPrepareHostdevPCIDevices(driver, vm->def->name, vm->def->uuid, + &hostdev, 1) < 0) return -1; if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { @@ -1886,6 +1887,14 @@ qemuDomainDetachHostPciDevice(struct qemud_driver *driver, if (ret < 0) return -1; + /* + * For SRIOV net host devices, unset mac and port profile before + * reset and reattach device + */ + if (detach->parent.data.net && + qemuDomainHostdevIsVirtualFunction(detach) == 1) + qemuDomainHostdevNetConfigRestore(detach, driver->stateDir); + pci = pciGetDevice(subsys->u.pci.domain, subsys->u.pci.bus, subsys->u.pci.slot, subsys->u.pci.function); if (pci) {
participants (2)
-
Laine Stump -
Roopa Prabhu