* [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages
@ 2018-12-12 22:04 Eric Blake
2018-12-12 22:11 ` Richard W.M. Jones
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: Eric Blake @ 2018-12-12 22:04 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, rjones, Kevin Wolf, Max Reitz
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 <rjones@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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",
+ 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",
+ 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;
}
--
2.17.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-12 22:04 [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages Eric Blake
@ 2018-12-12 22:11 ` Richard W.M. Jones
2018-12-12 23:52 ` [Qemu-devel] [Qemu-block] " Nir Soffer
` (3 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Richard W.M. Jones @ 2018-12-12 22:11 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz
On Wed, Dec 12, 2018 at 04:04:10PM -0600, 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 <rjones@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
A straightforward, albeit lengthy, replacement of ‘printf’ by
‘fprintf(stderr,’ along all the error paths, therefore:
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> 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).
This is unfortunate, but it sounds to me like the test is broken ...
Rich.
> 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",
> + 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",
> + 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;
> }
> --
> 2.17.2
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-12 22:04 [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages Eric Blake
2018-12-12 22:11 ` Richard W.M. Jones
@ 2018-12-12 23:52 ` Nir Soffer
2018-12-13 1:26 ` Eric Blake
2018-12-13 10:47 ` Daniel P. Berrangé
2018-12-13 10:11 ` [Qemu-devel] " Daniel P. Berrangé
` (2 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Nir Soffer @ 2018-12-12 23:52 UTC (permalink / raw)
To: Eric Blake; +Cc: QEMU Developers, Kevin Wolf, qemu-block, Max Reitz
On Thu, Dec 13, 2018 at 12:13 AM Eric Blake <eblake@redhat.com> wrote:
>
> When a qemu-io command fails, it's best if the failure message
> goes to stderr rather than stdout.
This makes sense, but it will break users like this:
https://github.com/oVirt/vdsm/blob/a2836b1d58ffaa0f48cc9c814b6002161a81f044/tests/storage/qemuio.py#L45
We need a way to detect qemu-io verification failures, maybe a special
exit code?
0 - verification succeeded
1 - verification failed
2 - other error (e.g no such file)
Or, if qemu-io had a way to read data and write it to stdout, we could
compare the data and avoid the need for special exit code.
Nir
>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
> 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",
> + 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",
> + 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;
> }
> --
> 2.17.2
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-12 23:52 ` [Qemu-devel] [Qemu-block] " Nir Soffer
@ 2018-12-13 1:26 ` Eric Blake
2018-12-13 10:47 ` Daniel P. Berrangé
1 sibling, 0 replies; 18+ messages in thread
From: Eric Blake @ 2018-12-13 1:26 UTC (permalink / raw)
To: Nir Soffer; +Cc: QEMU Developers, Kevin Wolf, qemu-block, Max Reitz
On 12/12/18 5:52 PM, Nir Soffer wrote:
> On Thu, Dec 13, 2018 at 12:13 AM Eric Blake <eblake@redhat.com> wrote:
>>
>> When a qemu-io command fails, it's best if the failure message
>> goes to stderr rather than stdout.
>
> This makes sense, but it will break users like this:
>
> https://github.com/oVirt/vdsm/blob/a2836b1d58ffaa0f48cc9c814b6002161a81f044/tests/storage/qemuio.py#L45
>
> We need a way to detect qemu-io verification failures, maybe a special
> exit code?
>
> 0 - verification succeeded
> 1 - verification failed
> 2 - other error (e.g no such file)
>
> Or, if qemu-io had a way to read data and write it to stdout, we could
> compare the data and avoid the need for special exit code.
>
>> @@ -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;
Note that 'read -P ...' can fail for both pattern verification failure,
and for other reasons, both before and after this patch. The only thing
this patch is doing is changing where the failure messages are output.
Or is your complaint that your existing code is already catering to
older qemu that had 0 exit status, and parsing stdout for a specific
string rather than trusting exit status, and now stdout won't have that
specific string? qemu-io is not quite as worried about backwards
compatibility as qemu-img or qemu proper, but at least knowing what
might break might help us design something more user-friendly.
Can you redirect qemu-io to output both stdout and stderr to the same
file, if you have to parse the answers to learn if verification has failed?
And your idea of having distinguished error codes for verification fail
vs. overall failure makes some sense, but it would require some major
rework (right now, returning -errno codes does not really tell the
caller what exit status to report).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-12 22:04 [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages Eric Blake
2018-12-12 22:11 ` Richard W.M. Jones
2018-12-12 23:52 ` [Qemu-devel] [Qemu-block] " Nir Soffer
@ 2018-12-13 10:11 ` Daniel P. Berrangé
2018-12-13 14:22 ` Kevin Wolf
2018-12-13 12:21 ` Wainer dos Santos Moschetta
2018-12-13 14:38 ` Kevin Wolf
4 siblings, 1 reply; 18+ messages in thread
From: Daniel P. Berrangé @ 2018-12-13 10:11 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Kevin Wolf, rjones, qemu-block, Max Reitz
On Wed, Dec 12, 2018 at 04:04:10PM -0600, 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 <rjones@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
> 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).
I think the change is correct. Error messages should never go to stdout.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-12 23:52 ` [Qemu-devel] [Qemu-block] " Nir Soffer
2018-12-13 1:26 ` Eric Blake
@ 2018-12-13 10:47 ` Daniel P. Berrangé
2018-12-13 14:05 ` Kevin Wolf
2018-12-13 17:15 ` Nir Soffer
1 sibling, 2 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2018-12-13 10:47 UTC (permalink / raw)
To: Nir Soffer; +Cc: Eric Blake, Kevin Wolf, QEMU Developers, qemu-block, Max Reitz
On Thu, Dec 13, 2018 at 01:52:29AM +0200, Nir Soffer wrote:
> On Thu, Dec 13, 2018 at 12:13 AM Eric Blake <eblake@redhat.com> wrote:
> >
> > When a qemu-io command fails, it's best if the failure message
> > goes to stderr rather than stdout.
>
> This makes sense, but it will break users like this:
>
> https://github.com/oVirt/vdsm/blob/a2836b1d58ffaa0f48cc9c814b6002161a81f044/tests/storage/qemuio.py#L45
>
> We need a way to detect qemu-io verification failures, maybe a special
> exit code?
>
> 0 - verification succeeded
> 1 - verification failed
> 2 - other error (e.g no such file)
This makes sense. We should *never* expect applications to parse the
messages on stdout/err, because we reserve the right to change text
arbitrarily at any time. So we need to use exit status IMHO.
> Or, if qemu-io had a way to read data and write it to stdout, we could
> compare the data and avoid the need for special exit code.
That should be trivial to do, and quite desirable too IMHO - libvirt would
in fact quite like such a feature, as it would let us support format
conversions when using our upload/download APIs, without having to create
intermediate files. Alternatively 'qemu-img convert' could allow for
/dev/stdin and /dev/stdout as raw files, but that looks considerably
harder to implement.
For your usecase that feels rather inefficient as you're introducing
multiple data copies, which will be bad for large images. Much better
if we just make qemu-io set good exit codes.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-12 22:04 [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages Eric Blake
` (2 preceding siblings ...)
2018-12-13 10:11 ` [Qemu-devel] " Daniel P. Berrangé
@ 2018-12-13 12:21 ` Wainer dos Santos Moschetta
2018-12-13 14:04 ` Eric Blake
2018-12-13 14:38 ` Kevin Wolf
4 siblings, 1 reply; 18+ messages in thread
From: Wainer dos Santos Moschetta @ 2018-12-13 12:21 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: Kevin Wolf, rjones, qemu-block, Max Reitz
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 <rjones@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
> 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 <wainersm@redhat.com>
- 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;
> }
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-13 12:21 ` Wainer dos Santos Moschetta
@ 2018-12-13 14:04 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2018-12-13 14:04 UTC (permalink / raw)
To: Wainer dos Santos Moschetta, qemu-devel
Cc: Kevin Wolf, rjones, qemu-block, Max Reitz
On 12/13/18 6:21 AM, Wainer dos Santos Moschetta wrote:
>
> 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 <rjones@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> 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).
>> 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.
Pre-existing, but I don't mind fixing it while touching in the area.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-13 10:47 ` Daniel P. Berrangé
@ 2018-12-13 14:05 ` Kevin Wolf
2018-12-13 14:23 ` Eric Blake
2018-12-13 17:44 ` Nir Soffer
2018-12-13 17:15 ` Nir Soffer
1 sibling, 2 replies; 18+ messages in thread
From: Kevin Wolf @ 2018-12-13 14:05 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Nir Soffer, Eric Blake, QEMU Developers, qemu-block, Max Reitz
Am 13.12.2018 um 11:47 hat Daniel P. Berrangé geschrieben:
> On Thu, Dec 13, 2018 at 01:52:29AM +0200, Nir Soffer wrote:
> > On Thu, Dec 13, 2018 at 12:13 AM Eric Blake <eblake@redhat.com> wrote:
> > >
> > > When a qemu-io command fails, it's best if the failure message
> > > goes to stderr rather than stdout.
> >
> > This makes sense, but it will break users like this:
> >
> > https://github.com/oVirt/vdsm/blob/a2836b1d58ffaa0f48cc9c814b6002161a81f044/tests/storage/qemuio.py#L45
> >
> > We need a way to detect qemu-io verification failures, maybe a special
> > exit code?
> >
> > 0 - verification succeeded
> > 1 - verification failed
> > 2 - other error (e.g no such file)
>
> This makes sense. We should *never* expect applications to parse the
> messages on stdout/err, because we reserve the right to change text
> arbitrarily at any time. So we need to use exit status IMHO.
qemu-io processes more than just a single command. What would the exit
code be if one of the commands succeeds, one gets an I/O error, and the
third one succeeds for I/O, but fails pattern verification?
The things is, qemu-io was never meant to be used by other
applications that need to process the results, it's a tool for testing
and debugging. If we had meant it to be used by other programs, we would
have given it a machine-friendly interface.
The machine-friendly interface to the QEMU block layer is qemu-nbd.
> > Or, if qemu-io had a way to read data and write it to stdout, we could
> > compare the data and avoid the need for special exit code.
>
> That should be trivial to do, and quite desirable too IMHO - libvirt would
> in fact quite like such a feature, as it would let us support format
> conversions when using our upload/download APIs, without having to create
> intermediate files. Alternatively 'qemu-img convert' could allow for
> /dev/stdin and /dev/stdout as raw files, but that looks considerably
> harder to implement.
>
> For your usecase that feels rather inefficient as you're introducing
> multiple data copies, which will be bad for large images. Much better
> if we just make qemu-io set good exit codes.
'read -v' produces a hex dump on stdout, but you still need to separate
it from the other output and then parse the hexdump.
The human interface of qemu-io is honestly just not the right tool for
the job, and adding one-off tweaks to make it a little bit less horrible
to use for machines isn't the right approach because it's still not a
proper machine protocol.
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-13 10:11 ` [Qemu-devel] " Daniel P. Berrangé
@ 2018-12-13 14:22 ` Kevin Wolf
2018-12-13 16:02 ` Richard W.M. Jones
0 siblings, 1 reply; 18+ messages in thread
From: Kevin Wolf @ 2018-12-13 14:22 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Eric Blake, qemu-devel, rjones, qemu-block, Max Reitz
Am 13.12.2018 um 11:11 hat Daniel P. Berrangé geschrieben:
> On Wed, Dec 12, 2018 at 04:04:10PM -0600, 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 <rjones@redhat.com>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >
> > 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).
>
> I think the change is correct. Error messages should never go to stdout.
I'm not sure about this, to be honest. qemu-io is first and foremost an
interactive program, and spreading output across two streams, one
buffered and one unbuffered, in an interactive program feels like it has
to result in surprises.
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-13 14:05 ` Kevin Wolf
@ 2018-12-13 14:23 ` Eric Blake
2018-12-13 14:34 ` Kevin Wolf
2018-12-13 17:44 ` Nir Soffer
1 sibling, 1 reply; 18+ messages in thread
From: Eric Blake @ 2018-12-13 14:23 UTC (permalink / raw)
To: Kevin Wolf, Daniel P. Berrangé
Cc: Nir Soffer, QEMU Developers, qemu-block, Max Reitz
On 12/13/18 8:05 AM, Kevin Wolf wrote:
> Am 13.12.2018 um 11:47 hat Daniel P. Berrangé geschrieben:
>> On Thu, Dec 13, 2018 at 01:52:29AM +0200, Nir Soffer wrote:
>>> On Thu, Dec 13, 2018 at 12:13 AM Eric Blake <eblake@redhat.com> wrote:
>>>>
>>>> When a qemu-io command fails, it's best if the failure message
>>>> goes to stderr rather than stdout.
>>>
>>> This makes sense, but it will break users like this:
>>>
>>> https://github.com/oVirt/vdsm/blob/a2836b1d58ffaa0f48cc9c814b6002161a81f044/tests/storage/qemuio.py#L45
>>>
>>> We need a way to detect qemu-io verification failures, maybe a special
>>> exit code?
>>>
>>> 0 - verification succeeded
>>> 1 - verification failed
>>> 2 - other error (e.g no such file)
>>
>> This makes sense. We should *never* expect applications to parse the
>> messages on stdout/err, because we reserve the right to change text
>> arbitrarily at any time. So we need to use exit status IMHO.
>
> qemu-io processes more than just a single command. What would the exit
> code be if one of the commands succeeds, one gets an I/O error, and the
> third one succeeds for I/O, but fails pattern verification?
>
> The things is, qemu-io was never meant to be used by other
> applications that need to process the results, it's a tool for testing
> and debugging. If we had meant it to be used by other programs, we would
> have given it a machine-friendly interface.
>
> The machine-friendly interface to the QEMU block layer is qemu-nbd.
>
>>> Or, if qemu-io had a way to read data and write it to stdout, we could
>>> compare the data and avoid the need for special exit code.
>>
>> That should be trivial to do, and quite desirable too IMHO - libvirt would
>> in fact quite like such a feature, as it would let us support format
>> conversions when using our upload/download APIs, without having to create
>> intermediate files. Alternatively 'qemu-img convert' could allow for
>> /dev/stdin and /dev/stdout as raw files, but that looks considerably
>> harder to implement.
>>
>> For your usecase that feels rather inefficient as you're introducing
>> multiple data copies, which will be bad for large images. Much better
>> if we just make qemu-io set good exit codes.
>
> 'read -v' produces a hex dump on stdout, but you still need to separate
> it from the other output and then parse the hexdump.
>
> The human interface of qemu-io is honestly just not the right tool for
> the job, and adding one-off tweaks to make it a little bit less horrible
> to use for machines isn't the right approach because it's still not a
> proper machine protocol.
I actually agree with that sentiment - qemu-io is NOT a program where we
promise backwards compatibility (we're not going to break it without
reason, because we DO have to keep iotests running, but outside of
iotests, we are less concerned if other uses break).
But it DOES sound like teaching 'qemu-img convert' to optionally convert
only a subset of a file may be useful (I already tried once to make
'qemu-img dd' smarter, and the conclusion at the time is that it would
be better to just make qemu-img dd be syntactic sugar for a
full-featured qemu-img convert, which means making convert take an
offset and range limit to the source, as well as a separate offset into
the destination, for easily extracting portions of one file into
portions of another). And I also agree that qemu-nbd already has offset
and range support.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-13 14:23 ` Eric Blake
@ 2018-12-13 14:34 ` Kevin Wolf
0 siblings, 0 replies; 18+ messages in thread
From: Kevin Wolf @ 2018-12-13 14:34 UTC (permalink / raw)
To: Eric Blake
Cc: Daniel P. Berrangé,
Nir Soffer, QEMU Developers, qemu-block, Max Reitz
Am 13.12.2018 um 15:23 hat Eric Blake geschrieben:
> On 12/13/18 8:05 AM, Kevin Wolf wrote:
> > Am 13.12.2018 um 11:47 hat Daniel P. Berrangé geschrieben:
> > > On Thu, Dec 13, 2018 at 01:52:29AM +0200, Nir Soffer wrote:
> > > > On Thu, Dec 13, 2018 at 12:13 AM Eric Blake <eblake@redhat.com> wrote:
> > > > >
> > > > > When a qemu-io command fails, it's best if the failure message
> > > > > goes to stderr rather than stdout.
> > > >
> > > > This makes sense, but it will break users like this:
> > > >
> > > > https://github.com/oVirt/vdsm/blob/a2836b1d58ffaa0f48cc9c814b6002161a81f044/tests/storage/qemuio.py#L45
> > > >
> > > > We need a way to detect qemu-io verification failures, maybe a special
> > > > exit code?
> > > >
> > > > 0 - verification succeeded
> > > > 1 - verification failed
> > > > 2 - other error (e.g no such file)
> > >
> > > This makes sense. We should *never* expect applications to parse the
> > > messages on stdout/err, because we reserve the right to change text
> > > arbitrarily at any time. So we need to use exit status IMHO.
> >
> > qemu-io processes more than just a single command. What would the exit
> > code be if one of the commands succeeds, one gets an I/O error, and the
> > third one succeeds for I/O, but fails pattern verification?
> >
> > The things is, qemu-io was never meant to be used by other
> > applications that need to process the results, it's a tool for testing
> > and debugging. If we had meant it to be used by other programs, we would
> > have given it a machine-friendly interface.
> >
> > The machine-friendly interface to the QEMU block layer is qemu-nbd.
> >
> > > > Or, if qemu-io had a way to read data and write it to stdout, we could
> > > > compare the data and avoid the need for special exit code.
> > >
> > > That should be trivial to do, and quite desirable too IMHO - libvirt would
> > > in fact quite like such a feature, as it would let us support format
> > > conversions when using our upload/download APIs, without having to create
> > > intermediate files. Alternatively 'qemu-img convert' could allow for
> > > /dev/stdin and /dev/stdout as raw files, but that looks considerably
> > > harder to implement.
> > >
> > > For your usecase that feels rather inefficient as you're introducing
> > > multiple data copies, which will be bad for large images. Much better
> > > if we just make qemu-io set good exit codes.
> >
> > 'read -v' produces a hex dump on stdout, but you still need to separate
> > it from the other output and then parse the hexdump.
> >
> > The human interface of qemu-io is honestly just not the right tool for
> > the job, and adding one-off tweaks to make it a little bit less horrible
> > to use for machines isn't the right approach because it's still not a
> > proper machine protocol.
>
> I actually agree with that sentiment - qemu-io is NOT a program where we
> promise backwards compatibility (we're not going to break it without reason,
> because we DO have to keep iotests running, but outside of iotests, we are
> less concerned if other uses break).
>
> But it DOES sound like teaching 'qemu-img convert' to optionally convert
> only a subset of a file may be useful (I already tried once to make
> 'qemu-img dd' smarter, and the conclusion at the time is that it would be
> better to just make qemu-img dd be syntactic sugar for a full-featured
> qemu-img convert, which means making convert take an offset and range limit
> to the source, as well as a separate offset into the destination, for easily
> extracting portions of one file into portions of another). And I also agree
> that qemu-nbd already has offset and range support.
Can't you actually already achieve this with --image-opts and a raw
filter that has the offset/size options set?
It's not as nice as with a separate option, but it should do the job.
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-12 22:04 [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages Eric Blake
` (3 preceding siblings ...)
2018-12-13 12:21 ` Wainer dos Santos Moschetta
@ 2018-12-13 14:38 ` Kevin Wolf
4 siblings, 0 replies; 18+ messages in thread
From: Kevin Wolf @ 2018-12-13 14:38 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, qemu-block, rjones, Max Reitz
Am 12.12.2018 um 23:04 hat Eric Blake geschrieben:
> 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 <rjones@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
> 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).
So if we decide to actually do this, should this be error_report()
instead so that HMP 'qemu-io' gets the errors printed to the monitor
rather than stderr? Though I think in that case, we'd also want to
redirect the messages that still go stdout to the monitor as well.
But as I said in another reply, I'm not sure if mixing stdout and stderr
with their different buffering settings is even a good idea in a mostly
interactive program.
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-13 14:22 ` Kevin Wolf
@ 2018-12-13 16:02 ` Richard W.M. Jones
0 siblings, 0 replies; 18+ messages in thread
From: Richard W.M. Jones @ 2018-12-13 16:02 UTC (permalink / raw)
To: Kevin Wolf
Cc: Daniel P. Berrangé, Eric Blake, qemu-devel, qemu-block, Max Reitz
On Thu, Dec 13, 2018 at 03:22:10PM +0100, Kevin Wolf wrote:
> Am 13.12.2018 um 11:11 hat Daniel P. Berrangé geschrieben:
> > On Wed, Dec 12, 2018 at 04:04:10PM -0600, 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 <rjones@redhat.com>
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > ---
> > >
> > > 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).
> >
> > I think the change is correct. Error messages should never go to stdout.
>
> I'm not sure about this, to be honest. qemu-io is first and foremost an
> interactive program, and spreading output across two streams, one
> buffered and one unbuffered, in an interactive program feels like it has
> to result in surprises.
Just for the record we're using qemu-io extensively for testing
nbdkit, and it's always used non-interactively (ie. ‘qemu-io -c’).
The bug (if it is such) was found because I was using qemu-io to test
for expected errors.
https://github.com/libguestfs/nbdkit/search?q=qemu-io
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-13 10:47 ` Daniel P. Berrangé
2018-12-13 14:05 ` Kevin Wolf
@ 2018-12-13 17:15 ` Nir Soffer
1 sibling, 0 replies; 18+ messages in thread
From: Nir Soffer @ 2018-12-13 17:15 UTC (permalink / raw)
To: Daniel P. Berrange
Cc: Eric Blake, Kevin Wolf, QEMU Developers, qemu-block, Max Reitz
On Thu, Dec 13, 2018 at 12:47 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:
> On Thu, Dec 13, 2018 at 01:52:29AM +0200, Nir Soffer wrote:
> > On Thu, Dec 13, 2018 at 12:13 AM Eric Blake <eblake@redhat.com> wrote:
> > >
> > > When a qemu-io command fails, it's best if the failure message
> > > goes to stderr rather than stdout.
> >
> > This makes sense, but it will break users like this:
> >
> >
> https://github.com/oVirt/vdsm/blob/a2836b1d58ffaa0f48cc9c814b6002161a81f044/tests/storage/qemuio.py#L45
> >
> > We need a way to detect qemu-io verification failures, maybe a special
> > exit code?
> >
> > 0 - verification succeeded
> > 1 - verification failed
> > 2 - other error (e.g no such file)
>
> This makes sense. We should *never* expect applications to parse the
> messages on stdout/err, because we reserve the right to change text
> arbitrarily at any time. So we need to use exit status IMHO.
>
> > Or, if qemu-io had a way to read data and write it to stdout, we could
> > compare the data and avoid the need for special exit code.
>
> That should be trivial to do, and quite desirable too IMHO - libvirt would
> in fact quite like such a feature, as it would let us support format
> conversions when using our upload/download APIs, without having to create
> intermediate files. Alternatively 'qemu-img convert' could allow for
> /dev/stdin and /dev/stdout as raw files, but that looks considerably
> harder to implement.
>
> For your usecase that feels rather inefficient as you're introducing
> multiple data copies, which will be bad for large images. Much better
> if we just make qemu-io set good exit codes.
>
We use qemu-io for testing changes to tiny images, so multiple copies are
fine.
Nir
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-13 14:05 ` Kevin Wolf
2018-12-13 14:23 ` Eric Blake
@ 2018-12-13 17:44 ` Nir Soffer
2018-12-13 21:27 ` Eric Blake
1 sibling, 1 reply; 18+ messages in thread
From: Nir Soffer @ 2018-12-13 17:44 UTC (permalink / raw)
To: Kevin Wolf
Cc: Daniel P. Berrange, Eric Blake, QEMU Developers, qemu-block, Max Reitz
On Thu, Dec 13, 2018 at 4:05 PM Kevin Wolf <kwolf@redhat.com> wrote:
> Am 13.12.2018 um 11:47 hat Daniel P. Berrangé geschrieben:
> > On Thu, Dec 13, 2018 at 01:52:29AM +0200, Nir Soffer wrote:
> > > On Thu, Dec 13, 2018 at 12:13 AM Eric Blake <eblake@redhat.com> wrote:
> > > >
> > > > When a qemu-io command fails, it's best if the failure message
> > > > goes to stderr rather than stdout.
> > >
> > > This makes sense, but it will break users like this:
> > >
> > >
> https://github.com/oVirt/vdsm/blob/a2836b1d58ffaa0f48cc9c814b6002161a81f044/tests/storage/qemuio.py#L45
> > >
> > > We need a way to detect qemu-io verification failures, maybe a special
> > > exit code?
> > >
> > > 0 - verification succeeded
> > > 1 - verification failed
> > > 2 - other error (e.g no such file)
> >
> > This makes sense. We should *never* expect applications to parse the
> > messages on stdout/err, because we reserve the right to change text
> > arbitrarily at any time. So we need to use exit status IMHO.
>
> qemu-io processes more than just a single command. What would the exit
> code be if one of the commands succeeds, one gets an I/O error, and the
> third one succeeds for I/O, but fails pattern verification?
>
> The things is, qemu-io was never meant to be used by other
> applications that need to process the results, it's a tool for testing
> and debugging. If we had meant it to be used by other programs, we would
> have given it a machine-friendly interface.
>
> The machine-friendly interface to the QEMU block layer is qemu-nbd.
>
nbd is awesome, but much more complicated to use for testing. You need to:
1. start qemu-nbd
2. wait until it is ready
3. use nbd client (we have one now), or connect the qemu-nbd to /dev/ndbX,
which on
Fedora 28 leaves stale /dev/nbdX devices after disconnection
(I reported this few month ago here).
4. finally disconnect and wait until qemu-nbd terminates
qemu-io is so much easier to use, we need to make it more machine friendly.
> > Or, if qemu-io had a way to read data and write it to stdout, we could
> > > compare the data and avoid the need for special exit code.
> >
> > That should be trivial to do, and quite desirable too IMHO - libvirt
> would
> > in fact quite like such a feature, as it would let us support format
> > conversions when using our upload/download APIs, without having to create
> > intermediate files. Alternatively 'qemu-img convert' could allow for
> > /dev/stdin and /dev/stdout as raw files, but that looks considerably
> > harder to implement.
> >
> > For your usecase that feels rather inefficient as you're introducing
> > multiple data copies, which will be bad for large images. Much better
> > if we just make qemu-io set good exit codes.
>
> 'read -v' produces a hex dump on stdout, but you still need to separate
> it from the other output and then parse the hexdump.
>
The hex dump is nice for interactive use, but not for automated tests,
unless
you test by comparing the output of the tool to previously recorded output
like
iotests do. We don't use tool output for testing, it is very fragile and
hard to
maintain.
> The human interface of qemu-io is honestly just not the right tool for
> the job, and adding one-off tweaks to make it a little bit less horrible
> to use for machines isn't the right approach because it's still not a
> proper machine protocol.
>
Add --output json?
Nir
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-13 17:44 ` Nir Soffer
@ 2018-12-13 21:27 ` Eric Blake
2018-12-13 22:13 ` Nir Soffer
0 siblings, 1 reply; 18+ messages in thread
From: Eric Blake @ 2018-12-13 21:27 UTC (permalink / raw)
To: Nir Soffer, Kevin Wolf
Cc: Daniel P. Berrange, QEMU Developers, qemu-block, Max Reitz
On 12/13/18 11:44 AM, Nir Soffer wrote:
>> The things is, qemu-io was never meant to be used by other
>> applications that need to process the results, it's a tool for testing
>> and debugging. If we had meant it to be used by other programs, we would
>> have given it a machine-friendly interface.
>>
>> The machine-friendly interface to the QEMU block layer is qemu-nbd.
>>
>
> nbd is awesome, but much more complicated to use for testing. You need to:
>
> 1. start qemu-nbd
> 2. wait until it is ready
> 3. use nbd client (we have one now), or connect the qemu-nbd to /dev/ndbX,
> which on
> Fedora 28 leaves stale /dev/nbdX devices after disconnection
> (I reported this few month ago here).
Is that true even when you use 'qemu-nbd -d /dev/nbdX' after you are done?
> 4. finally disconnect and wait until qemu-nbd terminates
>
> qemu-io is so much easier to use, we need to make it more machine friendly.
Or rather, if there is something that a machine needs to drive, we
should figure out if qemu-img can be taught to do it instead of hacking
around the issue with qemu-io. When it comes to extracting portions of
a disk, qemu-img convert coupled with the raw driver's offset/length
gets us quite a bit of functionality - even if it's clunky to come up
with the command line, it can be programmed, and doesn't suffer from
having to post-process arbitrary qemu-io output changes.
>> The human interface of qemu-io is honestly just not the right tool for
>> the job, and adding one-off tweaks to make it a little bit less horrible
>> to use for machines isn't the right approach because it's still not a
>> proper machine protocol.
>>
>
> Add --output json?
Who's volunteering to do it? I've got too much else going on to spend
time on such a project.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH RFC] qemu-io: Prefer stderr for error messages
2018-12-13 21:27 ` Eric Blake
@ 2018-12-13 22:13 ` Nir Soffer
0 siblings, 0 replies; 18+ messages in thread
From: Nir Soffer @ 2018-12-13 22:13 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Daniel P. Berrange, QEMU Developers, qemu-block, Max Reitz
On Thu, Dec 13, 2018 at 11:27 PM Eric Blake <eblake@redhat.com> wrote:
> On 12/13/18 11:44 AM, Nir Soffer wrote:
>
> >> The things is, qemu-io was never meant to be used by other
> >> applications that need to process the results, it's a tool for testing
> >> and debugging. If we had meant it to be used by other programs, we would
> >> have given it a machine-friendly interface.
> >>
> >> The machine-friendly interface to the QEMU block layer is qemu-nbd.
> >>
> >
> > nbd is awesome, but much more complicated to use for testing. You need
> to:
> >
> > 1. start qemu-nbd
> > 2. wait until it is ready
> > 3. use nbd client (we have one now), or connect the qemu-nbd to
> /dev/ndbX,
> > which on
> > Fedora 28 leaves stale /dev/nbdX devices after disconnection
> > (I reported this few month ago here).
>
> Is that true even when you use 'qemu-nbd -d /dev/nbdX' after you are done?
>
It was true when I reported this here:
http://lists.nongnu.org/archive/html/qemu-block/2018-07/msg00168.html
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-12-13 22:14 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-12 22:04 [Qemu-devel] [PATCH RFC] qemu-io: Prefer stderr for error messages Eric Blake
2018-12-12 22:11 ` Richard W.M. Jones
2018-12-12 23:52 ` [Qemu-devel] [Qemu-block] " Nir Soffer
2018-12-13 1:26 ` Eric Blake
2018-12-13 10:47 ` Daniel P. Berrangé
2018-12-13 14:05 ` Kevin Wolf
2018-12-13 14:23 ` Eric Blake
2018-12-13 14:34 ` Kevin Wolf
2018-12-13 17:44 ` Nir Soffer
2018-12-13 21:27 ` Eric Blake
2018-12-13 22:13 ` Nir Soffer
2018-12-13 17:15 ` Nir Soffer
2018-12-13 10:11 ` [Qemu-devel] " Daniel P. Berrangé
2018-12-13 14:22 ` Kevin Wolf
2018-12-13 16:02 ` Richard W.M. Jones
2018-12-13 12:21 ` Wainer dos Santos Moschetta
2018-12-13 14:04 ` Eric Blake
2018-12-13 14:38 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.