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

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



On 11/15/2011 06:14 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange 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.


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