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

[John Ferlan]

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