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

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(a)redhat.com&gt; >> --- >> 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(a)redhat.com&gt; Date: Tue, 29 Jan 2019 16:44:01 -0500 Subject: [PATCH] Squash with previous Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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);