[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 01/14] Avoid polluting logs when querying LVM/SCSI ID



On 12/11/12 21:41, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange redhat com>

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. Also move the check for converting from
"" to NULL, after the cleanup label, since virCommandRun may
set the key to "", even on failure
---
  src/util/storage_file.c | 22 ++++++++++++++++++----
  1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/util/storage_file.c b/src/util/storage_file.c
index 4417404..776d4f2 100644
--- a/src/util/storage_file.c
+++ b/src/util/storage_file.c
@@ -1199,6 +1199,7 @@ const char *virStorageFileGetLVMKey(const char *path)
       *    06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky
       */
      char *key = NULL;
+    int status;
      virCommandPtr cmd = virCommandNewArgList(
          LVS,
          "--noheadings", "--unbuffered", "--nosuffix",
@@ -1208,7 +1209,13 @@ const char *virStorageFileGetLVMKey(const char *path)

      /* Run the program and capture its output */
      virCommandSetOutputBuffer(cmd, &key);
-    if (virCommandRun(cmd, NULL) < 0)
+    if (virCommandRun(cmd, &status) < 0)
+        goto cleanup;
+
+    /* Explicitly check, rather than passing NULL to virCommandRun
+     * because we don't want to pollute logs with an error message
+     */
+    if (status != 0)

You should report an error here or add a comment to explicitly note that this function isn't setting errors in some cases and cleanup paths of callers shouldn't rely on this.

          goto cleanup;

      if (key) {
@@ -1228,10 +1235,10 @@ const char *virStorageFileGetLVMKey(const char *path)
              *nl = '\0';
      }

+cleanup:
      if (key && STREQ(key, ""))
          VIR_FREE(key);

-cleanup:
      virCommandFree(cmd);

      return key;
@@ -1248,6 +1255,7 @@ const char *virStorageFileGetLVMKey(const char *path)
  const char *virStorageFileGetSCSIKey(const char *path)
  {
      char *key = NULL;
+    int status;
      virCommandPtr cmd = virCommandNewArgList(
          "/lib/udev/scsi_id",
          "--replace-whitespace",
@@ -1258,9 +1266,16 @@ const char *virStorageFileGetSCSIKey(const char *path)

      /* Run the program and capture its output */
      virCommandSetOutputBuffer(cmd, &key);
-    if (virCommandRun(cmd, NULL) < 0)
+    if (virCommandRun(cmd, &status) < 0)
          goto cleanup;

+    /* Explicitly check, rather than passing NULL to virCommandRun
+     * because we don't want to pollute logs with an error message
+     */
+    if (status != 0)

same here

+        goto cleanup;
+
+cleanup:
      if (key && STRNEQ(key, "")) {
          char *nl = strchr(key, '\n');
          if (nl)
@@ -1269,7 +1284,6 @@ const char *virStorageFileGetSCSIKey(const char *path)
          VIR_FREE(key);
      }

-cleanup:
      virCommandFree(cmd);

      return key;


Otherwise looks good, so ACK if you follow one of the options in my comments.

Peter

P.S: Hopefully somebody will continue the review on this series as I'm out for today.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]