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

[Peter Krempa]

On 12/11/12 21:41, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at 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.