[libvirt] [PATCH 3/4] virnetdevvportprofile: Changes to support portprofiles for hostdevs
Roopa Prabhu
roprabhu at cisco.com
Mon Mar 5 19:52:52 UTC 2012
On 3/5/12 11:16 AM, "Laine Stump" <laine at 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 at 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 at 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.
More information about the libvir-list
mailing list