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

Re: [libvirt] [PATCH 2/4] virtnetdev: Add support functions for mac and portprofile associations on a hostdev



On 03/05/2012 02:50 PM, Roopa Prabhu 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 ?

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.
>
>


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