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:
>
>
> scsi_generic_sg0
>
> /sys/devices/pci:00/:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0
> scsi_0_0_0_0
>
> /dev/sg06d8528e3e104c34d477d478e3adf608a6e1bf7e2
>
>
> ---
> 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,
> "\n");
> break;
> +case VIR_NODE_DEV_CAP_SCSI_GENERIC:
> +virBufferEscapeString(&buf, "%s\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);
> +b
Re: [libvirt] [PATCH 3/6] nodedev_udev: Enumerate scsi generic device
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:
scsi_generic_sg0
/sys/devices/pci:00/:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/scsi_generic/sg0
scsi_0_0_0_0
/dev/sg06d8528e3e104c34d477d478e3adf608a6e1bf7e2
---
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,
"\n");
break;
+case VIR_NODE_DEV_CAP_SCSI_GENERIC:
+virBufferEscapeString(&buf, "%s\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 co
