Re: [Qemu-block] [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages

[Wainer dos Santos Moschetta]

On 12/12/2018 08:04 PM, Eric Blake wrote:
> When a qemu-io command fails, it's best if the failure message
> goes to stderr rather than stdout.
>
> Reported-by: Richard W.M. Jones <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>
> RFC because at least iotest 60 (found by -qcow2 -g quick) breaks due
> to reordering of output lines, and I'd rather know if we like this
> idea before bothering to revisit all affected iotests (including
> discovering if other slower ones have similar problems).
>
>   qemu-io-cmds.c | 120 ++++++++++++++++++++++++++-----------------------
>   qemu-io.c      |   3 +-
>   2 files changed, 67 insertions(+), 56 deletions(-)
>
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 5363482213b..e4f3925b5c9 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -184,14 +184,14 @@ static void print_cvtnum_err(int64_t rc, const char *arg)
>   {
>       switch (rc) {
>       case -EINVAL:
> -        printf("Parsing error: non-numeric argument,"
> -               " or extraneous/unrecognized suffix -- %s\n", arg);
> +        fprintf(stderr, "Parsing error: non-numeric argument,"
> +                " or extraneous/unrecognized suffix -- %s\n", arg);
>           break;
>       case -ERANGE:
> -        printf("Parsing error: argument too large -- %s\n", arg);
> +        fprintf(stderr, "Parsing error: argument too large -- %s\n", arg);
>           break;
>       default:
> -        printf("Parsing error: %s\n", arg);
> +        fprintf(stderr, "Parsing error: %s\n", arg);
>       }
>   }
>
> @@ -312,7 +312,7 @@ static int parse_pattern(const char *arg)
>
>       pattern = strtol(arg, &endptr, 0);
>       if (pattern < 0 || pattern > UCHAR_MAX || *endptr != '\0') {
> -        printf("%s is not a valid pattern byte\n", arg);
> +        fprintf(stderr, "%s is not a valid pattern byte\n", arg);
>           return -1;
>       }
>
> @@ -421,14 +421,16 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char 
> **argv, int nr_iov,
>           }
>
>           if (len > BDRV_REQUEST_MAX_BYTES) {
> -            printf("Argument '%s' exceeds maximum size %" PRIu64 "\n", arg,
> -                   (uint64_t)BDRV_REQUEST_MAX_BYTES);
> +            fprintf(stderr,
> +                    "Argument '%s' exceeds maximum size %" PRIu64 "\n", arg,
> +                    (uint64_t)BDRV_REQUEST_MAX_BYTES);
>               goto fail;
>           }
>
>           if (count > BDRV_REQUEST_MAX_BYTES - len) {
> -            printf("The total number of bytes exceed the maximum size %" PRIu64
> -                   "\n", (uint64_t)BDRV_REQUEST_MAX_BYTES);
> +            fprintf(stderr,
> +                    "The total number of bytes exceed the maximum size %" 
> PRIu64
> +                    "\n", (uint64_t)BDRV_REQUEST_MAX_BYTES);
>               goto fail;
>           }
>
> @@ -723,8 +725,8 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>           print_cvtnum_err(count, argv[optind]);
>           return count;
>       } else if (count > BDRV_REQUEST_MAX_BYTES) {
> -        printf("length cannot exceed %" PRIu64 ", given %s\n",
> -               (uint64_t)BDRV_REQUEST_MAX_BYTES, argv[optind]);
> +        fprintf(stderr, "length cannot exceed %" PRIu64 ", given %s\n",
> +                (uint64_t)BDRV_REQUEST_MAX_BYTES, argv[optind]);
>           return -EINVAL;
>       }
>
> @@ -738,19 +740,22 @@ static int read_f(BlockBackend *blk, int argc, char 
> **argv)
>       }
>
>       if ((pattern_count < 0) || (pattern_count + pattern_offset > count))  {
> -        printf("pattern verification range exceeds end of read data\n");
> +        fprintf(stderr,
> +                "pattern verification range exceeds end of read data\n");
>           return -EINVAL;
>       }
>
>       if (bflag) {
>           if (!QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE)) {
> -            printf("%" PRId64 " is not a sector-aligned value for 'offset'\n",
> -                   offset);
> +            fprintf(stderr,
> +                    "%" PRId64 " is not a sector-aligned value for 'offset'\n",
> +                    offset);
>               return -EINVAL;
>           }
>           if (!QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE)) {
> -            printf("%"PRId64" is not a sector-aligned value for 'count'\n",
> -                   count);
> +            fprintf(stderr,
> +                    "%"PRId64" is not a sector-aligned value for 'count'\n",

Adding one space before and after PRId64 as in '"%" PRId64 " is not 
(...)"' increases readability IMHO.

> +                    count);
>               return -EINVAL;
>           }
>       }
> @@ -766,7 +771,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>       gettimeofday(&t2, NULL);
>
>       if (ret < 0) {
> -        printf("read failed: %s\n", strerror(-ret));
> +        fprintf(stderr, "read failed: %s\n", strerror(-ret));
>           goto out;
>       }
>       cnt = ret;
> @@ -777,9 +782,9 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>           void *cmp_buf = g_malloc(pattern_count);
>           memset(cmp_buf, pattern, pattern_count);
>           if (memcmp(buf + pattern_offset, cmp_buf, pattern_count)) {
> -            printf("Pattern verification failed at offset %"
> -                   PRId64 ", %"PRId64" bytes\n",
> -                   offset + pattern_offset, pattern_count);
> +            fprintf(stderr, "Pattern verification failed at offset %"
> +                    PRId64 ", %"PRId64" bytes\n",
> +                    offset + pattern_offset, pattern_count);
>               ret = -EINVAL;
>           }
>           g_free(cmp_buf);
> @@ -895,7 +900,7 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
>       gettimeofday(&t2, NULL);
>
>       if (ret < 0) {
> -        printf("readv failed: %s\n", strerror(-ret));
> +        fprintf(stderr, "readv failed: %s\n", strerror(-ret));
>           goto out;
>       }
>       cnt = ret;
> @@ -906,8 +911,8 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
>           void *cmp_buf = g_malloc(qiov.size);
>           memset(cmp_buf, pattern, qiov.size);
>           if (memcmp(buf, cmp_buf, qiov.size)) {
> -            printf("Pattern verification failed at offset %"
> -                   PRId64 ", %zu bytes\n", offset, qiov.size);
> +            fprintf(stderr, "Pattern verification failed at offset %"
> +                    PRId64 ", %zu bytes\n", offset, qiov.size);
>               ret = -EINVAL;
>           }
>           g_free(cmp_buf);
> @@ -1027,22 +1032,23 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>       }
>
>       if (bflag && zflag) {
> -        printf("-b and -z cannot be specified at the same time\n");
> +        fprintf(stderr, "-b and -z cannot be specified at the same time\n");
>           return -EINVAL;
>       }
>
>       if ((flags & BDRV_REQ_FUA) && (bflag || cflag)) {
> -        printf("-f and -b or -c cannot be specified at the same time\n");
> +        fprintf(stderr,
> +                "-f and -b or -c cannot be specified at the same time\n");
>           return -EINVAL;
>       }
>
>       if ((flags & BDRV_REQ_MAY_UNMAP) && !zflag) {
> -        printf("-u requires -z to be specified\n");
> +        fprintf(stderr, "-u requires -z to be specified\n");
>           return -EINVAL;
>       }
>
>       if (zflag && Pflag) {
> -        printf("-z and -P cannot be specified at the same time\n");
> +        fprintf(stderr, "-z and -P cannot be specified at the same time\n");
>           return -EINVAL;
>       }
>
> @@ -1058,21 +1064,23 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>           print_cvtnum_err(count, argv[optind]);
>           return count;
>       } else if (count > BDRV_REQUEST_MAX_BYTES) {
> -        printf("length cannot exceed %" PRIu64 ", given %s\n",
> -               (uint64_t)BDRV_REQUEST_MAX_BYTES, argv[optind]);
> +        fprintf(stderr, "length cannot exceed %" PRIu64 ", given %s\n",
> +                (uint64_t)BDRV_REQUEST_MAX_BYTES, argv[optind]);
>           return -EINVAL;
>       }
>
>       if (bflag || cflag) {
>           if (!QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE)) {
> -            printf("%" PRId64 " is not a sector-aligned value for 'offset'\n",
> -                   offset);
> +            fprintf(stderr,
> +                    "%" PRId64 " is not a sector-aligned value for 'offset'\n",
> +                    offset);
>               return -EINVAL;
>           }
>
>           if (!QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE)) {
> -            printf("%"PRId64" is not a sector-aligned value for 'count'\n",
> -                   count);
> +            fprintf(stderr,
> +                    "%"PRId64" is not a sector-aligned value for 'count'\n",

Likewise.

Reviewed-by: Wainer dos Santos Moschetta <address@hidden>

- Wainer

> +                    count);
>               return -EINVAL;
>           }
>       }
> @@ -1094,7 +1102,7 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>       gettimeofday(&t2, NULL);
>
>       if (ret < 0) {
> -        printf("write failed: %s\n", strerror(-ret));
> +        fprintf(stderr, "write failed: %s\n", strerror(-ret));
>           goto out;
>       }
>       cnt = ret;
> @@ -1208,7 +1216,7 @@ static int writev_f(BlockBackend *blk, int argc, char 
> **argv)
>       gettimeofday(&t2, NULL);
>
>       if (ret < 0) {
> -        printf("writev failed: %s\n", strerror(-ret));
> +        fprintf(stderr, "writev failed: %s\n", strerror(-ret));
>           goto out;
>       }
>       cnt = ret;
> @@ -1252,7 +1260,7 @@ static void aio_write_done(void *opaque, int ret)
>
>
>       if (ret < 0) {
> -        printf("aio_write failed: %s\n", strerror(-ret));
> +        fprintf(stderr, "aio_write failed: %s\n", strerror(-ret));
>           block_acct_failed(blk_get_stats(ctx->blk), &ctx->acct);
>           goto out;
>       }
> @@ -1283,7 +1291,7 @@ static void aio_read_done(void *opaque, int ret)
>       gettimeofday(&t2, NULL);
>
>       if (ret < 0) {
> -        printf("readv failed: %s\n", strerror(-ret));
> +        fprintf(stderr, "readv failed: %s\n", strerror(-ret));
>           block_acct_failed(blk_get_stats(ctx->blk), &ctx->acct);
>           goto out;
>       }
> @@ -1293,8 +1301,8 @@ static void aio_read_done(void *opaque, int ret)
>
>           memset(cmp_buf, ctx->pattern, ctx->qiov.size);
>           if (memcmp(ctx->buf, cmp_buf, ctx->qiov.size)) {
> -            printf("Pattern verification failed at offset %"
> -                   PRId64 ", %zu bytes\n", ctx->offset, ctx->qiov.size);
> +            fprintf(stderr, "Pattern verification failed at offset %"
> +                    PRId64 ", %zu bytes\n", ctx->offset, ctx->qiov.size);
>           }
>           g_free(cmp_buf);
>       }
> @@ -1513,19 +1521,19 @@ static int aio_write_f(BlockBackend *blk, int argc, 
> char **argv)
>       }
>
>       if (ctx->zflag && optind != argc - 2) {
> -        printf("-z supports only a single length parameter\n");
> +        fprintf(stderr, "-z supports only a single length parameter\n");
>           g_free(ctx);
>           return -EINVAL;
>       }
>
>       if ((flags & BDRV_REQ_MAY_UNMAP) && !ctx->zflag) {
> -        printf("-u requires -z to be specified\n");
> +        fprintf(stderr, "-u requires -z to be specified\n");
>           g_free(ctx);
>           return -EINVAL;
>       }
>
>       if (ctx->zflag && ctx->Pflag) {
> -        printf("-z and -P cannot be specified at the same time\n");
> +        fprintf(stderr, "-z and -P cannot be specified at the same time\n");
>           g_free(ctx);
>           return -EINVAL;
>       }
> @@ -1637,7 +1645,7 @@ static int length_f(BlockBackend *blk, int argc, char 
> **argv)
>
>       size = blk_getlength(blk);
>       if (size < 0) {
> -        printf("getlength: %s\n", strerror(-size));
> +        fprintf(stderr, "getlength: %s\n", strerror(-size));
>           return size;
>       }
>
> @@ -1767,9 +1775,9 @@ static int discard_f(BlockBackend *blk, int argc, char 
> **argv)
>           print_cvtnum_err(bytes, argv[optind]);
>           return bytes;
>       } else if (bytes >> BDRV_SECTOR_BITS > BDRV_REQUEST_MAX_SECTORS) {
> -        printf("length cannot exceed %"PRIu64", given %s\n",
> -               (uint64_t)BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS,
> -               argv[optind]);
> +        fprintf(stderr, "length cannot exceed %"PRIu64", given %s\n",
> +                (uint64_t)BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS,
> +                argv[optind]);
>           return -EINVAL;
>       }
>
> @@ -1778,7 +1786,7 @@ static int discard_f(BlockBackend *blk, int argc, char 
> **argv)
>       gettimeofday(&t2, NULL);
>
>       if (ret < 0) {
> -        printf("discard failed: %s\n", strerror(-ret));
> +        fprintf(stderr, "discard failed: %s\n", strerror(-ret));
>           return ret;
>       }
>
> @@ -1820,7 +1828,7 @@ static int alloc_f(BlockBackend *blk, int argc, char 
> **argv)
>       while (remaining) {
>           ret = bdrv_is_allocated(bs, offset, remaining, &num);
>           if (ret < 0) {
> -            printf("is_allocated failed: %s\n", strerror(-ret));
> +            fprintf(stderr, "is_allocated failed: %s\n", strerror(-ret));
>               return ret;
>           }
>           offset += num;
> @@ -2069,7 +2077,7 @@ static int break_f(BlockBackend *blk, int argc, char 
> **argv)
>
>       ret = bdrv_debug_breakpoint(blk_bs(blk), argv[1], argv[2]);
>       if (ret < 0) {
> -        printf("Could not set breakpoint: %s\n", strerror(-ret));
> +        fprintf(stderr, "Could not set breakpoint: %s\n", strerror(-ret));
>           return ret;
>       }
>
> @@ -2082,7 +2090,8 @@ static int remove_break_f(BlockBackend *blk, int argc, 
> char **argv)
>
>       ret = bdrv_debug_remove_breakpoint(blk_bs(blk), argv[1]);
>       if (ret < 0) {
> -        printf("Could not remove breakpoint %s: %s\n", argv[1], 
> strerror(-ret));
> +        fprintf(stderr, "Could not remove breakpoint %s: %s\n",
> +                argv[1], strerror(-ret));
>           return ret;
>       }
>
> @@ -2114,7 +2123,7 @@ static int resume_f(BlockBackend *blk, int argc, char 
> **argv)
>
>       ret = bdrv_debug_resume(blk_bs(blk), argv[1]);
>       if (ret < 0) {
> -        printf("Could not resume request: %s\n", strerror(-ret));
> +        fprintf(stderr, "Could not resume request: %s\n", strerror(-ret));
>           return ret;
>       }
>
> @@ -2193,8 +2202,9 @@ static int sigraise_f(BlockBackend *blk, int argc, char 
> **argv)
>           print_cvtnum_err(sig, argv[1]);
>           return sig;
>       } else if (sig > NSIG) {
> -        printf("signal argument '%s' is too large to be a valid signal\n",
> -               argv[1]);
> +        fprintf(stderr,
> +                "signal argument '%s' is too large to be a valid signal\n",
> +                argv[1]);
>           return -EINVAL;
>       }
>
> @@ -2224,7 +2234,7 @@ static int sleep_f(BlockBackend *blk, int argc, char 
> **argv)
>
>       ms = strtol(argv[1], &endptr, 0);
>       if (ms < 0 || *endptr != '\0') {
> -        printf("%s is not a valid number\n", argv[1]);
> +        fprintf(stderr, "%s is not a valid number\n", argv[1]);
>           return -EINVAL;
>       }
>
> @@ -2294,7 +2304,7 @@ static int help_f(BlockBackend *blk, int argc, char 
> **argv)
>
>       ct = find_command(argv[1]);
>       if (ct == NULL) {
> -        printf("command %s not found\n", argv[1]);
> +        fprintf(stderr, "command %s not found\n", argv[1]);
>           return -EINVAL;
>       }
>
> diff --git a/qemu-io.c b/qemu-io.c
> index 6df7731af49..36308abb0cc 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -206,7 +206,8 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
>               break;
>           case 'o':
>               if (imageOpts) {
> -                printf("--image-opts and 'open -o' are mutually exclusive\n");
> +                fprintf(stderr,
> +                        "--image-opts and 'open -o' are mutually exclusive\n");
>                   qemu_opts_reset(&empty_opts);
>                   return -EINVAL;
>               }