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

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

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

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

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