Re: [Qemu-devel] [PATCH v3] Improve error handling in do_snapshot_blkdev()

[Jes Sorensen]

On 03/08/11 14:42, Anthony Liguori wrote:
> On 03/08/2011 02:24 AM, Jes Sorensen wrote:
>> On 03/07/11 18:47, Anthony Liguori wrote:
>>> In your case, it's definitely a fatal error for the command.  The
>>> command is failing and you're just printing out information about the
>>> rollback information you're taking.
>> I guess the disconnect here is the definition of fatal. Fatal in my book
>> means we're dead, toast, gone ..... hardly the case if we manage to fail
>> back to the previous image.
> 
> Let me put it another way, you can't call qerror_report twice because
> there is only one QMP error object sent in the protocol.  You
> potentially call qerror_report twice which breaks QMP.
> 
> The way you ought to structure things is to return to the old image, and
> then throw an error saying that you couldn't open the new image.

I see, I had the impression QMP would create multiple objects and pass
them along. Guess not.

Thanks for the explanation.

>>>> The printfs are very valuable for the human monitor, but it isn't
>>>> really
>>>> clear to me what is the ideal return value.
>>> But error_printf() is meaningless in the context of QMP.  You can
>>> reproduce these printfs in HMP based on the errors returned by QMP.
>>>
>>> But if you're just doing an HMP command (and don't care about QMP) then
>>> you shouldn't use qerror_report().  But you need to care about QMP so
>>> you should focus on making it a well behaved QMP command.
>> The question here is then how to propagate the message back that we
>> failed to switch to the new image, but stayed on the old one, as opposed
>> to both of them failing? This part of QMP is really black magic and
>> there doesn't seem to be a good error for this. Time for a new QMP error?
> 
> If FileOpenFailed has the filename of the new image, then opening the
> file failed and we're using the old image.  If FileOpenFailed has the
> filename of the old image, we're toast.
> 
> That basically covers it, no?

It kinda sorta covers it. The problem with that is that you then have to
do a string match of the return values to determine which of the cases
happened, which isn't very nice. But I guess we can do that for now.

I'll have a look.

Cheers,
Jes