[Qemu-devel] [PATCH v3] block: Introduce "null" driver

[Qemu-devel] [PATCH v3] block: Introduce "null" driver

[Fam Zheng]

This is an analogue to Linux null_blk. It can be used for testing block
device emulation and general block layer functionalities such as
coroutines and throttling, where disk IO is not necessary or wanted.

Use null:// for AIO version, and null-co:// for coroutine version.

Signed-off-by: Fam Zheng <famz@redhat.com>

---
V3: Drop copy&paste blkdebug structure. (Beniot)

V2: Don't #ifdef code, add two drivers. (Benoit)
    Add to QAPI BlockdevOptions. (Eric)
    Add "file.size" option to override backend size. (What is a better
    way to associate /dev/vd{a,b,c} with command line devices, if sizes
    are the same?)
---
 block/Makefile.objs  |   1 +
 block/null.c         | 179 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json |  19 +++++-
 3 files changed, 197 insertions(+), 2 deletions(-)
 create mode 100644 block/null.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 858d2b3..087e281 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -9,6 +9,7 @@ block-obj-y += snapshot.o qapi.o
 block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
 block-obj-$(CONFIG_POSIX) += raw-posix.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
+block-obj-y += null.o
 
 ifeq ($(CONFIG_POSIX),y)
 block-obj-y += nbd.o nbd-client.o sheepdog.o
diff --git a/block/null.c b/block/null.c
new file mode 100644
index 0000000..171fd19
--- /dev/null
+++ b/block/null.c
@@ -0,0 +1,179 @@
+/*
+ * Null block driver
+ *
+ * Authors:
+ *  Fam Zheng <famz@redhat.com>
+ *
+ * Copyright (C) 2014 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "block/block_int.h"
+
+typedef struct {
+    int64_t length;
+} BDRVNullState;
+
+static QemuOptsList runtime_opts = {
+    .name = "null",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "filename",
+            .type = QEMU_OPT_STRING,
+            .help = "",
+        },
+        {
+            .name = BLOCK_OPT_SIZE,
+            .type = QEMU_OPT_SIZE,
+            .help = "size of the null block",
+        },
+        { /* end of list */ }
+    },
+};
+
+static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
+                          Error **errp)
+{
+    QemuOpts *opts;
+    BDRVNullState *s = bs->opaque;
+
+    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &error_abort);
+    s->length =
+        qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
+    qemu_opts_del(opts);
+    return 0;
+}
+
+static void null_close(BlockDriverState *bs)
+{
+}
+
+static int64_t null_getlength(BlockDriverState *bs)
+{
+    BDRVNullState *s = bs->opaque;
+    return s->length;
+}
+
+static coroutine_fn int null_co_read(BlockDriverState *bs, int64_t sector_num,
+                                     uint8_t *buf, int nb_sectors)
+{
+    return 0;
+}
+
+static coroutine_fn int null_co_write(BlockDriverState *bs, int64_t sector_num,
+                                      const uint8_t *buf, int nb_sectors)
+{
+    return 0;
+}
+
+static coroutine_fn int null_co_flush(BlockDriverState *bs)
+{
+    return 0;
+}
+
+typedef struct {
+    BlockDriverAIOCB common;
+    QEMUBH *bh;
+} NullAIOCB;
+
+static void null_aio_cancel(BlockDriverAIOCB *blockacb);
+
+static const AIOCBInfo null_aiocb_info = {
+    .aiocb_size = sizeof(NullAIOCB),
+    .cancel     = null_aio_cancel,
+};
+
+static void null_bh_cb(void *opaque)
+{
+    NullAIOCB *acb = opaque;
+    acb->common.cb(acb->common.opaque, 0);
+    qemu_bh_delete(acb->bh);
+    qemu_aio_release(acb);
+}
+
+static BlockDriverAIOCB *null_aio_readv(BlockDriverState *bs,
+                                        int64_t sector_num, QEMUIOVector *qiov,
+                                        int nb_sectors,
+                                        BlockDriverCompletionFunc *cb,
+                                        void *opaque)
+{
+    NullAIOCB *acb;
+
+    acb = qemu_aio_get(&null_aiocb_info, bs, cb, opaque);
+    acb->bh = aio_bh_new(bdrv_get_aio_context(bs), null_bh_cb, acb);
+    qemu_bh_schedule(acb->bh);
+    return &acb->common;
+}
+
+static BlockDriverAIOCB *null_aio_writev(BlockDriverState *bs,
+                                         int64_t sector_num, QEMUIOVector *qiov,
+                                         int nb_sectors,
+                                         BlockDriverCompletionFunc *cb,
+                                         void *opaque)
+{
+    NullAIOCB *acb;
+
+    acb = qemu_aio_get(&null_aiocb_info, bs, cb, opaque);
+    acb->bh = aio_bh_new(bdrv_get_aio_context(bs), null_bh_cb, acb);
+    qemu_bh_schedule(acb->bh);
+    return &acb->common;
+}
+
+static BlockDriverAIOCB *null_aio_flush(BlockDriverState *bs,
+                                        BlockDriverCompletionFunc *cb,
+                                        void *opaque)
+{
+    NullAIOCB *acb;
+
+    acb = qemu_aio_get(&null_aiocb_info, bs, cb, opaque);
+    acb->bh = aio_bh_new(bdrv_get_aio_context(bs), null_bh_cb, acb);
+    qemu_bh_schedule(acb->bh);
+    return &acb->common;
+}
+
+static void null_aio_cancel(BlockDriverAIOCB *blockacb)
+{
+    NullAIOCB *acb = container_of(blockacb, NullAIOCB, common);
+    qemu_bh_delete(acb->bh);
+    qemu_aio_release(acb);
+}
+
+static BlockDriver bdrv_null = {
+    .format_name            = "null",
+    .protocol_name          = "null",
+    .instance_size          = sizeof(BDRVNullState),
+
+    .bdrv_file_open         = null_file_open,
+    .bdrv_close             = null_close,
+    .bdrv_getlength         = null_getlength,
+
+    .bdrv_aio_readv         = null_aio_readv,
+    .bdrv_aio_writev        = null_aio_writev,
+    .bdrv_aio_flush         = null_aio_flush,
+};
+
+static BlockDriver bdrv_null_co = {
+    .format_name            = "null-co",
+    .protocol_name          = "null-co",
+    .instance_size          = sizeof(BDRVNullState),
+
+    .bdrv_file_open         = null_file_open,
+    .bdrv_close             = null_close,
+    .bdrv_getlength         = null_getlength,
+
+    .bdrv_read              = null_co_read,
+    .bdrv_write             = null_co_write,
+    .bdrv_co_flush_to_disk  = null_co_flush,
+};
+
+static void bdrv_null_init(void)
+{
+    bdrv_register(&bdrv_null);
+    bdrv_register(&bdrv_null_co);
+}
+
+block_init(bdrv_null_init);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index fb74c56..9b2c9c6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1150,7 +1150,8 @@
   'data': [ 'archipelago', 'file', 'host_device', 'host_cdrom', 'host_floppy',
             'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
             'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
-            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
+            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum',
+            'null' ] }
 
 ##
 # @BlockdevOptionsBase
@@ -1203,6 +1204,19 @@
   'data': { 'filename': 'str' } }
 
 ##
+# @BlockdevOptionsNull
+#
+# Driver specific block device options for the null backend.
+#
+# @size:    size of the device in bytes.
+#
+# Since: 2.2
+##
+{ 'type': 'BlockdevOptionsNull',
+  'base': 'BlockdevOptionsFile',
+  'data': { '*size': 'int' } }
+
+##
 # @BlockdevOptionsVVFAT
 #
 # Driver specific block device options for the vvfat protocol.
@@ -1484,7 +1498,8 @@
       'vhdx':       'BlockdevOptionsGenericFormat',
       'vmdk':       'BlockdevOptionsGenericCOWFormat',
       'vpc':        'BlockdevOptionsGenericFormat',
-      'quorum':     'BlockdevOptionsQuorum'
+      'quorum':     'BlockdevOptionsQuorum',
+      'null':       'BlockdevOptionsNull'
   } }
 
 ##
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3] block: Introduce "null" driver

[Benoît Canet]

The Thursday 28 Aug 2014 à 13:53:11 (+0800), Fam Zheng wrote :
> This is an analogue to Linux null_blk. It can be used for testing block
> device emulation and general block layer functionalities such as
> coroutines and throttling, where disk IO is not necessary or wanted.
> 
> Use null:// for AIO version, and null-co:// for coroutine version.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> ---
> V3: Drop copy&paste blkdebug structure. (Beniot)
> 
> V2: Don't #ifdef code, add two drivers. (Benoit)
>     Add to QAPI BlockdevOptions. (Eric)
>     Add "file.size" option to override backend size. (What is a better
>     way to associate /dev/vd{a,b,c} with command line devices, if sizes
>     are the same?)
> ---
>  block/Makefile.objs  |   1 +
>  block/null.c         | 179 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json |  19 +++++-
>  3 files changed, 197 insertions(+), 2 deletions(-)
>  create mode 100644 block/null.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 858d2b3..087e281 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -9,6 +9,7 @@ block-obj-y += snapshot.o qapi.o
>  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
>  block-obj-$(CONFIG_POSIX) += raw-posix.o
>  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> +block-obj-y += null.o
>  
>  ifeq ($(CONFIG_POSIX),y)
>  block-obj-y += nbd.o nbd-client.o sheepdog.o
> diff --git a/block/null.c b/block/null.c
> new file mode 100644
> index 0000000..171fd19
> --- /dev/null
> +++ b/block/null.c
> @@ -0,0 +1,179 @@
> +/*
> + * Null block driver
> + *
> + * Authors:
> + *  Fam Zheng <famz@redhat.com>
> + *
> + * Copyright (C) 2014 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "block/block_int.h"
> +
> +typedef struct {
> +    int64_t length;
> +} BDRVNullState;
> +
> +static QemuOptsList runtime_opts = {
> +    .name = "null",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> +    .desc = {
> +        {
> +            .name = "filename",
> +            .type = QEMU_OPT_STRING,
> +            .help = "",
> +        },

You seems to define filename both here and in QMP but null_file_open don't use it neither
any part of the code.

Do you declare it only to fool some other part of the block layer ?

> +        {
> +            .name = BLOCK_OPT_SIZE,
> +            .type = QEMU_OPT_SIZE,
> +            .help = "size of the null block",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
> +                          Error **errp)
> +{
> +    QemuOpts *opts;
> +    BDRVNullState *s = bs->opaque;
> +
> +    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> +    qemu_opts_absorb_qdict(opts, options, &error_abort);
> +    s->length =
> +        qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
> +    qemu_opts_del(opts);
> +    return 0;
> +}
> +
> +static void null_close(BlockDriverState *bs)
> +{
> +}
> +
> +static int64_t null_getlength(BlockDriverState *bs)
> +{
> +    BDRVNullState *s = bs->opaque;
> +    return s->length;
> +}
> +
> +static coroutine_fn int null_co_read(BlockDriverState *bs, int64_t sector_num,
> +                                     uint8_t *buf, int nb_sectors)
> +{
> +    return 0;
> +}
> +
> +static coroutine_fn int null_co_write(BlockDriverState *bs, int64_t sector_num,
> +                                      const uint8_t *buf, int nb_sectors)
> +{
> +    return 0;
> +}
> +
> +static coroutine_fn int null_co_flush(BlockDriverState *bs)
> +{
> +    return 0;
> +}
> +
> +typedef struct {
> +    BlockDriverAIOCB common;
> +    QEMUBH *bh;
> +} NullAIOCB;
> +
> +static void null_aio_cancel(BlockDriverAIOCB *blockacb);
> +
> +static const AIOCBInfo null_aiocb_info = {
> +    .aiocb_size = sizeof(NullAIOCB),
> +    .cancel     = null_aio_cancel,
> +};
> +
> +static void null_bh_cb(void *opaque)
> +{
> +    NullAIOCB *acb = opaque;
> +    acb->common.cb(acb->common.opaque, 0);
> +    qemu_bh_delete(acb->bh);
> +    qemu_aio_release(acb);
> +}
> +
> +static BlockDriverAIOCB *null_aio_readv(BlockDriverState *bs,
> +                                        int64_t sector_num, QEMUIOVector *qiov,
> +                                        int nb_sectors,
> +                                        BlockDriverCompletionFunc *cb,
> +                                        void *opaque)
> +{
> +    NullAIOCB *acb;
> +
> +    acb = qemu_aio_get(&null_aiocb_info, bs, cb, opaque);
> +    acb->bh = aio_bh_new(bdrv_get_aio_context(bs), null_bh_cb, acb);
> +    qemu_bh_schedule(acb->bh);
> +    return &acb->common;
> +}
> +
> +static BlockDriverAIOCB *null_aio_writev(BlockDriverState *bs,
> +                                         int64_t sector_num, QEMUIOVector *qiov,
> +                                         int nb_sectors,
> +                                         BlockDriverCompletionFunc *cb,
> +                                         void *opaque)
> +{
> +    NullAIOCB *acb;
> +
> +    acb = qemu_aio_get(&null_aiocb_info, bs, cb, opaque);
> +    acb->bh = aio_bh_new(bdrv_get_aio_context(bs), null_bh_cb, acb);
> +    qemu_bh_schedule(acb->bh);
> +    return &acb->common;
> +}
> +
> +static BlockDriverAIOCB *null_aio_flush(BlockDriverState *bs,
> +                                        BlockDriverCompletionFunc *cb,
> +                                        void *opaque)
> +{
> +    NullAIOCB *acb;
> +
> +    acb = qemu_aio_get(&null_aiocb_info, bs, cb, opaque);
> +    acb->bh = aio_bh_new(bdrv_get_aio_context(bs), null_bh_cb, acb);
> +    qemu_bh_schedule(acb->bh);
> +    return &acb->common;
> +}

The former three function body are strictly identical maybe you could factorize them.
They would become thin wrapper around one common function.

> +
> +static void null_aio_cancel(BlockDriverAIOCB *blockacb)
> +{
> +    NullAIOCB *acb = container_of(blockacb, NullAIOCB, common);
> +    qemu_bh_delete(acb->bh);
> +    qemu_aio_release(acb);
> +}
> +
> +static BlockDriver bdrv_null = {
> +    .format_name            = "null",
> +    .protocol_name          = "null",
> +    .instance_size          = sizeof(BDRVNullState),
> +
> +    .bdrv_file_open         = null_file_open,
> +    .bdrv_close             = null_close,
> +    .bdrv_getlength         = null_getlength,
> +
> +    .bdrv_aio_readv         = null_aio_readv,
> +    .bdrv_aio_writev        = null_aio_writev,
> +    .bdrv_aio_flush         = null_aio_flush,
> +};
> +
> +static BlockDriver bdrv_null_co = {
> +    .format_name            = "null-co",
> +    .protocol_name          = "null-co",
> +    .instance_size          = sizeof(BDRVNullState),
> +
> +    .bdrv_file_open         = null_file_open,
> +    .bdrv_close             = null_close,
> +    .bdrv_getlength         = null_getlength,
> +
> +    .bdrv_read              = null_co_read,
> +    .bdrv_write             = null_co_write,
> +    .bdrv_co_flush_to_disk  = null_co_flush,
> +};
> +
> +static void bdrv_null_init(void)
> +{
> +    bdrv_register(&bdrv_null);
> +    bdrv_register(&bdrv_null_co);
> +}
> +
> +block_init(bdrv_null_init);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index fb74c56..9b2c9c6 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1150,7 +1150,8 @@
>    'data': [ 'archipelago', 'file', 'host_device', 'host_cdrom', 'host_floppy',
>              'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
>              'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
> -            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
> +            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum',
> +            'null' ] }

Why not also adding null-co to QMP ?

>  
>  ##
>  # @BlockdevOptionsBase
> @@ -1203,6 +1204,19 @@
>    'data': { 'filename': 'str' } }
>  
>  ##
> +# @BlockdevOptionsNull
> +#
> +# Driver specific block device options for the null backend.
> +#
> +# @size:    size of the device in bytes.
> +#
> +# Since: 2.2
> +##
> +{ 'type': 'BlockdevOptionsNull',
> +  'base': 'BlockdevOptionsFile',
> +  'data': { '*size': 'int' } }
> +
> +##
>  # @BlockdevOptionsVVFAT
>  #
>  # Driver specific block device options for the vvfat protocol.
> @@ -1484,7 +1498,8 @@
>        'vhdx':       'BlockdevOptionsGenericFormat',
>        'vmdk':       'BlockdevOptionsGenericCOWFormat',
>        'vpc':        'BlockdevOptionsGenericFormat',
> -      'quorum':     'BlockdevOptionsQuorum'
> +      'quorum':     'BlockdevOptionsQuorum',
> +      'null':       'BlockdevOptionsNull'
>    } }
>  
>  ##
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [PATCH v3] block: Introduce "null" driver

[Eric Blake]

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

On 08/28/2014 09:22 AM, Benoît Canet wrote:
> The Thursday 28 Aug 2014 à 13:53:11 (+0800), Fam Zheng wrote :
>> This is an analogue to Linux null_blk. It can be used for testing block
>> device emulation and general block layer functionalities such as
>> coroutines and throttling, where disk IO is not necessary or wanted.
>>
>> Use null:// for AIO version, and null-co:// for coroutine version.
>>

>> +++ b/qapi/block-core.json
>> @@ -1150,7 +1150,8 @@
>>    'data': [ 'archipelago', 'file', 'host_device', 'host_cdrom', 'host_floppy',
>>              'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
>>              'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
>> -            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
>> +            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum',
>> +            'null' ] }
> 
> Why not also adding null-co to QMP ?

Or have just one driver 'null', but...

> 
>>  
>>  ##
>>  # @BlockdevOptionsBase
>> @@ -1203,6 +1204,19 @@
>>    'data': { 'filename': 'str' } }
>>  
>>  ##
>> +# @BlockdevOptionsNull
>> +#
>> +# Driver specific block device options for the null backend.
>> +#
>> +# @size:    size of the device in bytes.
>> +#
>> +# Since: 2.2
>> +##
>> +{ 'type': 'BlockdevOptionsNull',
>> +  'base': 'BlockdevOptionsFile',
>> +  'data': { '*size': 'int' } }

have a '*coroutine':'bool' flag here that chooses between the null: and
the null-co: protocol.  (I suspect we would do the same when finaly
adding gluster to BlockdevOptions: rather than having 'gluster+tcp' and
'gluster+udp', it would be a single 'gluster' element that can then
select transport of tcp vs. udp as an option).

-- 
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 v3] block: Introduce "null" driver

[Paolo Bonzini]

Il 28/08/2014 17:52, Eric Blake ha scritto:
> have a '*coroutine':'bool' flag here that chooses between the null: and
> the null-co: protocol.  (I suspect we would do the same when finaly
> adding gluster to BlockdevOptions: rather than having 'gluster+tcp' and
> 'gluster+udp', it would be a single 'gluster' element that can then
> select transport of tcp vs. udp as an option).

I'm not sure it's possible in the case of null, since the two
BlockDriver structs have different function pointers.  Instead, gluster
has the same function pointers and just multiple names to trigger the
parsing of all the URI schemes.

Paolo

Re: [Qemu-devel] [PATCH v3] block: Introduce "null" driver

[Eric Blake]

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

On 08/28/2014 01:42 PM, Paolo Bonzini wrote:
> Il 28/08/2014 17:52, Eric Blake ha scritto:
>> have a '*coroutine':'bool' flag here that chooses between the null: and
>> the null-co: protocol.  (I suspect we would do the same when finaly
>> adding gluster to BlockdevOptions: rather than having 'gluster+tcp' and
>> 'gluster+udp', it would be a single 'gluster' element that can then
>> select transport of tcp vs. udp as an option).
> 
> I'm not sure it's possible in the case of null, since the two
> BlockDriver structs have different function pointers.  Instead, gluster
> has the same function pointers and just multiple names to trigger the
> parsing of all the URI schemes.

But why can't you write a single driver, where each function in the
driver then bases a decision between one of the two original functions
based on the configuration?  Is it a case where the set of callback
functions differs with one driver having a NULL callback where the other
does not, and where the presence or absence of the NULL callback
actually matters to the point of requiring two drivers?

-- 
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 v3] block: Introduce "null" driver

[Eric Blake]

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

On 08/28/2014 04:15 PM, Eric Blake wrote:
> On 08/28/2014 01:42 PM, Paolo Bonzini wrote:
>> Il 28/08/2014 17:52, Eric Blake ha scritto:
>>> have a '*coroutine':'bool' flag here that chooses between the null: and
>>> the null-co: protocol.  (I suspect we would do the same when finaly
>>> adding gluster to BlockdevOptions: rather than having 'gluster+tcp' and
>>> 'gluster+udp', it would be a single 'gluster' element that can then
>>> select transport of tcp vs. udp as an option).
>>
>> I'm not sure it's possible in the case of null, since the two
>> BlockDriver structs have different function pointers.  Instead, gluster
>> has the same function pointers and just multiple names to trigger the
>> parsing of all the URI schemes.

Answering myself:

I read this paragraph without reading the code, and thought you were
talking about "null" having '.func = null_plain' vs. "null_co" having
'.func = null_co' (ie. each driver having different functions
registered, but in the same slots).

> 
> But why can't you write a single driver, where each function in the
> driver then bases a decision between one of the two original functions
> based on the configuration?  Is it a case where the set of callback
> functions differs with one driver having a NULL callback where the other
> does not, and where the presence or absence of the NULL callback
> actually matters to the point of requiring two drivers?

But now that I read the patch, I see my answer is yes, it DOES matter
where the NULL pointers are.

"null" has
+    .bdrv_aio_readv         = null_aio_readv,
+    .bdrv_aio_writev        = null_aio_writev,
+    .bdrv_aio_flush         = null_aio_flush,

while "null_co" has
+    .bdrv_read              = null_co_read,
+    .bdrv_write             = null_co_write,
+    .bdrv_co_flush_to_disk  = null_co_flush,

and the block driver code under test behaves differently depending on
which of those callbacks is NULL, where we want to test both behaviors.
 So it sounds like we really do want two separate QMP driver additions.

-- 
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 v3] block: Introduce "null" driver

[Eric Blake]

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

On 08/27/2014 11:53 PM, Fam Zheng wrote:
> This is an analogue to Linux null_blk. It can be used for testing block
> device emulation and general block layer functionalities such as
> coroutines and throttling, where disk IO is not necessary or wanted.
> 
> Use null:// for AIO version, and null-co:// for coroutine version.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> ---

> +++ b/qapi/block-core.json
> @@ -1150,7 +1150,8 @@
>    'data': [ 'archipelago', 'file', 'host_device', 'host_cdrom', 'host_floppy',
>              'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
>              'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
> -            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
> +            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum',
> +            'null' ] }

As mentioned elsewhere, you probably want 'null' AND 'null-co'.  Bummer
that this list is mostly alphabetical, but that 'quorum' and 'null' are
out of order.

>  
>  ##
>  # @BlockdevOptionsBase
> @@ -1203,6 +1204,19 @@
>    'data': { 'filename': 'str' } }
>  
>  ##
> +# @BlockdevOptionsNull
> +#
> +# Driver specific block device options for the null backend.
> +#
> +# @size:    size of the device in bytes.

missing an #optional marker

> +#
> +# Since: 2.2
> +##
> +{ 'type': 'BlockdevOptionsNull',
> +  'base': 'BlockdevOptionsFile',
> +  'data': { '*size': 'int' } }
> +
> +##
>  # @BlockdevOptionsVVFAT
>  #
>  # Driver specific block device options for the vvfat protocol.
> @@ -1484,7 +1498,8 @@
>        'vhdx':       'BlockdevOptionsGenericFormat',
>        'vmdk':       'BlockdevOptionsGenericCOWFormat',
>        'vpc':        'BlockdevOptionsGenericFormat',
> -      'quorum':     'BlockdevOptionsQuorum'
> +      'quorum':     'BlockdevOptionsQuorum',
> +      'null':       'BlockdevOptionsNull'
>    } }
>  
>  ##
> 

-- 
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 v3] block: Introduce "null" driver

[Fam Zheng]

On Thu, 08/28 17:22, Benoît Canet wrote:
> The Thursday 28 Aug 2014 à 13:53:11 (+0800), Fam Zheng wrote :
> > This is an analogue to Linux null_blk. It can be used for testing block
> > device emulation and general block layer functionalities such as
> > coroutines and throttling, where disk IO is not necessary or wanted.
> > 
> > Use null:// for AIO version, and null-co:// for coroutine version.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > ---
> > V3: Drop copy&paste blkdebug structure. (Beniot)
> > 
> > V2: Don't #ifdef code, add two drivers. (Benoit)
> >     Add to QAPI BlockdevOptions. (Eric)
> >     Add "file.size" option to override backend size. (What is a better
> >     way to associate /dev/vd{a,b,c} with command line devices, if sizes
> >     are the same?)
> > ---
> >  block/Makefile.objs  |   1 +
> >  block/null.c         | 179 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qapi/block-core.json |  19 +++++-
> >  3 files changed, 197 insertions(+), 2 deletions(-)
> >  create mode 100644 block/null.c
> > 
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index 858d2b3..087e281 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -9,6 +9,7 @@ block-obj-y += snapshot.o qapi.o
> >  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
> >  block-obj-$(CONFIG_POSIX) += raw-posix.o
> >  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> > +block-obj-y += null.o
> >  
> >  ifeq ($(CONFIG_POSIX),y)
> >  block-obj-y += nbd.o nbd-client.o sheepdog.o
> > diff --git a/block/null.c b/block/null.c
> > new file mode 100644
> > index 0000000..171fd19
> > --- /dev/null
> > +++ b/block/null.c
> > @@ -0,0 +1,179 @@
> > +/*
> > + * Null block driver
> > + *
> > + * Authors:
> > + *  Fam Zheng <famz@redhat.com>
> > + *
> > + * Copyright (C) 2014 Red Hat, Inc.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "block/block_int.h"
> > +
> > +typedef struct {
> > +    int64_t length;
> > +} BDRVNullState;
> > +
> > +static QemuOptsList runtime_opts = {
> > +    .name = "null",
> > +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> > +    .desc = {
> > +        {
> > +            .name = "filename",
> > +            .type = QEMU_OPT_STRING,
> > +            .help = "",
> > +        },
> 
> You seems to define filename both here and in QMP but null_file_open don't use it neither
> any part of the code.
> 
> Do you declare it only to fool some other part of the block layer ?

Yeah, it has to absorb the "filename" option in null_file_open. The value would
be "null://" or "null-co://", and is parsed in generic open code.

> 
> > +        {
> > +            .name = BLOCK_OPT_SIZE,
> > +            .type = QEMU_OPT_SIZE,
> > +            .help = "size of the null block",
> > +        },
> > +        { /* end of list */ }
> > +    },
> > +};
> > +
> > +static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
> > +                          Error **errp)
> > +{
> > +    QemuOpts *opts;
> > +    BDRVNullState *s = bs->opaque;
> > +
> > +    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> > +    qemu_opts_absorb_qdict(opts, options, &error_abort);
> > +    s->length =
> > +        qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
> > +    qemu_opts_del(opts);
> > +    return 0;
> > +}
> > +
> > +static void null_close(BlockDriverState *bs)
> > +{
> > +}
> > +
> > +static int64_t null_getlength(BlockDriverState *bs)
> > +{
> > +    BDRVNullState *s = bs->opaque;
> > +    return s->length;
> > +}
> > +
> > +static coroutine_fn int null_co_read(BlockDriverState *bs, int64_t sector_num,
> > +                                     uint8_t *buf, int nb_sectors)
> > +{
> > +    return 0;
> > +}
> > +
> > +static coroutine_fn int null_co_write(BlockDriverState *bs, int64_t sector_num,
> > +                                      const uint8_t *buf, int nb_sectors)
> > +{
> > +    return 0;
> > +}
> > +
> > +static coroutine_fn int null_co_flush(BlockDriverState *bs)
> > +{
> > +    return 0;
> > +}
> > +
> > +typedef struct {
> > +    BlockDriverAIOCB common;
> > +    QEMUBH *bh;
> > +} NullAIOCB;
> > +
> > +static void null_aio_cancel(BlockDriverAIOCB *blockacb);
> > +
> > +static const AIOCBInfo null_aiocb_info = {
> > +    .aiocb_size = sizeof(NullAIOCB),
> > +    .cancel     = null_aio_cancel,
> > +};
> > +
> > +static void null_bh_cb(void *opaque)
> > +{
> > +    NullAIOCB *acb = opaque;
> > +    acb->common.cb(acb->common.opaque, 0);
> > +    qemu_bh_delete(acb->bh);
> > +    qemu_aio_release(acb);
> > +}
> > +
> > +static BlockDriverAIOCB *null_aio_readv(BlockDriverState *bs,
> > +                                        int64_t sector_num, QEMUIOVector *qiov,
> > +                                        int nb_sectors,
> > +                                        BlockDriverCompletionFunc *cb,
> > +                                        void *opaque)
> > +{
> > +    NullAIOCB *acb;
> > +
> > +    acb = qemu_aio_get(&null_aiocb_info, bs, cb, opaque);
> > +    acb->bh = aio_bh_new(bdrv_get_aio_context(bs), null_bh_cb, acb);
> > +    qemu_bh_schedule(acb->bh);
> > +    return &acb->common;
> > +}
> > +
> > +static BlockDriverAIOCB *null_aio_writev(BlockDriverState *bs,
> > +                                         int64_t sector_num, QEMUIOVector *qiov,
> > +                                         int nb_sectors,
> > +                                         BlockDriverCompletionFunc *cb,
> > +                                         void *opaque)
> > +{
> > +    NullAIOCB *acb;
> > +
> > +    acb = qemu_aio_get(&null_aiocb_info, bs, cb, opaque);
> > +    acb->bh = aio_bh_new(bdrv_get_aio_context(bs), null_bh_cb, acb);
> > +    qemu_bh_schedule(acb->bh);
> > +    return &acb->common;
> > +}
> > +
> > +static BlockDriverAIOCB *null_aio_flush(BlockDriverState *bs,
> > +                                        BlockDriverCompletionFunc *cb,
> > +                                        void *opaque)
> > +{
> > +    NullAIOCB *acb;
> > +
> > +    acb = qemu_aio_get(&null_aiocb_info, bs, cb, opaque);
> > +    acb->bh = aio_bh_new(bdrv_get_aio_context(bs), null_bh_cb, acb);
> > +    qemu_bh_schedule(acb->bh);
> > +    return &acb->common;
> > +}
> 
> The former three function body are strictly identical maybe you could factorize them.
> They would become thin wrapper around one common function.

Good idea!

> 
> > +
> > +static void null_aio_cancel(BlockDriverAIOCB *blockacb)
> > +{
> > +    NullAIOCB *acb = container_of(blockacb, NullAIOCB, common);
> > +    qemu_bh_delete(acb->bh);
> > +    qemu_aio_release(acb);
> > +}
> > +
> > +static BlockDriver bdrv_null = {
> > +    .format_name            = "null",
> > +    .protocol_name          = "null",
> > +    .instance_size          = sizeof(BDRVNullState),
> > +
> > +    .bdrv_file_open         = null_file_open,
> > +    .bdrv_close             = null_close,
> > +    .bdrv_getlength         = null_getlength,
> > +
> > +    .bdrv_aio_readv         = null_aio_readv,
> > +    .bdrv_aio_writev        = null_aio_writev,
> > +    .bdrv_aio_flush         = null_aio_flush,
> > +};
> > +
> > +static BlockDriver bdrv_null_co = {
> > +    .format_name            = "null-co",
> > +    .protocol_name          = "null-co",
> > +    .instance_size          = sizeof(BDRVNullState),
> > +
> > +    .bdrv_file_open         = null_file_open,
> > +    .bdrv_close             = null_close,
> > +    .bdrv_getlength         = null_getlength,
> > +
> > +    .bdrv_read              = null_co_read,
> > +    .bdrv_write             = null_co_write,
> > +    .bdrv_co_flush_to_disk  = null_co_flush,
> > +};
> > +
> > +static void bdrv_null_init(void)
> > +{
> > +    bdrv_register(&bdrv_null);
> > +    bdrv_register(&bdrv_null_co);
> > +}
> > +
> > +block_init(bdrv_null_init);
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index fb74c56..9b2c9c6 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1150,7 +1150,8 @@
> >    'data': [ 'archipelago', 'file', 'host_device', 'host_cdrom', 'host_floppy',
> >              'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
> >              'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
> > -            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
> > +            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum',
> > +            'null' ] }
> 
> Why not also adding null-co to QMP ?

Will do.

Fam

Re: [Qemu-devel] [PATCH v3] block: Introduce "null" driver

[Fam Zheng]

On Thu, 08/28 16:23, Eric Blake wrote:
> On 08/27/2014 11:53 PM, Fam Zheng wrote:
> > This is an analogue to Linux null_blk. It can be used for testing block
> > device emulation and general block layer functionalities such as
> > coroutines and throttling, where disk IO is not necessary or wanted.
> > 
> > Use null:// for AIO version, and null-co:// for coroutine version.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > ---
> 
> > +++ b/qapi/block-core.json
> > @@ -1150,7 +1150,8 @@
> >    'data': [ 'archipelago', 'file', 'host_device', 'host_cdrom', 'host_floppy',
> >              'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
> >              'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
> > -            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
> > +            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum',
> > +            'null' ] }
> 
> As mentioned elsewhere, you probably want 'null' AND 'null-co'.  Bummer
> that this list is mostly alphabetical, but that 'quorum' and 'null' are
> out of order.

I'll add null-co to the list. But this list is a bit far from alphabetical:

archipelago, ..., vvfat, blkdebug,...

> 
> >  
> >  ##
> >  # @BlockdevOptionsBase
> > @@ -1203,6 +1204,19 @@
> >    'data': { 'filename': 'str' } }
> >  
> >  ##
> > +# @BlockdevOptionsNull
> > +#
> > +# Driver specific block device options for the null backend.
> > +#
> > +# @size:    size of the device in bytes.
> 
> missing an #optional marker

Will add.

Thanks,
Fam

Re: [Qemu-devel] [PATCH v3] block: Introduce "null" driver

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Thu, 08/28 16:23, Eric Blake wrote:
>> On 08/27/2014 11:53 PM, Fam Zheng wrote:
>> > This is an analogue to Linux null_blk. It can be used for testing block
>> > device emulation and general block layer functionalities such as
>> > coroutines and throttling, where disk IO is not necessary or wanted.
>> > 
>> > Use null:// for AIO version, and null-co:// for coroutine version.
>> > 
>> > Signed-off-by: Fam Zheng <famz@redhat.com>
>> > 
>> > ---
>> 
>> > +++ b/qapi/block-core.json
>> > @@ -1150,7 +1150,8 @@
>> >    'data': [ 'archipelago', 'file', 'host_device', 'host_cdrom', 'host_floppy',
>> >              'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
>> >              'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
>> > -            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
>> > +            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum',
>> > +            'null' ] }
>> 
>> As mentioned elsewhere, you probably want 'null' AND 'null-co'.  Bummer
>> that this list is mostly alphabetical, but that 'quorum' and 'null' are
>> out of order.
>
> I'll add null-co to the list. But this list is a bit far from alphabetical:
>
> archipelago, ..., vvfat, blkdebug,...

Two sensible ways to organize lists of things: keep it sorted
alphabetically by name, or order by some ontology.  Since ontology is
neither obvious nor relevant here, I'd go for alphabetic.

Separate cleanup patch, please.

Re: [Qemu-devel] [PATCH v3] block: Introduce "null" driver

[Kevin Wolf]

Am 29.08.2014 um 02:55 hat Fam Zheng geschrieben:
> On Thu, 08/28 16:23, Eric Blake wrote:
> > On 08/27/2014 11:53 PM, Fam Zheng wrote:
> > > This is an analogue to Linux null_blk. It can be used for testing block
> > > device emulation and general block layer functionalities such as
> > > coroutines and throttling, where disk IO is not necessary or wanted.
> > > 
> > > Use null:// for AIO version, and null-co:// for coroutine version.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > 
> > > ---
> > 
> > > +++ b/qapi/block-core.json
> > > @@ -1150,7 +1150,8 @@
> > >    'data': [ 'archipelago', 'file', 'host_device', 'host_cdrom', 'host_floppy',
> > >              'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
> > >              'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
> > > -            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
> > > +            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum',
> > > +            'null' ] }
> > 
> > As mentioned elsewhere, you probably want 'null' AND 'null-co'.  Bummer
> > that this list is mostly alphabetical, but that 'quorum' and 'null' are
> > out of order.
> 
> I'll add null-co to the list. But this list is a bit far from alphabetical:
> 
> archipelago, ..., vvfat, blkdebug,...

Actually it's pretty close to alphabetical as the secondary criterion
(the primary one is that all protocols are listed first, and only then
the formats).

Kevin

Re: [Qemu-devel] [PATCH v3] block: Introduce "null" driver

[Eric Blake]

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

On 09/03/2014 05:10 AM, Kevin Wolf wrote:

>>>> @@ -1150,7 +1150,8 @@
>>>>    'data': [ 'archipelago', 'file', 'host_device', 'host_cdrom', 'host_floppy',
>>>>              'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
>>>>              'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
>>>> -            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
>>>> +            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum',
>>>> +            'null' ] }
>>>
>>> As mentioned elsewhere, you probably want 'null' AND 'null-co'.  Bummer
>>> that this list is mostly alphabetical, but that 'quorum' and 'null' are
>>> out of order.
>>
>> I'll add null-co to the list. But this list is a bit far from alphabetical:
>>
>> archipelago, ..., vvfat, blkdebug,...
> 
> Actually it's pretty close to alphabetical as the secondary criterion
> (the primary one is that all protocols are listed first, and only then
> the formats).

Then a (separate) patch that lists:

'data': [
   # protocols
   list...,
   # formats
   list...
]

might be nice :)

-- 
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 v3] block: Introduce "null" driver

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/03/2014 05:10 AM, Kevin Wolf wrote:
>
>>>>> @@ -1150,7 +1150,8 @@
>>>>>    'data': [ 'archipelago', 'file', 'host_device', 'host_cdrom', 'host_floppy',
>>>>>              'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
>>>>>              'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
>>>>> -            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
>>>>> +            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum',
>>>>> +            'null' ] }
>>>>
>>>> As mentioned elsewhere, you probably want 'null' AND 'null-co'.  Bummer
>>>> that this list is mostly alphabetical, but that 'quorum' and 'null' are
>>>> out of order.
>>>
>>> I'll add null-co to the list. But this list is a bit far from alphabetical:
>>>
>>> archipelago, ..., vvfat, blkdebug,...
>> 
>> Actually it's pretty close to alphabetical as the secondary criterion
>> (the primary one is that all protocols are listed first, and only then
>> the formats).
>
> Then a (separate) patch that lists:
>
> 'data': [
>    # protocols
>    list...,
>    # formats
>    list...
> ]
>
> might be nice :)

I doubt differentiating between protocols and formats is useful here.
Just sort it alphabetically.