[Qemu-devel] [PATCH 00/10] block/json: Add JSON protocol driver

[Qemu-devel] [PATCH 00/10] block/json: Add JSON protocol driver

[Max Reitz]

This series adds a passthrough JSON protocol block driver. Its filenames
are JSON objects prefixed by "json:". The objects are used as options
for opening another block device which will be the child of the JSON
device. Regarding this child device, the JSON driver behaves nearly the
same as raw_bsd in that it is just a passthrough driver. The only
difference is probably that the JSON driver identifies itself as a block
filter, in contrast to raw_bsd.

The purpose of this driver is that it may sometimes be desirable to
specify options for a block device where only a filename can be given,
e.g., for backing files. Using this should obviously be the exception,
but it is nice to have if actually needed.


Max Reitz (10):
  qdict: Add qdict_join()
  block/json: Add JSON protocol driver
  block/json: Add functions for cache control
  block/json: Add functions for writing zeroes etc.
  block/json: Add bdrv_co_get_block_status()
  block/json: Add ioctl etc.
  block/json: Add bdrv_get_specific_info()
  block/raw_bsd: Add bdrv_get_specific_info()
  block/qapi: Ignore filters on top for format name
  iotests: Add test for the JSON protocol

 block/Makefile.objs        |   2 +-
 block/json.c               | 236 +++++++++++++++++++++++++++++++++++++++++++++
 block/qapi.c               |  20 +++-
 block/raw_bsd.c            |   6 ++
 include/qapi/qmp/qdict.h   |   3 +
 qobject/qdict.c            |  32 ++++++
 tests/qemu-iotests/084     | 114 ++++++++++++++++++++++
 tests/qemu-iotests/084.out |  39 ++++++++
 tests/qemu-iotests/group   |   1 +
 9 files changed, 450 insertions(+), 3 deletions(-)
 create mode 100644 block/json.c
 create mode 100755 tests/qemu-iotests/084
 create mode 100644 tests/qemu-iotests/084.out

-- 
1.9.0

[Qemu-devel] [PATCH 01/10] qdict: Add qdict_join()

[Max Reitz]

This function joins two QDicts by absorbing one into the other.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qapi/qmp/qdict.h |  3 +++
 qobject/qdict.c          | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 1ddf97b..d68f4eb 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -16,6 +16,7 @@
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qlist.h"
 #include "qemu/queue.h"
+#include <stdbool.h>
 #include <stdint.h>
 
 #define QDICT_BUCKET_MAX 512
@@ -70,4 +71,6 @@ void qdict_flatten(QDict *qdict);
 void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
 void qdict_array_split(QDict *src, QList **dst);
 
+void qdict_join(QDict *dest, QDict *src, bool overwrite);
+
 #endif /* QDICT_H */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 42ec4c0..ea239f0 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -665,3 +665,35 @@ void qdict_array_split(QDict *src, QList **dst)
         qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
     }
 }
+
+/**
+ * qdict_join(): Absorb the src QDict into the dest QDict, that is, move all
+ * elements from src to dest.
+ *
+ * If an element from src has a key already present in dest, it will not be
+ * moved unless overwrite is true.
+ *
+ * If overwrite is true, the conflicting values in dest will be discarded and
+ * replaced by the corresponding values from src.
+ *
+ * Therefore, with overwrite being true, the src QDict will always be empty when
+ * this function returns. If overwrite is false, the src QDict will be empty
+ * iff there were no conflicts.
+ */
+void qdict_join(QDict *dest, QDict *src, bool overwrite)
+{
+    const QDictEntry *entry, *next;
+
+    entry = qdict_first(src);
+    while (entry) {
+        next = qdict_next(src, entry);
+
+        if (overwrite || !qdict_haskey(dest, entry->key)) {
+            qobject_incref(entry->value);
+            qdict_put_obj(dest, entry->key, entry->value);
+            qdict_del(src, entry->key);
+        }
+
+        entry = next;
+    }
+}
-- 
1.9.0

[Qemu-devel] [PATCH 02/10] block/json: Add JSON protocol driver

[Max Reitz]

Add a JSON protocol driver which allows supplying block driver options
through the filename rather than separately. Other than that, it is a
pure passthrough driver which identifies itself as a filter.

This patch implements the functions bdrv_parse_filename(),
bdrv_file_open(), bdrv_close(), bdrv_aio_readv(), bdrv_aio_writev(),
bdrv_getlength(), bdrv_refresh_limits() and bdrv_get_info().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/Makefile.objs |   2 +-
 block/json.c        | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 block/json.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index fd88c03..d4b70f4 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -5,7 +5,7 @@ block-obj-y += qed-check.o
 block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
 block-obj-$(CONFIG_QUORUM) += quorum.o
 block-obj-y += parallels.o blkdebug.o blkverify.o
-block-obj-y += snapshot.o qapi.o
+block-obj-y += snapshot.o qapi.o json.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
diff --git a/block/json.c b/block/json.c
new file mode 100644
index 0000000..6d63cf6
--- /dev/null
+++ b/block/json.c
@@ -0,0 +1,136 @@
+/*
+ * JSON filename wrapper protocol driver
+ *
+ * Copyright (c) 2014 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu-common.h"
+#include "block/block_int.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qjson.h"
+
+static void json_parse_filename(const char *filename, QDict *options,
+                                Error **errp)
+{
+    QObject *file_options_obj;
+    QDict *full_options;
+
+    if (!strstart(filename, "json:", &filename)) {
+        error_setg(errp, "Unknown protocol prefix for JSON block driver");
+        return;
+    }
+
+    file_options_obj = qobject_from_json(filename);
+    if (!file_options_obj) {
+        error_setg(errp, "Could not parse the JSON options");
+        return;
+    }
+
+    if (qobject_type(file_options_obj) != QTYPE_QDICT) {
+        qobject_decref(file_options_obj);
+        error_setg(errp, "Invalid JSON object");
+        return;
+    }
+
+    full_options = qdict_new();
+    qdict_put_obj(full_options, "x-options", file_options_obj);
+    qdict_flatten(full_options);
+
+    qdict_join(options, full_options, true);
+    assert(qdict_size(full_options) == 0);
+    QDECREF(full_options);
+}
+
+static int json_open(BlockDriverState *bs, QDict *options, int flags,
+                     Error **errp)
+{
+    int ret;
+
+    assert(bs->file == NULL);
+    ret = bdrv_open_image(&bs->file, NULL, options, "x-options", flags, false,
+                          errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return 0;
+}
+
+static void json_close(BlockDriverState *bs)
+{
+}
+
+static BlockDriverAIOCB *json_aio_readv(BlockDriverState *bs,
+                                        int64_t sector_num, QEMUIOVector *qiov,
+                                        int nb_sectors,
+                                        BlockDriverCompletionFunc *cb,
+                                        void *opaque)
+{
+    return bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
+}
+
+static BlockDriverAIOCB *json_aio_writev(BlockDriverState *bs,
+                                         int64_t sector_num, QEMUIOVector *qiov,
+                                         int nb_sectors,
+                                         BlockDriverCompletionFunc *cb,
+                                         void *opaque)
+{
+    return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
+}
+
+static int64_t json_getlength(BlockDriverState *bs)
+{
+    return bdrv_getlength(bs->file);
+}
+
+static int json_refresh_limits(BlockDriverState *bs)
+{
+    bs->bl = bs->file->bl;
+    return 0;
+}
+
+static int json_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+    return bdrv_get_info(bs->file, bdi);
+}
+
+static BlockDriver bdrv_json = {
+    .format_name                = "json",
+    .protocol_name              = "json",
+    .instance_size              = 0,
+
+    .bdrv_parse_filename        = json_parse_filename,
+    .bdrv_file_open             = json_open,
+    .bdrv_close                 = json_close,
+
+    .bdrv_aio_readv             = json_aio_readv,
+    .bdrv_aio_writev            = json_aio_writev,
+
+    .has_variable_length        = true,
+    .bdrv_getlength             = json_getlength,
+
+    .bdrv_refresh_limits        = json_refresh_limits,
+    .bdrv_get_info              = json_get_info,
+
+    .authorizations             = { true, true },
+};
+
+static void bdrv_json_init(void)
+{
+    bdrv_register(&bdrv_json);
+}
+
+block_init(bdrv_json_init);
-- 
1.9.0

[Qemu-devel] [PATCH 03/10] block/json: Add functions for cache control

[Max Reitz]

Add passthrough functions for bdrv_aio_flush() and
bdrv_invalidate_cache().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/json.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block/json.c b/block/json.c
index 6d63cf6..2f885cc 100644
--- a/block/json.c
+++ b/block/json.c
@@ -91,6 +91,18 @@ static BlockDriverAIOCB *json_aio_writev(BlockDriverState *bs,
     return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
 }
 
+static BlockDriverAIOCB *json_aio_flush(BlockDriverState *bs,
+                                        BlockDriverCompletionFunc *cb,
+                                        void *opaque)
+{
+    return bdrv_aio_flush(bs->file, cb, opaque);
+}
+
+static void json_invalidate_cache(BlockDriverState *bs)
+{
+    return bdrv_invalidate_cache(bs->file);
+}
+
 static int64_t json_getlength(BlockDriverState *bs)
 {
     return bdrv_getlength(bs->file);
@@ -118,6 +130,9 @@ static BlockDriver bdrv_json = {
 
     .bdrv_aio_readv             = json_aio_readv,
     .bdrv_aio_writev            = json_aio_writev,
+    .bdrv_aio_flush             = json_aio_flush,
+
+    .bdrv_invalidate_cache      = json_invalidate_cache,
 
     .has_variable_length        = true,
     .bdrv_getlength             = json_getlength,
-- 
1.9.0

[Qemu-devel] [PATCH 04/10] block/json: Add functions for writing zeroes etc.

[Max Reitz]

Add passthrough functions for bdrv_aio_discard(),
bdrv_co_write_zeroes(), bdrv_truncate() and bdrv_has_zero_init().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/json.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/block/json.c b/block/json.c
index 2f885cc..a2f4691 100644
--- a/block/json.c
+++ b/block/json.c
@@ -98,6 +98,21 @@ static BlockDriverAIOCB *json_aio_flush(BlockDriverState *bs,
     return bdrv_aio_flush(bs->file, cb, opaque);
 }
 
+static BlockDriverAIOCB *json_aio_discard(BlockDriverState *bs,
+                                          int64_t sector_num, int nb_sectors,
+                                          BlockDriverCompletionFunc *cb,
+                                          void *opaque)
+{
+    return bdrv_aio_discard(bs, sector_num, nb_sectors, cb, opaque);
+}
+
+static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
+                                             int64_t sector_num, int nb_sectors,
+                                             BdrvRequestFlags flags)
+{
+    return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
+}
+
 static void json_invalidate_cache(BlockDriverState *bs)
 {
     return bdrv_invalidate_cache(bs->file);
@@ -108,6 +123,16 @@ static int64_t json_getlength(BlockDriverState *bs)
     return bdrv_getlength(bs->file);
 }
 
+static int json_truncate(BlockDriverState *bs, int64_t offset)
+{
+    return bdrv_truncate(bs->file, offset);
+}
+
+static int json_has_zero_init(BlockDriverState *bs)
+{
+    return bdrv_has_zero_init(bs->file);
+}
+
 static int json_refresh_limits(BlockDriverState *bs)
 {
     bs->bl = bs->file->bl;
@@ -131,12 +156,17 @@ static BlockDriver bdrv_json = {
     .bdrv_aio_readv             = json_aio_readv,
     .bdrv_aio_writev            = json_aio_writev,
     .bdrv_aio_flush             = json_aio_flush,
+    .bdrv_aio_discard           = json_aio_discard,
+
+    .bdrv_co_write_zeroes       = json_co_write_zeroes,
 
     .bdrv_invalidate_cache      = json_invalidate_cache,
 
     .has_variable_length        = true,
     .bdrv_getlength             = json_getlength,
+    .bdrv_truncate              = json_truncate,
 
+    .bdrv_has_zero_init         = json_has_zero_init,
     .bdrv_refresh_limits        = json_refresh_limits,
     .bdrv_get_info              = json_get_info,
 
-- 
1.9.0

[Qemu-devel] [PATCH 05/10] block/json: Add bdrv_co_get_block_status()

[Max Reitz]

Implement this function in the same way as raw_bsd does: Acknowledge
that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID
and BDRV_BLOCK_DATA and derive the offset directly from the sector
index) and add BDRV_BLOCK_RAW to the returned value.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/json.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/json.c b/block/json.c
index a2f4691..7392802 100644
--- a/block/json.c
+++ b/block/json.c
@@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
     return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
 }
 
+static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs,
+                                                     int64_t sector_num,
+                                                     int nb_sectors, int *pnum)
+{
+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+           (sector_num << BDRV_SECTOR_BITS);
+}
+
 static void json_invalidate_cache(BlockDriverState *bs)
 {
     return bdrv_invalidate_cache(bs->file);
@@ -159,6 +167,7 @@ static BlockDriver bdrv_json = {
     .bdrv_aio_discard           = json_aio_discard,
 
     .bdrv_co_write_zeroes       = json_co_write_zeroes,
+    .bdrv_co_get_block_status   = json_co_get_block_status,
 
     .bdrv_invalidate_cache      = json_invalidate_cache,
 
-- 
1.9.0

[Qemu-devel] [PATCH 06/10] block/json: Add ioctl etc.

[Max Reitz]

Add passthrough functions for bdrv_aio_ioctl(), bdrv_is_inserted(),
bdrv_media_changed(), bdrv_eject(), bdrv_lock_medium() and bdrv_ioctl().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/json.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/block/json.c b/block/json.c
index 7392802..ce718e8 100644
--- a/block/json.c
+++ b/block/json.c
@@ -106,6 +106,14 @@ static BlockDriverAIOCB *json_aio_discard(BlockDriverState *bs,
     return bdrv_aio_discard(bs, sector_num, nb_sectors, cb, opaque);
 }
 
+static BlockDriverAIOCB *json_aio_ioctl(BlockDriverState *bs,
+                                        unsigned long int req, void *buf,
+                                        BlockDriverCompletionFunc *cb,
+                                        void *opaque)
+{
+    return bdrv_aio_ioctl(bs->file, req, buf, cb, opaque);
+}
+
 static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
                                              int64_t sector_num, int nb_sectors,
                                              BdrvRequestFlags flags)
@@ -121,6 +129,31 @@ static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs,
            (sector_num << BDRV_SECTOR_BITS);
 }
 
+static int json_is_inserted(BlockDriverState *bs)
+{
+    return bdrv_is_inserted(bs->file);
+}
+
+static int json_media_changed(BlockDriverState *bs)
+{
+    return bdrv_media_changed(bs->file);
+}
+
+static void json_eject(BlockDriverState *bs, bool eject_flag)
+{
+    bdrv_eject(bs->file, eject_flag);
+}
+
+static void json_lock_medium(BlockDriverState *bs, bool locked)
+{
+    bdrv_lock_medium(bs->file, locked);
+}
+
+static int json_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
+{
+    return bdrv_ioctl(bs->file, req, buf);
+}
+
 static void json_invalidate_cache(BlockDriverState *bs)
 {
     return bdrv_invalidate_cache(bs->file);
@@ -165,10 +198,17 @@ static BlockDriver bdrv_json = {
     .bdrv_aio_writev            = json_aio_writev,
     .bdrv_aio_flush             = json_aio_flush,
     .bdrv_aio_discard           = json_aio_discard,
+    .bdrv_aio_ioctl             = json_aio_ioctl,
 
     .bdrv_co_write_zeroes       = json_co_write_zeroes,
     .bdrv_co_get_block_status   = json_co_get_block_status,
 
+    .bdrv_is_inserted           = json_is_inserted,
+    .bdrv_media_changed         = json_media_changed,
+    .bdrv_eject                 = json_eject,
+    .bdrv_lock_medium           = json_lock_medium,
+    .bdrv_ioctl                 = json_ioctl,
+
     .bdrv_invalidate_cache      = json_invalidate_cache,
 
     .has_variable_length        = true,
-- 
1.9.0

[Qemu-devel] [PATCH 07/10] block/json: Add bdrv_get_specific_info()

[Max Reitz]

Add a passthrough function for bdrv_get_specific_info().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/json.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/json.c b/block/json.c
index ce718e8..6aa0544 100644
--- a/block/json.c
+++ b/block/json.c
@@ -185,6 +185,11 @@ static int json_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return bdrv_get_info(bs->file, bdi);
 }
 
+static ImageInfoSpecific *json_get_specific_info(BlockDriverState *bs)
+{
+    return bdrv_get_specific_info(bs->file);
+}
+
 static BlockDriver bdrv_json = {
     .format_name                = "json",
     .protocol_name              = "json",
@@ -218,6 +223,7 @@ static BlockDriver bdrv_json = {
     .bdrv_has_zero_init         = json_has_zero_init,
     .bdrv_refresh_limits        = json_refresh_limits,
     .bdrv_get_info              = json_get_info,
+    .bdrv_get_specific_info     = json_get_specific_info,
 
     .authorizations             = { true, true },
 };
-- 
1.9.0

[Qemu-devel] [PATCH 08/10] block/raw_bsd: Add bdrv_get_specific_info()

[Max Reitz]

Add a passthrough function for bdrv_get_specific_info().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/raw_bsd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 01ea692..e93ccd3 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -90,6 +90,11 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return bdrv_get_info(bs->file, bdi);
 }
 
+static ImageInfoSpecific *raw_get_specific_info(BlockDriverState *bs)
+{
+    return bdrv_get_specific_info(bs->file);
+}
+
 static int raw_refresh_limits(BlockDriverState *bs)
 {
     bs->bl = bs->file->bl;
@@ -187,6 +192,7 @@ static BlockDriver bdrv_raw = {
     .bdrv_getlength       = &raw_getlength,
     .has_variable_length  = true,
     .bdrv_get_info        = &raw_get_info,
+    .bdrv_get_specific_info = &raw_get_specific_info,
     .bdrv_refresh_limits  = &raw_refresh_limits,
     .bdrv_is_inserted     = &raw_is_inserted,
     .bdrv_media_changed   = &raw_media_changed,
-- 
1.9.0

[Qemu-devel] [PATCH 09/10] block/qapi: Ignore filters on top for format name

[Max Reitz]

bdrv_query_image_info() currently deduces the image filename and the
format name from the top BDS. However, it is probably more reasonable to
ignore as many filters as possible on top of the BDS chain since those
neither change the type nor the filename of the underlying image.

Filters like quorum which have multiple underlying BDS should not be
removed, however, since the underlying BDS chains may lead to different
image formats and most certainly to different filenames. Therefore, only
simple filters with a single underlying BDS may be skipped.

In addition, any "raw" BDS on top of such a simple filter should be
removed, since they have probably been automatically created by
bdrv_open() but basically function as a simple filter as well.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qapi.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 8f2b4db..c4ddced 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -171,11 +171,27 @@ void bdrv_query_image_info(BlockDriverState *bs,
     int ret;
     Error *err = NULL;
     ImageInfo *info = g_new0(ImageInfo, 1);
+    BlockDriverState *ubs;
 
     bdrv_get_geometry(bs, &total_sectors);
 
-    info->filename        = g_strdup(bs->filename);
-    info->format          = g_strdup(bdrv_get_format_name(bs));
+    /* Remove the top layer of filters; that is, remove every "raw" BDS on top
+     * of a simple filter and every simple filter itself until a BDS is
+     * encountered which cannot be removed through these rules; a simple filter
+     * is a filter BDS which has its child in .file (that is,
+     * bdrv_recurse_is_first_non_filter is NULL). */
+    ubs = bs;
+    while ((ubs->drv && ubs->drv->authorizations[BS_IS_A_FILTER]) ||
+           (ubs->drv && !strcmp(ubs->drv->format_name, "raw") && ubs->file &&
+            ubs->file->drv && ubs->file->drv->authorizations[BS_IS_A_FILTER] &&
+            !ubs->file->drv->bdrv_recurse_is_first_non_filter))
+    {
+        ubs = ubs->file;
+    }
+
+    info->filename        = g_strdup(ubs->filename);
+    info->format          = g_strdup(bdrv_get_format_name(ubs));
+
     info->virtual_size    = total_sectors * 512;
     info->actual_size     = bdrv_get_allocated_file_size(bs);
     info->has_actual_size = info->actual_size >= 0;
-- 
1.9.0

[Qemu-devel] [PATCH 10/10] iotests: Add test for the JSON protocol

[Max Reitz]

Add a test for the JSON protocol driver.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/084     | 114 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/084.out |  39 ++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 154 insertions(+)
 create mode 100755 tests/qemu-iotests/084
 create mode 100644 tests/qemu-iotests/084.out

diff --git a/tests/qemu-iotests/084 b/tests/qemu-iotests/084
new file mode 100755
index 0000000..46be241
--- /dev/null
+++ b/tests/qemu-iotests/084
@@ -0,0 +1,114 @@
+#!/bin/bash
+#
+# Test case for the JSON block protocol
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+IMG_SIZE=64M
+
+# Taken from test 072
+echo
+echo "=== Testing nested image formats (072) ==="
+echo
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $IMG_SIZE
+
+$QEMU_IO -c 'write -P 42 0 512' -c 'write -P 23 512 512' \
+         -c 'write -P 66 1024 512' "$TEST_IMG.base" | _filter_qemu_io
+
+$QEMU_IMG convert -f raw -O $IMGFMT "$TEST_IMG.base" "$TEST_IMG"
+
+$QEMU_IO -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
+         -c 'read -P 66 1024 512' "json:{
+    \"driver\": \"$IMGFMT\",
+    \"file\": {
+        \"driver\": \"$IMGFMT\",
+        \"file\": {
+            \"filename\": \"$TEST_IMG\"
+        }
+    }
+}" | _filter_qemu_io
+
+# This should fail (see test 072)
+$QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
+
+
+# Taken from test 071
+echo
+echo "=== Testing blkdebug (071) ==="
+echo
+
+_make_test_img $IMG_SIZE
+
+$QEMU_IO -c 'write -P 42 0x38000 512' "$TEST_IMG" | _filter_qemu_io
+
+# The "image.filename" part tests whether "a": { "b": "c" } and "a.b": "c" do
+# the same (which they should).
+# This has to use -g to force qemu-io to use BDRV_O_PROTOCOL, since it will try
+# to determine the format of the file otherwise; due to the complexity of the
+# filename, only raw (or json itself) will work and this will then result in an
+# error because of the blkdebug part. Thus, use -g.
+$QEMU_IO -c 'read -P 42 0x38000 512' -g "json:{
+    \"driver\": \"$IMGFMT\",
+    \"file\": {
+        \"driver\": \"blkdebug\",
+        \"inject-error\": [{
+            \"event\": \"l2_load\"
+        }],
+        \"image.filename\": \"$TEST_IMG\"
+    }
+}" | _filter_qemu_io
+
+
+echo
+echo "=== Testing qemu-img info output ==="
+echo
+
+# This should output information about the image itself, not about the JSON
+# block device.
+$QEMU_IMG info "json:{\"driver\":\"qcow2\",\"file.filename\":\"$TEST_IMG\"}" \
+    | _filter_testdir | _filter_imgfmt
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/084.out b/tests/qemu-iotests/084.out
new file mode 100644
index 0000000..d475aec
--- /dev/null
+++ b/tests/qemu-iotests/084.out
@@ -0,0 +1,39 @@
+QA output created by 084
+
+=== Testing nested image formats (072) ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 1024
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 1024
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Pattern verification failed at offset 0, 512 bytes
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing blkdebug (071) ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+wrote 512/512 bytes at offset 229376
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read failed: Input/output error
+
+=== Testing qemu-img info output ===
+
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+disk size: 324K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index db127d9..3b7d0e6 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -85,3 +85,4 @@
 079 rw auto
 081 rw auto
 082 rw auto quick
+084 rw auto quick
-- 
1.9.0

Re: [Qemu-devel] [PATCH 01/10] qdict: Add qdict_join()

[Benoît Canet]

The Monday 03 Mar 2014 à 16:28:44 (+0100), Max Reitz wrote :
> This function joins two QDicts by absorbing one into the other.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qapi/qmp/qdict.h |  3 +++
>  qobject/qdict.c          | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index 1ddf97b..d68f4eb 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -16,6 +16,7 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qapi/qmp/qlist.h"
>  #include "qemu/queue.h"
> +#include <stdbool.h>
>  #include <stdint.h>
>  
>  #define QDICT_BUCKET_MAX 512
> @@ -70,4 +71,6 @@ void qdict_flatten(QDict *qdict);
>  void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
>  void qdict_array_split(QDict *src, QList **dst);
>  
> +void qdict_join(QDict *dest, QDict *src, bool overwrite);
> +
>  #endif /* QDICT_H */
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index 42ec4c0..ea239f0 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -665,3 +665,35 @@ void qdict_array_split(QDict *src, QList **dst)
>          qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
>      }
>  }
> +
> +/**
> + * qdict_join(): Absorb the src QDict into the dest QDict, that is, move all
> + * elements from src to dest.
> + *
> + * If an element from src has a key already present in dest, it will not be
> + * moved unless overwrite is true.
> + *
> + * If overwrite is true, the conflicting values in dest will be discarded and
> + * replaced by the corresponding values from src.
> + *
> + * Therefore, with overwrite being true, the src QDict will always be empty when
> + * this function returns. If overwrite is false, the src QDict will be empty
> + * iff there were no conflicts.
s/iff/if/

> + */
> +void qdict_join(QDict *dest, QDict *src, bool overwrite)
> +{
> +    const QDictEntry *entry, *next;
> +
> +    entry = qdict_first(src);
> +    while (entry) {
> +        next = qdict_next(src, entry);
> +
> +        if (overwrite || !qdict_haskey(dest, entry->key)) {
> +            qobject_incref(entry->value);
> +            qdict_put_obj(dest, entry->key, entry->value);
> +            qdict_del(src, entry->key);
> +        }
> +
> +        entry = next;
> +    }
> +}
> -- 
> 1.9.0
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 01/10] qdict: Add qdict_join()

[Kevin Wolf]

Am 05.03.2014 um 16:55 hat Benoît Canet geschrieben:
> The Monday 03 Mar 2014 à 16:28:44 (+0100), Max Reitz wrote :
> > This function joins two QDicts by absorbing one into the other.
> > 
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  include/qapi/qmp/qdict.h |  3 +++
> >  qobject/qdict.c          | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> > 
> > diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> > index 1ddf97b..d68f4eb 100644
> > --- a/include/qapi/qmp/qdict.h
> > +++ b/include/qapi/qmp/qdict.h
> > @@ -16,6 +16,7 @@
> >  #include "qapi/qmp/qobject.h"
> >  #include "qapi/qmp/qlist.h"
> >  #include "qemu/queue.h"
> > +#include <stdbool.h>
> >  #include <stdint.h>
> >  
> >  #define QDICT_BUCKET_MAX 512
> > @@ -70,4 +71,6 @@ void qdict_flatten(QDict *qdict);
> >  void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
> >  void qdict_array_split(QDict *src, QList **dst);
> >  
> > +void qdict_join(QDict *dest, QDict *src, bool overwrite);
> > +
> >  #endif /* QDICT_H */
> > diff --git a/qobject/qdict.c b/qobject/qdict.c
> > index 42ec4c0..ea239f0 100644
> > --- a/qobject/qdict.c
> > +++ b/qobject/qdict.c
> > @@ -665,3 +665,35 @@ void qdict_array_split(QDict *src, QList **dst)
> >          qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
> >      }
> >  }
> > +
> > +/**
> > + * qdict_join(): Absorb the src QDict into the dest QDict, that is, move all
> > + * elements from src to dest.
> > + *
> > + * If an element from src has a key already present in dest, it will not be
> > + * moved unless overwrite is true.
> > + *
> > + * If overwrite is true, the conflicting values in dest will be discarded and
> > + * replaced by the corresponding values from src.
> > + *
> > + * Therefore, with overwrite being true, the src QDict will always be empty when
> > + * this function returns. If overwrite is false, the src QDict will be empty
> > + * iff there were no conflicts.
> s/iff/if/

"iff" means "if and only if" and it seems to be correct here.

Kevin

Re: [Qemu-devel] [PATCH 02/10] block/json: Add JSON protocol driver

[Benoît Canet]

The Monday 03 Mar 2014 à 16:28:45 (+0100), Max Reitz wrote :
> Add a JSON protocol driver which allows supplying block driver options
> through the filename rather than separately. Other than that, it is a
> pure passthrough driver which identifies itself as a filter.
> 
> This patch implements the functions bdrv_parse_filename(),
> bdrv_file_open(), bdrv_close(), bdrv_aio_readv(), bdrv_aio_writev(),
> bdrv_getlength(), bdrv_refresh_limits() and bdrv_get_info().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/Makefile.objs |   2 +-
>  block/json.c        | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 137 insertions(+), 1 deletion(-)
>  create mode 100644 block/json.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index fd88c03..d4b70f4 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -5,7 +5,7 @@ block-obj-y += qed-check.o
>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
>  block-obj-$(CONFIG_QUORUM) += quorum.o
>  block-obj-y += parallels.o blkdebug.o blkverify.o
> -block-obj-y += snapshot.o qapi.o
> +block-obj-y += snapshot.o qapi.o json.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
> diff --git a/block/json.c b/block/json.c
> new file mode 100644
> index 0000000..6d63cf6
> --- /dev/null
> +++ b/block/json.c
> @@ -0,0 +1,136 @@
> +/*
> + * JSON filename wrapper protocol driver
> + *
> + * Copyright (c) 2014 Red Hat Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu-common.h"
> +#include "block/block_int.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qjson.h"
> +
> +static void json_parse_filename(const char *filename, QDict *options,
> +                                Error **errp)
> +{
> +    QObject *file_options_obj;
> +    QDict *full_options;
> +
> +    if (!strstart(filename, "json:", &filename)) {
> +        error_setg(errp, "Unknown protocol prefix for JSON block driver");
> +        return;
> +    }
> +
> +    file_options_obj = qobject_from_json(filename);
> +    if (!file_options_obj) {
> +        error_setg(errp, "Could not parse the JSON options");
> +        return;
> +    }
> +
> +    if (qobject_type(file_options_obj) != QTYPE_QDICT) {
> +        qobject_decref(file_options_obj);
> +        error_setg(errp, "Invalid JSON object");
> +        return;
> +    }
> +
> +    full_options = qdict_new();
> +    qdict_put_obj(full_options, "x-options", file_options_obj);
> +    qdict_flatten(full_options);
> +
> +    qdict_join(options, full_options, true);
> +    assert(qdict_size(full_options) == 0);
> +    QDECREF(full_options);
> +}
> +
> +static int json_open(BlockDriverState *bs, QDict *options, int flags,
> +                     Error **errp)
> +{
> +    int ret;
> +
> +    assert(bs->file == NULL);
> +    ret = bdrv_open_image(&bs->file, NULL, options, "x-options", flags, false,
> +                          errp);


> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return 0;
Why not ?
    return ret;
}

Do you plan to enrich this code path later ?


> +}
> +
> +static void json_close(BlockDriverState *bs)
> +{
> +}
> +
> +static BlockDriverAIOCB *json_aio_readv(BlockDriverState *bs,
> +                                        int64_t sector_num, QEMUIOVector *qiov,
> +                                        int nb_sectors,
> +                                        BlockDriverCompletionFunc *cb,
> +                                        void *opaque)
> +{
> +    return bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
> +}
> +
> +static BlockDriverAIOCB *json_aio_writev(BlockDriverState *bs,
> +                                         int64_t sector_num, QEMUIOVector *qiov,
> +                                         int nb_sectors,
> +                                         BlockDriverCompletionFunc *cb,
> +                                         void *opaque)
> +{
> +    return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
> +}
> +
> +static int64_t json_getlength(BlockDriverState *bs)
> +{
> +    return bdrv_getlength(bs->file);
> +}
> +
> +static int json_refresh_limits(BlockDriverState *bs)
> +{
> +    bs->bl = bs->file->bl;
> +    return 0;
> +}
> +
> +static int json_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> +{
> +    return bdrv_get_info(bs->file, bdi);
> +}
> +
> +static BlockDriver bdrv_json = {
> +    .format_name                = "json",
> +    .protocol_name              = "json",
> +    .instance_size              = 0,
> +
> +    .bdrv_parse_filename        = json_parse_filename,
> +    .bdrv_file_open             = json_open,
> +    .bdrv_close                 = json_close,
> +
> +    .bdrv_aio_readv             = json_aio_readv,
> +    .bdrv_aio_writev            = json_aio_writev,
> +
> +    .has_variable_length        = true,
> +    .bdrv_getlength             = json_getlength,
> +
> +    .bdrv_refresh_limits        = json_refresh_limits,
> +    .bdrv_get_info              = json_get_info,
> +
> +    .authorizations             = { true, true },

Paolo make rewrite the snapshot authorization code.
In the new version you need
    .is_filter = true;
    .bdrv_recurse_is_first_children = your_implementation;

I don't know when these patchset will be reviewed and merged so maybe this
version is correct.

> +};
> +
> +static void bdrv_json_init(void)
> +{
> +    bdrv_register(&bdrv_json);
> +}
> +
> +block_init(bdrv_json_init);
> -- 
> 1.9.0
> 
> 

Re: [Qemu-devel] [PATCH 01/10] qdict: Add qdict_join()

[Benoît Canet]

The Wednesday 05 Mar 2014 à 17:01:43 (+0100), Kevin Wolf wrote :
> Am 05.03.2014 um 16:55 hat Benoît Canet geschrieben:
> > The Monday 03 Mar 2014 à 16:28:44 (+0100), Max Reitz wrote :
> > > This function joins two QDicts by absorbing one into the other.
> > > 
> > > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > > ---
> > >  include/qapi/qmp/qdict.h |  3 +++
> > >  qobject/qdict.c          | 32 ++++++++++++++++++++++++++++++++
> > >  2 files changed, 35 insertions(+)
> > > 
> > > diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> > > index 1ddf97b..d68f4eb 100644
> > > --- a/include/qapi/qmp/qdict.h
> > > +++ b/include/qapi/qmp/qdict.h
> > > @@ -16,6 +16,7 @@
> > >  #include "qapi/qmp/qobject.h"
> > >  #include "qapi/qmp/qlist.h"
> > >  #include "qemu/queue.h"
> > > +#include <stdbool.h>
> > >  #include <stdint.h>
> > >  
> > >  #define QDICT_BUCKET_MAX 512
> > > @@ -70,4 +71,6 @@ void qdict_flatten(QDict *qdict);
> > >  void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
> > >  void qdict_array_split(QDict *src, QList **dst);
> > >  
> > > +void qdict_join(QDict *dest, QDict *src, bool overwrite);
> > > +
> > >  #endif /* QDICT_H */
> > > diff --git a/qobject/qdict.c b/qobject/qdict.c
> > > index 42ec4c0..ea239f0 100644
> > > --- a/qobject/qdict.c
> > > +++ b/qobject/qdict.c
> > > @@ -665,3 +665,35 @@ void qdict_array_split(QDict *src, QList **dst)
> > >          qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
> > >      }
> > >  }
> > > +
> > > +/**
> > > + * qdict_join(): Absorb the src QDict into the dest QDict, that is, move all
> > > + * elements from src to dest.
> > > + *
> > > + * If an element from src has a key already present in dest, it will not be
> > > + * moved unless overwrite is true.
> > > + *
> > > + * If overwrite is true, the conflicting values in dest will be discarded and
> > > + * replaced by the corresponding values from src.
> > > + *
> > > + * Therefore, with overwrite being true, the src QDict will always be empty when
> > > + * this function returns. If overwrite is false, the src QDict will be empty
> > > + * iff there were no conflicts.
> > s/iff/if/
> 
> "iff" means "if and only if" and it seems to be correct here.

Thanks I didn't knew that I though it was a typo :(

Best regards

Benoît

> 
> Kevin
> 

Re: [Qemu-devel] [PATCH 03/10] block/json: Add functions for cache control

[Benoît Canet]

The Monday 03 Mar 2014 à 16:28:46 (+0100), Max Reitz wrote :
> Add passthrough functions for bdrv_aio_flush() and
> bdrv_invalidate_cache().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/json.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/block/json.c b/block/json.c
> index 6d63cf6..2f885cc 100644
> --- a/block/json.c
> +++ b/block/json.c
> @@ -91,6 +91,18 @@ static BlockDriverAIOCB *json_aio_writev(BlockDriverState *bs,
>      return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
>  }
>  
> +static BlockDriverAIOCB *json_aio_flush(BlockDriverState *bs,
> +                                        BlockDriverCompletionFunc *cb,
> +                                        void *opaque)
> +{
> +    return bdrv_aio_flush(bs->file, cb, opaque);
> +}
> +
> +static void json_invalidate_cache(BlockDriverState *bs)
> +{
> +    return bdrv_invalidate_cache(bs->file);
> +}
> +
>  static int64_t json_getlength(BlockDriverState *bs)
>  {
>      return bdrv_getlength(bs->file);
> @@ -118,6 +130,9 @@ static BlockDriver bdrv_json = {
>  
>      .bdrv_aio_readv             = json_aio_readv,
>      .bdrv_aio_writev            = json_aio_writev,
> +    .bdrv_aio_flush             = json_aio_flush,
> +
> +    .bdrv_invalidate_cache      = json_invalidate_cache,
>  
>      .has_variable_length        = true,
>      .bdrv_getlength             = json_getlength,
> -- 
> 1.9.0
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 04/10] block/json: Add functions for writing zeroes etc.

[Benoît Canet]

The Monday 03 Mar 2014 à 16:28:47 (+0100), Max Reitz wrote :
> Add passthrough functions for bdrv_aio_discard(),
> bdrv_co_write_zeroes(), bdrv_truncate() and bdrv_has_zero_init().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/json.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/block/json.c b/block/json.c
> index 2f885cc..a2f4691 100644
> --- a/block/json.c
> +++ b/block/json.c
> @@ -98,6 +98,21 @@ static BlockDriverAIOCB *json_aio_flush(BlockDriverState *bs,
>      return bdrv_aio_flush(bs->file, cb, opaque);
>  }
>  
> +static BlockDriverAIOCB *json_aio_discard(BlockDriverState *bs,
> +                                          int64_t sector_num, int nb_sectors,
> +                                          BlockDriverCompletionFunc *cb,
> +                                          void *opaque)
> +{
> +    return bdrv_aio_discard(bs, sector_num, nb_sectors, cb, opaque);

Isn't it bs->file ?

> +}
> +
> +static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
> +                                             int64_t sector_num, int nb_sectors,
> +                                             BdrvRequestFlags flags)
> +{
> +    return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
same

> +}
> +
>  static void json_invalidate_cache(BlockDriverState *bs)
>  {
>      return bdrv_invalidate_cache(bs->file);
> @@ -108,6 +123,16 @@ static int64_t json_getlength(BlockDriverState *bs)
>      return bdrv_getlength(bs->file);
>  }
>  
> +static int json_truncate(BlockDriverState *bs, int64_t offset)
> +{
> +    return bdrv_truncate(bs->file, offset);
> +}
> +
> +static int json_has_zero_init(BlockDriverState *bs)
> +{
> +    return bdrv_has_zero_init(bs->file);
> +}
> +
>  static int json_refresh_limits(BlockDriverState *bs)
>  {
>      bs->bl = bs->file->bl;
> @@ -131,12 +156,17 @@ static BlockDriver bdrv_json = {
>      .bdrv_aio_readv             = json_aio_readv,
>      .bdrv_aio_writev            = json_aio_writev,
>      .bdrv_aio_flush             = json_aio_flush,
> +    .bdrv_aio_discard           = json_aio_discard,
> +
> +    .bdrv_co_write_zeroes       = json_co_write_zeroes,
>  
>      .bdrv_invalidate_cache      = json_invalidate_cache,
>  
>      .has_variable_length        = true,
>      .bdrv_getlength             = json_getlength,
> +    .bdrv_truncate              = json_truncate,
>  
> +    .bdrv_has_zero_init         = json_has_zero_init,
>      .bdrv_refresh_limits        = json_refresh_limits,
>      .bdrv_get_info              = json_get_info,
>  
> -- 
> 1.9.0
> 
> 

Re: [Qemu-devel] [PATCH 05/10] block/json: Add bdrv_co_get_block_status()

[Benoît Canet]

The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote :
> Implement this function in the same way as raw_bsd does: Acknowledge
> that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID
> and BDRV_BLOCK_DATA and derive the offset directly from the sector
> index) and add BDRV_BLOCK_RAW to the returned value.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/json.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/block/json.c b/block/json.c
> index a2f4691..7392802 100644
> --- a/block/json.c
> +++ b/block/json.c
> @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
>      return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
>  }
>  
> +static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs,
> +                                                     int64_t sector_num,
> +                                                     int nb_sectors, int *pnum)
> +{
> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> +           (sector_num << BDRV_SECTOR_BITS);
> +}

I don't understand what is the selling point of this method instead of calling
bdrv_co_get_block_status on bs->file.
Some information risk to be lost and it does look like magic.

Best regards

Benoît

> +
>  static void json_invalidate_cache(BlockDriverState *bs)
>  {
>      return bdrv_invalidate_cache(bs->file);
> @@ -159,6 +167,7 @@ static BlockDriver bdrv_json = {
>      .bdrv_aio_discard           = json_aio_discard,
>  
>      .bdrv_co_write_zeroes       = json_co_write_zeroes,
> +    .bdrv_co_get_block_status   = json_co_get_block_status,
>  
>      .bdrv_invalidate_cache      = json_invalidate_cache,
>  
> -- 
> 1.9.0
> 
> 

Re: [Qemu-devel] [PATCH 06/10] block/json: Add ioctl etc.

[Benoît Canet]

The Monday 03 Mar 2014 à 16:28:49 (+0100), Max Reitz wrote :
> Add passthrough functions for bdrv_aio_ioctl(), bdrv_is_inserted(),
> bdrv_media_changed(), bdrv_eject(), bdrv_lock_medium() and bdrv_ioctl().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/json.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/block/json.c b/block/json.c
> index 7392802..ce718e8 100644
> --- a/block/json.c
> +++ b/block/json.c
> @@ -106,6 +106,14 @@ static BlockDriverAIOCB *json_aio_discard(BlockDriverState *bs,
>      return bdrv_aio_discard(bs, sector_num, nb_sectors, cb, opaque);
>  }
>  
> +static BlockDriverAIOCB *json_aio_ioctl(BlockDriverState *bs,
> +                                        unsigned long int req, void *buf,
> +                                        BlockDriverCompletionFunc *cb,
> +                                        void *opaque)
> +{
> +    return bdrv_aio_ioctl(bs->file, req, buf, cb, opaque);
> +}
> +
>  static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
>                                               int64_t sector_num, int nb_sectors,
>                                               BdrvRequestFlags flags)
> @@ -121,6 +129,31 @@ static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs,
>             (sector_num << BDRV_SECTOR_BITS);
>  }
>  
> +static int json_is_inserted(BlockDriverState *bs)
> +{
> +    return bdrv_is_inserted(bs->file);
> +}
> +
> +static int json_media_changed(BlockDriverState *bs)
> +{
> +    return bdrv_media_changed(bs->file);
> +}
> +
> +static void json_eject(BlockDriverState *bs, bool eject_flag)
> +{
> +    bdrv_eject(bs->file, eject_flag);
> +}
> +
> +static void json_lock_medium(BlockDriverState *bs, bool locked)
> +{
> +    bdrv_lock_medium(bs->file, locked);
> +}
> +
> +static int json_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
> +{
> +    return bdrv_ioctl(bs->file, req, buf);
> +}
> +
>  static void json_invalidate_cache(BlockDriverState *bs)
>  {
>      return bdrv_invalidate_cache(bs->file);
> @@ -165,10 +198,17 @@ static BlockDriver bdrv_json = {
>      .bdrv_aio_writev            = json_aio_writev,
>      .bdrv_aio_flush             = json_aio_flush,
>      .bdrv_aio_discard           = json_aio_discard,
> +    .bdrv_aio_ioctl             = json_aio_ioctl,
>  
>      .bdrv_co_write_zeroes       = json_co_write_zeroes,
>      .bdrv_co_get_block_status   = json_co_get_block_status,
>  
> +    .bdrv_is_inserted           = json_is_inserted,
> +    .bdrv_media_changed         = json_media_changed,
> +    .bdrv_eject                 = json_eject,
> +    .bdrv_lock_medium           = json_lock_medium,
> +    .bdrv_ioctl                 = json_ioctl,
> +
>      .bdrv_invalidate_cache      = json_invalidate_cache,
>  
>      .has_variable_length        = true,
> -- 
> 1.9.0
> 
> 

Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 07/10] block/json: Add bdrv_get_specific_info()

[Benoît Canet]

The Monday 03 Mar 2014 à 16:28:50 (+0100), Max Reitz wrote :
> Add a passthrough function for bdrv_get_specific_info().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/json.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/json.c b/block/json.c
> index ce718e8..6aa0544 100644
> --- a/block/json.c
> +++ b/block/json.c
> @@ -185,6 +185,11 @@ static int json_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>      return bdrv_get_info(bs->file, bdi);
>  }
>  
> +static ImageInfoSpecific *json_get_specific_info(BlockDriverState *bs)
> +{
> +    return bdrv_get_specific_info(bs->file);
> +}
> +
>  static BlockDriver bdrv_json = {
>      .format_name                = "json",
>      .protocol_name              = "json",
> @@ -218,6 +223,7 @@ static BlockDriver bdrv_json = {
>      .bdrv_has_zero_init         = json_has_zero_init,
>      .bdrv_refresh_limits        = json_refresh_limits,
>      .bdrv_get_info              = json_get_info,
> +    .bdrv_get_specific_info     = json_get_specific_info,
>  
>      .authorizations             = { true, true },
>  };
> -- 
> 1.9.0
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 08/10] block/raw_bsd: Add bdrv_get_specific_info()

[Benoît Canet]

The Monday 03 Mar 2014 à 16:28:51 (+0100), Max Reitz wrote :
> Add a passthrough function for bdrv_get_specific_info().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/raw_bsd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 01ea692..e93ccd3 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -90,6 +90,11 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>      return bdrv_get_info(bs->file, bdi);
>  }
>  
> +static ImageInfoSpecific *raw_get_specific_info(BlockDriverState *bs)
> +{
> +    return bdrv_get_specific_info(bs->file);
> +}
> +
>  static int raw_refresh_limits(BlockDriverState *bs)
>  {
>      bs->bl = bs->file->bl;
> @@ -187,6 +192,7 @@ static BlockDriver bdrv_raw = {
>      .bdrv_getlength       = &raw_getlength,
>      .has_variable_length  = true,
>      .bdrv_get_info        = &raw_get_info,
> +    .bdrv_get_specific_info = &raw_get_specific_info,
>      .bdrv_refresh_limits  = &raw_refresh_limits,
>      .bdrv_is_inserted     = &raw_is_inserted,
>      .bdrv_media_changed   = &raw_media_changed,
> -- 
> 1.9.0
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 09/10] block/qapi: Ignore filters on top for format name

[Benoît Canet]

The Monday 03 Mar 2014 à 16:28:52 (+0100), Max Reitz wrote :
> bdrv_query_image_info() currently deduces the image filename and the
> format name from the top BDS. However, it is probably more reasonable to
> ignore as many filters as possible on top of the BDS chain since those
> neither change the type nor the filename of the underlying image.
> 
> Filters like quorum which have multiple underlying BDS should not be
> removed, however, since the underlying BDS chains may lead to different
> image formats and most certainly to different filenames. Therefore, only
> simple filters with a single underlying BDS may be skipped.
> 
> In addition, any "raw" BDS on top of such a simple filter should be
> removed, since they have probably been automatically created by
> bdrv_open() but basically function as a simple filter as well.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qapi.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 8f2b4db..c4ddced 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -171,11 +171,27 @@ void bdrv_query_image_info(BlockDriverState *bs,
>      int ret;
>      Error *err = NULL;
>      ImageInfo *info = g_new0(ImageInfo, 1);
> +    BlockDriverState *ubs;
>  
>      bdrv_get_geometry(bs, &total_sectors);
>  
> -    info->filename        = g_strdup(bs->filename);
> -    info->format          = g_strdup(bdrv_get_format_name(bs));
> +    /* Remove the top layer of filters; that is, remove every "raw" BDS on top
> +     * of a simple filter and every simple filter itself until a BDS is
> +     * encountered which cannot be removed through these rules; a simple filter
> +     * is a filter BDS which has its child in .file (that is,
> +     * bdrv_recurse_is_first_non_filter is NULL). */
> +    ubs = bs;
> +    while ((ubs->drv && ubs->drv->authorizations[BS_IS_A_FILTER]) ||
> +           (ubs->drv && !strcmp(ubs->drv->format_name, "raw") && ubs->file &&
> +            ubs->file->drv && ubs->file->drv->authorizations[BS_IS_A_FILTER] &&
> +            !ubs->file->drv->bdrv_recurse_is_first_non_filter))
> +    {
> +        ubs = ubs->file;
> +    }

The rewrite Paolo asked me to do don't allow to do it anymore: there is no
distinction between simple filter driver and quorum like filter drivers.

Maybe this rewrite was not so nice.

Best regards

Benoît

> +
> +    info->filename        = g_strdup(ubs->filename);
> +    info->format          = g_strdup(bdrv_get_format_name(ubs));
> +
>      info->virtual_size    = total_sectors * 512;
>      info->actual_size     = bdrv_get_allocated_file_size(bs);
>      info->has_actual_size = info->actual_size >= 0;
> -- 
> 1.9.0
> 
> 

Re: [Qemu-devel] [PATCH 00/10] block/json: Add JSON protocol driver

[Eric Blake]

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

On 03/03/2014 08:28 AM, Max Reitz wrote:
> This series adds a passthrough JSON protocol block driver. Its filenames
> are JSON objects prefixed by "json:". The objects are used as options
> for opening another block device which will be the child of the JSON
> device. Regarding this child device, the JSON driver behaves nearly the
> same as raw_bsd in that it is just a passthrough driver. The only
> difference is probably that the JSON driver identifies itself as a block
> filter, in contrast to raw_bsd.
> 
> The purpose of this driver is that it may sometimes be desirable to
> specify options for a block device where only a filename can be given,
> e.g., for backing files. Using this should obviously be the exception,
> but it is nice to have if actually needed.

I like the idea!  It will be more work for libvirt to actually
understand how a json: protocol is actually interpreted if it encounters
a qcow2 file with a json: backing file, but it does open up
possibilities that are hard to encode in any other sane way.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH 01/10] qdict: Add qdict_join()

[Eric Blake]

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

On 03/03/2014 08:28 AM, Max Reitz wrote:
> This function joins two QDicts by absorbing one into the other.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qapi/qmp/qdict.h |  3 +++
>  qobject/qdict.c          | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 

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

However, I think you should expand the testsuite to cover this addition
(can be a followup)

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH 10/10] iotests: Add test for the JSON protocol

[Eric Blake]

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

On 03/03/2014 08:28 AM, Max Reitz wrote:
> Add a test for the JSON protocol driver.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/084     | 114 +++++++++++++++++++++++++++++++++++++++++++++

> +
> +# Taken from test 072

The comment is okay, but...

> +echo
> +echo "=== Testing nested image formats (072) ==="

...maybe this echo should be updated to mention test 084.

> +$QEMU_IO -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
> +         -c 'read -P 66 1024 512' "json:{
> +    \"driver\": \"$IMGFMT\",
> +    \"file\": {
> +        \"driver\": \"$IMGFMT\",
> +        \"file\": {
> +            \"filename\": \"$TEST_IMG\"

Are we guaranteed that $TEST_IMG will not contain any " which would
render this invalid JSON?

> +
> +# Taken from test 071
> +echo
> +echo "=== Testing blkdebug (071) ==="

Hmm - now you're mentioning yet another test id different than 084.  So
I guess this was just a hint that you are reproducing earlier tests but
now with the context of a json: protocol.  Still, doesn't "Testing
blkdebug" convey sufficient information, without also needing "(071)"
for confusion?  At any rate, I don't think this affects the coverage of
the test.

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: 604 bytes --]

Re: [Qemu-devel] [PATCH 02/10] block/json: Add JSON protocol driver

[Max Reitz]

On 05.03.2014 17:04, Benoît Canet wrote:
> The Monday 03 Mar 2014 à 16:28:45 (+0100), Max Reitz wrote :
>> Add a JSON protocol driver which allows supplying block driver options
>> through the filename rather than separately. Other than that, it is a
>> pure passthrough driver which identifies itself as a filter.
>>
>> This patch implements the functions bdrv_parse_filename(),
>> bdrv_file_open(), bdrv_close(), bdrv_aio_readv(), bdrv_aio_writev(),
>> bdrv_getlength(), bdrv_refresh_limits() and bdrv_get_info().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/Makefile.objs |   2 +-
>>   block/json.c        | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 137 insertions(+), 1 deletion(-)
>>   create mode 100644 block/json.c
>>
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index fd88c03..d4b70f4 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -5,7 +5,7 @@ block-obj-y += qed-check.o
>>   block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
>>   block-obj-$(CONFIG_QUORUM) += quorum.o
>>   block-obj-y += parallels.o blkdebug.o blkverify.o
>> -block-obj-y += snapshot.o qapi.o
>> +block-obj-y += snapshot.o qapi.o json.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
>> diff --git a/block/json.c b/block/json.c
>> new file mode 100644
>> index 0000000..6d63cf6
>> --- /dev/null
>> +++ b/block/json.c
>> @@ -0,0 +1,136 @@
>> +/*
>> + * JSON filename wrapper protocol driver
>> + *
>> + * Copyright (c) 2014 Red Hat Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "qemu-common.h"
>> +#include "block/block_int.h"
>> +#include "qapi/qmp/qdict.h"
>> +#include "qapi/qmp/qjson.h"
>> +
>> +static void json_parse_filename(const char *filename, QDict *options,
>> +                                Error **errp)
>> +{
>> +    QObject *file_options_obj;
>> +    QDict *full_options;
>> +
>> +    if (!strstart(filename, "json:", &filename)) {
>> +        error_setg(errp, "Unknown protocol prefix for JSON block driver");
>> +        return;
>> +    }
>> +
>> +    file_options_obj = qobject_from_json(filename);
>> +    if (!file_options_obj) {
>> +        error_setg(errp, "Could not parse the JSON options");
>> +        return;
>> +    }
>> +
>> +    if (qobject_type(file_options_obj) != QTYPE_QDICT) {
>> +        qobject_decref(file_options_obj);
>> +        error_setg(errp, "Invalid JSON object");
>> +        return;
>> +    }
>> +
>> +    full_options = qdict_new();
>> +    qdict_put_obj(full_options, "x-options", file_options_obj);
>> +    qdict_flatten(full_options);
>> +
>> +    qdict_join(options, full_options, true);
>> +    assert(qdict_size(full_options) == 0);
>> +    QDECREF(full_options);
>> +}
>> +
>> +static int json_open(BlockDriverState *bs, QDict *options, int flags,
>> +                     Error **errp)
>> +{
>> +    int ret;
>> +
>> +    assert(bs->file == NULL);
>> +    ret = bdrv_open_image(&bs->file, NULL, options, "x-options", flags, false,
>> +                          errp);
>
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    return 0;
> Why not ?
>      return ret;
> }
>
> Do you plan to enrich this code path later ?

You're right, that'd be shorter. It's just that I'm so in the habit of 
writing "ret = foo(); if (ret < 0) { return ret; / goto fail; }" that I 
didn't notice. ;-)

>> +}
>> +
>> +static void json_close(BlockDriverState *bs)
>> +{
>> +}
>> +
>> +static BlockDriverAIOCB *json_aio_readv(BlockDriverState *bs,
>> +                                        int64_t sector_num, QEMUIOVector *qiov,
>> +                                        int nb_sectors,
>> +                                        BlockDriverCompletionFunc *cb,
>> +                                        void *opaque)
>> +{
>> +    return bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
>> +}
>> +
>> +static BlockDriverAIOCB *json_aio_writev(BlockDriverState *bs,
>> +                                         int64_t sector_num, QEMUIOVector *qiov,
>> +                                         int nb_sectors,
>> +                                         BlockDriverCompletionFunc *cb,
>> +                                         void *opaque)
>> +{
>> +    return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
>> +}
>> +
>> +static int64_t json_getlength(BlockDriverState *bs)
>> +{
>> +    return bdrv_getlength(bs->file);
>> +}
>> +
>> +static int json_refresh_limits(BlockDriverState *bs)
>> +{
>> +    bs->bl = bs->file->bl;
>> +    return 0;
>> +}
>> +
>> +static int json_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>> +{
>> +    return bdrv_get_info(bs->file, bdi);
>> +}
>> +
>> +static BlockDriver bdrv_json = {
>> +    .format_name                = "json",
>> +    .protocol_name              = "json",
>> +    .instance_size              = 0,
>> +
>> +    .bdrv_parse_filename        = json_parse_filename,
>> +    .bdrv_file_open             = json_open,
>> +    .bdrv_close                 = json_close,
>> +
>> +    .bdrv_aio_readv             = json_aio_readv,
>> +    .bdrv_aio_writev            = json_aio_writev,
>> +
>> +    .has_variable_length        = true,
>> +    .bdrv_getlength             = json_getlength,
>> +
>> +    .bdrv_refresh_limits        = json_refresh_limits,
>> +    .bdrv_get_info              = json_get_info,
>> +
>> +    .authorizations             = { true, true },
> Paolo make rewrite the snapshot authorization code.
> In the new version you need
>      .is_filter = true;
>      .bdrv_recurse_is_first_children = your_implementation;
>
> I don't know when these patchset will be reviewed and merged so maybe this
> version is correct.

I'll just rebase on top of Paolo's series then.


Thanks,

Max

>> +};
>> +
>> +static void bdrv_json_init(void)
>> +{
>> +    bdrv_register(&bdrv_json);
>> +}
>> +
>> +block_init(bdrv_json_init);
>> -- 
>> 1.9.0
>>
>>

Re: [Qemu-devel] [PATCH 04/10] block/json: Add functions for writing zeroes etc.

[Max Reitz]

On 05.03.2014 17:09, Benoît Canet wrote:
> The Monday 03 Mar 2014 à 16:28:47 (+0100), Max Reitz wrote :
>> Add passthrough functions for bdrv_aio_discard(),
>> bdrv_co_write_zeroes(), bdrv_truncate() and bdrv_has_zero_init().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/json.c | 30 ++++++++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/block/json.c b/block/json.c
>> index 2f885cc..a2f4691 100644
>> --- a/block/json.c
>> +++ b/block/json.c
>> @@ -98,6 +98,21 @@ static BlockDriverAIOCB *json_aio_flush(BlockDriverState *bs,
>>       return bdrv_aio_flush(bs->file, cb, opaque);
>>   }
>>   
>> +static BlockDriverAIOCB *json_aio_discard(BlockDriverState *bs,
>> +                                          int64_t sector_num, int nb_sectors,
>> +                                          BlockDriverCompletionFunc *cb,
>> +                                          void *opaque)
>> +{
>> +    return bdrv_aio_discard(bs, sector_num, nb_sectors, cb, opaque);
> Isn't it bs->file ?

Oh, you're right, I'll fix it.

Max

>
>> +}
>> +
>> +static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
>> +                                             int64_t sector_num, int nb_sectors,
>> +                                             BdrvRequestFlags flags)
>> +{
>> +    return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
> same
>
>> +}
>> +
>>   static void json_invalidate_cache(BlockDriverState *bs)
>>   {
>>       return bdrv_invalidate_cache(bs->file);
>> @@ -108,6 +123,16 @@ static int64_t json_getlength(BlockDriverState *bs)
>>       return bdrv_getlength(bs->file);
>>   }
>>   
>> +static int json_truncate(BlockDriverState *bs, int64_t offset)
>> +{
>> +    return bdrv_truncate(bs->file, offset);
>> +}
>> +
>> +static int json_has_zero_init(BlockDriverState *bs)
>> +{
>> +    return bdrv_has_zero_init(bs->file);
>> +}
>> +
>>   static int json_refresh_limits(BlockDriverState *bs)
>>   {
>>       bs->bl = bs->file->bl;
>> @@ -131,12 +156,17 @@ static BlockDriver bdrv_json = {
>>       .bdrv_aio_readv             = json_aio_readv,
>>       .bdrv_aio_writev            = json_aio_writev,
>>       .bdrv_aio_flush             = json_aio_flush,
>> +    .bdrv_aio_discard           = json_aio_discard,
>> +
>> +    .bdrv_co_write_zeroes       = json_co_write_zeroes,
>>   
>>       .bdrv_invalidate_cache      = json_invalidate_cache,
>>   
>>       .has_variable_length        = true,
>>       .bdrv_getlength             = json_getlength,
>> +    .bdrv_truncate              = json_truncate,
>>   
>> +    .bdrv_has_zero_init         = json_has_zero_init,
>>       .bdrv_refresh_limits        = json_refresh_limits,
>>       .bdrv_get_info              = json_get_info,
>>   
>> -- 
>> 1.9.0
>>
>>

Re: [Qemu-devel] [PATCH 05/10] block/json: Add bdrv_co_get_block_status()

[Max Reitz]

On 05.03.2014 17:11, Benoît Canet wrote:
> The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote :
>> Implement this function in the same way as raw_bsd does: Acknowledge
>> that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID
>> and BDRV_BLOCK_DATA and derive the offset directly from the sector
>> index) and add BDRV_BLOCK_RAW to the returned value.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/json.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/block/json.c b/block/json.c
>> index a2f4691..7392802 100644
>> --- a/block/json.c
>> +++ b/block/json.c
>> @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
>>       return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
>>   }
>>   
>> +static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs,
>> +                                                     int64_t sector_num,
>> +                                                     int nb_sectors, int *pnum)
>> +{
>> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
>> +           (sector_num << BDRV_SECTOR_BITS);
>> +}
> I don't understand what is the selling point of this method instead of calling
> bdrv_co_get_block_status on bs->file.
> Some information risk to be lost and it does look like magic.

This is the same what "raw" does. It just is more meaningful: This way, 
this function does not pretend to provide the blocks itself but instead 
tells the truth; that is, the blocks are provided by an underlying BDS 
(bs->file).

I wasn't really sure what to do myself. Generally, this driver is 
actually meant to pretend that it provides the blocks itself. On the 
other hand, I tried to imitate the behavior or "raw", since this is 
something I can hope to be approximately correct. Also, as I've said 
before, the value returned here is in fact at least technically correct.


Max

> Best regards
>
> Benoît
>
>> +
>>   static void json_invalidate_cache(BlockDriverState *bs)
>>   {
>>       return bdrv_invalidate_cache(bs->file);
>> @@ -159,6 +167,7 @@ static BlockDriver bdrv_json = {
>>       .bdrv_aio_discard           = json_aio_discard,
>>   
>>       .bdrv_co_write_zeroes       = json_co_write_zeroes,
>> +    .bdrv_co_get_block_status   = json_co_get_block_status,
>>   
>>       .bdrv_invalidate_cache      = json_invalidate_cache,
>>   
>> -- 
>> 1.9.0
>>
>>

Re: [Qemu-devel] [PATCH 09/10] block/qapi: Ignore filters on top for format name

[Max Reitz]

On 05.03.2014 17:20, Benoît Canet wrote:
> The Monday 03 Mar 2014 à 16:28:52 (+0100), Max Reitz wrote :
>> bdrv_query_image_info() currently deduces the image filename and the
>> format name from the top BDS. However, it is probably more reasonable to
>> ignore as many filters as possible on top of the BDS chain since those
>> neither change the type nor the filename of the underlying image.
>>
>> Filters like quorum which have multiple underlying BDS should not be
>> removed, however, since the underlying BDS chains may lead to different
>> image formats and most certainly to different filenames. Therefore, only
>> simple filters with a single underlying BDS may be skipped.
>>
>> In addition, any "raw" BDS on top of such a simple filter should be
>> removed, since they have probably been automatically created by
>> bdrv_open() but basically function as a simple filter as well.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qapi.c | 20 ++++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index 8f2b4db..c4ddced 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -171,11 +171,27 @@ void bdrv_query_image_info(BlockDriverState *bs,
>>       int ret;
>>       Error *err = NULL;
>>       ImageInfo *info = g_new0(ImageInfo, 1);
>> +    BlockDriverState *ubs;
>>   
>>       bdrv_get_geometry(bs, &total_sectors);
>>   
>> -    info->filename        = g_strdup(bs->filename);
>> -    info->format          = g_strdup(bdrv_get_format_name(bs));
>> +    /* Remove the top layer of filters; that is, remove every "raw" BDS on top
>> +     * of a simple filter and every simple filter itself until a BDS is
>> +     * encountered which cannot be removed through these rules; a simple filter
>> +     * is a filter BDS which has its child in .file (that is,
>> +     * bdrv_recurse_is_first_non_filter is NULL). */
>> +    ubs = bs;
>> +    while ((ubs->drv && ubs->drv->authorizations[BS_IS_A_FILTER]) ||
>> +           (ubs->drv && !strcmp(ubs->drv->format_name, "raw") && ubs->file &&
>> +            ubs->file->drv && ubs->file->drv->authorizations[BS_IS_A_FILTER] &&
>> +            !ubs->file->drv->bdrv_recurse_is_first_non_filter))
>> +    {
>> +        ubs = ubs->file;
>> +    }
> The rewrite Paolo asked me to do don't allow to do it anymore: there is no
> distinction between simple filter driver and quorum like filter drivers.
>
> Maybe this rewrite was not so nice.

I'll try to think of something. *g*


Max

> Best regards
>
> Benoît
>
>> +
>> +    info->filename        = g_strdup(ubs->filename);
>> +    info->format          = g_strdup(bdrv_get_format_name(ubs));
>> +
>>       info->virtual_size    = total_sectors * 512;
>>       info->actual_size     = bdrv_get_allocated_file_size(bs);
>>       info->has_actual_size = info->actual_size >= 0;
>> -- 
>> 1.9.0
>>
>>

Re: [Qemu-devel] [PATCH 01/10] qdict: Add qdict_join()

[Max Reitz]

On 05.03.2014 17:52, Eric Blake wrote:
> On 03/03/2014 08:28 AM, Max Reitz wrote:
>> This function joins two QDicts by absorbing one into the other.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   include/qapi/qmp/qdict.h |  3 +++
>>   qobject/qdict.c          | 32 ++++++++++++++++++++++++++++++++
>>   2 files changed, 35 insertions(+)
>>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> However, I think you should expand the testsuite to cover this addition
> (can be a followup)

I keep forgetting the tests for QDict... Yes, I'll include it in v2.


Thanks,

Max

Re: [Qemu-devel] [PATCH 10/10] iotests: Add test for the JSON protocol

[Max Reitz]

On 05.03.2014 18:27, Eric Blake wrote:
> On 03/03/2014 08:28 AM, Max Reitz wrote:
>> Add a test for the JSON protocol driver.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/084     | 114 +++++++++++++++++++++++++++++++++++++++++++++
>> +
>> +# Taken from test 072
> The comment is okay, but...
>
>> +echo
>> +echo "=== Testing nested image formats (072) ==="
> ...maybe this echo should be updated to mention test 084.
>
>> +$QEMU_IO -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
>> +         -c 'read -P 66 1024 512' "json:{
>> +    \"driver\": \"$IMGFMT\",
>> +    \"file\": {
>> +        \"driver\": \"$IMGFMT\",
>> +        \"file\": {
>> +            \"filename\": \"$TEST_IMG\"
> Are we guaranteed that $TEST_IMG will not contain any " which would
> render this invalid JSON?

Probably not, but do you have an idea to circumvent this?

>> +
>> +# Taken from test 071
>> +echo
>> +echo "=== Testing blkdebug (071) ==="
> Hmm - now you're mentioning yet another test id different than 084.  So
> I guess this was just a hint that you are reproducing earlier tests but
> now with the context of a json: protocol.  Still, doesn't "Testing
> blkdebug" convey sufficient information, without also needing "(071)"
> for confusion?

Probably, yes. I think I'll leave it in the comments for reference and 
remove it from the echoes.


Max

> At any rate, I don't think this affects the coverage of
> the test.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Re: [Qemu-devel] [PATCH 02/10] block/json: Add JSON protocol driver

[Benoît Canet]

The Wednesday 05 Mar 2014 à 20:58:12 (+0100), Max Reitz wrote :
> On 05.03.2014 17:04, Benoît Canet wrote:
> >The Monday 03 Mar 2014 à 16:28:45 (+0100), Max Reitz wrote :
> >>Add a JSON protocol driver which allows supplying block driver options
> >>through the filename rather than separately. Other than that, it is a
> >>pure passthrough driver which identifies itself as a filter.
> >>
> >>This patch implements the functions bdrv_parse_filename(),
> >>bdrv_file_open(), bdrv_close(), bdrv_aio_readv(), bdrv_aio_writev(),
> >>bdrv_getlength(), bdrv_refresh_limits() and bdrv_get_info().
> >>
> >>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>---
> >>  block/Makefile.objs |   2 +-
> >>  block/json.c        | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 137 insertions(+), 1 deletion(-)
> >>  create mode 100644 block/json.c
> >>
> >>diff --git a/block/Makefile.objs b/block/Makefile.objs
> >>index fd88c03..d4b70f4 100644
> >>--- a/block/Makefile.objs
> >>+++ b/block/Makefile.objs
> >>@@ -5,7 +5,7 @@ block-obj-y += qed-check.o
> >>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> >>  block-obj-$(CONFIG_QUORUM) += quorum.o
> >>  block-obj-y += parallels.o blkdebug.o blkverify.o
> >>-block-obj-y += snapshot.o qapi.o
> >>+block-obj-y += snapshot.o qapi.o json.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
> >>diff --git a/block/json.c b/block/json.c
> >>new file mode 100644
> >>index 0000000..6d63cf6
> >>--- /dev/null
> >>+++ b/block/json.c
> >>@@ -0,0 +1,136 @@
> >>+/*
> >>+ * JSON filename wrapper protocol driver
> >>+ *
> >>+ * Copyright (c) 2014 Red Hat Inc.
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify
> >>+ * it under the terms of the GNU General Public License as published by
> >>+ * the Free Software Foundation; either version 2 of the License, or
> >>+ * (at your option) any later version.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>+ * GNU General Public License for more details.
> >>+ *
> >>+ * You should have received a copy of the GNU General Public License
> >>+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >>+ */
> >>+
> >>+#include "qemu-common.h"
> >>+#include "block/block_int.h"
> >>+#include "qapi/qmp/qdict.h"
> >>+#include "qapi/qmp/qjson.h"
> >>+
> >>+static void json_parse_filename(const char *filename, QDict *options,
> >>+                                Error **errp)
> >>+{
> >>+    QObject *file_options_obj;
> >>+    QDict *full_options;
> >>+
> >>+    if (!strstart(filename, "json:", &filename)) {
> >>+        error_setg(errp, "Unknown protocol prefix for JSON block driver");
> >>+        return;
> >>+    }
> >>+
> >>+    file_options_obj = qobject_from_json(filename);
> >>+    if (!file_options_obj) {
> >>+        error_setg(errp, "Could not parse the JSON options");
> >>+        return;
> >>+    }
> >>+
> >>+    if (qobject_type(file_options_obj) != QTYPE_QDICT) {
> >>+        qobject_decref(file_options_obj);
> >>+        error_setg(errp, "Invalid JSON object");
> >>+        return;
> >>+    }
> >>+
> >>+    full_options = qdict_new();
> >>+    qdict_put_obj(full_options, "x-options", file_options_obj);
> >>+    qdict_flatten(full_options);
> >>+
> >>+    qdict_join(options, full_options, true);
> >>+    assert(qdict_size(full_options) == 0);
> >>+    QDECREF(full_options);
> >>+}
> >>+
> >>+static int json_open(BlockDriverState *bs, QDict *options, int flags,
> >>+                     Error **errp)
> >>+{
> >>+    int ret;
> >>+
> >>+    assert(bs->file == NULL);
> >>+    ret = bdrv_open_image(&bs->file, NULL, options, "x-options", flags, false,
> >>+                          errp);
> >
> >>+    if (ret < 0) {
> >>+        return ret;
> >>+    }
> >>+
> >>+    return 0;
> >Why not ?
> >     return ret;
> >}
> >
> >Do you plan to enrich this code path later ?
> 
> You're right, that'd be shorter. It's just that I'm so in the habit
> of writing "ret = foo(); if (ret < 0) { return ret; / goto fail; }"
> that I didn't notice. ;-)
> 
> >>+}
> >>+
> >>+static void json_close(BlockDriverState *bs)
> >>+{
> >>+}
> >>+
> >>+static BlockDriverAIOCB *json_aio_readv(BlockDriverState *bs,
> >>+                                        int64_t sector_num, QEMUIOVector *qiov,
> >>+                                        int nb_sectors,
> >>+                                        BlockDriverCompletionFunc *cb,
> >>+                                        void *opaque)
> >>+{
> >>+    return bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
> >>+}
> >>+
> >>+static BlockDriverAIOCB *json_aio_writev(BlockDriverState *bs,
> >>+                                         int64_t sector_num, QEMUIOVector *qiov,
> >>+                                         int nb_sectors,
> >>+                                         BlockDriverCompletionFunc *cb,
> >>+                                         void *opaque)
> >>+{
> >>+    return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
> >>+}
> >>+
> >>+static int64_t json_getlength(BlockDriverState *bs)
> >>+{
> >>+    return bdrv_getlength(bs->file);
> >>+}
> >>+
> >>+static int json_refresh_limits(BlockDriverState *bs)
> >>+{
> >>+    bs->bl = bs->file->bl;
> >>+    return 0;
> >>+}
> >>+
> >>+static int json_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> >>+{
> >>+    return bdrv_get_info(bs->file, bdi);
> >>+}
> >>+
> >>+static BlockDriver bdrv_json = {
> >>+    .format_name                = "json",
> >>+    .protocol_name              = "json",
> >>+    .instance_size              = 0,
> >>+
> >>+    .bdrv_parse_filename        = json_parse_filename,
> >>+    .bdrv_file_open             = json_open,
> >>+    .bdrv_close                 = json_close,
> >>+
> >>+    .bdrv_aio_readv             = json_aio_readv,
> >>+    .bdrv_aio_writev            = json_aio_writev,
> >>+
> >>+    .has_variable_length        = true,
> >>+    .bdrv_getlength             = json_getlength,
> >>+
> >>+    .bdrv_refresh_limits        = json_refresh_limits,
> >>+    .bdrv_get_info              = json_get_info,
> >>+
> >>+    .authorizations             = { true, true },
> >Paolo make rewrite the snapshot authorization code.
> >In the new version you need
> >     .is_filter = true;
> >     .bdrv_recurse_is_first_children = your_implementation;
> >
> >I don't know when these patchset will be reviewed and merged so maybe this
> >version is correct.
> 
> I'll just rebase on top of Paolo's series then.

It's a sugestion made by Paolo. I wrote the actual code.
The series is :
"[PATCH] Rewrite block filter snapshot authorization mechanism"

> 
> 
> Thanks,
> 
> Max
> 
> >>+};
> >>+
> >>+static void bdrv_json_init(void)
> >>+{
> >>+    bdrv_register(&bdrv_json);
> >>+}
> >>+
> >>+block_init(bdrv_json_init);
> >>-- 
> >>1.9.0
> >>
> >>
> 
> 

Re: [Qemu-devel] [PATCH 02/10] block/json: Add JSON protocol driver

[Max Reitz]

On 05.03.2014 21:20, Benoît Canet wrote:
> The Wednesday 05 Mar 2014 à 20:58:12 (+0100), Max Reitz wrote :
>> On 05.03.2014 17:04, Benoît Canet wrote:
>>> The Monday 03 Mar 2014 à 16:28:45 (+0100), Max Reitz wrote :
>>>> Add a JSON protocol driver which allows supplying block driver options
>>>> through the filename rather than separately. Other than that, it is a
>>>> pure passthrough driver which identifies itself as a filter.
>>>>
>>>> This patch implements the functions bdrv_parse_filename(),
>>>> bdrv_file_open(), bdrv_close(), bdrv_aio_readv(), bdrv_aio_writev(),
>>>> bdrv_getlength(), bdrv_refresh_limits() and bdrv_get_info().
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>   block/Makefile.objs |   2 +-
>>>>   block/json.c        | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 137 insertions(+), 1 deletion(-)
>>>>   create mode 100644 block/json.c
>>>>
>>>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>>>> index fd88c03..d4b70f4 100644
>>>> --- a/block/Makefile.objs
>>>> +++ b/block/Makefile.objs
>>>> @@ -5,7 +5,7 @@ block-obj-y += qed-check.o
>>>>   block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
>>>>   block-obj-$(CONFIG_QUORUM) += quorum.o
>>>>   block-obj-y += parallels.o blkdebug.o blkverify.o
>>>> -block-obj-y += snapshot.o qapi.o
>>>> +block-obj-y += snapshot.o qapi.o json.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
>>>> diff --git a/block/json.c b/block/json.c
>>>> new file mode 100644
>>>> index 0000000..6d63cf6
>>>> --- /dev/null
>>>> +++ b/block/json.c
>>>> @@ -0,0 +1,136 @@
>>>> +/*
>>>> + * JSON filename wrapper protocol driver
>>>> + *
>>>> + * Copyright (c) 2014 Red Hat Inc.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License
>>>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>> + */
>>>> +
>>>> +#include "qemu-common.h"
>>>> +#include "block/block_int.h"
>>>> +#include "qapi/qmp/qdict.h"
>>>> +#include "qapi/qmp/qjson.h"
>>>> +
>>>> +static void json_parse_filename(const char *filename, QDict *options,
>>>> +                                Error **errp)
>>>> +{
>>>> +    QObject *file_options_obj;
>>>> +    QDict *full_options;
>>>> +
>>>> +    if (!strstart(filename, "json:", &filename)) {
>>>> +        error_setg(errp, "Unknown protocol prefix for JSON block driver");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    file_options_obj = qobject_from_json(filename);
>>>> +    if (!file_options_obj) {
>>>> +        error_setg(errp, "Could not parse the JSON options");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (qobject_type(file_options_obj) != QTYPE_QDICT) {
>>>> +        qobject_decref(file_options_obj);
>>>> +        error_setg(errp, "Invalid JSON object");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    full_options = qdict_new();
>>>> +    qdict_put_obj(full_options, "x-options", file_options_obj);
>>>> +    qdict_flatten(full_options);
>>>> +
>>>> +    qdict_join(options, full_options, true);
>>>> +    assert(qdict_size(full_options) == 0);
>>>> +    QDECREF(full_options);
>>>> +}
>>>> +
>>>> +static int json_open(BlockDriverState *bs, QDict *options, int flags,
>>>> +                     Error **errp)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    assert(bs->file == NULL);
>>>> +    ret = bdrv_open_image(&bs->file, NULL, options, "x-options", flags, false,
>>>> +                          errp);
>>>> +    if (ret < 0) {
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    return 0;
>>> Why not ?
>>>      return ret;
>>> }
>>>
>>> Do you plan to enrich this code path later ?
>> You're right, that'd be shorter. It's just that I'm so in the habit
>> of writing "ret = foo(); if (ret < 0) { return ret; / goto fail; }"
>> that I didn't notice. ;-)
>>
>>>> +}
>>>> +
>>>> +static void json_close(BlockDriverState *bs)
>>>> +{
>>>> +}
>>>> +
>>>> +static BlockDriverAIOCB *json_aio_readv(BlockDriverState *bs,
>>>> +                                        int64_t sector_num, QEMUIOVector *qiov,
>>>> +                                        int nb_sectors,
>>>> +                                        BlockDriverCompletionFunc *cb,
>>>> +                                        void *opaque)
>>>> +{
>>>> +    return bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
>>>> +}
>>>> +
>>>> +static BlockDriverAIOCB *json_aio_writev(BlockDriverState *bs,
>>>> +                                         int64_t sector_num, QEMUIOVector *qiov,
>>>> +                                         int nb_sectors,
>>>> +                                         BlockDriverCompletionFunc *cb,
>>>> +                                         void *opaque)
>>>> +{
>>>> +    return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
>>>> +}
>>>> +
>>>> +static int64_t json_getlength(BlockDriverState *bs)
>>>> +{
>>>> +    return bdrv_getlength(bs->file);
>>>> +}
>>>> +
>>>> +static int json_refresh_limits(BlockDriverState *bs)
>>>> +{
>>>> +    bs->bl = bs->file->bl;
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int json_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>>>> +{
>>>> +    return bdrv_get_info(bs->file, bdi);
>>>> +}
>>>> +
>>>> +static BlockDriver bdrv_json = {
>>>> +    .format_name                = "json",
>>>> +    .protocol_name              = "json",
>>>> +    .instance_size              = 0,
>>>> +
>>>> +    .bdrv_parse_filename        = json_parse_filename,
>>>> +    .bdrv_file_open             = json_open,
>>>> +    .bdrv_close                 = json_close,
>>>> +
>>>> +    .bdrv_aio_readv             = json_aio_readv,
>>>> +    .bdrv_aio_writev            = json_aio_writev,
>>>> +
>>>> +    .has_variable_length        = true,
>>>> +    .bdrv_getlength             = json_getlength,
>>>> +
>>>> +    .bdrv_refresh_limits        = json_refresh_limits,
>>>> +    .bdrv_get_info              = json_get_info,
>>>> +
>>>> +    .authorizations             = { true, true },
>>> Paolo make rewrite the snapshot authorization code.
>>> In the new version you need
>>>      .is_filter = true;
>>>      .bdrv_recurse_is_first_children = your_implementation;
>>>
>>> I don't know when these patchset will be reviewed and merged so maybe this
>>> version is correct.
>> I'll just rebase on top of Paolo's series then.
> It's a sugestion made by Paolo. I wrote the actual code.
> The series is :
> "[PATCH] Rewrite block filter snapshot authorization mechanism"

Okay, thank you again. :-)

Max

>>
>> Thanks,
>>
>> Max
>>
>>>> +};
>>>> +
>>>> +static void bdrv_json_init(void)
>>>> +{
>>>> +    bdrv_register(&bdrv_json);
>>>> +}
>>>> +
>>>> +block_init(bdrv_json_init);
>>>> -- 
>>>> 1.9.0
>>>>
>>>>
>>

Re: [Qemu-devel] [PATCH 10/10] iotests: Add test for the JSON protocol

[Eric Blake]

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

On 03/05/2014 01:15 PM, Max Reitz wrote:

>>> +            \"filename\": \"$TEST_IMG\"
>> Are we guaranteed that $TEST_IMG will not contain any " which would
>> render this invalid JSON?
> 
> Probably not, but do you have an idea to circumvent this?

If nothing else, we can at least be paranoid with something like:

case $TEST_IMG in
 *'"'*) skip the test... ;;
esac

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH 05/10] block/json: Add bdrv_co_get_block_status()

[Benoît Canet]

The Wednesday 05 Mar 2014 à 21:10:03 (+0100), Max Reitz wrote :
> On 05.03.2014 17:11, Benoît Canet wrote:
> >The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote :
> >>Implement this function in the same way as raw_bsd does: Acknowledge
> >>that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID
> >>and BDRV_BLOCK_DATA and derive the offset directly from the sector
> >>index) and add BDRV_BLOCK_RAW to the returned value.
> >>
> >>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>---
> >>  block/json.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >>diff --git a/block/json.c b/block/json.c
> >>index a2f4691..7392802 100644
> >>--- a/block/json.c
> >>+++ b/block/json.c
> >>@@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
> >>      return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
> >>  }
> >>+static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs,
> >>+                                                     int64_t sector_num,
> >>+                                                     int nb_sectors, int *pnum)
> >>+{
> >>+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> >>+           (sector_num << BDRV_SECTOR_BITS);
> >>+}
> >I don't understand what is the selling point of this method instead of calling
> >bdrv_co_get_block_status on bs->file.
> >Some information risk to be lost and it does look like magic.
> 
> This is the same what "raw" does. It just is more meaningful: This
> way, this function does not pretend to provide the blocks itself but
> instead tells the truth; that is, the blocks are provided by an
> underlying BDS (bs->file).
> 
> I wasn't really sure what to do myself. Generally, this driver is
> actually meant to pretend that it provides the blocks itself. On the
> other hand, I tried to imitate the behavior or "raw", since this is
> something I can hope to be approximately correct. Also, as I've said
> before, the value returned here is in fact at least technically
> correct.

The raw_bsd driver have an additional *pnum = nb_sectors; at the begining of the
function. Did you left it on purpose ?

Best regards

Benoît 
> 
> 
> Max
> 
> >Best regards
> >
> >Benoît
> >
> >>+
> >>  static void json_invalidate_cache(BlockDriverState *bs)
> >>  {
> >>      return bdrv_invalidate_cache(bs->file);
> >>@@ -159,6 +167,7 @@ static BlockDriver bdrv_json = {
> >>      .bdrv_aio_discard           = json_aio_discard,
> >>      .bdrv_co_write_zeroes       = json_co_write_zeroes,
> >>+    .bdrv_co_get_block_status   = json_co_get_block_status,
> >>      .bdrv_invalidate_cache      = json_invalidate_cache,
> >>-- 
> >>1.9.0
> >>
> >>
> 

Re: [Qemu-devel] [PATCH 05/10] block/json: Add bdrv_co_get_block_status()

[Max Reitz]

On 05.03.2014 21:41, Benoît Canet wrote:
> The Wednesday 05 Mar 2014 à 21:10:03 (+0100), Max Reitz wrote :
>> On 05.03.2014 17:11, Benoît Canet wrote:
>>> The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote :
>>>> Implement this function in the same way as raw_bsd does: Acknowledge
>>>> that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID
>>>> and BDRV_BLOCK_DATA and derive the offset directly from the sector
>>>> index) and add BDRV_BLOCK_RAW to the returned value.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>   block/json.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/block/json.c b/block/json.c
>>>> index a2f4691..7392802 100644
>>>> --- a/block/json.c
>>>> +++ b/block/json.c
>>>> @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
>>>>       return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
>>>>   }
>>>> +static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs,
>>>> +                                                     int64_t sector_num,
>>>> +                                                     int nb_sectors, int *pnum)
>>>> +{
>>>> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
>>>> +           (sector_num << BDRV_SECTOR_BITS);
>>>> +}
>>> I don't understand what is the selling point of this method instead of calling
>>> bdrv_co_get_block_status on bs->file.
>>> Some information risk to be lost and it does look like magic.
>> This is the same what "raw" does. It just is more meaningful: This
>> way, this function does not pretend to provide the blocks itself but
>> instead tells the truth; that is, the blocks are provided by an
>> underlying BDS (bs->file).
>>
>> I wasn't really sure what to do myself. Generally, this driver is
>> actually meant to pretend that it provides the blocks itself. On the
>> other hand, I tried to imitate the behavior or "raw", since this is
>> something I can hope to be approximately correct. Also, as I've said
>> before, the value returned here is in fact at least technically
>> correct.
> The raw_bsd driver have an additional *pnum = nb_sectors; at the begining of the
> function. Did you left it on purpose ?

Oops, no, I did not. Okay, if I even fail at copying code, the argument 
about "raw" being at least probably correct isn't worth anything here, I 
guess. ;-)

Max

> Best regards
>
> Benoît
>>
>> Max
>>
>>> Best regards
>>>
>>> Benoît
>>>
>>>> +
>>>>   static void json_invalidate_cache(BlockDriverState *bs)
>>>>   {
>>>>       return bdrv_invalidate_cache(bs->file);
>>>> @@ -159,6 +167,7 @@ static BlockDriver bdrv_json = {
>>>>       .bdrv_aio_discard           = json_aio_discard,
>>>>       .bdrv_co_write_zeroes       = json_co_write_zeroes,
>>>> +    .bdrv_co_get_block_status   = json_co_get_block_status,
>>>>       .bdrv_invalidate_cache      = json_invalidate_cache,
>>>> -- 
>>>> 1.9.0
>>>>
>>>>

Re: [Qemu-devel] [PATCH 02/10] block/json: Add JSON protocol driver

[Benoît Canet]

The Wednesday 05 Mar 2014 à 21:21:46 (+0100), Max Reitz wrote :
> On 05.03.2014 21:20, Benoît Canet wrote:
> >The Wednesday 05 Mar 2014 à 20:58:12 (+0100), Max Reitz wrote :
> >>On 05.03.2014 17:04, Benoît Canet wrote:
> >>>The Monday 03 Mar 2014 à 16:28:45 (+0100), Max Reitz wrote :
> >>>>Add a JSON protocol driver which allows supplying block driver options
> >>>>through the filename rather than separately. Other than that, it is a
> >>>>pure passthrough driver which identifies itself as a filter.
> >>>>
> >>>>This patch implements the functions bdrv_parse_filename(),
> >>>>bdrv_file_open(), bdrv_close(), bdrv_aio_readv(), bdrv_aio_writev(),
> >>>>bdrv_getlength(), bdrv_refresh_limits() and bdrv_get_info().
> >>>>
> >>>>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>>>---
> >>>>  block/Makefile.objs |   2 +-
> >>>>  block/json.c        | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 137 insertions(+), 1 deletion(-)
> >>>>  create mode 100644 block/json.c
> >>>>
> >>>>diff --git a/block/Makefile.objs b/block/Makefile.objs
> >>>>index fd88c03..d4b70f4 100644
> >>>>--- a/block/Makefile.objs
> >>>>+++ b/block/Makefile.objs
> >>>>@@ -5,7 +5,7 @@ block-obj-y += qed-check.o
> >>>>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> >>>>  block-obj-$(CONFIG_QUORUM) += quorum.o
> >>>>  block-obj-y += parallels.o blkdebug.o blkverify.o
> >>>>-block-obj-y += snapshot.o qapi.o
> >>>>+block-obj-y += snapshot.o qapi.o json.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
> >>>>diff --git a/block/json.c b/block/json.c
> >>>>new file mode 100644
> >>>>index 0000000..6d63cf6
> >>>>--- /dev/null
> >>>>+++ b/block/json.c
> >>>>@@ -0,0 +1,136 @@
> >>>>+/*
> >>>>+ * JSON filename wrapper protocol driver
> >>>>+ *
> >>>>+ * Copyright (c) 2014 Red Hat Inc.
> >>>>+ *
> >>>>+ * This program is free software; you can redistribute it and/or modify
> >>>>+ * it under the terms of the GNU General Public License as published by
> >>>>+ * the Free Software Foundation; either version 2 of the License, or
> >>>>+ * (at your option) any later version.
> >>>>+ *
> >>>>+ * This program is distributed in the hope that it will be useful,
> >>>>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>>>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>>>+ * GNU General Public License for more details.
> >>>>+ *
> >>>>+ * You should have received a copy of the GNU General Public License
> >>>>+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >>>>+ */
> >>>>+
> >>>>+#include "qemu-common.h"
> >>>>+#include "block/block_int.h"
> >>>>+#include "qapi/qmp/qdict.h"
> >>>>+#include "qapi/qmp/qjson.h"
> >>>>+
> >>>>+static void json_parse_filename(const char *filename, QDict *options,
> >>>>+                                Error **errp)
> >>>>+{
> >>>>+    QObject *file_options_obj;
> >>>>+    QDict *full_options;
> >>>>+
> >>>>+    if (!strstart(filename, "json:", &filename)) {
> >>>>+        error_setg(errp, "Unknown protocol prefix for JSON block driver");
> >>>>+        return;
> >>>>+    }
> >>>>+
> >>>>+    file_options_obj = qobject_from_json(filename);
> >>>>+    if (!file_options_obj) {
> >>>>+        error_setg(errp, "Could not parse the JSON options");
> >>>>+        return;
> >>>>+    }
> >>>>+
> >>>>+    if (qobject_type(file_options_obj) != QTYPE_QDICT) {
> >>>>+        qobject_decref(file_options_obj);
> >>>>+        error_setg(errp, "Invalid JSON object");
> >>>>+        return;
> >>>>+    }
> >>>>+
> >>>>+    full_options = qdict_new();
> >>>>+    qdict_put_obj(full_options, "x-options", file_options_obj);
> >>>>+    qdict_flatten(full_options);
> >>>>+
> >>>>+    qdict_join(options, full_options, true);
> >>>>+    assert(qdict_size(full_options) == 0);
> >>>>+    QDECREF(full_options);
> >>>>+}
> >>>>+
> >>>>+static int json_open(BlockDriverState *bs, QDict *options, int flags,
> >>>>+                     Error **errp)
> >>>>+{
> >>>>+    int ret;
> >>>>+
> >>>>+    assert(bs->file == NULL);
> >>>>+    ret = bdrv_open_image(&bs->file, NULL, options, "x-options", flags, false,
> >>>>+                          errp);
> >>>>+    if (ret < 0) {
> >>>>+        return ret;
> >>>>+    }
> >>>>+
> >>>>+    return 0;
> >>>Why not ?
> >>>     return ret;
> >>>}
> >>>
> >>>Do you plan to enrich this code path later ?
> >>You're right, that'd be shorter. It's just that I'm so in the habit
> >>of writing "ret = foo(); if (ret < 0) { return ret; / goto fail; }"
> >>that I didn't notice. ;-)
> >>
> >>>>+}
> >>>>+
> >>>>+static void json_close(BlockDriverState *bs)
> >>>>+{
> >>>>+}
> >>>>+
> >>>>+static BlockDriverAIOCB *json_aio_readv(BlockDriverState *bs,
> >>>>+                                        int64_t sector_num, QEMUIOVector *qiov,
> >>>>+                                        int nb_sectors,
> >>>>+                                        BlockDriverCompletionFunc *cb,
> >>>>+                                        void *opaque)
> >>>>+{
> >>>>+    return bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
> >>>>+}
> >>>>+
> >>>>+static BlockDriverAIOCB *json_aio_writev(BlockDriverState *bs,
> >>>>+                                         int64_t sector_num, QEMUIOVector *qiov,
> >>>>+                                         int nb_sectors,
> >>>>+                                         BlockDriverCompletionFunc *cb,
> >>>>+                                         void *opaque)
> >>>>+{
> >>>>+    return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
> >>>>+}
> >>>>+
> >>>>+static int64_t json_getlength(BlockDriverState *bs)
> >>>>+{
> >>>>+    return bdrv_getlength(bs->file);
> >>>>+}
> >>>>+
> >>>>+static int json_refresh_limits(BlockDriverState *bs)
> >>>>+{
> >>>>+    bs->bl = bs->file->bl;
> >>>>+    return 0;
> >>>>+}
> >>>>+
> >>>>+static int json_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> >>>>+{
> >>>>+    return bdrv_get_info(bs->file, bdi);
> >>>>+}
> >>>>+
> >>>>+static BlockDriver bdrv_json = {
> >>>>+    .format_name                = "json",
> >>>>+    .protocol_name              = "json",
> >>>>+    .instance_size              = 0,
> >>>>+
> >>>>+    .bdrv_parse_filename        = json_parse_filename,
> >>>>+    .bdrv_file_open             = json_open,
> >>>>+    .bdrv_close                 = json_close,
> >>>>+
> >>>>+    .bdrv_aio_readv             = json_aio_readv,
> >>>>+    .bdrv_aio_writev            = json_aio_writev,
> >>>>+
> >>>>+    .has_variable_length        = true,
> >>>>+    .bdrv_getlength             = json_getlength,
> >>>>+
> >>>>+    .bdrv_refresh_limits        = json_refresh_limits,
> >>>>+    .bdrv_get_info              = json_get_info,
> >>>>+
> >>>>+    .authorizations             = { true, true },
> >>>Paolo make rewrite the snapshot authorization code.
> >>>In the new version you need
> >>>     .is_filter = true;
> >>>     .bdrv_recurse_is_first_children = your_implementation;
> >>>
> >>>I don't know when these patchset will be reviewed and merged so maybe this
> >>>version is correct.
> >>I'll just rebase on top of Paolo's series then.
> >It's a sugestion made by Paolo. I wrote the actual code.
> >The series is :
> >"[PATCH] Rewrite block filter snapshot authorization mechanism"
> 
> Okay, thank you again. :-)
> 
> Max
> 
> >>
> >>Thanks,
> >>
> >>Max
> >>
> >>>>+};
> >>>>+
> >>>>+static void bdrv_json_init(void)
> >>>>+{
> >>>>+    bdrv_register(&bdrv_json);
> >>>>+}
> >>>>+
> >>>>+block_init(bdrv_json_init);
> >>>>-- 
> >>>>1.9.0
> >>>>
> >>>>
> >>
> 
> 

I have a request: if you have some spare bandwith could you please review.
[QEMU 2.0 Fix] Fix bdrv_swap behavior regarding the node graph
and
[PATCH 0/2] Allow to repair broken quorum files

The first one is a fix that should go in 2.0.
The second would render Quorum usable in production so I would like to make it
merge before the 2.0 hard freeze if it's doable.

Best regards

Benoît

Re: [Qemu-devel] [PATCH 05/10] block/json: Add bdrv_co_get_block_status()

[Benoît Canet]

The Wednesday 05 Mar 2014 à 21:44:57 (+0100), Max Reitz wrote :
> On 05.03.2014 21:41, Benoît Canet wrote:
> >The Wednesday 05 Mar 2014 à 21:10:03 (+0100), Max Reitz wrote :
> >>On 05.03.2014 17:11, Benoît Canet wrote:
> >>>The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote :
> >>>>Implement this function in the same way as raw_bsd does: Acknowledge
> >>>>that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID
> >>>>and BDRV_BLOCK_DATA and derive the offset directly from the sector
> >>>>index) and add BDRV_BLOCK_RAW to the returned value.
> >>>>
> >>>>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>>>---
> >>>>  block/json.c | 9 +++++++++
> >>>>  1 file changed, 9 insertions(+)
> >>>>
> >>>>diff --git a/block/json.c b/block/json.c
> >>>>index a2f4691..7392802 100644
> >>>>--- a/block/json.c
> >>>>+++ b/block/json.c
> >>>>@@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
> >>>>      return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
> >>>>  }
> >>>>+static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs,
> >>>>+                                                     int64_t sector_num,
> >>>>+                                                     int nb_sectors, int *pnum)
> >>>>+{
> >>>>+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> >>>>+           (sector_num << BDRV_SECTOR_BITS);
> >>>>+}
> >>>I don't understand what is the selling point of this method instead of calling
> >>>bdrv_co_get_block_status on bs->file.
> >>>Some information risk to be lost and it does look like magic.
> >>This is the same what "raw" does. It just is more meaningful: This
> >>way, this function does not pretend to provide the blocks itself but
> >>instead tells the truth; that is, the blocks are provided by an
> >>underlying BDS (bs->file).
> >>
> >>I wasn't really sure what to do myself. Generally, this driver is
> >>actually meant to pretend that it provides the blocks itself. On the
> >>other hand, I tried to imitate the behavior or "raw", since this is
> >>something I can hope to be approximately correct. Also, as I've said
> >>before, the value returned here is in fact at least technically
> >>correct.
> >The raw_bsd driver have an additional *pnum = nb_sectors; at the begining of the
> >function. Did you left it on purpose ?
> 
> Oops, no, I did not. Okay, if I even fail at copying code, the
> argument about "raw" being at least probably correct isn't worth
> anything here, I guess. ;-)

In fact I wonder i posix_raw act this way because one of the BDRV it does wrap
have simply no clue on how to answer to this request.

Best regards

Benoît

> 
> Max
> 
> >Best regards
> >
> >Benoît
> >>
> >>Max
> >>
> >>>Best regards
> >>>
> >>>Benoît
> >>>
> >>>>+
> >>>>  static void json_invalidate_cache(BlockDriverState *bs)
> >>>>  {
> >>>>      return bdrv_invalidate_cache(bs->file);
> >>>>@@ -159,6 +167,7 @@ static BlockDriver bdrv_json = {
> >>>>      .bdrv_aio_discard           = json_aio_discard,
> >>>>      .bdrv_co_write_zeroes       = json_co_write_zeroes,
> >>>>+    .bdrv_co_get_block_status   = json_co_get_block_status,
> >>>>      .bdrv_invalidate_cache      = json_invalidate_cache,
> >>>>-- 
> >>>>1.9.0
> >>>>
> >>>>
> 

Re: [Qemu-devel] [PATCH 05/10] block/json: Add bdrv_co_get_block_status()

[Max Reitz]

On 06.03.2014 00:22, Benoît Canet wrote:
> The Wednesday 05 Mar 2014 à 21:44:57 (+0100), Max Reitz wrote :
>> On 05.03.2014 21:41, Benoît Canet wrote:
>>> The Wednesday 05 Mar 2014 à 21:10:03 (+0100), Max Reitz wrote :
>>>> On 05.03.2014 17:11, Benoît Canet wrote:
>>>>> The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote :
>>>>>> Implement this function in the same way as raw_bsd does: Acknowledge
>>>>>> that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID
>>>>>> and BDRV_BLOCK_DATA and derive the offset directly from the sector
>>>>>> index) and add BDRV_BLOCK_RAW to the returned value.
>>>>>>
>>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>>> ---
>>>>>>   block/json.c | 9 +++++++++
>>>>>>   1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/block/json.c b/block/json.c
>>>>>> index a2f4691..7392802 100644
>>>>>> --- a/block/json.c
>>>>>> +++ b/block/json.c
>>>>>> @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
>>>>>>       return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
>>>>>>   }
>>>>>> +static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs,
>>>>>> +                                                     int64_t sector_num,
>>>>>> +                                                     int nb_sectors, int *pnum)
>>>>>> +{
>>>>>> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
>>>>>> +           (sector_num << BDRV_SECTOR_BITS);
>>>>>> +}
>>>>> I don't understand what is the selling point of this method instead of calling
>>>>> bdrv_co_get_block_status on bs->file.
>>>>> Some information risk to be lost and it does look like magic.
>>>> This is the same what "raw" does. It just is more meaningful: This
>>>> way, this function does not pretend to provide the blocks itself but
>>>> instead tells the truth; that is, the blocks are provided by an
>>>> underlying BDS (bs->file).
>>>>
>>>> I wasn't really sure what to do myself. Generally, this driver is
>>>> actually meant to pretend that it provides the blocks itself. On the
>>>> other hand, I tried to imitate the behavior or "raw", since this is
>>>> something I can hope to be approximately correct. Also, as I've said
>>>> before, the value returned here is in fact at least technically
>>>> correct.
>>> The raw_bsd driver have an additional *pnum = nb_sectors; at the begining of the
>>> function. Did you left it on purpose ?
>> Oops, no, I did not. Okay, if I even fail at copying code, the
>> argument about "raw" being at least probably correct isn't worth
>> anything here, I guess. ;-)
> In fact I wonder i posix_raw act this way because one of the BDRV it does wrap
> have simply no clue on how to answer to this request.

If you take a look at bdrv_co_get_block_status() in block.c, you'll see 
that BDRV_BLOCK_RAW actually results in a recursive call to 
bdrv_get_block_status().


Max

> Best regards
>
> Benoît
>
>> Max
>>
>>> Best regards
>>>
>>> Benoît
>>>> Max
>>>>
>>>>> Best regards
>>>>>
>>>>> Benoît
>>>>>
>>>>>> +
>>>>>>   static void json_invalidate_cache(BlockDriverState *bs)
>>>>>>   {
>>>>>>       return bdrv_invalidate_cache(bs->file);
>>>>>> @@ -159,6 +167,7 @@ static BlockDriver bdrv_json = {
>>>>>>       .bdrv_aio_discard           = json_aio_discard,
>>>>>>       .bdrv_co_write_zeroes       = json_co_write_zeroes,
>>>>>> +    .bdrv_co_get_block_status   = json_co_get_block_status,
>>>>>>       .bdrv_invalidate_cache      = json_invalidate_cache,
>>>>>> -- 
>>>>>> 1.9.0
>>>>>>
>>>>>>