[Top][All Lists]

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

Re: [Qemu-block] [PATCH RFC v2 1/1] block: pass the right options for Bl

From: Max Reitz
Subject: Re: [Qemu-block] [PATCH RFC v2 1/1] block: pass the right options for BlockDriver.bdrv_open
Date: Mon, 27 Mar 2017 17:27:16 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

On 27.03.2017 05:05, Dong Jia Shi wrote:
> raw_open() expects the caller always passing in the right actual
> @options parameter. But when trying to applying snapshot on a RBD
> image, bdrv_snapshot_goto() calls raw_open() (by calling the
> bdrv_open callback on the BlockDriver) with a NULL @options, and
> that will result in a Segmentation fault.
> For the other non-raw format drivers, it also make sense to passing
> in the actual options, althought they don't trigger the problem so
> far.
> Let's prepare a @options by adding the "file" key-value pair to a
> copy of the actual options that were given for the node (i.e.
> bs->options), and pass it to the callback.
> Signed-off-by: Dong Jia Shi <address@hidden>
> ---
>  block/snapshot.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> diff --git a/block/snapshot.c b/block/snapshot.c
> index bf5c2ca..dfec139 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -27,6 +27,7 @@
>  #include "block/block_int.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
> +#include "qapi/qmp/qstring.h"
>  QemuOptsList internal_snapshot_opts = {
>      .name = "snapshot",
> @@ -189,9 +190,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>      }
>      if (bs->file) {
> +        QDict *options = qdict_clone_shallow(bs->options);
> +        qdict_put(options, "file",
> +                  qstring_from_str(bdrv_get_node_name(bs->file->bs)));

I don't think you're guaranteed that "file" is a a nested QDict (if it
has been specified as a nested dict). Then you'd have both options like
"file.driver" and "file" here which will break later on.


$ ./qemu-img snapshot -a foo
qemu-img: Cannot reference an existing block device with additional
options or a new filename

$ ./qemu-img snapshot -a foo --image-opts
qemu-img: Cannot reference an existing block device with additional
options or a new filename

By the way, afterwards both commands segfault (and I had to add a manual
error_report_err() in this function to see the error message because
normally this just passes NULL for errp...). The segfault comes from...

> +
>          drv->bdrv_close(bs);
>          ret = bdrv_snapshot_goto(bs->file->bs, snapshot_id);
> -        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags, NULL);
> +        open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
> +        QDECREF(options);
>          if (open_ret < 0) {
>              bdrv_unref(bs->file->bs);

...here. bs->file is NULL. This is because BlockDriver.bdrv_open()
expects bs->file to be NULL and just overwrites it with the result from

Now if that bdrv_open_child() fails, the field becomes NULL, the old
child is leaked and the above bdrv_unref() fails.

I get that this is never supposed to happen because bdrv_open_child()
should always succeed with the node name of the existing BDS given as a
reference... but we shouldn't rely on that, I guess.

All of this is a horrible mess. It kind of worked in the past when all
of this bdrv_open()/bdrv_close() stuff was a horrible mess but now it
stands out as exceptionally ugly (and obviously broken).

Of course, we can't fix all of this for 2.9, so what I'd propose for
this patch is:

(1) Remove every option in "options" that has a "file." prefix before
the qdict_put() call.

(2) Use bdrv_unref_child(bs->file) instead of bdrv_unref(bs->file->bs).

I guess it should work with those changes. At least I hope it will.

By the way, the easiest way to do (1) is probably:

QDict *file_options;

qdict_extract_subqdict(options, &file_options, "file.");


>              bs->drv = NULL;

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

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