Patchwork [3/3] block: Fix the use of protocols in backing files

login
register
mail settings
Submitter Stefan Hajnoczi
Date Nov. 30, 2010, 3:14 p.m.
Message ID <1291130056-32441-4-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/73718/
State New
Headers show

Comments

Stefan Hajnoczi - Nov. 30, 2010, 3:14 p.m.
Backing filenames may contain a protocol.  The code currently doesn't
consider this case and produces filenames that embed "<protocol>:".
Don't combine filenames if the backing filename contains a protocol.

Based on an earlier patch by Anthony Liguori <aliguori@us.ibm.com>.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)
Kevin Wolf - Dec. 2, 2010, 2:31 p.m.
Am 30.11.2010 16:14, schrieb Stefan Hajnoczi:
> Backing filenames may contain a protocol.  The code currently doesn't
> consider this case and produces filenames that embed "<protocol>:".
> Don't combine filenames if the backing filename contains a protocol.
> 
> Based on an earlier patch by Anthony Liguori <aliguori@us.ibm.com>.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.c |   15 +++++++++++----
>  1 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 59b69e4..c9a6720 100644
> --- a/block.c
> +++ b/block.c
> @@ -611,10 +611,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>          BlockDriver *back_drv = NULL;
>  
>          bs->backing_hd = bdrv_new("");
> -        path_combine(backing_filename, sizeof(backing_filename),
> -                     filename, bs->backing_file);
> -        if (bs->backing_format[0] != '\0')
> -            back_drv = bdrv_find_format(bs->backing_format);
> +        if (path_has_protocol(bs->backing_file)) {
> +            back_drv = bdrv_find_protocol(bs->backing_file);
> +            pstrcpy(backing_filename, sizeof(backing_filename),
> +                    bs->backing_file);
> +        } else {
> +            path_combine(backing_filename, sizeof(backing_filename),
> +                         filename, bs->backing_file);
> +            if (bs->backing_format[0] != '\0') {
> +                back_drv = bdrv_find_format(bs->backing_format);
> +            }
> +        }

Now we ignore bs->backing_format when a protocol is used. We don't even
allow using anything on top of that protocol any more, so for example
qcow2 over sheepdog would be broken. Maybe a use case that you like
better would be QED over http or something.

It might still not be a very common use case, but this doesn't look
right to me. Probably only the pstrcpy/path_combine should be conditional.

I've applied the first two patches of the series to the block branch.

Kevin
Stefan Hajnoczi - Dec. 2, 2010, 3:48 p.m.
On Thu, Dec 02, 2010 at 03:31:42PM +0100, Kevin Wolf wrote:
> Am 30.11.2010 16:14, schrieb Stefan Hajnoczi:
> > diff --git a/block.c b/block.c
> > index 59b69e4..c9a6720 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -611,10 +611,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
> >          BlockDriver *back_drv = NULL;
> >  
> >          bs->backing_hd = bdrv_new("");
> > -        path_combine(backing_filename, sizeof(backing_filename),
> > -                     filename, bs->backing_file);
> > -        if (bs->backing_format[0] != '\0')
> > -            back_drv = bdrv_find_format(bs->backing_format);
> > +        if (path_has_protocol(bs->backing_file)) {
> > +            back_drv = bdrv_find_protocol(bs->backing_file);
> > +            pstrcpy(backing_filename, sizeof(backing_filename),
> > +                    bs->backing_file);
> > +        } else {
> > +            path_combine(backing_filename, sizeof(backing_filename),
> > +                         filename, bs->backing_file);
> > +            if (bs->backing_format[0] != '\0') {
> > +                back_drv = bdrv_find_format(bs->backing_format);
> > +            }
> > +        }
> 
> Now we ignore bs->backing_format when a protocol is used. We don't even
> allow using anything on top of that protocol any more, so for example
> qcow2 over sheepdog would be broken. Maybe a use case that you like
> better would be QED over http or something.
> 
> It might still not be a very common use case, but this doesn't look
> right to me. Probably only the pstrcpy/path_combine should be conditional.

True, I'll take a look at that.  This reminds me of the write-up that
Markus did on block driver trees and formats vs protocols.

Stefan

Patch

diff --git a/block.c b/block.c
index 59b69e4..c9a6720 100644
--- a/block.c
+++ b/block.c
@@ -611,10 +611,17 @@  int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         BlockDriver *back_drv = NULL;
 
         bs->backing_hd = bdrv_new("");
-        path_combine(backing_filename, sizeof(backing_filename),
-                     filename, bs->backing_file);
-        if (bs->backing_format[0] != '\0')
-            back_drv = bdrv_find_format(bs->backing_format);
+        if (path_has_protocol(bs->backing_file)) {
+            back_drv = bdrv_find_protocol(bs->backing_file);
+            pstrcpy(backing_filename, sizeof(backing_filename),
+                    bs->backing_file);
+        } else {
+            path_combine(backing_filename, sizeof(backing_filename),
+                         filename, bs->backing_file);
+            if (bs->backing_format[0] != '\0') {
+                back_drv = bdrv_find_format(bs->backing_format);
+            }
+        }
 
         /* backing files always opened read-only */
         back_flags =