Patchwork [01/15] block: Fail gracefully when using a format driver on protocol level

login
register
mail settings
Submitter Kevin Wolf
Date April 12, 2013, 8:47 p.m.
Message ID <1365799688-19918-2-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/236198/
State New
Headers show

Comments

Kevin Wolf - April 12, 2013, 8:47 p.m.
Specifying the wrong driver could fail an assertion:

$ qemu-system-x86_64 -drive file.driver=qcow2,file=x
qemu-system-x86_64: block.c:721: bdrv_open_common: Assertion `file !=
((void *)0)' failed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                    |  7 +++++++
 tests/qemu-iotests/051     |  7 +++++++
 tests/qemu-iotests/051.out | 10 ++++++++++
 3 files changed, 24 insertions(+)
Eric Blake - April 12, 2013, 10:50 p.m.
On 04/12/2013 02:47 PM, Kevin Wolf wrote:
> Specifying the wrong driver could fail an assertion:
> 
> $ qemu-system-x86_64 -drive file.driver=qcow2,file=x
> qemu-system-x86_64: block.c:721: bdrv_open_common: Assertion `file !=
> ((void *)0)' failed.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                    |  7 +++++++
>  tests/qemu-iotests/051     |  7 +++++++
>  tests/qemu-iotests/051.out | 10 ++++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 602d8a4..f23bdcc 100644
> --- a/block.c
> +++ b/block.c
> @@ -718,6 +718,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>          assert(drv->bdrv_parse_filename || filename != NULL);
>          ret = drv->bdrv_file_open(bs, filename, options, open_flags);
>      } else {
> +        if (file == NULL) {
> +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "The '%s' block driver is "
> +                          "not suitable for the bottom level",
> +                          drv->format_name);
> +            ret = -EINVAL;
> +            goto free_and_fail;
> +        }
>          assert(file != NULL);

Is it really necessary to leave the assert in place, now that you have a
check for NULL followed by unconditional goto?

Just reading that error message, I'm not quite sure what you meant by
"not suitable for the bottom level".  I guess the intent is that
file.driver specifies the protocol, and that both raw and qcow2 are
formats possible on the file protocol, rather than qcow2 being a file
protocol itself.

> +Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2
> +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: The 'qcow2' block driver is not suitable for the bottom level
> +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not open disk image TEST_DIR/t.qcow2: Invalid argument

Maybe a better error message would be this?

Attempt to use format driver 'qcow2' where a protocol driver was expected
Kevin Wolf - April 15, 2013, 9:06 a.m.
Am 13.04.2013 um 00:50 hat Eric Blake geschrieben:
> On 04/12/2013 02:47 PM, Kevin Wolf wrote:
> > Specifying the wrong driver could fail an assertion:
> > 
> > $ qemu-system-x86_64 -drive file.driver=qcow2,file=x
> > qemu-system-x86_64: block.c:721: bdrv_open_common: Assertion `file !=
> > ((void *)0)' failed.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c                    |  7 +++++++
> >  tests/qemu-iotests/051     |  7 +++++++
> >  tests/qemu-iotests/051.out | 10 ++++++++++
> >  3 files changed, 24 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 602d8a4..f23bdcc 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -718,6 +718,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
> >          assert(drv->bdrv_parse_filename || filename != NULL);
> >          ret = drv->bdrv_file_open(bs, filename, options, open_flags);
> >      } else {
> > +        if (file == NULL) {
> > +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "The '%s' block driver is "
> > +                          "not suitable for the bottom level",
> > +                          drv->format_name);
> > +            ret = -EINVAL;
> > +            goto free_and_fail;
> > +        }
> >          assert(file != NULL);
> 
> Is it really necessary to leave the assert in place, now that you have a
> check for NULL followed by unconditional goto?

Not really. I'll send a cleanup.

> Just reading that error message, I'm not quite sure what you meant by
> "not suitable for the bottom level".  I guess the intent is that
> file.driver specifies the protocol, and that both raw and qcow2 are
> formats possible on the file protocol, rather than qcow2 being a file
> protocol itself.
> 
> > +Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2
> > +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: The 'qcow2' block driver is not suitable for the bottom level
> > +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not open disk image TEST_DIR/t.qcow2: Invalid argument
> 
> Maybe a better error message would be this?
> 
> Attempt to use format driver 'qcow2' where a protocol driver was expected

I was trying to avoid the format/protocol discussion this time by
choosing a different phrasing in the first place. Seems this approach
doesn't work better either...

Problems with this terminology start when you have more than two drivers
involved. Currently, this is only with the more exotic cases like when
you have qcow2 -> blkdebug -> file, but if we follow through with our
-blockdev plans, arbitrary stacking of BlockDriverStates will become
more common.

Markus likes to describe it as a graph of nodes of the same kind, where
the leaves (i.e. the protocols) just happen to not have a .file option.

Kevin
Markus Armbruster - April 15, 2013, 12:02 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 13.04.2013 um 00:50 hat Eric Blake geschrieben:
>> On 04/12/2013 02:47 PM, Kevin Wolf wrote:
>> > Specifying the wrong driver could fail an assertion:
>> > 
>> > $ qemu-system-x86_64 -drive file.driver=qcow2,file=x
>> > qemu-system-x86_64: block.c:721: bdrv_open_common: Assertion `file !=
>> > ((void *)0)' failed.
>> > 
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >  block.c                    |  7 +++++++
>> >  tests/qemu-iotests/051     |  7 +++++++
>> >  tests/qemu-iotests/051.out | 10 ++++++++++
>> >  3 files changed, 24 insertions(+)
>> > 
>> > diff --git a/block.c b/block.c
>> > index 602d8a4..f23bdcc 100644
>> > --- a/block.c
>> > +++ b/block.c
>> > @@ -718,6 +718,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>> >          assert(drv->bdrv_parse_filename || filename != NULL);
>> >          ret = drv->bdrv_file_open(bs, filename, options, open_flags);
>> >      } else {
>> > +        if (file == NULL) {
>> > +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "The '%s' block driver is "
>> > +                          "not suitable for the bottom level",
>> > +                          drv->format_name);
>> > +            ret = -EINVAL;
>> > +            goto free_and_fail;
>> > +        }
>> >          assert(file != NULL);
>> 
>> Is it really necessary to leave the assert in place, now that you have a
>> check for NULL followed by unconditional goto?
>
> Not really. I'll send a cleanup.
>
>> Just reading that error message, I'm not quite sure what you meant by
>> "not suitable for the bottom level".  I guess the intent is that
>> file.driver specifies the protocol, and that both raw and qcow2 are
>> formats possible on the file protocol, rather than qcow2 being a file
>> protocol itself.
>> 
>> > +Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2
>> > +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: The 'qcow2'
>> > block driver is not suitable for the bottom level
>> > +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not
>> > open disk image TEST_DIR/t.qcow2: Invalid argument
>> 
>> Maybe a better error message would be this?
>> 
>> Attempt to use format driver 'qcow2' where a protocol driver was expected

It's an improvement, but it's still gobbledygook :)

I don't think users understand "format" vs. "protocol".  Heck, I don't
half of the time!

> I was trying to avoid the format/protocol discussion this time by
> choosing a different phrasing in the first place. Seems this approach
> doesn't work better either...
>
> Problems with this terminology start when you have more than two drivers
> involved. Currently, this is only with the more exotic cases like when
> you have qcow2 -> blkdebug -> file, but if we follow through with our
> -blockdev plans, arbitrary stacking of BlockDriverStates will become
> more common.

Exactly.

> Markus likes to describe it as a graph of nodes of the same kind, where
> the leaves (i.e. the protocols) just happen to not have a .file option.

I view block drivers as building blocks for building block backends.  A
block driver has one plug and may have sockets.  You combine them by
plugging plugs into sockets.  Sockets need to be plugged for the thing
to work[*].  The root plug plugs into the block frontend, via qdev drive
property.

What the error message above is trying to report is "I got an empty
socket here, and I need it filled".

Begs the question which socket.

In simple cases, there's just one.  Block driver "raw", for instance.
The current code is designed for this case, and provides this socket as
BlockDriverState member file.

Block driver "qcow2" actually has two sockets, one for the QCOW2 image,
one for its backing image.  But we don't (yet) expose the backing image
as a first class pluggable socket, so we can pretend there's just one.

For truly general block backend configuration, we need to ditch this
single socket mindset.

In the context of such general block backend configuration, where users
combine block drivers into backends by plugging plugs into named
sockets, the most useful way to report an empty socket is to identify it
by name.

How to best report it when plugs and sockets are obscured by a heavy
layer of syntactic sugar isn't obvious to me.


[*] Unless we find a use for optional sockets.  Then only the mandatory
sockets need to be plugged.

Patch

diff --git a/block.c b/block.c
index 602d8a4..f23bdcc 100644
--- a/block.c
+++ b/block.c
@@ -718,6 +718,13 @@  static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
         assert(drv->bdrv_parse_filename || filename != NULL);
         ret = drv->bdrv_file_open(bs, filename, options, open_flags);
     } else {
+        if (file == NULL) {
+            qerror_report(ERROR_CLASS_GENERIC_ERROR, "The '%s' block driver is "
+                          "not suitable for the bottom level",
+                          drv->format_name);
+            ret = -EINVAL;
+            goto free_and_fail;
+        }
         assert(file != NULL);
         bs->file = file;
         ret = drv->bdrv_open(bs, options, open_flags);
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 17a1d92..8f96ef4 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -142,6 +142,13 @@  run_qemu -drive media=cdrom,cache=writethrough
 run_qemu -drive media=cdrom,cache=unsafe
 run_qemu -drive media=cdrom,cache=invalid_value
 
+echo
+echo === Specifying the protocol layer ===
+echo
+
+run_qemu -drive file=$TEST_IMG,file.driver=file
+run_qemu -drive file=$TEST_IMG,file.driver=qcow2
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index da0d18b..194f2d2 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -159,4 +159,14 @@  qququiquit
 Testing: -drive media=cdrom,cache=invalid_value
 qemu: -drive media=cdrom,cache=invalid_value: invalid cache option
 
+
+=== Specifying the protocol layer ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,file.driver=file
+qququiquit
+
+Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2
+qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: The 'qcow2' block driver is not suitable for the bottom level
+qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not open disk image TEST_DIR/t.qcow2: Invalid argument
+
 *** done