qemu-devel
[Top][All Lists]
Advanced

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

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


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject
Date: Fri, 25 Feb 2011 16:20:29 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.15) Gecko/20101027 Lightning/1.0b1 Thunderbird/3.0.10

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.

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.

Regards,

Anthony Liguori

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to address@hidden
More majordomo info at  http://vger.kernel.org/majordomo-info.html




reply via email to

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