[PATCH v2] Fix error reporting when fetching SCSI/LVM keys

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>

The current  virStorageFileGet{LVM,SCSI}Key methods return
the key as the return value. Unfortunately it is desirable
for "NULL" to be a valid return value, as well as an error
indicator. Thus the returned key must instead be provided
as an out-parameter.

When we invoke lvs or scsi_id to extract ID for block devices,
we don't want virCommandWait logging errors messages. Thus we
must explicitly check 'status != 0', rather than letting
virCommandWait do it.

Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
---
 src/util/storage_file.c | 74 ++++++++++++++++++++++++++++++++-----------------
 src/util/storage_file.h |  6 ++--
 2 files changed, 53 insertions(+), 27 deletions(-)

diff --git a/src/util/storage_file.c b/src/util/storage_file.c
index 17a47cf..fe7e8b8 100644
--- a/src/util/storage_file.c
+++ b/src/util/storage_file.c
@@ -1192,62 +1192,75 @@ int virStorageFileIsClusterFS(const char *path)
 }
 
 #ifdef LVS
-char *virStorageFileGetLVMKey(const char *path)
+int virStorageFileGetLVMKey(const char *path,
+                            char **key)
 {
     /*
      *  # lvs --noheadings --unbuffered --nosuffix --options "uuid" LVNAME
      *    06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky
      */
-    char *key = NULL;
+    int status;
     virCommandPtr cmd = virCommandNewArgList(
         LVS,
         "--noheadings", "--unbuffered", "--nosuffix",
         "--options", "uuid", path,
         NULL
         );
+    int ret = -1;
+
+    *key = NULL;
 
     /* Run the program and capture its output */
-    virCommandSetOutputBuffer(cmd, &key);
-    if (virCommandRun(cmd, NULL) < 0)
+    virCommandSetOutputBuffer(cmd, key);
+    if (virCommandRun(cmd, &status) < 0)
         goto cleanup;
 
-    if (key) {
+    /* 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 && *key) {
         char *nl;
-        char *tmp = key;
+        char *tmp = *key;
 
         /* Find first non-space character */
         while (*tmp && c_isspace(*tmp)) {
             tmp++;
         }
         /* Kill leading spaces */
-        if (tmp != key)
-            memmove(key, tmp, strlen(tmp)+1);
+        if (tmp != *key)
+            memmove(*key, tmp, strlen(tmp)+1);
 
         /* Kill trailing newline */
-        if ((nl = strchr(key, '\n')))
+        if ((nl = strchr(*key, '\n')))
             *nl = '\0';
     }
 
-    if (key && STREQ(key, ""))
-        VIR_FREE(key);
+    ret = 0;
 
 cleanup:
+    if (*key && STREQ(*key, ""))
+        VIR_FREE(*key);
+
     virCommandFree(cmd);
 
-    return key;
+    return ret;
 }
 #else
-char *virStorageFileGetLVMKey(const char *path)
+int virStorageFileGetLVMKey(const char *path,
+                            char **key)
 {
     virReportSystemError(ENOSYS, _("Unable to get LVM key for %s"), path);
-    return NULL;
+    return -1;
 }
 #endif
 
 #ifdef HAVE_UDEV
-char *virStorageFileGetSCSIKey(const char *path)
+int virStorageFileGetSCSIKey(const char *path,
+                             char **key)
 {
-    char *key = NULL;
+    int status;
     virCommandPtr cmd = virCommandNewArgList(
         "/lib/udev/scsi_id",
         "--replace-whitespace",
@@ -1255,30 +1268,41 @@ char *virStorageFileGetSCSIKey(const char *path)
         "--device", path,
         NULL
         );
+    int ret = -1;
+
+    *key = NULL;
 
     /* Run the program and capture its output */
-    virCommandSetOutputBuffer(cmd, &key);
-    if (virCommandRun(cmd, NULL) < 0)
+    virCommandSetOutputBuffer(cmd, key);
+    if (virCommandRun(cmd, &status) < 0)
         goto cleanup;
 
-    if (key && STRNEQ(key, "")) {
-        char *nl = strchr(key, '\n');
+    /* 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 && *key) {
+        char *nl = strchr(*key, '\n');
         if (nl)
             *nl = '\0';
-    } else {
-        VIR_FREE(key);
     }
 
+    ret = 0;
+
 cleanup:
+    if (*key && STREQ(*key, ""))
+        VIR_FREE(*key);
+
     virCommandFree(cmd);
 
-    return key;
+    return ret;
 }
 #else
-char *virStorageFileGetSCSIKey(const char *path)
+int virStorageFileGetSCSIKey(const char *path,
+                             char **key)
 {
     virReportSystemError(ENOSYS, _("Unable to get SCSI key for %s"), path);
-    return NULL;
+    return -1;
 }
 #endif
 
diff --git a/src/util/storage_file.h b/src/util/storage_file.h
index 9e4516e..6fbd275 100644
--- a/src/util/storage_file.h
+++ b/src/util/storage_file.h
@@ -101,7 +101,9 @@ int virStorageFileIsClusterFS(const char *path);
 int virStorageFileIsSharedFSType(const char *path,
                                  int fstypes);
 
-char *virStorageFileGetLVMKey(const char *path);
-char *virStorageFileGetSCSIKey(const char *path);
+int virStorageFileGetLVMKey(const char *path,
+                            char **key);
+int virStorageFileGetSCSIKey(const char *path,
+                             char **key);
 
 #endif /* __VIR_STORAGE_FILE_H__ */
-- 
1.7.11.7

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list