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

[Vladimir Sementsov-Ogievskiy <vsementsov@parallels.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 <vsementsov@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.
>