Re: [libvirt] [PATCH 3/4] virnetdevvportprofile: Changes to support portprofiles for hostdevs

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(a)cisco.com&gt; > > 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(a)cisco.com&gt; > --- > 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.