Re: [Qemu-devel] [PATCH 3/4] qcow2: add a migration blocker

[Stefan Hajnoczi]

On Mon, Nov 14, 2011 at 4:49 PM, Anthony Liguori <address@hidden> wrote:
> On 11/14/2011 10:45 AM, Stefan Hajnoczi wrote:
>>
>> On Mon, Nov 14, 2011 at 4:25 PM, Anthony Liguori<address@hidden>
>>  wrote:
>>>
>>> On 11/14/2011 09:51 AM, Stefan Hajnoczi wrote:
>>>>
>>>> On Fri, Nov 11, 2011 at 8:29 PM, Anthony Liguori<address@hidden>
>>>>  wrote:
>>>>>
>>>>> +#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
>>>>> +    "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format':
>>>>> %s, 'name': %s, 'feature': %s } }"
>>>>> +
>>>>
>>>> Isn't having a separate error going to make life harder for management
>>>> tool writers?  I would have expected one "migration not supported"
>>>> error, regardless of whether the reason is ivshmem, qcow2, or anything
>>>> else.
>>>
>>> Errors shouldn't be tied to verbs.  IOW, if you have a migrate command,
>>> you
>>> don't want to have a MigrationFailed error because that's tied to a
>>> specific
>>> verb.
>>>
>>> Instead, you want the errors to provide additional information about the
>>> verb failed.  In this case, the verb is failing because you're requesting
>>> to
>>> use a feature that is not supported by this particular block format.
>>
>> Okay, this error is semantically different from the earlier error in
>> this series.
>>
>> We need QMP documentation updates in this series so management tool
>> writers know to look out for.  Otherwise libvirt and friends can only
>> provide generic "This operation failed.  Opaque QEMU error:<blob>"
>> rather than responding with graceful error handling.
>
> Current QMP documentation does not document errors.
>
> The QAPI changes document the errors but it also changes the error paths
> such that you can do this in a sane fashion.
>
> So I agree with you 100% but it's a bigger thing than just this series.

Okay, I thought about it because I've been documenting errors in the
generic image streaming series.

Stefan