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

Re: [libvirt] [PATCH] macvtap: Fix error return value convention/inconsistencies



On 10/28/2011 04:04 PM, Roopa Prabhu wrote:
From: Roopa Prabhu<roprabhu cisco com>

- changed some return 1's to return -1
- changed if (rc) error checks to if (rc<  0)
- fixed some other minor convention violations

I might have missed some. Can fix in another patch or can respin

Signed-off-by: Roopa Prabhu<roprabhu cisco com>
Reported-by: Eric Blake<eblake redhat com>
Reported-by: Laine Stump<laine laine org>

Looks mostly good. I'll squash in some fixes as I finish auditing your changes...

--- a/src/util/macvtap.c
+++ b/src/util/macvtap.c
@@ -210,8 +210,11 @@ configMacvtapTap(int tapfd, int vnet_hdr)
          rc_on_fail = -1;
          errmsg = _("cannot clean IFF_VNET_HDR flag on macvtap tap");
      } else if ((ifreq.ifr_flags&  IFF_VNET_HDR) == 0&&  vnet_hdr) {
-        if (ioctl(tapfd, TUNGETFEATURES,&features) != 0)
-            return errno;
+        if (ioctl(tapfd, TUNGETFEATURES,&features)<  0) {
+            virReportSystemError(errno, "%s",
+                   _("cannot get feature flags on macvtap tap"));
+            return -1;
+	}

No TABs.

          if ((features&  IFF_VNET_HDR)) {
              new_flags = ifreq.ifr_flags | IFF_VNET_HDR;
              errmsg = _("cannot set IFF_VNET_HDR flag on macvtap tap");
@@ -335,7 +338,7 @@ create_name:
                                   macaddress,
                                   linkdev,
                                   virtPortProfile,
-                                 vmuuid, vmOp) != 0) {
+                                 vmuuid, vmOp)<  0) {

Needed some touchups before vpAssociatePortProfileId had a safe return value. I also adjusted global callers, such as in src/qemu.

@@ -552,6 +554,8 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf,
          }
      }

+    return rc;
+

Spurious addition.

@@ -963,10 +964,13 @@ getPhysfnDev(const char *linkdev,
          *physfndev = strdup(linkdev);
          if (!*physfndev) {
              virReportOOMError();
-            rc = -1;
-        }
+            goto err_exit;
+	}
+	rc = 0;

More TABs.

@@ -1011,7 +1015,7 @@ doPortProfileOp8021Qbh(const char *ifname,
      case PREASSOCIATE_RR:
      case ASSOCIATE:
          rc = virGetHostUUID(hostuuid);
-        if (rc)
+        if (rc<  0)

Won't work unless we also fix virGetHostUUID. Let's save that one for later, since it touches 7 or so files.

@@ -1971,7 +1971,7 @@ pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link,
      }

      for (i = 0; i<  num_virt_fns; i++) {
-         if (pciConfigAddressEqual(vf_bdf, virt_fns[i])) {
+         if (pciConfigAddressEqual(vf_bdf, virt_fns[i]) == 1) {

Spurious change.

Here's what I added before pushing:


diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c
index decb0f2..838a31f 100644
--- i/src/qemu/qemu_migration.c
+++ w/src/qemu/qemu_migration.c
@@ -2527,7 +2527,7 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) {

virDomainNetGetActualDirectDev(net),

virDomainNetGetActualDirectVirtPortProfile(net),
                                          def->uuid,
-                                         VIR_VM_OP_MIGRATE_IN_FINISH) != 0)
+                                         VIR_VM_OP_MIGRATE_IN_FINISH) < 0)
                 goto err_exit;
         }
         last_good_net = i;
diff --git i/src/util/interface.c w/src/util/interface.c
index 4ab74b5..12bf7bc 100644
--- i/src/util/interface.c
+++ w/src/util/interface.c
@@ -1016,7 +1016,7 @@ ifaceMacvtapLinkDump(bool nltarget_kernel ATTRIBUTE_UNUSED,
  * Get the nth parent interface of the given interface. 0 is the interface
  * itself.
  *
- * Return 0 on success, != 0 otherwise
+ * Return 0 on success, < 0 otherwise
  */
 #if defined(__linux__) && WITH_MACVTAP
 int
@@ -1037,7 +1037,7 @@ ifaceGetNthParent(int ifindex, const char *ifname, unsigned int nthParent,

     while (!end && i <= nthParent) {
rc = ifaceMacvtapLinkDump(true, ifname, ifindex, tb, &recvbuf, NULL);
-        if (rc)
+        if (rc < 0)
             break;

         if (tb[IFLA_IFNAME]) {
diff --git i/src/util/macvtap.c w/src/util/macvtap.c
index 329cbf1..54dc670 100644
--- i/src/util/macvtap.c
+++ w/src/util/macvtap.c
@@ -214,7 +214,7 @@ configMacvtapTap(int tapfd, int vnet_hdr)
             virReportSystemError(errno, "%s",
                    _("cannot get feature flags on macvtap tap"));
             return -1;
-	}
+        }
         if ((features & IFF_VNET_HDR)) {
             new_flags = ifreq.ifr_flags | IFF_VNET_HDR;
             errmsg = _("cannot set IFF_VNET_HDR flag on macvtap tap");
@@ -295,7 +295,7 @@ openMacvtapTap(const char *tgifname,
      * emulate their switch in firmware.
      */
     if (mode == VIR_MACVTAP_MODE_PASSTHRU) {
-        if (ifaceReplaceMacAddress(macaddress, linkdev, stateDir) != 0) {
+        if (ifaceReplaceMacAddress(macaddress, linkdev, stateDir) < 0) {
             return -1;
         }
     }
@@ -473,7 +473,7 @@ getLldpadPid(void) {
  * status: pointer to a uint16 where the status will be written into
  *
  * Get the status from the IFLA_PORT_RESPONSE field; Returns 0 in
- * case of success, != 0 otherwise with error having been reported
+ * case of success, < 0 otherwise with error having been reported
  */
 static int
 getPortProfileStatus(struct nlattr **tb, int32_t vf,
@@ -482,7 +482,7 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf,
                      bool is8021Qbg,
                      uint16_t *status)
 {
-    int rc = 1;
+    int rc = -1;
     const char *msg = NULL;
     struct nlattr *tb_port[IFLA_PORT_MAX + 1] = { NULL, };

@@ -554,8 +554,6 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf,
         }
     }

-    return rc;
-
 err_exit:
     if (msg)
         macvtapError(VIR_ERR_INTERNAL_ERROR, "%s", msg);
@@ -753,6 +751,7 @@ buffer_too_small:
 }


+/* Returns 0 on success, -1 on general failure, and -2 on timeout */
 static int
 doPortProfileOpCommon(bool nltarget_kernel,
                       const char *ifname, int ifindex,
@@ -808,7 +807,7 @@ doPortProfileOpCommon(bool nltarget_kernel,
                     _("error %d during port-profile setlink on "
                       "interface %s (%d)"),
                     status, ifname, ifindex);
-            rc = 1;
+            rc = -1;
             break;
         }

@@ -863,13 +862,14 @@ getPhysdevAndVlan(const char *ifname, int *root_ifindex, char *root_ifname,

 # endif

+/* Returns 0 on success, -1 on general failure, and -2 on timeout */
 static int
 doPortProfileOp8021Qbg(const char *ifname,
                        const unsigned char *macaddr,
                        const virVirtualPortProfileParamsPtr virtPort,
                        enum virVirtualPortOp virtPortOp)
 {
-    int rc;
+    int rc = 0;

 # ifndef IFLA_VF_PORT_MAX

@@ -879,7 +879,7 @@ doPortProfileOp8021Qbg(const char *ifname,
     (void)virtPortOp;
     macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Kernel VF Port support was missing at compile time."));
-    rc = 1;
+    rc = -1;

 # else /* IFLA_VF_PORT_MAX */

@@ -896,7 +896,7 @@ doPortProfileOp8021Qbg(const char *ifname,

     if (getPhysdevAndVlan(ifname, &physdev_ifindex, physdev_ifname,
                           &vlanid) < 0) {
-        rc = 1;
+        rc = -1;
         goto err_exit;
     }

@@ -920,7 +920,7 @@ doPortProfileOp8021Qbg(const char *ifname,
     default:
         macvtapError(VIR_ERR_INTERNAL_ERROR,
                      _("operation type %d not supported"), virtPortOp);
-        rc = 1;
+        rc = -1;
         goto err_exit;
     }

@@ -969,8 +969,8 @@ getPhysfnDev(const char *linkdev,
         if (!*physfndev) {
             virReportOOMError();
             goto err_exit;
-	}
-	rc = 0;
+        }
+        rc = 0;
     }

 err_exit:
@@ -979,6 +979,7 @@ err_exit:
 }
 # endif /* IFLA_VF_PORT_MAX */

+/* Returns 0 on success, -1 on general failure, and -2 on timeout */
 static int
 doPortProfileOp8021Qbh(const char *ifname,
                        const unsigned char *macaddr,
@@ -986,7 +987,7 @@ doPortProfileOp8021Qbh(const char *ifname,
                        const unsigned char *vm_uuid,
                        enum virVirtualPortOp virtPortOp)
 {
-    int rc;
+    int rc = 0;

 # ifndef IFLA_VF_PORT_MAX

@@ -997,7 +998,7 @@ doPortProfileOp8021Qbh(const char *ifname,
     (void)virtPortOp;
     macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Kernel VF Port support was missing at compile time."));
-    rc = 1;
+    rc = -1;

 # else /* IFLA_VF_PORT_MAX */

@@ -1019,9 +1020,11 @@ doPortProfileOp8021Qbh(const char *ifname,
     switch (virtPortOp) {
     case PREASSOCIATE_RR:
     case ASSOCIATE:
-        rc = virGetHostUUID(hostuuid);
-        if (rc < 0)
+        errno = virGetHostUUID(hostuuid);
+        if (errno) {
+            rc = -1;
             goto err_exit;
+        }

         rc = doPortProfileOpCommon(nltarget_kernel, NULL, ifindex,
                                    macaddr,
@@ -1062,7 +1065,7 @@ doPortProfileOp8021Qbh(const char *ifname,
     default:
         macvtapError(VIR_ERR_INTERNAL_ERROR,
                      _("operation type %d not supported"), virtPortOp);
-        rc = 1;
+        rc = -1;
     }

 err_exit:
@@ -1087,7 +1090,7 @@ err_exit:
  * by the user, then this function returns without doing
  * anything.
  *
- * Returns 0 in case of success, != 0 otherwise with error
+ * Returns 0 in case of success, < 0 otherwise with error
  * having been reported.
  */
 int
diff --git i/src/util/pci.c w/src/util/pci.c
index 077f3a2..9d44edf 100644
--- i/src/util/pci.c
+++ w/src/util/pci.c
@@ -1707,9 +1707,9 @@ int pciDeviceIsAssignable(pciDevice *dev,
 #ifdef __linux__

 /*
- * returns 1 if equal and 0 if not
+ * returns true if equal
  */
-static int
+static bool
 pciConfigAddressEqual(struct pci_config_address *bdf1,
                       struct pci_config_address *bdf2)
 {
@@ -1971,7 +1971,7 @@ pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link,
     }

     for (i = 0; i < num_virt_fns; i++) {
-         if (pciConfigAddressEqual(vf_bdf, virt_fns[i]) == 1) {
+         if (pciConfigAddressEqual(vf_bdf, virt_fns[i])) {
              *vf_index = i;
              ret = 0;
              break;

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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