[Qemu-devel] [PATCH for-2.2 0/2] qemu-nbd: add option to set detect-zeroes mode

[Qemu-devel] [PATCH for-2.2 0/2] qemu-nbd: add option to set detect-zeroes mode

[Peter Lieven]

Peter Lieven (2):
  rename parse_enum_option to qapi_enum_parse and make it public
  qemu-nbd: add option to set detect-zeroes mode

 blockdev.c          |   30 ++++---------------
 include/qapi/util.h |   17 +++++++++++
 qapi/Makefile.objs  |    2 +-
 qapi/qapi-util.c    |   34 +++++++++++++++++++++
 qemu-nbd.c          |   81 +++++++++++++++++++++++++++++++++------------------
 5 files changed, 111 insertions(+), 53 deletions(-)
 create mode 100644 include/qapi/util.h
 create mode 100644 qapi/qapi-util.c

-- 
1.7.9.5

[Qemu-devel] [PATCH for-2.2 1/2] rename parse_enum_option to qapi_enum_parse and make it public

[Peter Lieven]

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 blockdev.c          |   30 ++++++------------------------
 include/qapi/util.h |   17 +++++++++++++++++
 qapi/Makefile.objs  |    2 +-
 qapi/qapi-util.c    |   34 ++++++++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+), 25 deletions(-)
 create mode 100644 include/qapi/util.h
 create mode 100644 qapi/qapi-util.c

diff --git a/blockdev.c b/blockdev.c
index 48bd9a3..9b93cf2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -39,6 +39,7 @@
 #include "qapi/qmp/types.h"
 #include "qapi-visit.h"
 #include "qapi/qmp-output-visitor.h"
+#include "qapi/util.h"
 #include "sysemu/sysemu.h"
 #include "block/block_int.h"
 #include "qmp-commands.h"
@@ -274,25 +275,6 @@ static int parse_block_error_action(const char *buf, bool is_read, Error **errp)
     }
 }
 
-static inline int parse_enum_option(const char *lookup[], const char *buf,
-                                    int max, int def, Error **errp)
-{
-    int i;
-
-    if (!buf) {
-        return def;
-    }
-
-    for (i = 0; i < max; i++) {
-        if (!strcmp(buf, lookup[i])) {
-            return i;
-        }
-    }
-
-    error_setg(errp, "invalid parameter value: %s", buf);
-    return def;
-}
-
 static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
 {
     if (throttle_conflicting(cfg)) {
@@ -456,11 +438,11 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     }
 
     detect_zeroes =
-        parse_enum_option(BlockdevDetectZeroesOptions_lookup,
-                          qemu_opt_get(opts, "detect-zeroes"),
-                          BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
-                          BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
-                          &error);
+        qapi_enum_parse(BlockdevDetectZeroesOptions_lookup,
+                        qemu_opt_get(opts, "detect-zeroes"),
+                        BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
+                        BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
+                        &error);
     if (error) {
         error_propagate(errp, error);
         goto early_err;
diff --git a/include/qapi/util.h b/include/qapi/util.h
new file mode 100644
index 0000000..de9238b
--- /dev/null
+++ b/include/qapi/util.h
@@ -0,0 +1,17 @@
+/*
+ * QAPI util functions
+ *
+ * Copyright Fujitsu, Inc. 2014
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef QAPI_UTIL_H
+#define QAPI_UTIL_H
+
+int qapi_enum_parse(const char *lookup[], const char *buf,
+                    int max, int def, Error **errp);
+
+#endif
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index d14b769..2278970 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -1,6 +1,6 @@
 util-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o
 util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o
 util-obj-y += string-input-visitor.o string-output-visitor.o
-
 util-obj-y += opts-visitor.o
 util-obj-y += qmp-event.o
+util-obj-y += qapi-util.o
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
new file mode 100644
index 0000000..1d8fb96
--- /dev/null
+++ b/qapi/qapi-util.c
@@ -0,0 +1,34 @@
+/*
+ * QAPI util functions
+ *
+ * Authors:
+ *  Hu Tao       <hutao@cn.fujitsu.com>
+ *  Peter Lieven <pl@kamp.de>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "qapi/util.h"
+
+int qapi_enum_parse(const char *lookup[], const char *buf,
+                    int max, int def, Error **errp)
+{
+    int i;
+
+    if (!buf) {
+        return def;
+    }
+
+    for (i = 0; i < max; i++) {
+        if (!strcmp(buf, lookup[i])) {
+            return i;
+        }
+    }
+
+    error_setg(errp, "invalid parameter value: %s", buf);
+    return def;
+}
-- 
1.7.9.5

[Qemu-devel] [PATCH for-2.2 2/2] qemu-nbd: add option to set detect-zeroes mode

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-nbd.c |   81 +++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 28 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 626e584..75cf54f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -18,11 +18,13 @@
 
 #include "qemu-common.h"
 #include "block/block.h"
+#include "block/block_int.h"
 #include "block/nbd.h"
 #include "qemu/main-loop.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
 #include "block/snapshot.h"
+#include "qapi/util.h"
 
 #include <stdarg.h>
 #include <stdio.h>
@@ -38,9 +40,10 @@
 #include <pthread.h>
 
 #define SOCKET_PATH          "/var/lock/qemu-nbd-%s"
-#define QEMU_NBD_OPT_CACHE   1
-#define QEMU_NBD_OPT_AIO     2
-#define QEMU_NBD_OPT_DISCARD 3
+#define QEMU_NBD_OPT_CACHE         1
+#define QEMU_NBD_OPT_AIO           2
+#define QEMU_NBD_OPT_DISCARD       3
+#define QEMU_NBD_OPT_DETECT_ZEROES 4
 
 static NBDExport *exp;
 static int verbose;
@@ -57,45 +60,47 @@ static void usage(const char *name)
 "Usage: %s [OPTIONS] FILE\n"
 "QEMU Disk Network Block Device Server\n"
 "\n"
-"  -h, --help           display this help and exit\n"
-"  -V, --version        output version information and exit\n"
+"  -h, --help                display this help and exit\n"
+"  -V, --version             output version information and exit\n"
 "\n"
 "Connection properties:\n"
-"  -p, --port=PORT      port to listen on (default `%d')\n"
-"  -b, --bind=IFACE     interface to bind to (default `0.0.0.0')\n"
-"  -k, --socket=PATH    path to the unix socket\n"
-"                       (default '"SOCKET_PATH"')\n"
-"  -e, --shared=NUM     device can be shared by NUM clients (default '1')\n"
-"  -t, --persistent     don't exit on the last connection\n"
-"  -v, --verbose        display extra debugging information\n"
+"  -p, --port=PORT           port to listen on (default `%d')\n"
+"  -b, --bind=IFACE          interface to bind to (default `0.0.0.0')\n"
+"  -k, --socket=PATH         path to the unix socket\n"
+"                            (default '"SOCKET_PATH"')\n"
+"  -e, --shared=NUM          device can be shared by NUM clients (default '1')\n"
+"  -t, --persistent          don't exit on the last connection\n"
+"  -v, --verbose             display extra debugging information\n"
 "\n"
 "Exposing part of the image:\n"
-"  -o, --offset=OFFSET  offset into the image\n"
-"  -P, --partition=NUM  only expose partition NUM\n"
+"  -o, --offset=OFFSET       offset into the image\n"
+"  -P, --partition=NUM       only expose partition NUM\n"
 "\n"
 #ifdef __linux__
 "Kernel NBD client support:\n"
-"  -c, --connect=DEV    connect FILE to the local NBD device DEV\n"
-"  -d, --disconnect     disconnect the specified device\n"
+"  -c, --connect=DEV         connect FILE to the local NBD device DEV\n"
+"  -d, --disconnect          disconnect the specified device\n"
 "\n"
 #endif
 "\n"
 "Block device options:\n"
-"  -f, --format=FORMAT  set image format (raw, qcow2, ...)\n"
-"  -r, --read-only      export read-only\n"
-"  -s, --snapshot       use FILE as an external snapshot, create a temporary\n"
-"                       file with backing_file=FILE, redirect the write to\n"
-"                       the temporary one\n"
+"  -f, --format=FORMAT       set image format (raw, qcow2, ...)\n"
+"  -r, --read-only           export read-only\n"
+"  -s, --snapshot            use FILE as an external snapshot, create a temporary\n"
+"                            file with backing_file=FILE, redirect the write to\n"
+"                            the temporary one\n"
 "  -l, --load-snapshot=SNAPSHOT_PARAM\n"
-"                       load an internal snapshot inside FILE and export it\n"
-"                       as an read-only device, SNAPSHOT_PARAM format is\n"
-"                       'snapshot.id=[ID],snapshot.name=[NAME]', or\n"
-"                       '[ID_OR_NAME]'\n"
-"  -n, --nocache        disable host cache\n"
-"      --cache=MODE     set cache mode (none, writeback, ...)\n"
+"                            load an internal snapshot inside FILE and export it\n"
+"                            as an read-only device, SNAPSHOT_PARAM format is\n"
+"                            'snapshot.id=[ID],snapshot.name=[NAME]', or\n"
+"                            '[ID_OR_NAME]'\n"
+"  -n, --nocache             disable host cache\n"
+"      --cache=MODE          set cache mode (none, writeback, ...)\n"
 #ifdef CONFIG_LINUX_AIO
-"      --aio=MODE       set AIO mode (native or threads)\n"
+"      --aio=MODE            set AIO mode (native or threads)\n"
 #endif
+"      --discard=MODE        set discard mode (ignore, unmap)\n"
+"      --detect-zeroes=MODE  set detect-zeroes mode (off, on, discard)\n"
 "\n"
 "Report bugs to <qemu-devel@nongnu.org>\n"
     , name, NBD_DEFAULT_PORT, "DEVICE");
@@ -410,6 +415,7 @@ int main(int argc, char **argv)
         { "aio", 1, NULL, QEMU_NBD_OPT_AIO },
 #endif
         { "discard", 1, NULL, QEMU_NBD_OPT_DISCARD },
+        { "detect-zeroes", 1, NULL, QEMU_NBD_OPT_DETECT_ZEROES },
         { "shared", 1, NULL, 'e' },
         { "format", 1, NULL, 'f' },
         { "persistent", 0, NULL, 't' },
@@ -432,6 +438,7 @@ int main(int argc, char **argv)
     pthread_t client_thread;
     const char *fmt = NULL;
     Error *local_err = NULL;
+    BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -483,6 +490,23 @@ int main(int argc, char **argv)
                 errx(EXIT_FAILURE, "Invalid discard mode `%s'", optarg);
             }
             break;
+        case QEMU_NBD_OPT_DETECT_ZEROES:
+            detect_zeroes =
+                qapi_enum_parse(BlockdevDetectZeroesOptions_lookup,
+                                optarg,
+                                BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
+                                BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
+                                &local_err);
+            if (local_err) {
+                errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s",
+                     error_get_pretty(local_err));
+            }
+            if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
+                !(flags & BDRV_O_UNMAP)) {
+                errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed "
+                                   "without setting discard operation to unmap");
+            }
+            break;
         case 'b':
             bindto = optarg;
             break;
@@ -686,6 +710,7 @@ int main(int argc, char **argv)
             error_get_pretty(local_err));
     }
 
+    bs->detect_zeroes = detect_zeroes;
     fd_size = bdrv_getlength(bs);
 
     if (partition != -1) {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH for-2.2 1/2] rename parse_enum_option to qapi_enum_parse and make it public

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

On 07/28/2014 01:43 PM, Peter Lieven wrote:
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  blockdev.c          |   30 ++++++------------------------
>  include/qapi/util.h |   17 +++++++++++++++++
>  qapi/Makefile.objs  |    2 +-
>  qapi/qapi-util.c    |   34 ++++++++++++++++++++++++++++++++++
>  4 files changed, 58 insertions(+), 25 deletions(-)
>  create mode 100644 include/qapi/util.h
>  create mode 100644 qapi/qapi-util.c

> +++ b/blockdev.c
> @@ -39,6 +39,7 @@
>  #include "qapi/qmp/types.h"
>  #include "qapi-visit.h"
>  #include "qapi/qmp-output-visitor.h"
> +#include "qapi/util.h"
>  #include "sysemu/sysemu.h"
>  #include "block/block_int.h"
>  #include "qmp-commands.h"
> @@ -274,25 +275,6 @@ static int parse_block_error_action(const char *buf, bool is_read, Error **errp)
>      }
>  }
>  
> -static inline int parse_enum_option(const char *lookup[], const char *buf,
> -                                    int max, int def, Error **errp)

This file is GPLv2+,...

> +++ b/qapi/qapi-util.c
> @@ -0,0 +1,34 @@
> +/*
> + * QAPI util functions
> + *
> + * Authors:
> + *  Hu Tao       <hutao@cn.fujitsu.com>
> + *  Peter Lieven <pl@kamp.de>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.

...but you have relaxed the license to LGPLv2+ in your code motion.
Then again, Peter is the original author of this code in commit
82a402e9, so you have the legal right to relax things.

If this was intentional, then it probably should have been mentioned in
the commit message.  With that done:
Reviewed-by: Eric Blake <eblake@redhat.com>
If it was inadvertent, then post a v2 that makes the new file GPLv2+,
for the avoidance of doubt.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH for-2.2 2/2] qemu-nbd: add option to set detect-zeroes mode

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

On 07/28/2014 01:43 PM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-nbd.c |   81 +++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 53 insertions(+), 28 deletions(-)
> 

> @@ -57,45 +60,47 @@ static void usage(const char *name)
>  "Usage: %s [OPTIONS] FILE\n"
>  "QEMU Disk Network Block Device Server\n"
>  "\n"
> -"  -h, --help           display this help and exit\n"
> -"  -V, --version        output version information and exit\n"
> +"  -h, --help                display this help and exit\n"
> +"  -V, --version             output version information and exit\n"

More than 50% of this patch is reformatting existing code to prepare for
new whitespace alignment.  It makes it a bit harder to find the meat of
the patch; I might have split the realignment and the code additions
into separate commits.  Not a reason for a respin by itself, but food
for thought if you do a v2.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH for-2.2 1/2] rename parse_enum_option to qapi_enum_parse and make it public

[Peter Lieven]

Am 28.07.2014 um 21:58 schrieb Eric Blake <eblake@redhat.com>:

> On 07/28/2014 01:43 PM, Peter Lieven wrote:
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> blockdev.c          |   30 ++++++------------------------
>> include/qapi/util.h |   17 +++++++++++++++++
>> qapi/Makefile.objs  |    2 +-
>> qapi/qapi-util.c    |   34 ++++++++++++++++++++++++++++++++++
>> 4 files changed, 58 insertions(+), 25 deletions(-)
>> create mode 100644 include/qapi/util.h
>> create mode 100644 qapi/qapi-util.c
> 
>> +++ b/blockdev.c
>> @@ -39,6 +39,7 @@
>> #include "qapi/qmp/types.h"
>> #include "qapi-visit.h"
>> #include "qapi/qmp-output-visitor.h"
>> +#include "qapi/util.h"
>> #include "sysemu/sysemu.h"
>> #include "block/block_int.h"
>> #include "qmp-commands.h"
>> @@ -274,25 +275,6 @@ static int parse_block_error_action(const char *buf, bool is_read, Error **errp)
>>     }
>> }
>> 
>> -static inline int parse_enum_option(const char *lookup[], const char *buf,
>> -                                    int max, int def, Error **errp)
> 
> This file is GPLv2+,...
> 
>> +++ b/qapi/qapi-util.c
>> @@ -0,0 +1,34 @@
>> +/*
>> + * QAPI util functions
>> + *
>> + * Authors:
>> + *  Hu Tao       <hutao@cn.fujitsu.com>
>> + *  Peter Lieven <pl@kamp.de>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>> + * See the COPYING.LIB file in the top-level directory.
> 
> ...but you have relaxed the license to LGPLv2+ in your code motion.
> Then again, Peter is the original author of this code in commit
> 82a402e9, so you have the legal right to relax things.

Actually, I was not aware. I just took Hu Taos original commit. As we want
to make a collection of utilities I would go as far as put a BSD license here.

I will send a v2 with your comments regarding Patch 2. And do that license
change as well.

Peter

> 
> If this was intentional, then it probably should have been mentioned in
> the commit message.  With that done:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> If it was inadvertent, then post a v2 that makes the new file GPLv2+,
> for the avoidance of doubt.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH for-2.2 1/2] rename parse_enum_option to qapi_enum_parse and make it public

[Paolo Bonzini]

Il 28/07/2014 22:06, Peter Lieven ha scritto:
>> > ...but you have relaxed the license to LGPLv2+ in your code motion.
>> > Then again, Peter is the original author of this code in commit
>> > 82a402e9, so you have the legal right to relax things.
> Actually, I was not aware. I just took Hu Taos original commit. As we want
> to make a collection of utilities I would go as far as put a BSD license here.
> 
> I will send a v2 with your comments regarding Patch 2. And do that license
> change as well.

LGPL v2.1+ is the standard license for qapi/, so I think it's okay to
use either.

Paolo