[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH v3] Improve error handling in do_snapshot_blkdev()
Date: Tue, 08 Mar 2011 07:42:21 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: Gecko/20110223 Lightning/1.0b2 Thunderbird/3.1.8

On 03/08/2011 02:24 AM, Jes Sorensen wrote:
On 03/07/11 18:47, Anthony Liguori wrote:
On 03/07/2011 10:39 AM, Jes Sorensen wrote:
On 03/07/11 17:34, Anthony Liguori wrote:
You should restore the original image file before doing qerror_report()
and just drop the error_printf()s as it's all redundant information.
I would hardly consider it redundant information that it failed and we
try to go back to the original image. That is an error in itself, even
though rolling back is better than abort()ing.

If qerror_report() is a fatal situation, that is problematic.
It's fatal for the command, yes.  You should do qerror_report() in the
exit path.

   Then we
need qerror_warn() or something as well, which can return non fatal
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.

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?


Anthony Liguori

BTW, there shouldn't be an abort() in any of these paths.  If you fail
to reopen, just let the failure propagate.
In this particular case it can be argued that the situation is so fatal
that it is better to fail than to let the guest continue.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]