[libvirt] [PATCH 10/11] Improve SCSI volume key and name generation

[Dave Allan]

On Fri, Nov 12, 2010 at 04:22:43PM +0000, Daniel P. Berrange wrote:
> The SCSI volumes currently get a name like '17:0:0:1' based
> on $host:$bus:$target:$lun. The names are intended to be unique
> per pool and stable across pool restarts. The inclusion of the
> $host component breaks this, because the $host number for iSCSI
> pools is dynamically allocated by the kernel at time of login.
> This changes the name to be 'unit:0:0:1', ie removes the leading
> host component. THe 'unit:' prefix is just to ensure the volume
> name doesn't start with a number and make it clearer when seen
> out of context.

None of the host/bus/target/LUN values are really stable, although for
our purposes LUN is likely to be.  Have you considered LUN$lun for the
volume names?

> The SCSI volumes also get a 'key' field based on the fully
> qualified volume path. All SCSI volumes have a unique serial
> available in hardware which can be obtained by sending a
> suitable SCSI command. Call out to udev's 'scsi_id' command
> to fetch this value

This is a great addition.

> * src/storage/storage_backend_scsi.c: Improve key and name
>   field value stability and uniqness
> ---
>  src/storage/storage_backend_scsi.c |   71 +++++++++++++++++++++++++++++++++---
>  1 files changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index 0d6b1ac..aeabd36 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -163,9 +163,66 @@ cleanup:
>      return ret;
>  }
>  
> +static char *
> +virStorageBackendSCSISerial(const char *dev)
> +{
> +    const char *cmdargv[] = {
> +        "/lib/udev/scsi_id",
> +        "--replace-whitespace",
> +        "--whitelisted",
> +        "--device", dev,
> +        NULL
> +    };
> +    int fd = -1;
> +    pid_t child;
> +    FILE *list = NULL;
> +    char line[1024];
> +    char *serial = NULL;
> +    int err;
> +    int exitstatus;
> +
> +    /* Run the program and capture its output */
> +    if (virExec(cmdargv, NULL, NULL,
> +                &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0)
> +        goto cleanup;
> +
> +    if ((list = fdopen(fd, "r")) == NULL) {
> +        virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> +                              "%s", _("cannot read fd"));
> +        goto cleanup;
> +    }
> +
> +    if (fgets(line, sizeof(line), list)) {
> +        char *nl = strchr(line, '\n');
> +        if (nl)
> +            *nl = '\0';
> +        VIR_ERROR("GOT ID %s\n", line);
> +        serial = strdup(line);
> +    } else {
> +        VIR_ERROR("NO ID %s\n", dev);
> +        serial = strdup(dev);
> +    }
> +
> +    if (!serial) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +cleanup:
> +    if (list)
> +        fclose(list);
> +    else
> +        VIR_FORCE_CLOSE(fd);
> +
> +    while ((err = waitpid(child, &exitstatus, 0) == -1) && errno == EINTR);
> +
> +    return serial;
> +}
> +
> +
>  static int
>  virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
> -                            uint32_t host,
> +                            uint32_t host ATTRIBUTE_UNUSED,
>                              uint32_t bus,
>                              uint32_t target,
>                              uint32_t lun,
> @@ -183,7 +240,12 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>  
>      vol->type = VIR_STORAGE_VOL_BLOCK;
>  
> -    if (virAsprintf(&(vol->name), "%u.%u.%u.%u", host, bus, target, lun) < 0) {
> +    /* 'host' is dynamically allocated by the kernel, first come,
> +     * first served, per HBA. As such it isn't suitable for use
> +     * in the volume name. We only need uniquness per-pool, so
> +     * just leave 'host' out
> +     */
> +    if (virAsprintf(&(vol->name), "unit:%u:%u:%u", bus, target, lun) < 0) {
>          virReportOOMError();
>          retval = -1;
>          goto free_vol;
> @@ -231,10 +293,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>          goto free_vol;
>      }
>  
> -    /* XXX should use logical unit's UUID instead */
> -    vol->key = strdup(vol->target.path);
> -    if (vol->key == NULL) {
> -        virReportOOMError();
> +    if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) {
>          retval = -1;
>          goto free_vol;
>      }
> -- 
> 1.7.2.3
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list