| Message ID | 20170524202842.26724-2-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <eblake@redhat.com> > > --- > 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 <famz@redhat.com>
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 <eblake@redhat.com> 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
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 <eblake@redhat.com> > > 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?
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 <eblake@redhat.com> >> >> 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
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)
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 <eblake@redhat.com> --- v2: fix open_f(), not openfile() --- qemu-io.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)