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.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list