[libvirt] [PATCH v2 2/4] storage: Rework virStorageBackendSCSISerial

[John Ferlan]

On 1/29/19 10:14 AM, Ján Tomko wrote:
> On Fri, Jan 18, 2019 at 09:42:35AM -0500, John Ferlan wrote:
>> Alter the code to use the virStorageFileGetSCSIKey helper
>> to fetch the unique key for the SCSI disk. Alter the logic
>> to follow the former code which would return a duplicate
>> of @dev when either the virCommandRun succeeded, but returned
>> an empty string or when WITH_UDEV was not true.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> src/storage/storage_util.c | 34 ++++++++--------------------------
>> 1 file changed, 8 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
>> index a84ee5b600..aa1af434de 100644
>> --- a/src/storage/storage_util.c
>> +++ b/src/storage/storage_util.c
>> @@ -3758,36 +3758,18 @@
>> virStorageBackendRefreshLocal(virStoragePoolObjPtr pool)
>> static char *
>> virStorageBackendSCSISerial(const char *dev)
>> {
>> +    int rc;
>>     char *serial = NULL;
>> -#ifdef WITH_UDEV
>> -    virCommandPtr cmd = virCommandNewArgList(
>> -        "/lib/udev/scsi_id",
>> -        "--replace-whitespace",
>> -        "--whitelisted",
>> -        "--device", dev,
>> -        NULL
>> -        );
>> -
>> -    /* Run the program and capture its output */
>> -    virCommandSetOutputBuffer(cmd, &serial);
>> -    if (virCommandRun(cmd, NULL) < 0)
>> -        goto cleanup;
>> -#endif
>>
>> -    if (serial && STRNEQ(serial, "")) {
>> -        char *nl = strchr(serial, '\n');
>> -        if (nl)
>> -            *nl = '\0';
>> -    } else {
>> -        VIR_FREE(serial);
>> -        ignore_value(VIR_STRDUP(serial, dev));
>> -    }
>> +    rc = virStorageFileGetSCSIKey(dev, &serial);
>> +    if (rc == 0 && serial)
>> +        return serial;
>>
>> -#ifdef WITH_UDEV
>> - cleanup:
>> -    virCommandFree(cmd);
>> -#endif
>> +    if (rc == -2)
>> +        return NULL;
>>
>> +    virResetLastError();
> 
> Every virReportError call logs the error into the configured log outputs
> and sets the thread-local error object.
> 
> This only resets the error object, there's no way to unlog the error.
> If it's expected operation, we should not log an error in the first
> place.
> 

It's not necessarily expected that we cannot run the command because
WITH_UDEV is undefined; however, leaving an error message that you can
recover from is something that many code paths use virResetLastError
without the need to alter called methods to tell them not to report an
error such as this.

Would you "prefer" to see a 3rd parameter to virStorageFileGetSCSIKey
(such as @ignoreError) which is only used to not log the ENOSYS? And do
you want to see that code?

John


> Jano
> 
> 
>> +    ignore_value(VIR_STRDUP(serial, dev));
>>     return serial;
>> }
>>
>> -- 
>> 2.20.1
>>
>> -- 
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list