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

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/07/2011 10:39 AM, Jes Sorensen wrote:
> On 03/07/11 17:34, Anthony Liguori wrote:
>> On 03/07/2011 09:27 AM, Jes.Sorensen@redhat.com wrote:
>>>        if (ret != 0) {
>>> -        abort();
>>> +        qerror_report(QERR_OPEN_FILE_FAILED, filename);
>>> +        error_printf("do_snapshot_blkdev(): Unable to open newly
>>> created "
>>> +                     "snapshot file: \n");
>>> +        error_printf("%s. Attempting to revert to original image: %s\n",
>>> +                     filename, old_filename);
>> You can't combine qerror_report with continued action.  qerror_report()
>> should be a terminal action.  You also shouldn't combine error_printf()
>> with qerror_report().
>>
>> 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
> information.

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.

> 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.

BTW, there shouldn't be an abort() in any of these paths.  If you fail 
to reopen, just let the failure propagate.

Regards,

Anthony Liguori

> Cheers,
> Jes
>