* Max Reitz <mre...@redhat.com> [2017-03-27 17:27:16 +0200]:
> 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 <bjsdj...@linux.vnet.ibm.com>
> > ---
> > 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.
>
> Compare:
>
> $ ./qemu-img snapshot -a foo
> "json:{'driver':'raw','file':{'driver':'qcow2','file':{'driver':'file','filename':'foo.qcow2'}}}"
> qemu-img: Cannot reference an existing block device with additional
> options or a new filename
>
> $ ./qemu-img snapshot -a foo --image-opts
> driver=raw,file.driver=qcow2,file.file.driver=file,file.file.filename=foo.qcow2
> 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
Hi Max,
You are right. Using your commands, I can easily reproduce the segfaults
problem.
> 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
> bdrv_open_child().
>
> Now if that bdrv_open_child() fails, the field becomes NULL, the old
> child is leaked and the above bdrv_unref() fails.
Nod.
>
> 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:
I definitely will listen to your suggestions on this.
>
> (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).
:> You must mean:
bdrv_unref_child(bs, bs->file);
>
> I guess it should work with those changes. At least I hope it will.
Yes, with my test results, it works!
>
> By the way, the easiest way to do (1) is probably:
>
> QDict *file_options;
>
> qdict_extract_subqdict(options, &file_options, "file.");
> QDECREF(file_options);
Cool. I adopted your code and it works well.
Thanks for these very detailed analysis and the code example.
Mind if I add a:
Suggested-by: Max Reitz <mre...@redhat.com>
in the following version?
>
>
> Max
>
> > bs->drv = NULL;
> >
>
>
--
Dong Jia Shi
