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.