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

[Markus Armbruster]

Eric Blake <address@hidden> 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 <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.

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.