[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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





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. 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]