[libvirt] [PATCH v3 11/34] Adapt to VIR_STRDUP and VIR_STRNDUP in src/node_device/*

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130508/7d570dbe/attachment-0001.sig>