Re: [Qemu-devel] [PATCH v2 1/5] qemu-io: Don't die on second open
On 2017-05-31 17:12, Eric Blake wrote: > On 05/31/2017 09:18 AM, Max Reitz wrote: >> On 2017-05-24 22:28, Eric Blake wrote: >>> 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 >>> >>> Meanwhile, we WANT openfile() to report failures, as it is the >>> way that 'qemu-io -c "$something" no_such_file' knows to exit >>> early rather than attempting $something. 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(). >>> >>> This has been awkward since at least as far back as commit >>> e3aff4f, in 2009. >>> >>> Signed-off-by: Eric Blake >> >> This still makes qemu-io -c "$something" no_such_file fail, right. But >> now qemu-io -c "open no_such_file" -c "$something" will execute >> $something; and I'm not sure we can convert all -c open users to not use >> that command (maybe we can, now that we have --image-opts). > > Oh, so we do have some uses of -c "open..." -c "$something" (iotest 103 > for example). > > What happens during "$something" if there was no file currently open? It > may be a command-by-command behavior. But as long as we get graceful > secondary failures rather than crashes for those subsequent $somethings, > we may be okay. Sure, but this is arguing against "we want [qemu-io no_such_file] to exit early". :-) >> I don't think it's absolutely necessary for -c open to exit on error, >> but you seem to do, if I understand your penultimate paragraph >> correctly. :-) > > I don't think it's absolutely necessary either. You're right that this > patch is just worried about 'qemu-img name', not 'qemu-img -c "open > name"'. So maybe all I need to do is update the commit message to > document that the change in semantics to -c "open..." is a side-effect, > that may or may not be changed in a followup patch? If you're OK with that, I am, too. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/5] qemu-io: Don't die on second open
On 05/31/2017 09:18 AM, Max Reitz wrote: > On 2017-05-24 22:28, Eric Blake wrote: >> 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 >> >> Meanwhile, we WANT openfile() to report failures, as it is the >> way that 'qemu-io -c "$something" no_such_file' knows to exit >> early rather than attempting $something. 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(). >> >> This has been awkward since at least as far back as commit >> e3aff4f, in 2009. >> >> Signed-off-by: Eric Blake > > This still makes qemu-io -c "$something" no_such_file fail, right. But > now qemu-io -c "open no_such_file" -c "$something" will execute > $something; and I'm not sure we can convert all -c open users to not use > that command (maybe we can, now that we have --image-opts). Oh, so we do have some uses of -c "open..." -c "$something" (iotest 103 for example). What happens during "$something" if there was no file currently open? It may be a command-by-command behavior. But as long as we get graceful secondary failures rather than crashes for those subsequent $somethings, we may be okay. > > I don't think it's absolutely necessary for -c open to exit on error, > but you seem to do, if I understand your penultimate paragraph > correctly. :-) I don't think it's absolutely necessary either. You're right that this patch is just worried about 'qemu-img name', not 'qemu-img -c "open name"'. So maybe all I need to do is update the commit message to document that the change in semantics to -c "open..." is a side-effect, that may or may not be changed in a followup patch? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/5] qemu-io: Don't die on second open
On 2017-05-24 22:28, Eric Blake wrote: > 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 > > Meanwhile, we WANT openfile() to report failures, as it is the > way that 'qemu-io -c "$something" no_such_file' knows to exit > early rather than attempting $something. 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(). > > This has been awkward since at least as far back as commit > e3aff4f, in 2009. > > Signed-off-by: Eric Blake This still makes qemu-io -c "$something" no_such_file fail, right. But now qemu-io -c "open no_such_file" -c "$something" will execute $something; and I'm not sure we can convert all -c open users to not use that command (maybe we can, now that we have --image-opts). I don't think it's absolutely necessary for -c open to exit on error, but you seem to do, if I understand your penultimate paragraph correctly. :-) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/5] qemu-io: Don't die on second open
On Wed, 05/24 15:28, Eric Blake wrote:
> 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
>
> Meanwhile, we WANT openfile() to report failures, as it is the
> way that 'qemu-io -c "$something" no_such_file' knows to exit
> early rather than attempting $something. 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().
>
> This has been awkward since at least as far back as commit
> e3aff4f, in 2009.
>
> Signed-off-by: Eric Blake
>
> ---
> v2: fix open_f(), not openfile()
> ---
> qemu-io.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/qemu-io.c b/qemu-io.c
> index 34fa8a1..b3febc2 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)
> --
> 2.9.4
>
>
This is better, thanks!
Reviewed-by: Fam Zheng
