On 12/11/12 21:41, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list