Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] qapi: add unmap to BlockDeviceStats

From: Eric Blake <eblake@redhat.com>
To: Alberto Garcia <berto@igalia.com>,
	Anton Nefedov <anton.nefedov@virtuozzo.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: kwolf@redhat.com, den@virtuozzo.com, armbru@redhat.com,
	mreitz@redhat.com, pbonzini@redhat.com
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] qapi: add unmap to BlockDeviceStats
Date: Tue, 5 Dec 2017 09:19:06 -0600	[thread overview]
Message-ID: <b88c73db-c3a5-014e-0ee8-0ce157642834@redhat.com> (raw)
In-Reply-To: <w51shcp5i0c.fsf@maestria.local.igalia.com>

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On 12/05/2017 09:09 AM, Alberto Garcia wrote:
> On Mon 20 Nov 2017 05:50:58 PM CET, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  qapi/block-core.json       | 20 ++++++++++++++++++++
>>  include/block/accounting.h |  1 +
>>  block/qapi.c               |  6 ++++++
>>  3 files changed, 27 insertions(+)
>>

> 
> Admittedtly this is just a style issue and I don't know what others
> think, but wouldn't it be nicer if the documentation of new fields is
> located together with the existing ones?
> 
> E.g
> 
>  @rd_bytes
>  @wr_bytes
> +@unmap_bytes
>  @rd_operations
>  @wr_operations
>  @flush_operations
> +@unmap_operations
> 

Grouping is fine, and your suggestion makes sense to me.  One of the
nice things of QMP is that since it is name/value based, you don't have
to worry about inserting a name in the middle if that makes more sense
logically (it doesn't break backwards compatibility with old QMP
clients, the way that inserting something into the middle of a C struct
breaks all callers that were using the old struct layout).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]