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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list