Re: [Qemu-devel] [PATCH RFC v2 1/1] block: pass the right options for BlockDriver.bdrv_open

[Dong Jia Shi]

* Max Reitz <address@hidden> [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 <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.
> 
> 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 <address@hidden>
in the following version?

> 
> 
> Max
> 
> >              bs->drv = NULL;
> > 
> 
> 




-- 
Dong Jia Shi