[Qemu-devel] [PATCH 0/6] block: Let drivers reconstruct the filename

[Qemu-devel] [PATCH 0/6] block: Let drivers reconstruct the filename

[Max Reitz]

We may sometimes want a filename for BDSs which do not have one because
they weren't opened using a simple filename (but rather just through
options, for example).

Some block drivers are always capable of reconstructing a valid filename
(e.g. NBD), even if the BDS has been opened using the options QDict
only. For others (e.g. Quorum) this is impossible. To accommodate this
case, the function which reconstructs ("refreshes") the filename for a
BDS also generates an options QDict (which should always work). If some
layer cannot generate a plain filename (e.g. a Quorum instance), we can
still generate a QDict which contains all options necessary for opening
the block device in (basically) the same state.

If a filename can be generated, the one stored in the BDS is
overwritten. Otherwise, the QDict is converted to JSON, prefixed with
"json:" and then used as the filename (making use of the JSON
pseudo-protocol).


Block drivers which probably need to implement bdrv_refresh_filename()
besides blkdebug, blkverify, NBD and Quorum but which this series does
not cover, are the following: curl, ssh and vvfat.


This series supersedes my previous 'block: Fix unset "filename" for
certain drivers'.


Max Reitz (6):
  block: Add bdrv_refresh_filename()
  blkdebug: Implement bdrv_refresh_filename()
  blkverify: Implement bdrv_refresh_filename()
  nbd: Implement bdrv_refresh_filename()
  quorum: Implement bdrv_refresh_filename()
  iotests: Add test for image filename construction

 block.c                    | 135 +++++++++++++++++++++++++++++++++++++++++++++
 block/blkdebug.c           |  97 ++++++++++++++++++++++++++++++++
 block/blkverify.c          |  29 ++++++++++
 block/nbd.c                |  36 ++++++++++++
 block/quorum.c             |  39 +++++++++++++
 include/block/block.h      |   1 +
 include/block/block_int.h  |   6 ++
 tests/qemu-iotests/099     | 116 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/099.out |  20 +++++++
 tests/qemu-iotests/group   |   1 +
 10 files changed, 480 insertions(+)
 create mode 100755 tests/qemu-iotests/099
 create mode 100644 tests/qemu-iotests/099.out

-- 
2.0.1

[Qemu-devel] [PATCH 1/6] block: Add bdrv_refresh_filename()

[Max Reitz]

Some block devices may not have a filename in their BDS; and for some,
there may not even be a normal filename at all. To work around this, add
a function which tries to construct a valid filename for the
BDS.filename field.

If a filename exists or a block driver is able to reconstruct a valid
filename (which is placed in BDS.exact_filename), this can directly be
used.

If no filename can be constructed, we can still construct an options
QDict which is then converted to a JSON object and prefixed with the
"json:" pseudo protocol prefix. The QDict is placed in
BDS.full_open_options.

For most block drivers, this process can be done automatically; those
that need special handling may define a .bdrv_refresh_filename() method
to fill BDS.exact_filename and BDS.full_open_options themselves.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
In this version, bdrv_refresh_filename() leaves the filename unmodified
if neither a new filename nor an options QDict can be generated. Another
idea would be to clear the filename in this case as it is probably
obsolete then. I was not sure which to pick, so I just used the first
version I wrote.
---
 block.c                   | 135 ++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |   1 +
 include/block/block_int.h |   6 +++
 3 files changed, 142 insertions(+)

diff --git a/block.c b/block.c
index cb95e08..16067fe 100644
--- a/block.c
+++ b/block.c
@@ -955,6 +955,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     } else {
         bs->filename[0] = '\0';
     }
+    pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->filename);
 
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
@@ -1490,6 +1491,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         }
     }
 
+    bdrv_refresh_filename(bs);
+
     /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
      * temporary snapshot afterwards. */
     if (snapshot_flags) {
@@ -1832,6 +1835,8 @@ void bdrv_close(BlockDriverState *bs)
         bs->zero_beyond_eof = false;
         QDECREF(bs->options);
         bs->options = NULL;
+        QDECREF(bs->full_open_options);
+        bs->full_open_options = NULL;
 
         if (bs->file != NULL) {
             bdrv_unref(bs->file);
@@ -5889,3 +5894,133 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
         bdrv_flush_io_queue(bs->file);
     }
 }
+
+static bool append_open_options(QDict *d, BlockDriverState *bs)
+{
+    const QDictEntry *entry;
+    bool found_any = false;
+
+    for (entry = qdict_first(bs->options); entry;
+         entry = qdict_next(bs->options, entry))
+    {
+        /* Only take options for this level and exclude all non-driver-specific
+         * options */
+        if (!strchr(qdict_entry_key(entry), '.') &&
+            strcmp(qdict_entry_key(entry), "node-name"))
+        {
+            qobject_incref(qdict_entry_value(entry));
+            qdict_put_obj(d, qdict_entry_key(entry), qdict_entry_value(entry));
+            found_any = true;
+        }
+    }
+
+    return found_any;
+}
+
+/* Updates the following BDS fields:
+ *  - exact_filename: A filename which may be used for opening a block device
+ *                    which (mostly) equals the given BDS (even without any
+ *                    other options; so reading and writing must return the same
+ *                    results, but caching etc. may be different)
+ *  - full_open_options: Options which, when given when opening a block device
+ *                       (without a filename), result in a BDS (mostly)
+ *                       equalling the given one
+ *  - filename: If exact_filename is set, it is copied here. Otherwise,
+ *              full_open_options is converted to a JSON object, prefixed with
+ *              "json:" (for use through the JSON pseudo protocol) and put here.
+ */
+void bdrv_refresh_filename(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    QDict *opts;
+
+    if (!drv) {
+        return;
+    }
+
+    /* This BDS's file name will most probably depend on its file's name, so
+     * refresh that first */
+    if (bs->file) {
+        bdrv_refresh_filename(bs->file);
+    }
+
+    if (drv->bdrv_refresh_filename) {
+        /* Obsolete information is of no use here, so drop the old file name
+         * information before refreshing it */
+        bs->exact_filename[0] = '\0';
+        if (bs->full_open_options) {
+            QDECREF(bs->full_open_options);
+            bs->full_open_options = NULL;
+        }
+
+        drv->bdrv_refresh_filename(bs);
+    } else if (bs->file) {
+        /* Try to reconstruct valid information from the underlying file */
+        bool has_open_options;
+
+        bs->exact_filename[0] = '\0';
+        if (bs->full_open_options) {
+            QDECREF(bs->full_open_options);
+            bs->full_open_options = NULL;
+        }
+
+        opts = qdict_new();
+        has_open_options = append_open_options(opts, bs);
+
+        /* If no specific options have been given for this BDS, the filename of
+         * the underlying file should suffice for this one as well */
+        if (bs->file->exact_filename[0] && !has_open_options) {
+            strcpy(bs->exact_filename, bs->file->exact_filename);
+        }
+        /* Reconstructing the full options QDict is simple for most format block
+         * drivers, as long as the full options are known for the underlying
+         * file BDS. The full options QDict of that file BDS should somehow
+         * contain a representation of the filename, therefore the following
+         * suffices without querying the (exact_)filename of this BDS. */
+        if (bs->file->full_open_options) {
+            qdict_put_obj(opts, "driver",
+                          QOBJECT(qstring_from_str(drv->format_name)));
+            QINCREF(bs->file->full_open_options);
+            qdict_put_obj(opts, "file", QOBJECT(bs->file->full_open_options));
+
+            bs->full_open_options = opts;
+        } else {
+            QDECREF(opts);
+        }
+    } else if (!bs->full_open_options && qdict_size(bs->options)) {
+        /* There is no underlying file BDS (at least referenced by BDS.file),
+         * so the full options QDict should be equal to the options given
+         * specifically for this block device when it was opened (plus the
+         * driver specification).
+         * Because those options don't change, there is no need to update
+         * full_open_options when it's already set. */
+
+        opts = qdict_new();
+        append_open_options(opts, bs);
+        qdict_put_obj(opts, "driver",
+                      QOBJECT(qstring_from_str(drv->format_name)));
+
+        if (bs->exact_filename[0]) {
+            /* This may not work for all block protocol drivers (some may
+             * require this filename to be parsed), but we have to find some
+             * default solution here, so just include it. If some block driver
+             * does not support pure options without any filename at all or
+             * needs some special format of the options QDict, it needs to
+             * implement the driver-specific bdrv_refresh_filename() function.
+             */
+            qdict_put_obj(opts, "filename",
+                          QOBJECT(qstring_from_str(bs->exact_filename)));
+        }
+
+        bs->full_open_options = opts;
+    }
+
+    if (bs->exact_filename[0]) {
+        pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
+    } else if (bs->full_open_options) {
+        QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
+        snprintf(bs->filename, sizeof(bs->filename), "json:%s",
+                 qstring_get_str(json));
+        QDECREF(json);
+    }
+}
diff --git a/include/block/block.h b/include/block/block.h
index ce42cf0..6e1e19d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -274,6 +274,7 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     const char *backing_file);
 int bdrv_get_backing_file_depth(BlockDriverState *bs);
+void bdrv_refresh_filename(BlockDriverState *bs);
 int bdrv_truncate(BlockDriverState *bs, int64_t offset);
 int64_t bdrv_nb_sectors(BlockDriverState *bs);
 int64_t bdrv_getlength(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b420b37..16c70fa 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -123,6 +123,9 @@ struct BlockDriver {
     int (*bdrv_create)(const char *filename, QemuOpts *opts, Error **errp);
     int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
     int (*bdrv_make_empty)(BlockDriverState *bs);
+
+    void (*bdrv_refresh_filename)(BlockDriverState *bs);
+
     /* aio */
     BlockDriverAIOCB *(*bdrv_aio_readv)(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
@@ -336,6 +339,9 @@ struct BlockDriverState {
                                 this file image */
     char backing_format[16]; /* if non-zero and backing_file exists */
 
+    QDict *full_open_options;
+    char exact_filename[1024];
+
     BlockDriverState *backing_hd;
     BlockDriverState *file;
 
-- 
2.0.1

Re: [Qemu-devel] [PATCH 1/6] block: Add bdrv_refresh_filename()

[Kevin Wolf]

Am 18.07.2014 um 20:24 hat Max Reitz geschrieben:
> Some block devices may not have a filename in their BDS; and for some,
> there may not even be a normal filename at all. To work around this, add
> a function which tries to construct a valid filename for the
> BDS.filename field.
> 
> If a filename exists or a block driver is able to reconstruct a valid
> filename (which is placed in BDS.exact_filename), this can directly be
> used.
> 
> If no filename can be constructed, we can still construct an options
> QDict which is then converted to a JSON object and prefixed with the
> "json:" pseudo protocol prefix. The QDict is placed in
> BDS.full_open_options.
> 
> For most block drivers, this process can be done automatically; those
> that need special handling may define a .bdrv_refresh_filename() method
> to fill BDS.exact_filename and BDS.full_open_options themselves.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> In this version, bdrv_refresh_filename() leaves the filename unmodified
> if neither a new filename nor an options QDict can be generated. Another
> idea would be to clear the filename in this case as it is probably
> obsolete then. I was not sure which to pick, so I just used the first
> version I wrote.

To be honest, many things in this patch don't feel quite right. This
isn't necessarily your fault, I can imagine that the infrastructure is
just lacking the right properties for you to use.

My hope is that soon bs->options would be the only BDS field keeping
configuration information and that bs->filename would go away. Now with
this patch series we get both of them duplicated instead. I'm not quite
sure if this is progress, but it may still be an acceptable intermediate
step.

Kevin

Re: [Qemu-devel] [PATCH 1/6] block: Add bdrv_refresh_filename()

[Max Reitz]

On 20.08.2014 17:07, Kevin Wolf wrote:
> Am 18.07.2014 um 20:24 hat Max Reitz geschrieben:
>> Some block devices may not have a filename in their BDS; and for some,
>> there may not even be a normal filename at all. To work around this, add
>> a function which tries to construct a valid filename for the
>> BDS.filename field.
>>
>> If a filename exists or a block driver is able to reconstruct a valid
>> filename (which is placed in BDS.exact_filename), this can directly be
>> used.
>>
>> If no filename can be constructed, we can still construct an options
>> QDict which is then converted to a JSON object and prefixed with the
>> "json:" pseudo protocol prefix. The QDict is placed in
>> BDS.full_open_options.
>>
>> For most block drivers, this process can be done automatically; those
>> that need special handling may define a .bdrv_refresh_filename() method
>> to fill BDS.exact_filename and BDS.full_open_options themselves.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> In this version, bdrv_refresh_filename() leaves the filename unmodified
>> if neither a new filename nor an options QDict can be generated. Another
>> idea would be to clear the filename in this case as it is probably
>> obsolete then. I was not sure which to pick, so I just used the first
>> version I wrote.
> To be honest, many things in this patch don't feel quite right. This
> isn't necessarily your fault, I can imagine that the infrastructure is
> just lacking the right properties for you to use.
>
> My hope is that soon bs->options would be the only BDS field keeping
> configuration information and that bs->filename would go away. Now with
> this patch series we get both of them duplicated instead. I'm not quite
> sure if this is progress, but it may still be an acceptable intermediate
> step.

This code just needs some way to cache the filename and I thought it 
fine to use the filename field for that purpose. If we remove it, we'll 
have to reconstruct it every time (recursively through all the BDS 
layers) when someone needs it.

We may be able to cull many such places (where the filename is needed), 
but considering the processing time, I don't think it will get any 
better than using a cache array (such as BDS.filename).

So for me, the advantages of dumping BDS.filename would be: We don't 
have to consider when the filename field needs to an update; we save 
some memory (negligible, imho).

The advantages of keeping it, on the other hand, are: It's easy to read 
the filename (no need to change existing code); we may save some 
processing time (probably also negligible, if done right).

After considering this, I guess we'd have to look at all the places 
which use BDS.filename, how many of those we can cull and how often the 
rest is invoked. If BDS.filename is only rarely really needed, we can 
really remove it and fully replace it by a callback (which basically is 
bdrv_refresh_filename()).

Max

Re: [Qemu-devel] [PATCH 1/6] block: Add bdrv_refresh_filename()

[Kevin Wolf]

Am 20.08.2014 um 21:06 hat Max Reitz geschrieben:
> On 20.08.2014 17:07, Kevin Wolf wrote:
> >Am 18.07.2014 um 20:24 hat Max Reitz geschrieben:
> >>Some block devices may not have a filename in their BDS; and for some,
> >>there may not even be a normal filename at all. To work around this, add
> >>a function which tries to construct a valid filename for the
> >>BDS.filename field.
> >>
> >>If a filename exists or a block driver is able to reconstruct a valid
> >>filename (which is placed in BDS.exact_filename), this can directly be
> >>used.
> >>
> >>If no filename can be constructed, we can still construct an options
> >>QDict which is then converted to a JSON object and prefixed with the
> >>"json:" pseudo protocol prefix. The QDict is placed in
> >>BDS.full_open_options.
> >>
> >>For most block drivers, this process can be done automatically; those
> >>that need special handling may define a .bdrv_refresh_filename() method
> >>to fill BDS.exact_filename and BDS.full_open_options themselves.
> >>
> >>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>---
> >>In this version, bdrv_refresh_filename() leaves the filename unmodified
> >>if neither a new filename nor an options QDict can be generated. Another
> >>idea would be to clear the filename in this case as it is probably
> >>obsolete then. I was not sure which to pick, so I just used the first
> >>version I wrote.
> >To be honest, many things in this patch don't feel quite right. This
> >isn't necessarily your fault, I can imagine that the infrastructure is
> >just lacking the right properties for you to use.
> >
> >My hope is that soon bs->options would be the only BDS field keeping
> >configuration information and that bs->filename would go away. Now with
> >this patch series we get both of them duplicated instead. I'm not quite
> >sure if this is progress, but it may still be an acceptable intermediate
> >step.
> 
> This code just needs some way to cache the filename and I thought it
> fine to use the filename field for that purpose. If we remove it,
> we'll have to reconstruct it every time (recursively through all the
> BDS layers) when someone needs it.

Hm. Thinking more about it, the part that I really dislike isn't even
that bs->filename exists and is used as a cache. It's just that the
filename argument of bdrv_open() is used to initialise it instead of
only using the options QDict. But that's an independent problem that
isn't made worse by this series.

I guess it's time to dig out the next part of my bdrv_open() series and
complete that work...

> We may be able to cull many such places (where the filename is
> needed), but considering the processing time, I don't think it will
> get any better than using a cache array (such as BDS.filename).
> 
> So for me, the advantages of dumping BDS.filename would be: We don't
> have to consider when the filename field needs to an update; we save
> some memory (negligible, imho).
> 
> The advantages of keeping it, on the other hand, are: It's easy to
> read the filename (no need to change existing code); we may save
> some processing time (probably also negligible, if done right).
> 
> After considering this, I guess we'd have to look at all the places
> which use BDS.filename, how many of those we can cull and how often
> the rest is invoked. If BDS.filename is only rarely really needed,
> we can really remove it and fully replace it by a callback (which
> basically is bdrv_refresh_filename()).

Most of it is probably in monitor command implementations.

Kevin

[Qemu-devel] [PATCH 2/6] blkdebug: Implement bdrv_refresh_filename()

[Max Reitz]

Because blkdebug cannot simply create a configuration file, simply
refuse to reconstruct a plain filename and only generate an options
QDict from the rules instead.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
Instead of this rather complicated implementation, we could decide to
just drop it and let this be handled by the default implementation. The
default implementation however cannot generate full_open_options in case
a configuration file was given; in that case, it would just return the
filename containing the name of the configuration file. On the other
hand, blkdebug is just a debug driver anyway, so it probably wouldn't
hurt too much.
---
 block/blkdebug.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index f51407d..8674d0c 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -26,6 +26,10 @@
 #include "qemu/config-file.h"
 #include "block/block_int.h"
 #include "qemu/module.h"
+#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qint.h"
+#include "qapi/qmp/qstring.h"
 
 typedef struct BDRVBlkdebugState {
     int state;
@@ -687,6 +691,98 @@ static int64_t blkdebug_getlength(BlockDriverState *bs)
     return bdrv_getlength(bs->file);
 }
 
+static void blkdebug_refresh_filename(BlockDriverState *bs)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    struct BlkdebugRule *rule;
+    QDict *opts;
+    QList *inject_error_list = NULL, *set_state_list = NULL;
+    QList *suspend_list = NULL;
+    int event;
+
+    if (!bs->file->full_open_options) {
+        /* The config file cannot be recreated, so creating a plain filename
+         * is impossible */
+        return;
+    }
+
+    opts = qdict_new();
+    qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("blkdebug")));
+
+    QINCREF(bs->file->full_open_options);
+    qdict_put_obj(opts, "image", QOBJECT(bs->file->full_open_options));
+
+    for (event = 0; event < BLKDBG_EVENT_MAX; event++) {
+        QLIST_FOREACH(rule, &s->rules[event], next) {
+            if (rule->action == ACTION_INJECT_ERROR) {
+                QDict *inject_error = qdict_new();
+
+                qdict_put_obj(inject_error, "event", QOBJECT(qstring_from_str(
+                              BlkdebugEvent_lookup[rule->event])));
+                qdict_put_obj(inject_error, "state",
+                              QOBJECT(qint_from_int(rule->state)));
+                qdict_put_obj(inject_error, "errno", QOBJECT(qint_from_int(
+                              rule->options.inject.error)));
+                qdict_put_obj(inject_error, "sector", QOBJECT(qint_from_int(
+                              rule->options.inject.sector)));
+                qdict_put_obj(inject_error, "once", QOBJECT(qbool_from_int(
+                              rule->options.inject.once)));
+                qdict_put_obj(inject_error, "immediately",
+                              QOBJECT(qbool_from_int(
+                              rule->options.inject.immediately)));
+
+                if (!inject_error_list) {
+                    inject_error_list = qlist_new();
+                }
+
+                qlist_append_obj(inject_error_list, QOBJECT(inject_error));
+            } else if (rule->action == ACTION_SET_STATE) {
+                QDict *set_state = qdict_new();
+
+                qdict_put_obj(set_state, "event", QOBJECT(qstring_from_str(
+                              BlkdebugEvent_lookup[rule->event])));
+                qdict_put_obj(set_state, "state",
+                              QOBJECT(qint_from_int(rule->state)));
+                qdict_put_obj(set_state, "new_state", QOBJECT(qint_from_int(
+                              rule->options.set_state.new_state)));
+
+                if (!set_state_list) {
+                    set_state_list = qlist_new();
+                }
+
+                qlist_append_obj(set_state_list, QOBJECT(set_state));
+            } else if (rule->action == ACTION_SUSPEND) {
+                QDict *suspend = qdict_new();
+
+                qdict_put_obj(suspend, "event", QOBJECT(qstring_from_str(
+                              BlkdebugEvent_lookup[rule->event])));
+                qdict_put_obj(suspend, "state",
+                              QOBJECT(qint_from_int(rule->state)));
+                qdict_put_obj(suspend, "tag", QOBJECT(qstring_from_str(
+                              rule->options.suspend.tag)));
+
+                if (!suspend_list) {
+                    suspend_list = qlist_new();
+                }
+
+                qlist_append_obj(suspend_list, QOBJECT(suspend));
+            }
+        }
+    }
+
+    if (inject_error_list) {
+        qdict_put_obj(opts, "inject-error", QOBJECT(inject_error_list));
+    }
+    if (set_state_list) {
+        qdict_put_obj(opts, "set-state", QOBJECT(set_state_list));
+    }
+    if (suspend_list) {
+        qdict_put_obj(opts, "suspend", QOBJECT(suspend_list));
+    }
+
+    bs->full_open_options = opts;
+}
+
 static BlockDriver bdrv_blkdebug = {
     .format_name            = "blkdebug",
     .protocol_name          = "blkdebug",
@@ -696,6 +792,7 @@ static BlockDriver bdrv_blkdebug = {
     .bdrv_file_open         = blkdebug_open,
     .bdrv_close             = blkdebug_close,
     .bdrv_getlength         = blkdebug_getlength,
+    .bdrv_refresh_filename  = blkdebug_refresh_filename,
 
     .bdrv_aio_readv         = blkdebug_aio_readv,
     .bdrv_aio_writev        = blkdebug_aio_writev,
-- 
2.0.1

Re: [Qemu-devel] [PATCH 2/6] blkdebug: Implement bdrv_refresh_filename()

[Kevin Wolf]

Am 18.07.2014 um 20:24 hat Max Reitz geschrieben:
> Because blkdebug cannot simply create a configuration file, simply
> refuse to reconstruct a plain filename and only generate an options
> QDict from the rules instead.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> Instead of this rather complicated implementation, we could decide to
> just drop it and let this be handled by the default implementation. The
> default implementation however cannot generate full_open_options in case
> a configuration file was given; in that case, it would just return the
> filename containing the name of the configuration file. On the other
> hand, blkdebug is just a debug driver anyway, so it probably wouldn't
> hurt too much.

I think you really want to have the config file name used here for
options that were loaded from a config file, and the explicit JSON
notation only for options explicitly specified on the command line or in
blockdev-add.

We still only have 1024 characters for bs->filename... ;-)

Kevin

Re: [Qemu-devel] [PATCH 2/6] blkdebug: Implement bdrv_refresh_filename()

[Max Reitz]

On 20.08.2014 17:14, Kevin Wolf wrote:
> Am 18.07.2014 um 20:24 hat Max Reitz geschrieben:
>> Because blkdebug cannot simply create a configuration file, simply
>> refuse to reconstruct a plain filename and only generate an options
>> QDict from the rules instead.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> Instead of this rather complicated implementation, we could decide to
>> just drop it and let this be handled by the default implementation. The
>> default implementation however cannot generate full_open_options in case
>> a configuration file was given; in that case, it would just return the
>> filename containing the name of the configuration file. On the other
>> hand, blkdebug is just a debug driver anyway, so it probably wouldn't
>> hurt too much.
> I think you really want to have the config file name used here for
> options that were loaded from a config file, and the explicit JSON
> notation only for options explicitly specified on the command line or in
> blockdev-add.
>
> We still only have 1024 characters for bs->filename... ;-)

But who uses blkdebug anyway. *g*

I'll take a look into this, along with whether we can minimize use of 
BDS.filename enough to justify completely removing it.

Max

[Qemu-devel] [PATCH 3/6] blkverify: Implement bdrv_refresh_filename()

[Max Reitz]

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

diff --git a/block/blkverify.c b/block/blkverify.c
index 621b785..7c78ca4 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -10,6 +10,8 @@
 #include <stdarg.h>
 #include "qemu/sockets.h" /* for EINPROGRESS on Windows */
 #include "block/block_int.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qstring.h"
 
 typedef struct {
     BlockDriverState *test_file;
@@ -320,6 +322,32 @@ static void blkverify_attach_aio_context(BlockDriverState *bs,
     bdrv_attach_aio_context(s->test_file, new_context);
 }
 
+static void blkverify_refresh_filename(BlockDriverState *bs)
+{
+    BDRVBlkverifyState *s = bs->opaque;
+
+    /* bs->file has already been refreshed */
+    bdrv_refresh_filename(s->test_file);
+
+    if (bs->file->full_open_options && s->test_file->full_open_options) {
+        QDict *opts = qdict_new();
+        qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("blkverify")));
+
+        QINCREF(bs->file->full_open_options);
+        qdict_put_obj(opts, "raw", QOBJECT(bs->file->full_open_options));
+        QINCREF(s->test_file->full_open_options);
+        qdict_put_obj(opts, "test", QOBJECT(s->test_file->full_open_options));
+
+        bs->full_open_options = opts;
+    }
+
+    if (bs->file->exact_filename[0] && s->test_file->exact_filename[0]) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "blkverify:%s:%s",
+                 bs->file->exact_filename, s->test_file->exact_filename);
+    }
+}
+
 static BlockDriver bdrv_blkverify = {
     .format_name                      = "blkverify",
     .protocol_name                    = "blkverify",
@@ -329,6 +357,7 @@ static BlockDriver bdrv_blkverify = {
     .bdrv_file_open                   = blkverify_open,
     .bdrv_close                       = blkverify_close,
     .bdrv_getlength                   = blkverify_getlength,
+    .bdrv_refresh_filename            = blkverify_refresh_filename,
 
     .bdrv_aio_readv                   = blkverify_aio_readv,
     .bdrv_aio_writev                  = blkverify_aio_writev,
-- 
2.0.1

[Qemu-devel] [PATCH 4/6] nbd: Implement bdrv_refresh_filename()

[Max Reitz]

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

diff --git a/block/nbd.c b/block/nbd.c
index 4eda095..89775e1 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -31,8 +31,10 @@
 #include "block/block_int.h"
 #include "qemu/module.h"
 #include "qemu/sockets.h"
+#include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qint.h"
+#include "qapi/qmp/qstring.h"
 
 #include <sys/types.h>
 #include <unistd.h>
@@ -338,6 +340,37 @@ static void nbd_attach_aio_context(BlockDriverState *bs,
     nbd_client_session_attach_aio_context(&s->client, new_context);
 }
 
+static void nbd_refresh_filename(BlockDriverState *bs)
+{
+    BDRVNBDState *s = bs->opaque;
+    QDict *opts = qdict_new();
+    const char *path   = qemu_opt_get(s->socket_opts, "path");
+    const char *host   = qemu_opt_get(s->socket_opts, "host");
+    const char *port   = qemu_opt_get(s->socket_opts, "port");
+    const char *export = qemu_opt_get(s->socket_opts, "export");
+
+    qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nbd")));
+
+    if (path) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nbd+unix:%s", path);
+        qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(path)));
+    } else if (export) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nbd:%s:%s/%s", host, port, export);
+        qdict_put_obj(opts, "host",   QOBJECT(qstring_from_str(host)));
+        qdict_put_obj(opts, "port",   QOBJECT(qstring_from_str(port)));
+        qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(export)));
+    } else {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nbd:%s:%s", host, port);
+        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(host)));
+        qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(port)));
+    }
+
+    bs->full_open_options = opts;
+}
+
 static BlockDriver bdrv_nbd = {
     .format_name                = "nbd",
     .protocol_name              = "nbd",
@@ -352,6 +385,7 @@ static BlockDriver bdrv_nbd = {
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
+    .bdrv_refresh_filename      = nbd_refresh_filename,
 };
 
 static BlockDriver bdrv_nbd_tcp = {
@@ -368,6 +402,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
+    .bdrv_refresh_filename      = nbd_refresh_filename,
 };
 
 static BlockDriver bdrv_nbd_unix = {
@@ -384,6 +419,7 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
+    .bdrv_refresh_filename      = nbd_refresh_filename,
 };
 
 static void bdrv_nbd_init(void)
-- 
2.0.1

[Qemu-devel] [PATCH 5/6] quorum: Implement bdrv_refresh_filename()

[Max Reitz]

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

diff --git a/block/quorum.c b/block/quorum.c
index d5ee9c0..0de07bb 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -16,7 +16,12 @@
 #include <gnutls/gnutls.h>
 #include <gnutls/crypto.h>
 #include "block/block_int.h"
+#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qint.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qstring.h"
 #include "qapi-event.h"
 
 #define HASH_LENGTH 32
@@ -945,6 +950,39 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
     }
 }
 
+static void quorum_refresh_filename(BlockDriverState *bs)
+{
+    BDRVQuorumState *s = bs->opaque;
+    QDict *opts;
+    QList *children;
+    int i;
+
+    for (i = 0; i < s->num_children; i++) {
+        bdrv_refresh_filename(s->bs[i]);
+        if (!s->bs[i]->full_open_options) {
+            return;
+        }
+    }
+
+    children = qlist_new();
+    for (i = 0; i < s->num_children; i++) {
+        QINCREF(s->bs[i]->full_open_options);
+        qlist_append_obj(children, QOBJECT(s->bs[i]->full_open_options));
+    }
+
+    opts = qdict_new();
+    qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("quorum")));
+    qdict_put_obj(opts, QUORUM_OPT_VOTE_THRESHOLD,
+                  QOBJECT(qint_from_int(s->threshold)));
+    qdict_put_obj(opts, QUORUM_OPT_BLKVERIFY,
+                  QOBJECT(qbool_from_int(s->is_blkverify)));
+    qdict_put_obj(opts, QUORUM_OPT_REWRITE,
+                  QOBJECT(qbool_from_int(s->rewrite_corrupted)));
+    qdict_put_obj(opts, "children", QOBJECT(children));
+
+    bs->full_open_options = opts;
+}
+
 static BlockDriver bdrv_quorum = {
     .format_name                        = "quorum",
     .protocol_name                      = "quorum",
@@ -953,6 +991,7 @@ static BlockDriver bdrv_quorum = {
 
     .bdrv_file_open                     = quorum_open,
     .bdrv_close                         = quorum_close,
+    .bdrv_refresh_filename              = quorum_refresh_filename,
 
     .bdrv_co_flush_to_disk              = quorum_co_flush,
 
-- 
2.0.1

[Qemu-devel] [PATCH 6/6] iotests: Add test for image filename construction

[Max Reitz]

Testing a real in-use protocol such as NBD is hard; testing blkdebug and
blkverify in its stead is easier and tests basically the same
functionality.

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

diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
new file mode 100755
index 0000000..a26d3d2
--- /dev/null
+++ b/tests/qemu-iotests/099
@@ -0,0 +1,116 @@
+#!/bin/bash
+#
+# Test valid filenames for blkdebug and blkverify representatively for
+# other protocols (such as NBD) when queried
+#
+# 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
+
+# Basically all formats, but "raw" has issues with _filter_imgfmt regarding the
+# raw comparison image for blkverify; also, all images have to support creation
+_supported_fmt cow qcow qcow2 qed vdi vhdx vmdk vpc
+_supported_proto file
+_supported_os Linux
+
+
+function do_run_qemu()
+{
+    $QEMU -nographic -qmp stdio -serial none "$@"
+}
+
+function run_qemu()
+{
+    # Get the "file": "foo" entry ($foo may only contain escaped double quotes,
+    # which is how we can extract it)
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_imgfmt | _filter_qmp \
+        | grep "drv0" \
+        | sed -e 's/^.*"file": "\(\(\\"\|[^"]\)*\)".*$/\1/' -e 's/\\"/"/g'
+}
+
+function test_qemu()
+{
+    run_qemu -drive "if=none,id=drv0,$1" <<EOF
+        { 'execute': 'qmp_capabilities' }
+        { 'execute': 'query-block' }
+        { 'execute': 'quit' }
+EOF
+}
+
+
+
+IMG_SIZE=128K
+
+_make_test_img $IMG_SIZE
+$QEMU_IMG create -f raw "$TEST_IMG.compare" $IMG_SIZE \
+    | _filter_testdir | _filter_imgfmt
+
+echo
+echo '=== Testing simple filename for blkverify ==='
+echo
+
+# This should return simply the filename itself
+test_qemu "file=blkverify:$TEST_IMG.compare:$TEST_IMG"
+
+echo
+echo '=== Testing filename reconstruction for blkverify ==='
+echo
+
+# This should return the same filename as above
+test_qemu "file.driver=blkverify,file.raw.filename=$TEST_IMG.compare,file.test.file.filename=$TEST_IMG"
+
+echo
+echo '=== Testing JSON filename for blkdebug ==='
+echo
+
+# blkdebug cannot create a configuration file, therefore it is unable to
+# generate a plain filename here; thus this should return a JSON filename
+test_qemu "file.driver=blkdebug,file.image.filename=$TEST_IMG,file.inject-error.0.event=l1_update"
+
+echo
+echo '=== Testing indirectly enforced JSON filename ==='
+echo
+
+# Because blkdebug cannot return a plain filename, blkverify is forced to
+# generate a JSON object here as well
+test_qemu "file.driver=blkverify,file.raw.filename=$TEST_IMG.compare,file.test.file.driver=blkdebug,file.test.file.image.filename=$TEST_IMG,file.test.file.inject-error.0.event=l1_update"
+
+
+rm -f "$TEST_IMG.compare"
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/099.out b/tests/qemu-iotests/099.out
new file mode 100644
index 0000000..55be4d4
--- /dev/null
+++ b/tests/qemu-iotests/099.out
@@ -0,0 +1,20 @@
+QA output created by 099
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 
+Formatting 'TEST_DIR/t.IMGFMT.compare', fmt=raw size=131072 
+
+=== Testing simple filename for blkverify ===
+
+blkverify:TEST_DIR/t.IMGFMT.compare:TEST_DIR/t.IMGFMT
+
+=== Testing filename reconstruction for blkverify ===
+
+blkverify:TEST_DIR/t.IMGFMT.compare:TEST_DIR/t.IMGFMT
+
+=== Testing JSON filename for blkdebug ===
+
+json:{"driver": "IMGFMT", "file": {"inject-error": [{"immediately": false, "once": false, "state": 0, "sector": -1, "event": "l1_update", "errno": 5}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}}
+
+=== Testing indirectly enforced JSON filename ===
+
+json:{"driver": "raw", "file": {"test": {"driver": "IMGFMT", "file": {"inject-error": [{"immediately": false, "once": false, "state": 0, "sector": -1, "event": "l1_update", "errno": 5}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}}, "driver": "blkverify", "raw": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.compare"}}}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 6e67f61..d894d16 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -100,3 +100,4 @@
 091 rw auto quick
 092 rw auto quick
 095 rw auto quick
+099 rw auto quick
-- 
2.0.1

Re: [Qemu-devel] [PATCH 0/6] block: Let drivers reconstruct the filename

[Max Reitz]

On 18.07.2014 20:24, Max Reitz wrote:
> We may sometimes want a filename for BDSs which do not have one because
> they weren't opened using a simple filename (but rather just through
> options, for example).
>
> Some block drivers are always capable of reconstructing a valid filename
> (e.g. NBD), even if the BDS has been opened using the options QDict
> only. For others (e.g. Quorum) this is impossible. To accommodate this
> case, the function which reconstructs ("refreshes") the filename for a
> BDS also generates an options QDict (which should always work). If some
> layer cannot generate a plain filename (e.g. a Quorum instance), we can
> still generate a QDict which contains all options necessary for opening
> the block device in (basically) the same state.
>
> If a filename can be generated, the one stored in the BDS is
> overwritten. Otherwise, the QDict is converted to JSON, prefixed with
> "json:" and then used as the filename (making use of the JSON
> pseudo-protocol).
>
>
> Block drivers which probably need to implement bdrv_refresh_filename()
> besides blkdebug, blkverify, NBD and Quorum but which this series does
> not cover, are the following: curl, ssh and vvfat.
>
>
> This series supersedes my previous 'block: Fix unset "filename" for
> certain drivers'.
>
>
> Max Reitz (6):
>    block: Add bdrv_refresh_filename()
>    blkdebug: Implement bdrv_refresh_filename()
>    blkverify: Implement bdrv_refresh_filename()
>    nbd: Implement bdrv_refresh_filename()
>    quorum: Implement bdrv_refresh_filename()
>    iotests: Add test for image filename construction
>
>   block.c                    | 135 +++++++++++++++++++++++++++++++++++++++++++++
>   block/blkdebug.c           |  97 ++++++++++++++++++++++++++++++++
>   block/blkverify.c          |  29 ++++++++++
>   block/nbd.c                |  36 ++++++++++++
>   block/quorum.c             |  39 +++++++++++++
>   include/block/block.h      |   1 +
>   include/block/block_int.h  |   6 ++
>   tests/qemu-iotests/099     | 116 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/099.out |  20 +++++++
>   tests/qemu-iotests/group   |   1 +
>   10 files changed, 480 insertions(+)
>   create mode 100755 tests/qemu-iotests/099
>   create mode 100644 tests/qemu-iotests/099.out

Ping

Re: [Qemu-devel] [PATCH 0/6] block: Let drivers reconstruct the filename

[Kevin Wolf]

Am 18.07.2014 um 20:24 hat Max Reitz geschrieben:
> We may sometimes want a filename for BDSs which do not have one because
> they weren't opened using a simple filename (but rather just through
> options, for example).
> 
> Some block drivers are always capable of reconstructing a valid filename
> (e.g. NBD), even if the BDS has been opened using the options QDict
> only. For others (e.g. Quorum) this is impossible. To accommodate this
> case, the function which reconstructs ("refreshes") the filename for a
> BDS also generates an options QDict (which should always work). If some
> layer cannot generate a plain filename (e.g. a Quorum instance), we can
> still generate a QDict which contains all options necessary for opening
> the block device in (basically) the same state.
> 
> If a filename can be generated, the one stored in the BDS is
> overwritten. Otherwise, the QDict is converted to JSON, prefixed with
> "json:" and then used as the filename (making use of the JSON
> pseudo-protocol).
> 
> 
> Block drivers which probably need to implement bdrv_refresh_filename()
> besides blkdebug, blkverify, NBD and Quorum but which this series does
> not cover, are the following: curl, ssh and vvfat.

Thanks, applied all to the block branch.

I don't think we're done yet. This series leads to some strange looking
things both in the source and in the output. I merged it anyway because
I think it does provide some value and we can fix the problems
incrementally.

Kevin