Re: [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap

2015-01-30 Thread Eric Blake 
On 01/30/2015 02:06 AM, Vladimir Sementsov-Ogievskiy wrote:
 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?

[please don't top-post on technical lists]

Extending an existing command may be reasonable, if the amount of
information being added is compact (if you are adding several kilobytes
of information, that might justify a new command, if only so that old
callers don't waste time receiving large amounts of data on the wire
just to sift through and discard it all to get at the older content they
cared about).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap

2015-01-30 Thread Vladimir Sementsov-Ogievskiy 
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 vsement...@parallels.com
---
+++ 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.

Re: [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap

2015-01-27 Thread Eric Blake 
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 vsement...@parallels.com
 ---

 +++ 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.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap

2015-01-27 Thread Markus Armbruster 
Eric Blake ebl...@redhat.com writes:

 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 vsement...@parallels.com
 ---

 +++ 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.

This isn't a case of would be better, actually.

HMP commands may print to the *monitor*.  Printing to stdout is wrong.

QMP commands return their result as a JSON object.  Printing results
anywhere is wrong.

We commonly implement the HMP command handler on top of the QMP command
handler.  The QMP command handler returns an object.  The QMP core sends
this object in JSON format to the QMP client.  The HMP command prints it
to the monitor in a suitable human-readable format.

 +++ 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.

Yup.

[Qemu-devel] [PATCH 7/8] qmp: print dirty bitmap

2015-01-13 Thread Vladimir Sementsov-Ogievskiy 
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 vsement...@parallels.com
---
 block.c   | 33 +
 blockdev.c| 13 +
 hmp-commands.hx   | 15 +++
 hmp.c |  8 
 hmp.h |  1 +
 include/block/block.h |  1 +
 qapi-schema.json  |  3 ++-
 qapi/block-core.json  |  3 +++
 qmp-commands.hx   |  5 +
 9 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 77419e9..3e6dedf 100644
--- a/block.c
+++ 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);
+
+while (true) {
+for (a = b; a  bitmap-size  !hbitmap_get(bitmap-bitmap, a); ++a) {
+;
+}
+if (a = bitmap-size) {
+break;
+}
+
+for (b = a + 1;
+ b  bitmap-size  hbitmap_get(bitmap-bitmap, b);
+ ++b) {
+;
+}
+
+printf(%ld - %ld\n, a, b - 1);
+if (b = bitmap-size) {
+break;
+}
+}
+
+printf(dirty regions end\n);
+}
+
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
   int granularity,
   const char *name,
diff --git a/blockdev.c b/blockdev.c
index 8a9be08..8b58c2e 100644
--- a/blockdev.c
+++ 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);
+}
+
 void qmp_block_dirty_bitmap_remove(const char *node_ref, const char *name,
Error **errp)
 {
diff --git a/hmp-commands.hx b/hmp-commands.hx
index e37bc8b..a9be506 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -58,6 +58,21 @@ Quit the emulator.
 ETEXI
 
 {
+.name   = print_dirty_bitmap,
+.args_type  = device:B,bitmap:s,
+.params = device bitmap,
+.help   = print dirty bitmap,
+.user_print = monitor_user_noop,
+.mhandler.cmd = hmp_print_dirty_bitmap,
+},
+
+STEXI
+@item print_dirty_bitmap device_id bitmap_name
+@findex print_dirty_bitmap
+Print dirty bitmap meta information and dirty regions.
+ETEXI
+
+{
 .name   = block_resize,
 .args_type  = device:B,size:o,
 .params = device size,
diff --git a/hmp.c b/hmp.c
index 63b19c7..a269145 100644
--- a/hmp.c
+++ b/hmp.c
@@ -782,6 +782,14 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
 qapi_free_TPMInfoList(info_list);
 }
 
+void hmp_print_dirty_bitmap(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, device);
+const char *name = qdict_get_str(qdict, bitmap);
+
+qmp_block_dirty_bitmap_print(device, name, NULL);
+}
+
 void hmp_quit(Monitor *mon, const QDict *qdict)
 {
 monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 4bb5dca..6bbbc33 100644
--- a/hmp.h
+++ b/hmp.h
@@ -19,6 +19,7 @@
 #include qapi-types.h
 #include qapi/qmp/qdict.h
 
+void hmp_print_dirty_bitmap(Monitor *mon, const QDict *qdict);
 void hmp_info_name(Monitor *mon, const QDict *qdict);
 void hmp_info_version(Monitor *mon, const QDict *qdict);
 void hmp_info_kvm(Monitor *mon, const QDict *qdict);
diff --git a/include/block/block.h b/include/block/block.h
index f36557f..7188791 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -466,6 +466,7 @@ BdrvDirtyBitmap *bdrv_load_dirty_bitmap(BlockDriverState 
*bs,
 const char *name,
 Error **errp);
 int bdrv_store_dirty_bitmap(BdrvDirtyBitmap *bitmap);
+void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
 void bdrv_disable_copy_on_read(BlockDriverState *bs);
diff --git a/qapi-schema.json b/qapi-schema.json
index 85f55d9..1475f69 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1263,7 +1263,8 @@
'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
-