Re: [Qemu-devel] [PATCH 4/5] block-stream: add compress option

[Anton Nefedov <anton.nefedov@virtuozzo.com>]

On 15/11/2017 10:16 PM, Max Reitz wrote:
> On 2017-11-14 11:16, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>>   qapi/block-core.json      |  4 ++++
>>   include/block/block_int.h |  4 +++-
>>   block/stream.c            | 16 ++++++++++++----
>>   blockdev.c                | 13 ++++++++++++-
>>   hmp.c                     |  2 ++
>>   hmp-commands.hx           |  4 ++--
>>   6 files changed, 35 insertions(+), 8 deletions(-)
> 
> Looks good to me overall, two notes below.
> 
> Max
> 
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ab96e34..b0d022f 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2007,6 +2007,9 @@
>>   #
>>   # @speed:  the maximum speed, in bytes per second
>>   #
>> +# @compress: true to compress data, if the target format supports it.
>> +#            (default: false). Since 2.12.
>> +#
> 
> Too many full stops (one before, one after the parentheses).  Also, this
> makes it sound a bit like it would still be possible to specify this
> parameter even if the target doesn't support it (and it would just be
> ignored then), but that's not true.
> 
> Also, the format most parameter documentation follows for block-stream
> is more "@parameter: Description. Default. (Since version)".
> 
> So I'd suggest:
> 
> "true to compress data; may only be set if the target format supports
> it.  Defaults to false if omitted.  (Since 2.12)"
> 
> But I won't argue too much over the format, so the following is OK, too:
> 
> "true to compress data; may only be set if the target format supports it
> (default: false). (Since 2.12)"
> 

Thanks, will fix.

[..]

>> @@ -3034,11 +3039,17 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>>           goto out;
>>       }
>>   
>> +    if (compress && bs->drv->bdrv_co_pwritev_compressed == NULL) {
>> +        error_setg(errp, "Compression is not supported for this drive %s",
>> +                   bdrv_get_device_name(bs));
> 
> I think just device instead of bdrv_get_device_name(bs) is better (or
> bdrv_get_device_or_node_name(bs) at least).
> 

device should be enough indeed, done.

/Anton