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

Re: [libvirt] [PATCH] Remove hard dependency on DMI



On 03/03/2010 07:20 PM, Ed Swierk wrote:
On Wed, Mar 3, 2010 at 2:57 PM, Dave Allan<dallan redhat com>  wrote:
Although I use goto a lot, I generally try to avoid multiple labels within a
function, just because I think it gets out of hand really quickly.  Although
it's a slightly more invasive patch, would you refactor the code to look
something like what I've attached?  I haven't even compile tested it as I'm
running late, but that's the idea.

Is there a piece of code in libvirt that exemplifies the preferred
error handling style? (http://libvirt.org/hacking.html doesn't cover
this issue, as far as I can tell.) Just in the very small part of
libvirt I've hacked on recently I've found a variety of styles,
including

Agreed that we should add a statement to the hacking guide. My preferences are as follows.

- pair every allocation with a goto label that frees the allocation
and all the earlier ones, and goto the appropriate label on error

I like Robert Love's description of this style at the very end of the thread at:

http://kerneltrap.org/node/553/2131

I like this style, but my impression is that generally the libvirt community prefers to have a single label that frees everything, perhaps conditionally on error, unless it's absolutely necessary to have multiple labels.

I reworked udevSetupSystemDev into this style (which also fixes the bug you pointed out that it didn't properly free resources on error). The patch also makes failure to find DMI data non-fatal.

- don't use goto at all, and on error, do the necessary frees and
return -1, with each error case having to do one more free

I find this style troublesome to maintain, as any additional allocations require modifications to each error case.

- a combination of the above, with each error case doing the necessary
frees, but using goto out more or less as an alias for return -1

Again, I think duplicating the frees in each error case is less maintainable than having them in on place.

- none of the above, not bothering to free anything when an allocation
fails (see udevSetupSystemDev for an example)

Failure to cleanup is a bug. Please send mail (and, even better, patches) about any other instances you find.

There are probably arguments to be made for each of these styles, but
it would be helpful to know which of them is preferred when writing
new code or refactoring existing code.

That said, I'll gladly refactor my patch towards the preferred style.

--Ed

>From 23526641083527139548c68b4637bae8350d2f98 Mon Sep 17 00:00:00 2001
From: David Allan <dallan redhat com>
Date: Thu, 4 Mar 2010 13:17:24 -0500
Subject: [PATCH 1/1] Free resources on error in udev startup

* The udev driver didn't properly free resources that it allocates when setting up the 'computer' device in the error case.
---
 src/node_device/node_device_udev.c |   67 +++++++++++++++++++++--------------
 1 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index eee44c4..a6ca785 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1440,33 +1440,15 @@ out:
 }


-static int udevSetupSystemDev(void)
+static void
+udevGetDMIData(union _virNodeDevCapData *data)
 {
-    virNodeDeviceDefPtr def = NULL;
-    virNodeDeviceObjPtr dev = NULL;
     struct udev *udev = NULL;
     struct udev_device *device = NULL;
-    union _virNodeDevCapData *data = NULL;
     char *tmp = NULL;
-    int ret = -1;
-
-    if (VIR_ALLOC(def) != 0) {
-        virReportOOMError();
-        goto out;
-    }
-
-    def->name = strdup("computer");
-    if (def->name == NULL) {
-        virReportOOMError();
-        goto out;
-    }
-
-    if (VIR_ALLOC(def->caps) != 0) {
-        virReportOOMError();
-        goto out;
-    }

     udev = udev_monitor_get_udev(DRV_STATE_UDEV_MONITOR(driverState));
+
     device = udev_device_new_from_syspath(udev, DMI_DEVPATH);
     if (device == NULL) {
         device = udev_device_new_from_syspath(udev, DMI_DEVPATH_FALLBACK);
@@ -1477,8 +1459,6 @@ static int udevSetupSystemDev(void)
         }
     }

-    data = &def->caps->data;
-
     if (udevGetStringSysfsAttr(device,
                                "product_name",
                                &data->system.product_name) == PROPERTY_ERROR) {
@@ -1508,8 +1488,7 @@ static int udevSetupSystemDev(void)
                                &tmp) == PROPERTY_ERROR) {
         goto out;
     }
-    virUUIDParse(tmp, def->caps->data.system.hardware.uuid);
-    VIR_FREE(tmp);
+    virUUIDParse(tmp, data->system.hardware.uuid);

     if (udevGetStringSysfsAttr(device,
                                "bios_vendor",
@@ -1530,12 +1509,42 @@ static int udevSetupSystemDev(void)
         goto out;
     }

-    udev_device_unref(device);
+out:
+    VIR_FREE(tmp);
+    if (device != NULL) {
+        udev_device_unref(device);
+    }
+    return;
+}
+
+
+static int udevSetupSystemDev(void)
+{
+    virNodeDeviceDefPtr def = NULL;
+    virNodeDeviceObjPtr dev = NULL;
+    int ret = -1;
+
+    if (VIR_ALLOC(def) != 0) {
+        virReportOOMError();
+        goto out;
+    }
+
+    def->name = strdup("computer");
+    if (def->name == NULL) {
+        virReportOOMError();
+        goto out;
+    }
+
+    if (VIR_ALLOC(def->caps) != 0) {
+        virReportOOMError();
+        goto out;
+    }
+
+    udevGetDMIData(&def->caps->data);

     dev = virNodeDeviceAssignDef(&driverState->devs, def);
     if (dev == NULL) {
         VIR_ERROR("Failed to create device for '%s'", def->name);
-        virNodeDeviceDefFree(def);
         goto out;
     }

@@ -1544,6 +1553,10 @@ static int udevSetupSystemDev(void)
     ret = 0;

 out:
+    if (ret == -1) {
+        virNodeDeviceDefFree(def);
+    }
+
     return ret;
 }

-- 
1.6.5.5


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