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@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();
+ ignore_value(VIR_STRDUP(serial, dev));
return serial;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
>+ ignore_value(VIR_STRDUP(serial, dev));
> return serial;
> }
>
>--
>2.20.1
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com>
>Date: Tue, 29 Jan 2019 16:44:01 -0500
>Subject: [PATCH] Squash with previous
>
>Signed-off-by: John Ferlan <jferlan@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@redhat.com>
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.