[libvirt] [PATCH v2 3/4] util: Introduce virStorageFileGetNPIVKey

[Michal Privoznik]

On 1/18/19 3:42 PM, John Ferlan wrote:
> The vHBA/NPIV LUNs created via the udev processing of the
> VPORT_CREATE command end up using the same serial value
> as seen/generated by the /lib/udev/scsi_id as returned
> during virStorageFileGetSCSIKey. Therefore, in order to
> generate a unique enough key to be used when adding the
> LUN as a volume during virStoragePoolObjAddVol a more
> unique key needs to be generated for an NPIV volume.
> 
> The problem is illustrated by the following example, where
> scsi_host5 is a vHBA used with the following LUNs:
> 
> $ lsscsi -tg
> ...
> [5:0:4:0]    disk    fc:0x5006016844602198,0x101f00  /dev/sdh   /dev/sg23
> [5:0:5:0]    disk    fc:0x5006016044602198,0x102000  /dev/sdi   /dev/sg24
> ...
> 
> Calling virStorageFileGetSCSIKey would return:
> 
> /lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace /dev/sdh
> 350060160c460219850060160c4602198
> /lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace /dev/sdi
> 350060160c460219850060160c4602198
> 
> Note that althrough /dev/sdh and /dev/sdi are separate LUNs, they
> end up with the same serial number used for the vol->key value.
> When virStoragePoolFCRefreshThread calls virStoragePoolObjAddVol
> the second LUN fails to be added with the following message
> getting logged:
> 
>      virHashAddOrUpdateEntry:341 : internal error: Duplicate key
> 
> To resolve this, virStorageFileGetNPIVKey will use a similar call
> sequence as virStorageFileGetSCSIKey, except that it will add the
> "--export" option to the call. This results in more detailed output
> which needs to be parsed in order to formulate a unique enough key
> to be used. In order to be unique enough, the returned value will
> concatenate the target port as returned in the "ID_TARGET_PORT"
> field from the command to the "ID_SERIAL" value.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>   src/libvirt_private.syms  |  1 +
>   src/util/virstoragefile.c | 80 +++++++++++++++++++++++++++++++++++++++
>   src/util/virstoragefile.h |  2 +
>   3 files changed, 83 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c3d6306809..bdc2877a9f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2861,6 +2861,7 @@ virStorageFileGetMetadata;
>   virStorageFileGetMetadataFromBuf;
>   virStorageFileGetMetadataFromFD;
>   virStorageFileGetMetadataInternal;
> +virStorageFileGetNPIVKey;
>   virStorageFileGetRelativeBackingPath;
>   virStorageFileGetSCSIKey;
>   virStorageFileGetUniqueIdentifier;
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 2511511d14..759d0625b6 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1490,6 +1490,86 @@ int virStorageFileGetSCSIKey(const char *path,
>   #endif
>   
>   
> +#ifdef WITH_UDEV
> +/* virStorageFileGetNPIVKey
> + * @path: Path to the NPIV device
> + * @key: Unique key to be returned
> + *
> + * Using a udev specific function, query the @path to get and return a
> + * unique @key for the caller to use. Unlike the GetSCSIKey method, an
> + * NPIV LUN is uniquely identified by it's ID_TARGET_PORT value.
> + *
> + * Returns:
> + *     0 On success, with the @key filled in or @key=NULL if the
> + *       returned output string didn't have the data we need to
> + *       formulate a unique key value
> + *    -1 When WITH_UDEV is undefined and a system error is reported
> + *    -2 When WITH_UDEV is defined, but calling virCommandRun fails
> + */
> +int
> +virStorageFileGetNPIVKey(const char *path,
> +                         char **key)
> +{
> +    int status;
> +    VIR_AUTOFREE(char *) outbuf = NULL;
> +    const char *serial;
> +    const char *port;
> +    virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id",
> +                                             "--replace-whitespace",
> +                                             "--whitelisted",
> +                                             "--export",
> +                                             "--device", path,
> +                                             NULL
> +                                             );
> +    int ret = -2;
> +
> +    *key = NULL;
> +
> +    /* Run the program and capture its output */
> +    virCommandSetOutputBuffer(cmd, &outbuf);
> +    if (virCommandRun(cmd, &status) < 0)
> +        goto cleanup;
> +
> +    /* Explicitly check status == 0, rather than passing NULL
> +     * to virCommandRun because we don't want to raise an actual
> +     * error in this scenario, just return a NULL key.
> +     */
> +    if (status == 0 && *outbuf &&
> +        (serial = strstr(outbuf, "ID_SERIAL=")) &&
> +        (port = strstr(outbuf, "ID_TARGET_PORT="))) {
> +        char *serial_eq = strchr(serial, '=');
> +        char *serial_nl = strchr(serial, '\n');
> +        char *port_eq = strchr(port, '=');
> +        char *port_nl = strchr(port, '\n');
> +
> +        if (serial_eq)
> +            serial = serial_eq + 1;
> +        if (serial_nl)
> +            *serial_nl = '\0';
> +        if (port_eq)
> +            port = port_eq + 1;
> +        if (port_nl)
> +            *port_nl = '\0';


This looks cleaner IMO:

# define ID_SERIAL "ID_SERIAL="
# define ID_TARGET_PORT "ID_TARGET_PORT="

and then the if() body:
         char *tmp;

         serial += strlen(ID_SERIAL);
         port += strlen(ID_TARGET_PORT);

         if ((tmp = strchr(serial, '\n')))
             *tmp = '\0';

         if ((tmp = strchr(port, '\n')))
             *tmp = '\0';

followed by virAsprintf() you already have there.
But I don't care that much.

Michal