5:29 p.m.
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