Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> On 02/25/2011 03:54 AM, Markus Armbruster wrote:
>> Anthony Liguori<address@hidden>  writes:
>>
>>    
>>> On 02/24/2011 10:20 AM, Markus Armbruster wrote:
>>>      
>>>> Anthony Liguori<address@hidden>   writes:
>>>>
>>>>
>>>>        
>>>>> On 02/24/2011 02:33 AM, Markus Armbruster wrote:
>>>>>
>>>>>          
>>>>>> Anthony Liguori<address@hidden>    writes:
>>>>>>            
>> [...]
>>    
>>>>>>> Please describe all expected errors.
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>> Quoting qmp-commands.hx:
>>>>>>
>>>>>>        3. Errors, in special, are not documented. Applications should 
>>>>>> NOT check
>>>>>>           for specific errors classes or data (it's strongly recommended 
>>>>>> to only
>>>>>>           check for the "error" key)
>>>>>>
>>>>>> Indeed, not a single error is documented there.  This is intentional.
>>>>>>
>>>>>>
>>>>>>            
>>>>> Yeah, but we're not 0.14 anymore and for 0.15, we need to document
>>>>> errors.  If you are suggesting I send a patch to remove that section,
>>>>> I'm more than happy to.
>>>>>
>>>>>          
>>>> Two separate issues here: 1. Are we ready to commit to the current
>>>> design of errors, and 2. Is it fair to reject Lai's patch now because he
>>>> doesn't document his errors.
>>>>
>>>> I'm not commenting on 1. here.
>>>>
>>>> Regarding 2.: rejecting a patch because it doesn't document an aspect
>>>> that current master intentionally leaves undocumented is not how you
>>>> treat contributors.  At least not if you want any other than certified
>>>> masochists who enjoy pain, and professionals who get adequately
>>>> compensated for it.
>>>>
>>>> Lead by example, not by fiat.
>>>>
>>>>        
>>> http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json
>>>
>>> I am in the process of documenting the errors of every command.  It's
>>> a royal pain but I'm going to document everything we have right now.
>>> It's actually the last bit of work I need to finish before sending
>>> QAPI out.
>>>
>>> So for new commands being added, it would be hugely helpful for the
>>> authors to document the errors such that I don't have to reverse
>>> engineer all of the possible error conditions.
>>>      
>> The moment this lands in master, you can begin to demand error
>> descriptions from contributors.  Until then, I'll NAK error descriptions
>> in qmp-commands.txt.  We left them undocumented there for good reasons:
>>    
>
> No, it was always a bad reason.  Good documentation is necessary to
> build good commands.  Errors are a huge part of the semantics of a
> command.  We cannot properly assess a command unless it's behavior in
> error conditions is well defined.

The decision to declare QMP stable was made at the KVM Forum after quite
some deliberation.  We explictly excluded errors.  We didn't do that
because errors are not worthy of specification (that would be silly).
We did it because there was too much unhappiness about the current state
of errors to close the debate on them at that time.  Luiz mindfully
documented that decision in qmp-commands.txt:

    3. Errors, in special, are not documented. Applications should NOT check
       for specific errors classes or data (it's strongly recommended to only
       check for the "error" key)

Nobody denies that errors need to be specified, and this clause needs to
go.  But I must insist on proper review.  No sneaking in of error
specification through the commit backdoor, please.

>>>>>> Once we have an error design in place that has a reasonable hope to
>>>>>> stand the test of time, and have errors documented for at least some of
>>>>>> the commands here, we can start to require proper error documentation
>>>>>> for new commands.  But not now.
>>>>>>            
>> I won't NAK non-normative error descriptions, say in commit messages, or
>> in comments.  And I won't object to you asking for them.  But I feel you
>> really shouldn't make it a condition for committing patches.  Especially
>> not for simple patches that have been on list for months.
>>    
>
> I'm strongly committed to making QMP a first class interface in QEMU
> for 0.15.  I feel documentation is a crucial part to making that
> happen.
>
> I'm not asking for test cases even though that's something that we'll
> need for 0.15 because there's not enough infrastructure in master yet
> to do that in a reasonable way.  I realize I'm likely going to have to
> write that test case and I'm happy to do that.
>
> But there's no reason that we shouldn't require thorough documentation
> for all new QMP commands moving forward including error semantics.
> This is a critical part of having a first class API and no additional
> infrastructure is needed in master to do this.

Broken record time, again.

Nobody denies that errors need to be specified.  Fair goal for 0.15.
But I must insist on proper review.  No sneaking in of error
specification through the commit backdoor, please.

I won't NAK non-normative error descriptions, say in commit messages, or
in comments.  And I won't object to you asking for them.  But I feel you
really shouldn't make it a condition for committing patches.  Especially
not for simple patches that have been on list for months.