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

[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 <address@hidden>
> > ---
> > +++ 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.