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

Re: [libvirt] [PATCH 2/6] nodedev_udev: Refactor udevGetDeviceType



On 06/03/2013 06:05 AM, Osier Yang wrote:
> Checking if the "devtype" is NULL along with each "if" statements
> is bad. It wastes the performance, and also not good for reading.
> And also when the "devtype" is NULL, the logic is also not clear.
> 
> This reorgnizes the logic of with "if...else" and a bunch of "else if".
> 
> Other changes:
>    * Change the function style.
>    * Remove the useless debug statement.
>    * Get rid of the goto
>    * New helper udevDeviceHasProperty to simplify the logic for checking
>      if a property is existing for the device.
>    * Add comment to clarify "PCI devices don't set the DEVTYPE property"
>    * s/sysfs path/sysfs name/, as udev_device_get_sysname returns the
>      name instead of the full path. E.g. "sg0"
>    * Refactor the comment for setting VIR_NODE_DEV_CAP_NET cap type
>      a bit.
> ---
>  src/node_device/node_device_udev.c | 119 ++++++++++++++++---------------------
>  1 file changed, 52 insertions(+), 67 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 620cd58..3c91298 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -106,7 +106,6 @@ static int udevStrToLong_i(char const *s,
>      return ret;
>  }
>  
> -

I have a recollection to other recent changes where the extra line was
specifically added.  I personally don't care, but I figured I'd point it
out in case someone did...

>  /* This function allocates memory from the heap for the property
>   * value.  That memory must be later freed by some other code. */
>  static int udevGetDeviceProperty(struct udev_device *udev_device,
> @@ -1100,78 +1099,64 @@ out:
>      return ret;
>  }
>  
> -
> -static int udevGetDeviceType(struct udev_device *device,
> -                             enum virNodeDevCapType *type)
> +static bool
> +udevDeviceHasProperty(struct udev_device *dev,
> +                      const char *key)

Ditto with the extra line thing...

Also 'udevDevice' seems to be redundant doesn't it?  Perhaps
udevHasDeviceProperty but it's not that important...

>  {
> -    const char *devtype = NULL;
> -    char *tmp_string = NULL;
> -    unsigned int tmp = 0;
> -    int ret = 0;
> +    const char *value = NULL;
> +    bool ret = false;
>  
> -    devtype = udev_device_get_devtype(device);
> -    VIR_DEBUG("Found device type '%s' for device '%s'",
> -              NULLSTR(devtype), udev_device_get_sysname(device));
> -
> -    if (devtype != NULL && STREQ(devtype, "usb_device")) {
> -        *type = VIR_NODE_DEV_CAP_USB_DEV;
> -        goto out;
> -    }
> -
> -    if (devtype != NULL && STREQ(devtype, "usb_interface")) {
> -        *type = VIR_NODE_DEV_CAP_USB_INTERFACE;
> -        goto out;
> -    }
> -
> -    if (devtype != NULL && STREQ(devtype, "scsi_host")) {
> -        *type = VIR_NODE_DEV_CAP_SCSI_HOST;
> -        goto out;
> -    }
> -
> -    if (devtype != NULL && STREQ(devtype, "scsi_target")) {
> -        *type = VIR_NODE_DEV_CAP_SCSI_TARGET;
> -        goto out;
> -    }
> -
> -    if (devtype != NULL && STREQ(devtype, "scsi_device")) {
> -        *type = VIR_NODE_DEV_CAP_SCSI;
> -        goto out;
> -    }
> +    if ((value = udev_device_get_property_value(dev, key)))
> +        ret = true;

Coverity complains (UNUSED_VALUE):
1123 	

(1) Event returned_pointer: 	Pointer "value" returned by
"udev_device_get_property_value(dev, key)" is never used.

1124 	    if ((value = udev_device_get_property_value(dev, key)))


Essentially it's pointing out that value really isn't necessary...

>  
> -    if (devtype != NULL && STREQ(devtype, "disk")) {
> -        *type = VIR_NODE_DEV_CAP_STORAGE;
> -        goto out;
> -    }
> -
> -    if (devtype != NULL && STREQ(devtype, "wlan")) {
> -        *type = VIR_NODE_DEV_CAP_NET;
> -        goto out;
> -    }
> -
> -    if (udevGetUintProperty(device, "PCI_CLASS", &tmp, 16) == PROPERTY_FOUND) {
> -        *type = VIR_NODE_DEV_CAP_PCI_DEV;
> -        goto out;
> -    }
> +    return ret;
> +}
>  
> -    /* It does not appear that wired network interfaces set the
> -     * DEVTYPE property.  USB devices also have an INTERFACE property,
> -     * but they do set DEVTYPE, so if devtype is NULL and the
> -     * INTERFACE property exists, we have a network device. */
> -    if (devtype == NULL &&
> -        udevGetStringProperty(device,
> -                              "INTERFACE",
> -                              &tmp_string) == PROPERTY_FOUND) {
> -        VIR_FREE(tmp_string);
> -        *type = VIR_NODE_DEV_CAP_NET;
> -        goto out;
> -    }
> +static int
> +udevGetDeviceType(struct udev_device *device,
> +                  enum virNodeDevCapType *type)
> +{
> +    const char *devtype = NULL;
> +    int ret = -1;
>  
> -    VIR_DEBUG("Could not determine device type for device "
> -              "with sysfs path '%s'",
> -              udev_device_get_sysname(device));
> -    ret = -1;
> +    devtype = udev_device_get_devtype(device);
> +    *type = 0;
> +
> +    if (devtype) {
> +        if (STREQ(devtype, "usb_device"))
> +            *type = VIR_NODE_DEV_CAP_USB_DEV;
> +        else if (STREQ(devtype, "usb_interface"))
> +            *type = VIR_NODE_DEV_CAP_USB_INTERFACE;
> +        else if (STREQ(devtype, "scsi_host"))
> +            *type = VIR_NODE_DEV_CAP_SCSI_HOST;
> +        else if (STREQ(devtype, "scsi_target"))
> +            *type = VIR_NODE_DEV_CAP_SCSI_TARGET;
> +        else if (STREQ(devtype, "scsi_device"))
> +            *type = VIR_NODE_DEV_CAP_SCSI;
> +        else if (STREQ(devtype, "disk"))
> +            *type = VIR_NODE_DEV_CAP_STORAGE;
> +        else if (STREQ(devtype, "wlan"))
> +            *type = VIR_NODE_DEV_CAP_NET;
> +    } else {
> +        /* PCI devices don't set the DEVTYPE property. */
> +        if (udevDeviceHasProperty(device, "PCI_CLASS"))
> +            *type = VIR_NODE_DEV_CAP_PCI_DEV;
> +
> +        /* Wired network interfaces don't set the DEVTYPE property,
> +         * USB devices also have an INTERFACE property, but they do
> +         * set DEVTYPE, so if devtype is NULL and the INTERFACE
> +         * property exists, we have a network device. */
> +        if (udevDeviceHasProperty(device, "INTERFACE"))
> +            *type = VIR_NODE_DEV_CAP_NET;
> +    }
> +
> +    if (!*type)
> +        VIR_DEBUG("Could not determine device type for device "
> +                  "with sysfs name '%s'",
> +                  udev_device_get_sysname(device));
> +    else
> +        ret = 0;
>  
> -out:
>      return ret;
>  }
>  
> 

This mechanism seems better.  Fix the Coverity complaint for an ACK.

John


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