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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list