[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Anton Nefedov
Subject: Re: [Qemu-devel] [PATCH 4/5] block-stream: add compress option
Date: Thu, 16 Nov 2017 13:11:37 +0300
User-agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

On 15/11/2017 10:16 PM, Max Reitz wrote:
On 2017-11-14 11:16, Anton Nefedov wrote:
Signed-off-by: Anton Nefedov <address@hidden>
  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.


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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]