Re: [Qemu-devel] [PATCH v2 1/5] qemu-io: Don't die on second open

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2706 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]