[v3,1/4] qemu-io: Don't die on second open

Commit Message

Eric Blake June 5, 2017, 7:08 p.m. UTC

Most callback commands in qemu-io return 0 to keep the interpreter
loop running, or 1 to quit immediately.  However, open_f() just
passed through the return value of openfile(), which has different
semantics of returning 0 if a file was opened, or 1 on any failure.

As a result of mixing the return semantics, we are forcing the
qemu-io interpreter to exit early on any failures, which is rather
annoying when some of the failures are obviously trying to give
the user a hint of how to proceed (if we didn't then kill qemu-io
out from under the user's feet):

$ qemu-io
qemu-io> open foo
qemu-io> open foo
file open already, try 'help close'
$ echo $?
0

In general, we WANT openfile() to report failures, since it is the
function used in the form 'qemu-io -c "$something" no_such_file'
for performing one or more -c options on a single file, and it is
not worth attempting $something if the file itself cannot be opened.
So the solution is to fix open_f() to always return 0 (when we are
in interactive mode, even failure to open should not end the
session), and save the return value of openfile() for command line
use in main().

Note, however, that we do have some qemu-iotests that do 'qemu-io
-c "open file" -c "$something"'; such tests will now proceed to
attempt $something whether or not the open succeeded, the same way
as if the two commands had been attempted in interactive mode; but it
also means that it is now possible to use -c close and have a single
qemu-io command line operate on more than one file even without
using interactive mode.  Although the '-c open' action is a subtle
change in behavior, remember that qemu-io is for debugging purposes,
so as long as it serves the needs of qemu-iotests while still being
reasonable for interactive use, it should not be a problem.

This has been awkward since at least as far back as commit
e3aff4f, in 2009.

Signed-off-by: Eric Blake <[e-mail]>
Reviewed-by: Fam Zheng <[e-mail]>

---
v3: tweak commit message to distinguish that '-c open' has a subtle
new behavior from the command line
v2: fix open_f(), not openfile()
---
 qemu-io.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Eric Blake June 5, 2017, 7:58 p.m. UTC | #1

On 06/05/2017 02:08 PM, Eric Blake wrote:

> 
> Note, however, that we do have some qemu-iotests that do 'qemu-io
> -c "open file" -c "$something"'; such tests will now proceed to
> attempt $something whether or not the open succeeded, the same way
> as if the two commands had been attempted in interactive mode; but it
> also means that it is now possible to use -c close and have a single
> qemu-io command line operate on more than one file even without
> using interactive mode.  Although the '-c open' action is a subtle
> change in behavior, remember that qemu-io is for debugging purposes,
> so as long as it serves the needs of qemu-iotests while still being
> reasonable for interactive use, it should not be a problem.

Bummer - iotest 60 catches me:

+++ 060.out.bad	2017-06-05 14:55:48.336814834 -0500
@@ -21,6 +21,7 @@
     refcount bits: 16
     corrupt: true
 can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot
be opened read/write
+no file open, try 'help open'
 read 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

Looks like 114 and 153 as well.  I'll have to post a v4 (serves me right
for JUST testing 177).

Also, I'm seeing a segfault on 68 that exists on current master:

068 1s ... - output mismatch (see 068.out.bad)
--- /home/eblake/qemu/tests/qemu-iotests/068.out	2017-06-01
17:01:25.113094957 -0500
+++ 068.out.bad	2017-06-05 14:55:54.140835402 -0500
@@ -6,6 +6,8 @@
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm 0
 (qemu) quit
+./common.config: line 107: 13333 Segmentation fault      (core dumped)
( if [ -n "${QEMU_NEED_PID}" ]; then
+    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done

Kevin Wolf June 6, 2017, 12:51 p.m. UTC | #2

Am 05.06.2017 um 21:58 hat Eric Blake geschrieben:
> Also, I'm seeing a segfault on 68 that exists on current master:
> 
> 068 1s ... - output mismatch (see 068.out.bad)
> --- /home/eblake/qemu/tests/qemu-iotests/068.out	2017-06-01
> 17:01:25.113094957 -0500
> +++ 068.out.bad	2017-06-05 14:55:54.140835402 -0500
> @@ -6,6 +6,8 @@
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) savevm 0
>  (qemu) quit
> +./common.config: line 107: 13333 Segmentation fault      (core dumped)
> ( if [ -n "${QEMU_NEED_PID}" ]; then
> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) quit
> -*** done
> +(qemu) *** done

This should be fixed with "qemu/migration: fix the migration bug found
by qemu-iotests case 068".

Kevin

Patch

diff --git a/qemu-io.c b/qemu-io.c
index 8e38b28..8074656 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -230,13 +230,14 @@  static int open_f(BlockBackend *blk, int argc, char **argv)
     qemu_opts_reset(&empty_opts);

     if (optind == argc - 1) {
-        return openfile(argv[optind], flags, writethrough, force_share, opts);
+        openfile(argv[optind], flags, writethrough, force_share, opts);
     } else if (optind == argc) {
-        return openfile(NULL, flags, writethrough, force_share, opts);
+        openfile(NULL, flags, writethrough, force_share, opts);
     } else {
         QDECREF(opts);
-        return qemuio_command_usage(&open_cmd);
+        qemuio_command_usage(&open_cmd);
     }
+    return 0;
 }

 static int quit_f(BlockBackend *blk, int argc, char **argv)