From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:44772)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <famz@redhat.com>) id 1WUteX-0003Wx-0v
	for qemu-devel@nongnu.org; Tue, 01 Apr 2014 04:02:43 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <famz@redhat.com>) id 1WUteQ-00033H-TD
	for qemu-devel@nongnu.org; Tue, 01 Apr 2014 04:02:36 -0400
Received: from mx1.redhat.com ([209.132.183.28]:20726)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <famz@redhat.com>) id 1WUteQ-00032K-LX
	for qemu-devel@nongnu.org; Tue, 01 Apr 2014 04:02:30 -0400
Date: Tue, 1 Apr 2014 16:02:33 +0800
From: Fam Zheng <famz@redhat.com>
Message-ID: <20140401080233.GE22447@T430.nay.redhat.com>
References: <1395911388-31027-1-git-send-email-famz@redhat.com>
	<1395911388-31027-3-git-send-email-famz@redhat.com>
	<533454C9.1000102@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <533454C9.1000102@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v4 2/9] qmp: Add dirty-bitmap-add and
	dirty-bitmap-remove
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Eric Blake <eblake@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>, Benoit Canet <benoit@irqsave.net>

On Thu, 03/27 10:41, Eric Blake wrote:
> On 03/27/2014 03:09 AM, Fam Zheng wrote:
> > The new command pair is added to manage user created dirty bitmap. The
> > dirty bitmap's name is mandatory and must be unique for the same device,
> > but different devices can have bitmaps with the same names.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -2209,6 +2209,51 @@
> >              '*on-target-error': 'BlockdevOnError' } }
> >  
> >  ##
> > +# @DirtyBitmap
> > +#
> > +# @device: name of device which the bitmap is tracking
> > +#
> > +# @name: name of the dirty bitmap
> > +#
> > +# @granularity: #optional the bitmap granularity, default is 64k for
> > +#               dirty-bitmap-add
> 
> Optional, but only affects dirty-bitmap-add.  You later document...
> 
> > +# @dirty-bitmap-remove
> > +#
> > +# Remove a dirty bitmap on the device
> > +#
> > +# Setting granularity has no effect here.
> 
> ...that it is silently ignored where it can't be used here, and again in
> 7/9 for both dirty-bitmap-disable and dirty-bitmap-enable.
> 
> I think it would be smarter to do:
> 
> { 'type': 'DirtyBitmap',
>   'data': { 'device': 'str', 'name': 'str' } }
> 
> {'command': 'dirty-bitmap-add',
>   'data': { 'map': 'DirtyBitmap', '*granularity': 'int' } }
> 
> Or:
> 
> { 'type': 'DirtyBitmap',
>   'data': { 'device': 'str', 'name': 'str' } }
> { 'type': 'DirtyBitmapGranularity',
>   'base': 'DirtyBitmap',
>   'data': { '*granularity': 'int' } }
> {'command': 'dirty-bitmap-add',
>   'data': 'DirtyBitmapGranularity' }
> 
> 
> which says that the 'DirtyBitmap' struct has no optional members, and
> instead of silently ignoring an optional member in 3 commands, we
> instead write the one command that takes the optional argument when we
> actually care about it.
> 

Yes, taking the later one since a type is needed for transaction support.
Thanks,

Fam