From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39415) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YH7Xa-0005Qs-Id for qemu-devel@nongnu.org; Fri, 30 Jan 2015 04:07:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YH7XX-0001xf-Ak for qemu-devel@nongnu.org; Fri, 30 Jan 2015 04:07:02 -0500 Received: from mx2.parallels.com ([199.115.105.18]:56569) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YH7XX-0001xN-4j for qemu-devel@nongnu.org; Fri, 30 Jan 2015 04:06:59 -0500 Message-ID: <54CB49A6.4030302@parallels.com> Date: Fri, 30 Jan 2015 12:06:46 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1421168546-6232-1-git-send-email-vsementsov@parallels.com> <1421168546-6232-8-git-send-email-vsementsov@parallels.com> <54C7B48A.4030606@redhat.com> In-Reply-To: <54C7B48A.4030606@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: kwolf@redhat.com, den@openvz.org, jsnow@redhat.com, stefanha@redhat.com, pbonzini@redhat.com is it better to add qmp_query_dirty_bitmap with underlying bdrv_query_dirty_bitmap, or to modify (add dirty regions information) existing qmp_query_block/qmp_query_dirty_bitmapS? Best regards, Vladimir On 27.01.2015 18:53, Eric Blake wrote: > On 01/13/2015 10:02 AM, Vladimir Sementsov-Ogievskiy wrote: >> Adds qmp and hmp commands to print dirty bitmap. This is needed only for >> testing persistent dirty bitmap feature. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> +++ b/block.c >> @@ -5445,6 +5445,39 @@ int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> return res; >> } >> >> +void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> +{ >> + unsigned long a = 0, b = 0; >> + >> + printf("bitmap '%s'\n", bitmap->name ? bitmap->name : "no name"); >> + printf("enabled: %s\n", bitmap->enabled ? "true" : "false"); >> + printf("size: %" PRId64 "\n", bitmap->size); >> + printf("granularity: %" PRId64 "\n", bitmap->granularity); >> + printf("dirty regions begin:\n"); >> +++ b/blockdev.c >> @@ -2079,6 +2079,19 @@ void qmp_block_dirty_bitmap_add(const char *node_ref, const char *name, >> aio_context_release(aio_context); >> } >> >> +void qmp_block_dirty_bitmap_print(const char *node_ref, const char *name, >> + Error **errp) >> +{ >> + BdrvDirtyBitmap *bitmap; >> + >> + bitmap = block_dirty_bitmap_lookup(node_ref, name, NULL, errp); >> + if (!bitmap) { >> + return; >> + } >> + >> + bdrv_print_dirty_bitmap(bitmap); > Eww. This assumes that stdout is usable. But that is not the case for > QMP commands. It would be better to package up the output in a > structured format and return it to the caller, and let the caller decide > how to print it. > >> +++ b/qapi/block-core.json >> @@ -982,6 +982,9 @@ >> {'command': 'block-dirty-bitmap-disable', >> 'data': 'BlockDirtyBitmap' } >> >> +{'command': 'block-dirty-bitmap-print', >> + 'data': 'BlockDirtyBitmap' } > Missing documentation, if we even want this command. As mentioned > above, this should return ALL of the information necessary for the > client to then decide how to print the information, rather than directly > printing information to stdout itself. And it might be better to name > this command in the 'query-' namespace. >