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

[Osier Yang]

On 07/06/13 04:33, John Ferlan wrote:
> 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...

Just curious, why one want to add more than 1 blank lines among
stuffs? Does it really make the code more readable? I think most of
the cases we still use 1 blank line in the source.

>
>>   /* 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...

Ack'ed udevHasDeviceProperty is better, to be consistent with the names
in current udev backend, though it needs thorough cleanup. But it will
be later patches.

>
>>   {
>> -    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...

Okay.

>
>>   
>> -    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