[libvirt] [PATCH 08/16] Rename ifaceGetIndex and ifaceGetVLAN

[Laine Stump]

On 11/15/2011 06:14 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> Rename the ifaceGetIndex method to virNetDevGetIndex and
> ifaceGetVlanID to virNetDevGetVLanID. Also change the error
> reporting behaviour to always raise errors and return -1 on
> failure
>
> * util/interface.c, util/interface.h: Rename ifaceGetIndex
>    and ifaceGetVLAN
> * nwfilter/nwfilter_gentech_driver.c, nwfilter/nwfilter_learnipaddr.c,
>    nwfilter/nwfilter_learnipaddr.c, util/virnetdevvportprofile.c: Update
>    for API renames and error handling changes
> ---
>   src/libvirt_private.syms               |    4 +-
>   src/nwfilter/nwfilter_gentech_driver.c |   13 +++--
>   src/nwfilter/nwfilter_learnipaddr.c    |   22 ++++---
>   src/util/interface.c                   |  103 ++++++++++++++++----------------
>   src/util/interface.h                   |    6 +-
>   src/util/virnetdevmacvlan.c            |   17 ++++--
>   src/util/virnetdevvportprofile.c       |    6 +-
>   7 files changed, 92 insertions(+), 79 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d71186b..e621f79 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -576,12 +576,12 @@ virHookPresent;
>
>   # interface.h
>   ifaceCheck;
> -ifaceGetIndex;
> +virNetDevGetIndex;
>   ifaceGetIPAddress;
>   ifaceGetNthParent;
>   ifaceGetPhysicalFunction;
>   ifaceGetVirtualFunctionIndex;
> -ifaceGetVlanID;
> +virNetDevGetVLanID;
>   ifaceIsVirtualFunction;
>   virNetDevMacVLanCreate;
>   virNetDevMacVLanDelete;
> diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
> index 899bd32..9f44aef 100644
> --- a/src/nwfilter/nwfilter_gentech_driver.c
> +++ b/src/nwfilter/nwfilter_gentech_driver.c
> @@ -903,9 +903,10 @@ _virNWFilterInstantiateFilter(virConnectPtr conn,
>       /* after grabbing the filter update lock check for the interface; if
>          it's not there anymore its filters will be or are being removed
>          (while holding the lock) and we don't want to build new ones */
> -    if (ifaceGetIndex(false, net->ifname,&ifindex)<  0) {
> +    if (virNetDevGetIndex(net->ifname,&ifindex)<  0) {
>           /* interfaces / VMs can disappear during filter instantiation;
>              don't mark it as an error */
> +        virResetLastError();


But since the error has already been logged, it isn't really being 
ignored. Based on past experience with other "errors that aren't really 
errors", I'm betting this will lead to bogus bug reports (unless it's 
*extremely* rare, and requires doing something way out of the ordinary 
such that the admin might expect to get spurious error messages). Maybe 
virNetDevGetIndex could have some sort of "allow/ignore/dontreport 
failure" flag added?


> diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
> index 68bdcfc..9a51fc2 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.c
> +++ b/src/nwfilter/nwfilter_learnipaddr.c
> @@ -252,21 +252,23 @@ virNWFilterTerminateLearnReq(const char *ifname) {
>       int ifindex;
>       virNWFilterIPAddrLearnReqPtr req;
>
> -    if (ifaceGetIndex(false, ifname,&ifindex) == 0) {
> -
> -        IFINDEX2STR(ifindex_str, ifindex);
> +    if (virNetDevGetIndex(ifname,&ifindex)<  0) {
> +        virResetLastError();
> +        return rc;
> +    }
>
> -        virMutexLock(&pendingLearnReqLock);
> +    IFINDEX2STR(ifindex_str, ifindex);
>
> -        req = virHashLookup(pendingLearnReq, ifindex_str);
> -        if (req) {
> -            rc = 0;
> -            req->terminate = true;
> -        }
> +    virMutexLock(&pendingLearnReqLock);
>
> -        virMutexUnlock(&pendingLearnReqLock);
> +    req = virHashLookup(pendingLearnReq, ifindex_str);
> +    if (req) {
> +        rc = 0;
> +        req->terminate = true;
>       }
>
> +    virMutexUnlock(&pendingLearnReqLock);
> +
>       return rc;

git/diff didn't go to any pains to make *that* hunk easy to follow :-/

original:

     if (ifaceGetIndex(false, ifname,&ifindex) == 0) {

         IFINDEX2STR(ifindex_str, ifindex);

         virMutexLock(&pendingLearnReqLock);

         req = virHashLookup(pendingLearnReq, ifindex_str);
         if (req) {
             rc = 0;
             req->terminate = true;
         }

         virMutexUnlock(&pendingLearnReqLock);
      }

      return rc;


vs new:

     if (virNetDevGetIndex(ifname,&ifindex)<  0) {
         virResetLastError();
         return rc;
     }

     IFINDEX2STR(ifindex_str, ifindex);

     virMutexLock(&pendingLearnReqLock);

     req = virHashLookup(pendingLearnReq, ifindex_str);
     if (req) {
         rc = 0;
         req->terminate = true;
      }

     virMutexUnlock(&pendingLearnReqLock);

     return rc;

so that looks okay (modulo the issue with virNetDevGetIndex errors being 
logged rather than ignored).


I'm uncomfortable enough with the change in error behavior that I don't 
want to ACK this as-is.