[Qemu-devel] [PATCH RFC 0/1] block: Handle NULL options correctly in raw_open

[Qemu-devel] [PATCH RFC 0/1] block: Handle NULL options correctly in raw_open

[Dong Jia Shi]

Trying to restore rbd image on ceph cluster from snapshot with
qemu-img could trigger a calling to raw_open with a NULL @options
after a calling to raw_close, and that will lead to a failure of
the snapshot applying.

[root@s8345007 ~]# gdb --args qemu-img snapshot -a snap1 rbd:test_pool/dj_image
... ...
Program received signal SIGSEGV, Segmentation fault.
0x00000000801395a8 in qdict_next_entry (qdict=0x0, first_bucket=0) at qobject/qdict.c:327
327	        if (!QLIST_EMPTY(&qdict->table[i])) {
(gdb) bt
#0  0x00000000801395a8 in qdict_next_entry (qdict=0x0, first_bucket=0) at qobject/qdict.c:327
#1  0x0000000080139626 in qdict_first (qdict=0x0) at qobject/qdict.c:340
#2  0x000000008013a00c in qdict_extract_subqdict (src=0x0, dst=0x3ffffffec50, start=0x80698260 "file.")
    at qobject/qdict.c:576
#3  0x0000000080019c26 in bdrv_open_child_bs (filename=0x0, options=0x0, bdref_key=0x8017ab38 "file", 
    parent=0x80630300, child_role=0x80176108 <child_file>, allow_none=false, errp=0x0) at block.c:2018
#4  0x0000000080019dfa in bdrv_open_child (filename=0x0, options=0x0, bdref_key=0x8017ab38 "file", 
    parent=0x80630300, child_role=0x80176108 <child_file>, allow_none=false, errp=0x0) at block.c:2065
#5  0x000000008002b9a0 in raw_open (bs=0x80630300, options=0x0, flags=8194, errp=0x0) at block/raw-format.c:387
#6  0x0000000080087516 in bdrv_snapshot_goto (bs=0x80630300, snapshot_id=0x3fffffff75c "snap1")
    at block/snapshot.c:194
#7  0x0000000080010b8c in img_snapshot (argc=4, argv=0x3fffffff4c0) at qemu-img.c:2937
#8  0x00000000800140e4 in main (argc=4, argv=0x3fffffff4c0) at qemu-img.c:4373

The problematic caller of raw_open is block/snapshot.c:194:
178 int bdrv_snapshot_goto(BlockDriverState *bs,
179                        const char *snapshot_id)
180 {
181     BlockDriver *drv = bs->drv;
182     int ret, open_ret;
183 
184     if (!drv) {
185         return -ENOMEDIUM;
186     }
187     if (drv->bdrv_snapshot_goto) {
188         return drv->bdrv_snapshot_goto(bs, snapshot_id);
189     }
190 
191     if (bs->file) {
192         drv->bdrv_close(bs);
193         ret = bdrv_snapshot_goto(bs->file->bs, snapshot_id);
194         open_ret = drv->bdrv_open(bs, NULL, bs->open_flags, NULL);
195         if (open_ret < 0) {
196             bdrv_unref(bs->file->bs);
197             bs->drv = NULL;
198             return open_ret;
199         }
200         return ret;
201     }
202 
203     return -ENOTSUP;
204 }

AFAIU, that's because raw_open does not take NULL @options as a
legal parameter. @options should have a "file" key-value pair to
ensure a success open.

I'm not familiar with these code, but after reading them for a
while, I think that raw_close always does nothing, so it seems to
me that a raw_open after a raw_close probably should also do nothing
as well.

I admit that there may be more than one way to detect whether
raw_open is called after raw_close. E.g. introducing a flag to
indicate whether the current BDS is closed, or try to reuse some
existing fields in BDS. But taking NULL @options as a sign of
trying to open again could be the simplest working method jumped
into my mind.

I'd like to send this as a RFC here for more advice, in case I
walked along a wrong way to far. Comments are welcome.

Dong Jia Shi (1):
  block: Handle NULL options correctly in raw_open

 block/raw-format.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.8.4

[Qemu-devel] [PATCH RFC 1/1] block: Handle NULL options correctly in raw_open

[Dong Jia Shi]

A normal call for raw_open should always pass in a non-NULL @options,
but for some certain cases (e.g. trying to applying snapshot on a RBD
image), they call raw_open with a NULL @options right after the calling
for raw_close.

Let's take the NULL @options as a sign of trying to do raw_open again,
and just simply return a success code.

Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
 block/raw-format.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/raw-format.c b/block/raw-format.c
index 86fbc65..ee05730 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -384,6 +384,14 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVRawState *s = bs->opaque;
     int ret;
 
+    /*
+     * Notice:
+     * NULL options is only sensible when applying a snapshot.
+     */
+    if (!options) {
+        return 0;
+    }
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {
-- 
2.8.4

Re: [Qemu-devel] [PATCH RFC 1/1] block: Handle NULL options correctly in raw_open

[Kevin Wolf]

Am 08.03.2017 um 03:15 hat Dong Jia Shi geschrieben:
> A normal call for raw_open should always pass in a non-NULL @options,
> but for some certain cases (e.g. trying to applying snapshot on a RBD
> image), they call raw_open with a NULL @options right after the calling
> for raw_close.
> 
> Let's take the NULL @options as a sign of trying to do raw_open again,
> and just simply return a success code.
> 
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

I think we rather need to fix bdrv_snapshot_goto() so that it doesn't
pass NULL, but the actual options that were given for the node (i.e.
bs->options).

Kevin

Re: [Qemu-devel] [PATCH RFC 1/1] block: Handle NULL options correctly in raw_open

[Dong Jia Shi]

* Kevin Wolf <kwolf@redhat.com> [2017-03-08 10:13:46 +0100]:

> Am 08.03.2017 um 03:15 hat Dong Jia Shi geschrieben:
> > A normal call for raw_open should always pass in a non-NULL @options,
> > but for some certain cases (e.g. trying to applying snapshot on a RBD
> > image), they call raw_open with a NULL @options right after the calling
> > for raw_close.
> > 
> > Let's take the NULL @options as a sign of trying to do raw_open again,
> > and just simply return a success code.
> > 
> > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> 
> I think we rather need to fix bdrv_snapshot_goto() so that it doesn't
> pass NULL, but the actual options that were given for the node (i.e.
> bs->options).
I've tried that before the current try. bs->options does not have the
"file" key-value pair, so that leads to a fail too. Should we put "file"
in to the options manually? I noticed that it was removed from
bs->options during the calling of bdrv_open_inherit.

> 
> Kevin
> 

-- 
Dong Jia

Re: [Qemu-devel] [PATCH RFC 1/1] block: Handle NULL options correctly in raw_open

[Dong Jia Shi]

* Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> [2017-03-08 17:31:05 +0800]:

> * Kevin Wolf <kwolf@redhat.com> [2017-03-08 10:13:46 +0100]:
> 
> > Am 08.03.2017 um 03:15 hat Dong Jia Shi geschrieben:
> > > A normal call for raw_open should always pass in a non-NULL @options,
> > > but for some certain cases (e.g. trying to applying snapshot on a RBD
> > > image), they call raw_open with a NULL @options right after the calling
> > > for raw_close.
> > > 
> > > Let's take the NULL @options as a sign of trying to do raw_open again,
> > > and just simply return a success code.
> > > 
> > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > 
> > I think we rather need to fix bdrv_snapshot_goto() so that it doesn't
> > pass NULL, but the actual options that were given for the node (i.e.
> > bs->options).
> I've tried that before the current try. bs->options does not have the
> "file" key-value pair, so that leads to a fail too. Should we put "file"
> in to the options manually? I noticed that it was removed from
> bs->options during the calling of bdrv_open_inherit.
> 
Hi Kevin,

After thinking for quite some time, I still don't think we need to fix
the caller. The reason is that raw_close always does nothing, so no
matter what the caller passing in, raw_open should do nothing but just
return 0.

The following is another proposal. Looking forward for your comments.
Thanks,

diff --git a/block/raw-format.c b/block/raw-format.c
index 86fbc65..c309d4c 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -384,6 +384,11 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVRawState *s = bs->opaque;
     int ret;
 
+    if (!bs->file) {
+        return 0;
+    }
+
+    assert(options != NULL);
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {

> > 
> > Kevin
> > 
> 
> -- 
> Dong Jia

-- 
Dong Jia

Re: [Qemu-devel] [PATCH RFC 1/1] block: Handle NULL options correctly in raw_open

[Kevin Wolf]

Am 13.03.2017 um 04:31 hat Dong Jia Shi geschrieben:
> * Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> [2017-03-08 17:31:05 +0800]:
> 
> > * Kevin Wolf <kwolf@redhat.com> [2017-03-08 10:13:46 +0100]:
> > 
> > > Am 08.03.2017 um 03:15 hat Dong Jia Shi geschrieben:
> > > > A normal call for raw_open should always pass in a non-NULL @options,
> > > > but for some certain cases (e.g. trying to applying snapshot on a RBD
> > > > image), they call raw_open with a NULL @options right after the calling
> > > > for raw_close.
> > > > 
> > > > Let's take the NULL @options as a sign of trying to do raw_open again,
> > > > and just simply return a success code.
> > > > 
> > > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > > 
> > > I think we rather need to fix bdrv_snapshot_goto() so that it doesn't
> > > pass NULL, but the actual options that were given for the node (i.e.
> > > bs->options).
> > I've tried that before the current try. bs->options does not have the
> > "file" key-value pair, so that leads to a fail too. Should we put "file"
> > in to the options manually? I noticed that it was removed from
> > bs->options during the calling of bdrv_open_inherit.
> > 
> Hi Kevin,
> 
> After thinking for quite some time, I still don't think we need to fix
> the caller. The reason is that raw_close always does nothing, so no
> matter what the caller passing in, raw_open should do nothing but just
> return 0.

raw is not the only format driver in qemu.

Kevin

Re: [Qemu-devel] [PATCH RFC 1/1] block: Handle NULL options correctly in raw_open

[Dong Jia Shi]

* Kevin Wolf <kwolf@redhat.com> [2017-03-13 11:15:22 +0100]:

> Am 13.03.2017 um 04:31 hat Dong Jia Shi geschrieben:
> > * Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> [2017-03-08 17:31:05 +0800]:
> > 
> > > * Kevin Wolf <kwolf@redhat.com> [2017-03-08 10:13:46 +0100]:
> > > 
> > > > Am 08.03.2017 um 03:15 hat Dong Jia Shi geschrieben:
> > > > > A normal call for raw_open should always pass in a non-NULL @options,
> > > > > but for some certain cases (e.g. trying to applying snapshot on a RBD
> > > > > image), they call raw_open with a NULL @options right after the calling
> > > > > for raw_close.
> > > > > 
> > > > > Let's take the NULL @options as a sign of trying to do raw_open again,
> > > > > and just simply return a success code.
> > > > > 
> > > > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > > > 
> > > > I think we rather need to fix bdrv_snapshot_goto() so that it doesn't
> > > > pass NULL, but the actual options that were given for the node (i.e.
> > > > bs->options).
> > > I've tried that before the current try. bs->options does not have the
> > > "file" key-value pair, so that leads to a fail too. Should we put "file"
> > > in to the options manually? I noticed that it was removed from
> > > bs->options during the calling of bdrv_open_inherit.
> > > 
> > Hi Kevin,
> > 
> > After thinking for quite some time, I still don't think we need to fix
> > the caller. The reason is that raw_close always does nothing, so no
> > matter what the caller passing in, raw_open should do nothing but just
> > return 0.
> 
> raw is not the only format driver in qemu.
> 
Hi Kevin,

Before this I assumed that the long existing code in bdrv_snapshot_goto
which passes in a NULL options to raw_open is on purpose, and that
implies to me raw_open (and any other .bdrv_open callback) takes the
responsibility to handle NULL options well. So at a first glance, I read
your above comment as:
"You should also fix .bdrv_open callback for every other formats to
handle NULL options as well."

But after staring it for a while, I read it from another point around:
"You should fix the caller."
If this is what you actually meant to tell, I have the following
proposal then:
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)));
+
         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);
             bs->drv = NULL;

I know I'm a little wordy, but that's because I want to make things
clear. Anyway, I have to rely on your advice on this, since you are the
expert.

> Kevin
> 

-- 
Dong Jia

Re: [Qemu-devel] [PATCH RFC 1/1] block: Handle NULL options correctly in raw_open

[Dong Jia Shi]

* Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> [2017-03-14 11:23:12 +0800]:

> * Kevin Wolf <kwolf@redhat.com> [2017-03-13 11:15:22 +0100]:
> 
> > Am 13.03.2017 um 04:31 hat Dong Jia Shi geschrieben:
> > > * Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> [2017-03-08 17:31:05 +0800]:
> > > 
> > > > * Kevin Wolf <kwolf@redhat.com> [2017-03-08 10:13:46 +0100]:
> > > > 
> > > > > Am 08.03.2017 um 03:15 hat Dong Jia Shi geschrieben:
> > > > > > A normal call for raw_open should always pass in a non-NULL @options,
> > > > > > but for some certain cases (e.g. trying to applying snapshot on a RBD
> > > > > > image), they call raw_open with a NULL @options right after the calling
> > > > > > for raw_close.
> > > > > > 
> > > > > > Let's take the NULL @options as a sign of trying to do raw_open again,
> > > > > > and just simply return a success code.
> > > > > > 
> > > > > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > > > > 
> > > > > I think we rather need to fix bdrv_snapshot_goto() so that it doesn't
> > > > > pass NULL, but the actual options that were given for the node (i.e.
> > > > > bs->options).
> > > > I've tried that before the current try. bs->options does not have the
> > > > "file" key-value pair, so that leads to a fail too. Should we put "file"
> > > > in to the options manually? I noticed that it was removed from
> > > > bs->options during the calling of bdrv_open_inherit.
> > > > 
> > > Hi Kevin,
> > > 
> > > After thinking for quite some time, I still don't think we need to fix
> > > the caller. The reason is that raw_close always does nothing, so no
> > > matter what the caller passing in, raw_open should do nothing but just
> > > return 0.
> > 
> > raw is not the only format driver in qemu.
> > 
> Hi Kevin,
> 
> Before this I assumed that the long existing code in bdrv_snapshot_goto
> which passes in a NULL options to raw_open is on purpose, and that
> implies to me raw_open (and any other .bdrv_open callback) takes the
> responsibility to handle NULL options well. So at a first glance, I read
> your above comment as:
> "You should also fix .bdrv_open callback for every other formats to
> handle NULL options as well."
> 
> But after staring it for a while, I read it from another point around:
> "You should fix the caller."
> If this is what you actually meant to tell, I have the following
> proposal then:
> 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)));
> +
>          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);
>              bs->drv = NULL;
> 
> I know I'm a little wordy, but that's because I want to make things
> clear. Anyway, I have to rely on your advice on this, since you are the
> expert.
> 
Ping. :>

-- 
Dong Jia