Re: [libvirt] [PATCH 3/6] nodedev_udev: Enumerate scsi generic device

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.