Patchwork block: Fix direct use of protocols as driver for bdrv_open()

login
register
mail settings
Submitter Kevin Wolf
Date March 27, 2013, 4:28 p.m.
Message ID <1364401698-21402-1-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/231742/
State New
Headers show

Comments

Kevin Wolf - March 27, 2013, 4:28 p.m.
bdrv_open_common() implements direct use of protocols by copying the
pre-opened BlockDriverStates to bs using bdrv_swap(). It did however
first set some fields in bs, which end up in file after the swap. When
bdrv_open() destroys file, it appears to be open, and because it isn't,
qemu could segfault while trying to close it.

Reorder the operations to return immediately in such cases so that file
is correctly detected as closed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)
Stefan Hajnoczi - March 28, 2013, 10:53 a.m.
On Wed, Mar 27, 2013 at 05:28:18PM +0100, Kevin Wolf wrote:
> bdrv_open_common() implements direct use of protocols by copying the
> pre-opened BlockDriverStates to bs using bdrv_swap(). It did however
> first set some fields in bs, which end up in file after the swap. When
> bdrv_open() destroys file, it appears to be open, and because it isn't,
> qemu could segfault while trying to close it.
> 
> Reorder the operations to return immediately in such cases so that file
> is correctly detected as closed.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)

Didn't apply cleanly, please check that I resolved the conflict
correctly.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan
Kevin Wolf - March 28, 2013, 11:13 a.m.
Am 28.03.2013 um 11:53 hat Stefan Hajnoczi geschrieben:
> On Wed, Mar 27, 2013 at 05:28:18PM +0100, Kevin Wolf wrote:
> > bdrv_open_common() implements direct use of protocols by copying the
> > pre-opened BlockDriverStates to bs using bdrv_swap(). It did however
> > first set some fields in bs, which end up in file after the swap. When
> > bdrv_open() destroys file, it appears to be open, and because it isn't,
> > qemu could segfault while trying to close it.
> > 
> > Reorder the operations to return immediately in such cases so that file
> > is correctly detected as closed.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 31 +++++++++++++++----------------
> >  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> Didn't apply cleanly, please check that I resolved the conflict
> correctly.

Sorry, my bad, this was accidentally on top of a temporary quick fix...
You conflict resolution is correct.

Kevin
Stefan Hajnoczi - March 28, 2013, 1:29 p.m.
On Thu, Mar 28, 2013 at 12:13:40PM +0100, Kevin Wolf wrote:
> Am 28.03.2013 um 11:53 hat Stefan Hajnoczi geschrieben:
> > On Wed, Mar 27, 2013 at 05:28:18PM +0100, Kevin Wolf wrote:
> > > bdrv_open_common() implements direct use of protocols by copying the
> > > pre-opened BlockDriverStates to bs using bdrv_swap(). It did however
> > > first set some fields in bs, which end up in file after the swap. When
> > > bdrv_open() destroys file, it appears to be open, and because it isn't,
> > > qemu could segfault while trying to close it.
> > > 
> > > Reorder the operations to return immediately in such cases so that file
> > > is correctly detected as closed.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block.c | 31 +++++++++++++++----------------
> > >  1 file changed, 15 insertions(+), 16 deletions(-)
> > 
> > Didn't apply cleanly, please check that I resolved the conflict
> > correctly.
> 
> Sorry, my bad, this was accidentally on top of a temporary quick fix...
> You conflict resolution is correct.

Thanks.

Stefan

Patch

diff --git a/block.c b/block.c
index 3cc3eb0..0ae2e93 100644
--- a/block.c
+++ b/block.c
@@ -680,6 +680,18 @@  static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     trace_bdrv_open_common(bs, filename, flags, drv->format_name);
 
+    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
+        return -ENOTSUP;
+    }
+
+    /* bdrv_open() with directly using a protocol as drv. This layer is already
+     * opened, so assign it to bs (while file becomes a closed BlockDriverState)
+     * and return immediately. */
+    if (file != NULL && drv->bdrv_file_open) {
+        bdrv_swap(file, bs);
+        return 0;
+    }
+
     bs->open_flags = flags;
     bs->buffer_alignment = 512;
 
@@ -694,15 +706,6 @@  static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
         bs->filename[0] = '\0';
     }
 
-    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
-        return -ENOTSUP;
-    }
-
-    if (file != NULL && drv->bdrv_file_open) {
-        bdrv_swap(file, bs);
-        return 0;
-    }
-
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
 
@@ -713,13 +716,9 @@  static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     /* Open the image, either directly or using a protocol */
     if (drv->bdrv_file_open) {
-        if (file != NULL) {
-            bdrv_swap(file, bs);
-            ret = 0;
-        } else {
-            assert(drv->bdrv_parse_filename || filename != NULL);
-            ret = drv->bdrv_file_open(bs, filename, options, open_flags);
-        }
+        assert(file == NULL);
+        assert(drv->bdrv_parse_filename || filename != NULL);
+        ret = drv->bdrv_file_open(bs, filename, options, open_flags);
     } else {
         assert(file != NULL);
         bs->file = file;