On 06/03/2013 06:05 AM, Osier Yang wrote: > The name format is constructed by libvirt, it's not that clear to > get what the device's sysfs path should be. This exposes the device's > sysfs path by a new tag <path>. > > Since the sysfspath is filled during enumerating the devices by > either udev or HAL. It's an output-only tag. > --- > src/conf/node_device_conf.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index 4eeb3b3..cc6f297 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -236,6 +236,7 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDefPtr def) > > virBufferAddLit(&buf, "<device>\n"); > virBufferEscapeString(&buf, " <name>%s</name>\n", def->name); > + virBufferEscapeString(&buf, " <path>%s</path>\n", def->sysfs_path); Is this necessarily filled in? EG, do you need to do an "if (def->sysfs_path)" first?

ACK John > if (def->parent) { > virBufferEscapeString(&buf, " <parent>%s</parent>\n", def->parent); > } >

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

On 06/03/2013 06:05 AM, Osier Yang wrote: > Since scsi generic device doesn't have DEVTYPE property set, the > only way to we have to get it's a scsi generic device is from the > "SUBSYSTEM" property. I think you meant possessive case not the conjunction of "it is", e.g. s/it's a/its/ and perhaps s/generic device is/generic device data is/ > The XML of the scsi generic device will be like: > > <device> > <name>scsi_generic_sg0</name> > <path>/sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0</path> > <parent>scsi_0_0_0_0</parent> > <capability type='scsi_generic'> > <char>/dev/sg0</char>6d8528e3e104c34d477d478e3adf608a6e1bf7e2 > </capability> > </device> > --- > src/conf/node_device_conf.c | 6 +++++- > src/conf/node_device_conf.h | 5 +++++ > src/node_device/node_device_udev.c | 24 ++++++++++++++++++++++++ > 3 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index cc6f297..a0b6338 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -49,7 +49,8 @@ VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST, > "scsi", > "storage", > "fc_host", > - "vports") > + "vports", > + "scsi_generic") > > VIR_ENUM_IMPL(virNodeDevNetCap, VIR_NODE_DEV_CAP_NET_LAST, > "80203", > @@ -472,6 +473,9 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDefPtr def) > virBufferAddLit(&buf, > " <capability type='hotpluggable' />\n"); > break; > + case VIR_NODE_DEV_CAP_SCSI_GENERIC: > + virBufferEscapeString(&buf, " <char>%s</char>\n", > + data->sg.path); break; Just so no one inadvertently changes any of the next cases.. > case VIR_NODE_DEV_CAP_FC_HOST: > case VIR_NODE_DEV_CAP_VPORTS: > case VIR_NODE_DEV_CAP_LAST: > diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h > index 1c5855c..e326e82 100644 > --- a/src/conf/node_device_conf.h > +++ b/src/conf/node_device_conf.h > @@ -48,6 +48,8 @@ enum virNodeDevCapType { > VIR_NODE_DEV_CAP_STORAGE, /* Storage device */ > VIR_NODE_DEV_CAP_FC_HOST, /* FC Host Bus Adapter */ > VIR_NODE_DEV_CAP_VPORTS, /* HBA which is capable of vports */ > + VIR_NODE_DEV_CAP_SCSI_GENERIC, /* SCSI generic device */ > + > VIR_NODE_DEV_CAP_LAST > }; > > @@ -165,6 +167,9 @@ struct _virNodeDevCapsDef { > char *media_label; > unsigned int flags; /* virNodeDevStorageCapFlags bits */ > } storage; > + struct { > + char *path; > + } sg; /* SCSI generic device */ > } data; > virNodeDevCapsDefPtr next; /* next capability */ > }; > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c > index 3c91298..27a48cf 100644 > --- a/src/node_device/node_device_udev.c > +++ b/src/node_device/node_device_udev.c > @@ -1099,6 +1099,21 @@ out: > return ret; > } > > +static int > +udevProcessScsiGeneric(struct udev_device *dev, > + virNodeDeviceDefPtr def) > +{ > + if (udevGetStringProperty(dev, > + "DEVNAME", > + &def->caps->data.sg.path) != PROPERTY_FOUND) > + return -1; > + When is data.sg.path VIR_FREE()'d? I think you need a case in the switch in 'virNodeDevCapsDefFree()' for VIR_NODE_DEV_CAP_SCSI_GENERIC: VIR_FREE(data->sg.path); break; > + if (udevGenerateDeviceName(dev, def, NULL) != 0) > + return -1; > + > + return 0; > +} > + > static bool > udevDeviceHasProperty(struct udev_device *dev, > const char *key) > @@ -1117,6 +1132,7 @@ udevGetDeviceType(struct udev_device *device, > enum virNodeDevCapType *type) > { > const char *devtype = NULL; > + char *subsystem = NULL; > int ret = -1; > > devtype = udev_device_get_devtype(device); > @@ -1148,6 +1164,11 @@ udevGetDevi6d8528e3e104c34d477d478e3adf608a6e1bf7e2ceType(struct udev_device *device, > * property exists, we have a network device. */ > if (udevDeviceHasProperty(device, "INTERFACE")) > *type = VIR_NODE_DEV_CAP_NET; How about a comment prior to the call like the other two in this else {} > + > + if (udevGetStringProperty(device, "SUBSYSTEM", &subsystem) == > + PROPERTY_FOUND && > + STREQ(subsystem, "scsi_generic")) > + *type = VIR_NODE_DEV_CAP_SCSI_GENERIC; VIR_FREE(subsystem); (memory leak) > } > > if (!*type) > @@ -1194,6 +1215,9 @@ static int udevGetDeviceDetails(struct udev_device *device, > case VIR_NODE_DEV_CAP_STORAGE: > ret = udevProcessStorage(device, def); > break; > + case VIR_NODE_DEV_CAP_SCSI_GENERIC: > + ret = udevProcessScsiGeneric(device, def); > + break; > default: > VIR_ERROR(_("Unknown device type %d"), def->caps->type); > ret = -1; > If I understood what you said in the patch comments - this data isn't kept in the xml file and thus you wouldn't need a case in the switch in virNodeDevCapsDefParseXML(), correct? I searched for VIR_NODE_DEV_CAP_STORAGE and made sure corresponding entries were made for VIR_NODE_DEV_CAP_SCSI_GENERIC. ACK w/ issues addressed (added break;, case in virNodeDevCapsDefFree(), comments, and VIR_FREE(). I didn't see a case in the DefParseXML() for SCSI_GENERIC and wanted to be sure.