Skip to contentSkip to conversation

qemu-io: Exit with error when a command failed

PATCH Applied Max Reitz 1 participant 7 messages May 9, 2018 – Jun 1, 2018

lore
Share & tools
lore

Max Reitz

author
#0 lore raw

v2 cover qemu-io: Exit with error when a command failed

Right now, qemu-io's exit code is rather useless as it is usually 0. Except sometimes, then it's 1 in case of an error (mostly when you specify a filename as an argument and it cannot open that).

At the same time, most command functions' return values are rather useless as they are usually 0 (meaning "continue execution"). There is only a single function that breaks that pattern, which is "quit". On one hand, this is pointless because "quit" is in qemu-io.c, so it can easily signal that fact through a global (yet static) variable. On the other, it breaks the usual pattern of I/O functions returning error codes.

This series resolves the overlap between both issues by making the command functions' return error values instead of whether to continue execution or not, and thus makes qemu-io return 1 if any of the commands executed has failed and 0 only if all of them have succeeded.

Patch 5 showcases how that may be useful for iotests.

See also: https://bugzilla.redhat.com/show_bug.cgi?id=1519617

v2:
- Patch 2: Added a comment on the interface of command functions (their parameters and their return value) [Eric]
- (Decided against replacing 0/1 by EXIT_SUCCESS/EXIT_FAILURE, because although I personally would prefer them slightly, neither are ever used in qemu-io so far.)

git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[----] [-C] 'qemu-io: Drop command functions' return values' 002/5:[0005] [FC] 'qemu-io: Let command functions return error code' 003/5:[----] [--] 'qemu-io: Exit with error when a command failed' 004/5:[----] [--] 'iotests.py: Add qemu_io_silent' 005/5:[----] [-C] 'iotests: Let 216 make use of qemu-io's exit code'

Max Reitz (5):
qemu-io: Drop command functions' return values
qemu-io: Let command functions return error code
qemu-io: Exit with error when a command failed iotests.py: Add qemu_io_silent
iotests: Let 216 make use of qemu-io's exit code

 include/qemu-io.h             |   9 +-
 qemu-io-cmds.c                | 276 ++++++++++++++++++++++++------------------
 qemu-io.c                     |  62 +++++++---
 tests/qemu-iotests/216        |  23 ++--
 tests/qemu-iotests/216.out    |  17 +--
 tests/qemu-iotests/iotests.py |   9 ++
 6 files changed, 231 insertions(+), 165 deletions(-)
Signature · 1 line
2.14.3
author
#1 lore raw

PATCH v2 1/5 qemu-io: Drop command functions' return values

For qemu-io, a function returns an integer with two possible values: 0 for "qemu-io may continue execution", or 1 for "qemu-io should exit". However, there is only a single command that returns 1, and that is "quit".

So let's turn this case into a global variable instead so we can make better use of the return value in a later patch.

---
 include/qemu-io.h |   6 +-
 qemu-io-cmds.c    | 294 +++++++++++++++++++++++++-----------------------------
 qemu-io.c         |  36 +++----
 3 files changed, 157 insertions(+), 179 deletions(-)
include/qemu-io.h +3 -3
index 196fde0f3a..06cdfbf660 100644
@@ -22,7 +22,7 @@
 
 #define CMD_FLAG_GLOBAL ((int)0x80000000) /* don't iterate "args" */
 
-typedef int (*cfunc_t)(BlockBackend *blk, int argc, char **argv);
+typedef void (*cfunc_t)(BlockBackend *blk, int argc, char **argv);
 typedef void (*helpfunc_t)(void);
 
 typedef struct cmdinfo {
@@ -41,10 +41,10 @@ typedef struct cmdinfo {
 
 extern bool qemuio_misalign;
 
-bool qemuio_command(BlockBackend *blk, const char *cmd);
+void qemuio_command(BlockBackend *blk, const char *cmd);
 
 void qemuio_add_command(const cmdinfo_t *ci);
-int qemuio_command_usage(const cmdinfo_t *ci);
+void qemuio_command_usage(const cmdinfo_t *ci);
 void qemuio_complete_command(const char *input,
                              void (*fn)(const char *cmd, void *opaque),
                              void *opaque);
qemu-io-cmds.c +136 -158

1,052 lines not on this page · raw message

qemu-io.c +18 -18
index 72fee0d8b7..6ab6254f6b 100644
@@ -37,6 +37,7 @@
 static char *progname;
 
 static BlockBackend *qemuio_blk;
+static bool quit_qemu_io;
 
 /* qemu-io commands passed using -c */
 static int ncmdline;
@@ -65,11 +66,10 @@ static int get_eof_char(void)
 #endif
 }
 
-static int close_f(BlockBackend *blk, int argc, char **argv)
+static void close_f(BlockBackend *blk, int argc, char **argv)
 {
     blk_unref(qemuio_blk);
     qemuio_blk = NULL;
-    return 0;
 }
 
 static const cmdinfo_t close_cmd = {
@@ -136,7 +136,7 @@ static void open_help(void)
 "\n");
 }
 
-static int open_f(BlockBackend *blk, int argc, char **argv);
+static void open_f(BlockBackend *blk, int argc, char **argv);
 
 static const cmdinfo_t open_cmd = {
     .name       = "open",
@@ -160,7 +160,7 @@ static QemuOptsList empty_opts = {
     },
 };
 
-static int open_f(BlockBackend *blk, int argc, char **argv)
+static void open_f(BlockBackend *blk, int argc, char **argv)
 {
     int flags = BDRV_O_UNMAP;
     int readonly = 0;
@@ -192,25 +192,25 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
             if (bdrv_parse_cache_mode(optarg, &flags, &writethrough) < 0) {
                 error_report("Invalid cache option: %s", optarg);
                 qemu_opts_reset(&empty_opts);
-                return 0;
+                return;
             }
             break;
         case 'd':
             if (bdrv_parse_discard_flags(optarg, &flags) < 0) {
                 error_report("Invalid discard option: %s", optarg);
                 qemu_opts_reset(&empty_opts);
-                return 0;
+                return;
             }
             break;
         case 'o':
             if (imageOpts) {
                 printf("--image-opts and 'open -o' are mutually exclusive\n");
                 qemu_opts_reset(&empty_opts);
-                return 0;
+                return;
             }
             if (!qemu_opts_parse_noisily(&empty_opts, optarg, false)) {
                 qemu_opts_reset(&empty_opts);
-                return 0;
+                return;
             }
             break;
         case 'U':
@@ -218,7 +218,8 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
             break;
         default:
             qemu_opts_reset(&empty_opts);
-            return qemuio_command_usage(&open_cmd);
+            qemuio_command_usage(&open_cmd);
+            return;
         }
     }
 
@@ -229,7 +230,7 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
     if (imageOpts && (optind == argc - 1)) {
         if (!qemu_opts_parse_noisily(&empty_opts, argv[optind], false)) {
             qemu_opts_reset(&empty_opts);
-            return 0;
+            return;
         }
         optind++;
     }
@@ -246,12 +247,11 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
         qobject_unref(opts);
         qemuio_command_usage(&open_cmd);
     }
-    return 0;
 }
 
-static int quit_f(BlockBackend *blk, int argc, char **argv)
+static void quit_f(BlockBackend *blk, int argc, char **argv)
 {
-    return 1;
+    quit_qemu_io = true;
 }
 
 static const cmdinfo_t quit_cmd = {
@@ -392,18 +392,18 @@ static void prep_fetchline(void *opaque)
 
 static void command_loop(void)
 {
-    int i, done = 0, fetchable = 0, prompted = 0;
+    int i, fetchable = 0, prompted = 0;
     char *input;
 
-    for (i = 0; !done && i < ncmdline; i++) {
-        done = qemuio_command(qemuio_blk, cmdline[i]);
+    for (i = 0; !quit_qemu_io && i < ncmdline; i++) {
+        qemuio_command(qemuio_blk, cmdline[i]);
     }
     if (cmdline) {
         g_free(cmdline);
         return;
     }
 
-    while (!done) {
+    while (!quit_qemu_io) {
         if (!prompted) {
             printf("%s", get_prompt());
             fflush(stdout);
@@ -421,7 +421,7 @@ static void command_loop(void)
         if (input == NULL) {
             break;
         }
-        done = qemuio_command(qemuio_blk, input);
+        qemuio_command(qemuio_blk, input);
         g_free(input);
 
         prompted = 0;
Signature · 1 line
2.14.3
author
#2 lore raw

PATCH v2 2/5 qemu-io: Let command functions return error code

In reply to Max Reitz

This is basically what everything else in the qemu code base does, so we can do it here, too.

---
 include/qemu-io.h |   9 +-
 qemu-io-cmds.c    | 346 ++++++++++++++++++++++++++++++++----------------------
 qemu-io.c         |  34 ++++--
 3 files changed, 232 insertions(+), 157 deletions(-)
include/qemu-io.h +7 -2
index 06cdfbf660..7433239372 100644
@@ -22,7 +22,12 @@
 
 #define CMD_FLAG_GLOBAL ((int)0x80000000) /* don't iterate "args" */
 
-typedef void (*cfunc_t)(BlockBackend *blk, int argc, char **argv);
+/* Implement a qemu-io command.
+ * Operate on @blk using @argc/@argv as the command's arguments, and
+ * return 0 on success or negative errno on failure.
+ */
+typedef int (*cfunc_t)(BlockBackend *blk, int argc, char **argv);
+
 typedef void (*helpfunc_t)(void);
 
 typedef struct cmdinfo {
@@ -41,7 +46,7 @@ typedef struct cmdinfo {
 
 extern bool qemuio_misalign;
 
-void qemuio_command(BlockBackend *blk, const char *cmd);
+int qemuio_command(BlockBackend *blk, const char *cmd);
 
 void qemuio_add_command(const cmdinfo_t *ci);
 void qemuio_command_usage(const cmdinfo_t *ci);
qemu-io-cmds.c +203 -143

1,189 lines not on this page · raw message

qemu-io.c +22 -12
index 6ab6254f6b..78eb7a6d2e 100644
@@ -66,10 +66,11 @@ static int get_eof_char(void)
 #endif
 }
 
-static void close_f(BlockBackend *blk, int argc, char **argv)
+static int close_f(BlockBackend *blk, int argc, char **argv)
 {
     blk_unref(qemuio_blk);
     qemuio_blk = NULL;
+    return 0;
 }
 
 static const cmdinfo_t close_cmd = {
@@ -136,7 +137,7 @@ static void open_help(void)
 "\n");
 }
 
-static void open_f(BlockBackend *blk, int argc, char **argv);
+static int open_f(BlockBackend *blk, int argc, char **argv);
 
 static const cmdinfo_t open_cmd = {
     .name       = "open",
@@ -160,12 +161,13 @@ static QemuOptsList empty_opts = {
     },
 };
 
-static void open_f(BlockBackend *blk, int argc, char **argv)
+static int open_f(BlockBackend *blk, int argc, char **argv)
 {
     int flags = BDRV_O_UNMAP;
     int readonly = 0;
     bool writethrough = true;
     int c;
+    int ret;
     QemuOpts *qopts;
     QDict *opts;
     bool force_share = false;
@@ -192,25 +194,25 @@ static void open_f(BlockBackend *blk, int argc, char **argv)
             if (bdrv_parse_cache_mode(optarg, &flags, &writethrough) < 0) {
                 error_report("Invalid cache option: %s", optarg);
                 qemu_opts_reset(&empty_opts);
-                return;
+                return -EINVAL;
             }
             break;
         case 'd':
             if (bdrv_parse_discard_flags(optarg, &flags) < 0) {
                 error_report("Invalid discard option: %s", optarg);
                 qemu_opts_reset(&empty_opts);
-                return;
+                return -EINVAL;
             }
             break;
         case 'o':
             if (imageOpts) {
                 printf("--image-opts and 'open -o' are mutually exclusive\n");
                 qemu_opts_reset(&empty_opts);
-                return;
+                return -EINVAL;
             }
             if (!qemu_opts_parse_noisily(&empty_opts, optarg, false)) {
                 qemu_opts_reset(&empty_opts);
-                return;
+                return -EINVAL;
             }
             break;
         case 'U':
@@ -219,7 +221,7 @@ static void open_f(BlockBackend *blk, int argc, char **argv)
         default:
             qemu_opts_reset(&empty_opts);
             qemuio_command_usage(&open_cmd);
-            return;
+            return -EINVAL;
         }
     }
 
@@ -230,7 +232,7 @@ static void open_f(BlockBackend *blk, int argc, char **argv)
     if (imageOpts && (optind == argc - 1)) {
         if (!qemu_opts_parse_noisily(&empty_opts, argv[optind], false)) {
             qemu_opts_reset(&empty_opts);
-            return;
+            return -EINVAL;
         }
         optind++;
     }
@@ -240,18 +242,26 @@ static void open_f(BlockBackend *blk, int argc, char **argv)
     qemu_opts_reset(&empty_opts);
 
     if (optind == argc - 1) {
-        openfile(argv[optind], flags, writethrough, force_share, opts);
+        ret = openfile(argv[optind], flags, writethrough, force_share, opts);
     } else if (optind == argc) {
-        openfile(NULL, flags, writethrough, force_share, opts);
+        ret = openfile(NULL, flags, writethrough, force_share, opts);
     } else {
         qobject_unref(opts);
         qemuio_command_usage(&open_cmd);
+        return -EINVAL;
+    }
+
+    if (ret) {
+        return -EINVAL;
     }
+
+    return 0;
 }
 
-static void quit_f(BlockBackend *blk, int argc, char **argv)
+static int quit_f(BlockBackend *blk, int argc, char **argv)
 {
     quit_qemu_io = true;
+    return 0;
 }
 
 static const cmdinfo_t quit_cmd = {
Signature · 1 line
2.14.3
author
#3 lore raw

PATCH v2 3/5 qemu-io: Exit with error when a command failed

In reply to Max Reitz

Currently, qemu-io basically always returns success when it gets to interactive mode (so once the whole command line has been parsed; even before the commands on the command line are interpreted). That is not very useful.

This patch makes qemu-io return failure when any of the executed commands failed.

---
 qemu-io.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)
qemu-io.c +22 -6
index 78eb7a6d2e..6737cbb221 100644
@@ -400,17 +400,21 @@ static void prep_fetchline(void *opaque)
     *fetchable= 1;
 }
 
-static void command_loop(void)
+static int command_loop(void)
 {
     int i, fetchable = 0, prompted = 0;
+    int ret, last_error = 0;
     char *input;
 
     for (i = 0; !quit_qemu_io && i < ncmdline; i++) {
-        qemuio_command(qemuio_blk, cmdline[i]);
+        ret = qemuio_command(qemuio_blk, cmdline[i]);
+        if (ret < 0) {
+            last_error = ret;
+        }
     }
     if (cmdline) {
         g_free(cmdline);
-        return;
+        return last_error;
     }
 
     while (!quit_qemu_io) {
@@ -431,13 +435,19 @@ static void command_loop(void)
         if (input == NULL) {
             break;
         }
-        qemuio_command(qemuio_blk, input);
+        ret = qemuio_command(qemuio_blk, input);
         g_free(input);
 
+        if (ret < 0) {
+            last_error = ret;
+        }
+
         prompted = 0;
         fetchable = 0;
     }
     qemu_set_fd_handler(STDIN_FILENO, NULL, NULL, NULL);
+
+    return last_error;
 }
 
 static void add_user_command(char *optarg)
@@ -502,6 +512,7 @@ int main(int argc, char **argv)
     int c;
     int opt_index = 0;
     int flags = BDRV_O_UNMAP;
+    int ret;
     bool writethrough = true;
     Error *local_error = NULL;
     QDict *opts = NULL;
@@ -663,7 +674,7 @@ int main(int argc, char **argv)
             }
         }
     }
-    command_loop();
+    ret = command_loop();
 
     /*
      * Make sure all outstanding requests complete before the program exits.
@@ -672,5 +683,10 @@ int main(int argc, char **argv)
 
     blk_unref(qemuio_blk);
     g_free(readline_state);
-    return 0;
+
+    if (ret < 0) {
+        return 1;
+    } else {
+        return 0;
+    }
 }
Signature · 1 line
2.14.3
author
#4 lore raw

PATCH v2 4/5 iotests.py: Add qemu_io_silent

In reply to Max Reitz

With qemu-io now returning a useful exit code, some tests may find it sufficient to just query that instead of logging (and filtering) the whole output.

---
 tests/qemu-iotests/iotests.py | 9 +++++++++
 1 file changed, 9 insertions(+)
tests/qemu-iotests/iotests.py +9 -0
index b25d48a91b..9747ca9eba 100644
@@ -119,6 +119,15 @@ def qemu_io(*args):
         sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
     return subp.communicate()[0]
 
+def qemu_io_silent(*args):
+    '''Run qemu-io and return the exit code, suppressing stdout'''
+    args = qemu_io_args + list(args)
+    exitcode = subprocess.call(args, stdout=open('/dev/null', 'w'))
+    if exitcode < 0:
+        sys.stderr.write('qemu-io received signal %i: %s\n' %
+                         (-exitcode, ' '.join(args)))
+    return exitcode
+
 
 class QemuIoInteractive:
     def __init__(self, *args):
Signature · 1 line
2.14.3
author
#5 lore raw

PATCH v2 5/5 iotests: Let 216 make use of qemu-io's exit code

In reply to Max Reitz

As a showcase of how you can use qemu-io's exit code to determine success or failure (same for qemu-img), this test is changed to use qemu_io_silent() instead of qemu_io(), and to assert the exit code instead of logging the filtered result.

One real advantage of this is that in case of an error, you get a backtrace that helps you locate the issue in the test file quickly.

---
 tests/qemu-iotests/216     | 23 ++++++++++++-----------
 tests/qemu-iotests/216.out | 17 ++---------------
 2 files changed, 14 insertions(+), 26 deletions(-)
tests/qemu-iotests/216 +12 -11
index ca9b47a7fd..3c0ae54b44 100755
@@ -20,7 +20,7 @@
 # Creator/Owner: Max Reitz <[email protected]>
 
 import iotests
-from iotests import log, qemu_img_pipe, qemu_io, filter_qemu_io
+from iotests import log, qemu_img, qemu_io_silent
 
 # Need backing file support
 iotests.verify_image_format(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk'])
@@ -50,14 +50,13 @@ with iotests.FilePath('base.img') as base_img_path, \
     log('--- Setting up images ---')
     log('')
 
-    qemu_img_pipe('create', '-f', iotests.imgfmt, base_img_path, '64M')
+    assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
+    assert qemu_io_silent(base_img_path, '-c', 'write -P 1 0M 1M') == 0
+    assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
+                    top_img_path) == 0
+    assert qemu_io_silent(top_img_path,  '-c', 'write -P 2 1M 1M') == 0
 
-    log(filter_qemu_io(qemu_io(base_img_path, '-c', 'write -P 1 0M 1M')))
-
-    qemu_img_pipe('create', '-f', iotests.imgfmt, '-b', base_img_path,
-                  top_img_path)
-
-    log(filter_qemu_io(qemu_io(top_img_path,  '-c', 'write -P 2 1M 1M')))
+    log('Done')
 
     log('')
     log('--- Doing COR ---')
@@ -110,6 +109,8 @@ with iotests.FilePath('base.img') as base_img_path, \
     log('--- Checking COR result ---')
     log('')
 
-    log(filter_qemu_io(qemu_io(base_img_path, '-c', 'discard 0 64M')))
-    log(filter_qemu_io(qemu_io(top_img_path,  '-c', 'read -P 1 0M 1M')))
-    log(filter_qemu_io(qemu_io(top_img_path,  '-c', 'read -P 2 1M 1M')))
+    assert qemu_io_silent(base_img_path, '-c', 'discard 0 64M') == 0
+    assert qemu_io_silent(top_img_path,  '-c', 'read -P 1 0M 1M') == 0
+    assert qemu_io_silent(top_img_path,  '-c', 'read -P 2 1M 1M') == 0
+
+    log('Done')
tests/qemu-iotests/216.out +2 -15
index d3fc590d29..45ea857ee1 100644
@@ -3,12 +3,7 @@
 
 --- Setting up images ---
 
-wrote 1048576/1048576 bytes at offset 0
-1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-wrote 1048576/1048576 bytes at offset 1048576
-1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
+Done
 
 --- Doing COR ---
 
@@ -17,12 +12,4 @@ wrote 1048576/1048576 bytes at offset 1048576
 
 --- Checking COR result ---
 
-discard 67108864/67108864 bytes at offset 0
-64 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-read 1048576/1048576 bytes at offset 0
-1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-read 1048576/1048576 bytes at offset 1048576
-1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
+Done
Signature · 1 line
2.14.3
author Applied
#6 lore raw
In reply to Max Reitz
Quoted 52 lines · Max Reitz

Right now, qemu-io's exit code is rather useless as it is usually 0. Except sometimes, then it's 1 in case of an error (mostly when you specify a filename as an argument and it cannot open that).

At the same time, most command functions' return values are rather useless as they are usually 0 (meaning "continue execution"). There is only a single function that breaks that pattern, which is "quit". On one hand, this is pointless because "quit" is in qemu-io.c, so it can easily signal that fact through a global (yet static) variable. On the other, it breaks the usual pattern of I/O functions returning error codes.

This series resolves the overlap between both issues by making the command functions' return error values instead of whether to continue execution or not, and thus makes qemu-io return 1 if any of the commands executed has failed and 0 only if all of them have succeeded.

Patch 5 showcases how that may be useful for iotests.

See also: https://bugzilla.redhat.com/show_bug.cgi?id=1519617

v2:
- Patch 2: Added a comment on the interface of command functions (their parameters and their return value) [Eric]
- (Decided against replacing 0/1 by EXIT_SUCCESS/EXIT_FAILURE, because although I personally would prefer them slightly, neither are ever used in qemu-io so far.)

git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[----] [-C] 'qemu-io: Drop command functions' return values' 002/5:[0005] [FC] 'qemu-io: Let command functions return error code' 003/5:[----] [--] 'qemu-io: Exit with error when a command failed' 004/5:[----] [--] 'iotests.py: Add qemu_io_silent' 005/5:[----] [-C] 'iotests: Let 216 make use of qemu-io's exit code'

Max Reitz (5):
qemu-io: Drop command functions' return values
qemu-io: Let command functions return error code
qemu-io: Exit with error when a command failed iotests.py: Add qemu_io_silent
iotests: Let 216 make use of qemu-io's exit code

Quoted 7 lines
 include/qemu-io.h             |   9 +-
 qemu-io-cmds.c                | 276 ++++++++++++++++++++++++------------------
 qemu-io.c                     |  62 +++++++---
 tests/qemu-iotests/216        |  23 ++--
 tests/qemu-iotests/216.out    |  17 +--
 tests/qemu-iotests/iotests.py |   9 ++
 6 files changed, 231 insertions(+), 165 deletions(-)

Applied to my block branch.

Max