| Message ID | 20170519023233.24461-2-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 05/18 21:32, Eric Blake wrote: > Failure to open a file in qemu-io should normally return 1 on > failure to end the command loop, on the presumption that when > batching commands all on the command line, failure to open means > nothing further can be attempted. But when executing qemu-io > interactively, there is a special case: if open is executed a > second time, we print a hint that the user should try the > interactive 'close' first. But the hint is useless if we don't > actually LET them try 'close'. > > This has been awkward since at least as far back as commit > 43642b3, in 2011 (probably earlier, but git blame has a harder > time going past the file renames at that point). > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > qemu-io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/qemu-io.c b/qemu-io.c > index 34fa8a1..0c82dac 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -63,7 +63,7 @@ static int openfile(char *name, int flags, bool writethrough, bool force_share, > if (qemuio_blk) { > error_report("file open already, try 'help close'"); > QDECREF(opts); > - return 1; > + return 0; > } > > if (force_share) { > -- > 2.9.4 > > Hmm, failure is failure, why is "return 0" better than "return 1"? If the control flow in the caller has a problem, fix it there? Specifically, I don't think failed interactive open need to exit program, at all. Fam
On 05/24/2017 01:28 AM, Fam Zheng wrote: > On Thu, 05/18 21:32, Eric Blake wrote: >> Failure to open a file in qemu-io should normally return 1 on >> failure to end the command loop, on the presumption that when >> batching commands all on the command line, failure to open means >> nothing further can be attempted. But when executing qemu-io >> interactively, there is a special case: if open is executed a >> second time, we print a hint that the user should try the >> interactive 'close' first. But the hint is useless if we don't >> actually LET them try 'close'. >> >> This has been awkward since at least as far back as commit >> 43642b3, in 2011 (probably earlier, but git blame has a harder >> time going past the file renames at that point). >> >> @@ -63,7 +63,7 @@ static int openfile(char *name, int flags, bool writethrough, bool force_share, >> if (qemuio_blk) { >> error_report("file open already, try 'help close'"); >> QDECREF(opts); >> - return 1; >> + return 0; >> } >> >> if (force_share) { >> -- >> 2.9.4 >> >> > > Hmm, failure is failure, why is "return 0" better than "return 1"? "return 0" is what tells the caller to keep interpreting commands. "return 1" is what tells the caller to exit immediately. Most qemu-io commands return 0 always. Only a few need to return 1 ("quit" absolutely needs to, and we are arguing about whether "open" should do so). Pre-patch, "open" always failed with 1, exiting the interpreter immediately with nonzero status. Which is wrong if we print a help message on the second attempt at open (since a second attempt is only possible in interactive mode), but DOES match what you want to have happen on a first open attempt from the command line: $ qemu-io unreadable_file But you are arguing that in batch mode, when you do: $ qemu-io qemu-io> open unreadable_file that you want to keep things in qemu-io, rather than exiting immediately. If so, then we need to make openfile() return 0 always, and instead teach the caller to check whether it is invoked in command line mode (cause non-zero exit status if nothing was opened) or in interactive mode (keep qemu-io running, to let the user try to open something else). In command line mode, the caller should then use the status of whether a file was actually opened (rather than the return code) when dealing with the call to openfile() for the command line argument. That's a bigger change; but I can try it if you think it is worth it. > > If the control flow in the caller has a problem, fix it there? Specifically, I > don't think failed interactive open need to exit program, at all. > > Fam >
diff --git a/qemu-io.c b/qemu-io.c index 34fa8a1..0c82dac 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -63,7 +63,7 @@ static int openfile(char *name, int flags, bool writethrough, bool force_share, if (qemuio_blk) { error_report("file open already, try 'help close'"); QDECREF(opts); - return 1; + return 0; } if (force_share) {
Failure to open a file in qemu-io should normally return 1 on failure to end the command loop, on the presumption that when batching commands all on the command line, failure to open means nothing further can be attempted. But when executing qemu-io interactively, there is a special case: if open is executed a second time, we print a hint that the user should try the interactive 'close' first. But the hint is useless if we don't actually LET them try 'close'. This has been awkward since at least as far back as commit 43642b3, in 2011 (probably earlier, but git blame has a harder time going past the file renames at that point). Signed-off-by: Eric Blake <eblake@redhat.com> --- qemu-io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)