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

[Ján Tomko]

On Wed, Jan 30, 2019 at 11:55:10AM -0500, John Ferlan wrote:
>
>
>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.
>>
>> Jano
>>
>
>So does the attached resolve the concern/issue you have?
>
>Tks,
>
>John
>

>>From 6f9385248a9e78cc3619d02e4adf3205cbad874d Mon Sep 17 00:00:00 2001
>From: John Ferlan <jferlan at redhat.com>
>Date: Tue, 29 Jan 2019 16:44:01 -0500
>Subject: [PATCH] Squash with previous
>
>Signed-off-by: John Ferlan <jferlan at redhat.com>
>---
> src/locking/lock_driver_lockd.c |  2 +-
> src/storage/storage_util.c      |  3 +--
> src/util/virstoragefile.c       | 10 +++++++---
> src/util/virstoragefile.h       |  3 ++-
> 4 files changed, 11 insertions(+), 7 deletions(-)
>
>diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
>index 16fce551c3..4ffa92fc9b 100644
>--- a/src/locking/lock_driver_lockd.c
>+++ b/src/locking/lock_driver_lockd.c
>@@ -531,7 +531,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>         if (STRPREFIX(name, "/dev") &&
>             driver->scsiLockSpaceDir) {
>             VIR_DEBUG("Trying to find an SCSI ID for %s", name);
>-            if (virStorageFileGetSCSIKey(name, &newName) < 0)
>+            if (virStorageFileGetSCSIKey(name, &newName, false) < 0)
>                 goto cleanup;
> 
>             if (newName) {
>diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
>index d8fd76f6b6..85f1cbb57d 100644
>--- a/src/storage/storage_util.c
>+++ b/src/storage/storage_util.c
>@@ -3782,14 +3782,13 @@ virStorageBackendSCSISerial(const char *dev)
>     int rc;
>     char *serial = NULL;
> 
>-    rc = virStorageFileGetSCSIKey(dev, &serial);
>+    rc = virStorageFileGetSCSIKey(dev, &serial, true);
>     if (rc == 0 && serial)
>         return serial;
> 
>     if (rc == -2)
>         return NULL;
> 
>-    virResetLastError();
>     ignore_value(VIR_STRDUP(serial, dev));
>     return serial;
> }
>diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>index 2a38a08241..39c8511bef 100644
>--- a/src/util/virstoragefile.c
>+++ b/src/util/virstoragefile.c
>@@ -1429,6 +1429,7 @@ int virStorageFileGetLVMKey(const char *path,
> /* virStorageFileGetSCSIKey
>  * @path: Path to the SCSI device
>  * @key: Unique key to be returned
>+ * @ignoreError: Used to not report ENOSYS
>  *
>  * Using a udev specific function, query the @path to get and return a
>  * unique @key for the caller to use.
>@@ -1441,7 +1442,8 @@ int virStorageFileGetLVMKey(const char *path,
>  */
> int
> virStorageFileGetSCSIKey(const char *path,
>-                         char **key)
>+                         char **key,
>+                         bool ignoreError ATTRIBUTE_UNUSED)
> {
>     int status;
>     virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id",
>@@ -1481,9 +1483,11 @@ virStorageFileGetSCSIKey(const char *path,
> }
> #else
> int virStorageFileGetSCSIKey(const char *path,
>-                             char **key ATTRIBUTE_UNUSED)
>+                             char **key ATTRIBUTE_UNUSED,
>+                             bool ignoreError)
> {
>-    virReportSystemError(ENOSYS, _("Unable to get SCSI key for %s"), path);
>+    if (!ignoreError)
>+        virReportSystemError(ENOSYS, _("Unable to get SCSI key for %s"), path);
>     return -1;
> }
> #endif
>diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
>index 1d6161a2c7..03837e9e58 100644
>--- a/src/util/virstoragefile.h
>+++ b/src/util/virstoragefile.h
>@@ -390,7 +390,8 @@ bool virStorageIsRelative(const char *backing);
> int virStorageFileGetLVMKey(const char *path,
>                             char **key);
> int virStorageFileGetSCSIKey(const char *path,
>-                             char **key);
>+                             char **key,
>+                             bool ignoreError);
> 
> void virStorageAuthDefFree(virStorageAuthDefPtr def);
> virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src);

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190201/ca60aa52/attachment-0001.sig>