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