[libvirt] [PATCH 3/6] nodedev_udev: Enumerate scsi generic device
Osier Yang
jyang at redhat.com
Tue Jun 18 09:35:49 UTC 2013
On 07/06/13 04:38, John Ferlan wrote:
> 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.
I don't see why we have to parse the XML of node device, except
the XML for vHBA, for which we provide API to create it, and PCI
device, qemu and xen drivers needs to collect host PCI device
addresses.
So I didn't add parsing helper for scsi generic device.
For other nits in this patch and other patches, I fixed them. Pushed
the set.
Thanks for the reviewing!
Osier
More information about the libvir-list
mailing list