Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

From: Markus Armbruster <armbru@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: wei@redhat.com, peter.maydell@linaro.org, drjones@redhat.com,
	mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org,
	qemu-arm@nongnu.org, abologna@redhat.com
Subject: Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct
Date: Wed, 23 Mar 2016 10:44:04 +0100	[thread overview]
Message-ID: <87mvppbkbf.fsf@blackfin.pond.sub.org> (raw)
In-Reply-To: <20160323030918.GE28183@pxdev.xzpeter.org> (Peter Xu's message of "Wed, 23 Mar 2016 11:09:18 +0800")

Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 22, 2016 at 12:32:28PM -0600, Eric Blake wrote:
>> On 03/17/2016 09:27 PM, Peter Xu wrote:
>> > +##
>> > +# @GICCapability:
>> > +#
>> > +# This struct describes capability for a specific GIC version. These
>> 
>> Might be nice to spell out what the acronym GIC means, but that's cosmetic.
>
> Ah! I thought I have added that... It's missing again. Will do in
> next spin.
>
>> 
>> > +# bits are not only decided by QEMU/KVM software version, but also
>> > +# decided by the hardware that the program is running upon.
>> > +#
>> > +# @version:  version of GIC to be described.
>> > +#
>> > +# @emulated: whether current QEMU/hardware supports emulated GIC
>> > +#            device in user space.
>> > +#
>> > +# @kernel:   whether current QEMU/hardware supports hardware
>> > +#            accelerated GIC device in kernel.
>> > +#
>> > +# Since: 2.6
>> > +##
>> > +{ 'struct': 'GICCapability',
>> > +  'data': { 'version': 'int',
>> > +            'emulated': 'bool',
>> > +            'kernel': 'bool' } }
>> > 
>> 
>> I might have squashed this with the patch that first uses GICCapability,
>> as defining a type in isolation doesn't do much.
>
> I can do the squash in next spin if you prefer that one. Actually I
> got this question before, about when I should split and when to
> squash. E.g., shall I make sure that I should have no "definition
> only" patches in the future?

Depends.

The general rule is to keep separate things separate, and patches
self-contained.  The narrow sense of self-contained is each patch
compiles and works.  The wider sense is each patch makes sense to its
readers on its own.  You can't always have a perfect score on the
latter, but you should try.

Adding a definition without a user is generally not advised, because you
generally need to see the user to make sense of it.

For a complex feature, adding its abstract interface before its concrete
implementation may help liberate interface review from implementation
details.

Note that your interface consists of type GICCapability and command
query-gic-capabilities.  You could add just the interface with a stub
implementation first, then flesh out the implementation.  But I doubt
the thing is complex enough to justify that.