On 05/03/2013 08:53 AM, Michal Privoznik wrote:
--- src/node_device/node_device_driver.c | 33 ++++-------- src/node_device/node_device_hal.c | 13 ++--- src/node_device/node_device_udev.c | 99 ++++++++++-------------------------- 3 files changed, 46 insertions(+), 99 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 05dc49c..8a84094 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -89,13 +89,8 @@ static int update_driver_name(virNodeDeviceObjPtr dev) }
p = strrchr(devpath, '/'); - if (p) { - dev->def->driver = strdup(p+1); - if (!dev->def->driver) { - virReportOOMError(); - goto cleanup; - } - } + if (p && VIR_STRDUP(dev->def->driver, p+1) < 0)
spaces around + while you are touching this
+++ b/src/node_device/node_device_hal.c @@ -446,10 +446,10 @@ static void dev_create(const char *udi) virNodeDeviceDefPtr def = NULL; const char *name = hal_name(udi); int rv; - char *privData = strdup(udi); + char *privData; char *devicePath = NULL;
- if (!privData) + if (VIR_STRDUP(privData, udi) < 0) return;
silent->noisy in a void function, but the rest of the function is just using VIR_DEBUG on failure, so I think this is an improvement. Not to mention that HAL code is used less and less these days.
+++ b/src/node_device/node_device_udev.c @@ -387,21 +383,9 @@ static int udevTranslatePCIIds(unsigned int vendor, NULL, NULL);
- if (vendor_name != NULL) { - *vendor_string = strdup(vendor_name); - if (*vendor_string == NULL) { - virReportOOMError(); - goto out; - } - } - - if (device_name != NULL) { - *product_string = strdup(device_name); - if (*product_string == NULL) { - virReportOOMError(); - goto out; - } - } + if ((vendor_name && VIR_STRDUP(*vendor_string, vendor_name) < 0) || + (device_name && VIR_STRDUP(*product_string, device_name) < 0)) + goto out;
Can be simplified now that we allow NULL source.
@@ -718,34 +699,34 @@ static int udevGetSCSIType(virNodeDeviceDefPtr def ATTRIBUTE_UNUSED,
switch (type) { case TYPE_DISK: - *typestring = strdup("disk"); + ignore_value(VIR_STRDUP(*typestring, "disk")); break; case TYPE_TAPE: - *typestring = strdup("tape"); + ignore_value(VIR_STRDUP(*typestring, "tape")); break;
Another situation where you could reduce the number of ignore_value() by assigning to a const char*, then saving the dup until after the switch finishes - although such a rework would be no real semantic change. For that matter, this whole switch statement could be rewritten with VIR_ENUM_* magic, but that would be a separate cleanup.
@@ -996,10 +973,7 @@ static int udevKludgeStorageType(virNodeDeviceDefPtr def)
if (STRPREFIX(def->caps->data.storage.block, "/dev/vd")) { /* virtio disk */ - def->caps->data.storage.drive_type = strdup("disk"); - if (def->caps->data.storage.drive_type != NULL) { - ret = 0; - } + ret = VIR_STRDUP(def->caps->data.storage.drive_type, "disk");
silent->noisy, but I think it's an improvement.
@@ -1043,7 +1017,7 @@ static int udevProcessStorage(struct udev_device *device, VIR_DEBUG("No devnode for '%s'", udev_device_get_devpath(device)); goto out; } - data->storage.block = strdup(devnode); + ignore_value(VIR_STRDUP(data->storage.block, devnode));
Pre-existing bug - udevKludgeStorageType() dereferences data->storage.block, and expects it to be non-null. Please touch this up to 'goto out' rather than silently pushing on in spite of allocation failure.
@@ -1294,32 +1266,20 @@ static int udevSetParent(struct udev_device *device, dev = virNodeDeviceFindBySysfsPath(&driverState->devs, parent_sysfs_path); if (dev != NULL) { - def->parent = strdup(dev->def->name); - virNodeDeviceObjUnlock(dev); - - if (def->parent == NULL) { - virReportOOMError(); + if (VIR_STRDUP(def->parent, dev->def->name) < 0) { goto out; + virNodeDeviceObjUnlock(dev);
Umm, that order won't work. Swap those two lines.
@@ -1339,7 +1299,7 @@ static int udevAddOneDevice(struct udev_device *device) goto out; }
- def->sysfs_path = strdup(udev_device_get_syspath(device)); + ignore_value(VIR_STRDUP(def->sysfs_path, udev_device_get_syspath(device)));
Another pre-existing bug where we should 'goto out' instead of leaving sysfs_path NULL, since several places in the rest of the file blindly dereference the field. For that matter, the 'out' label itself is one such place - fix that VIR_DEBUG to use NULLSTR(def->sysfs_path). ACK with the bugs fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org