[Qemu-devel] [PATCH 0/3] block: Fix the use of protocols in backing files

[Qemu-devel] [PATCH 0/3] block: Fix the use of protocols in backing files

[Stefan Hajnoczi]

Anthony sent out a patch to make backing filenames that contain a protocol
work.  An issue with bdrv_find_protocol() was identified because its interface
makes it impossible to distinguish between filenames that start with "file:"
and those that have no "<protocol>:" at all.

This patch series solves the issue by introducing a path_has_protocol()
function to test whether or not a filename contains a protocol.  I tried
Kevin's suggestion of adding a new bdrv_find_protocol()-like interface with
more specific error returns but found that option more complicated than
path_has_protocol().

For details on Anthony's patch see http://patchwork.ozlabs.org/patch/69384/.

[Qemu-devel] [PATCH 1/3] block: Make bdrv_create_file() ':' handling consistent

[Stefan Hajnoczi]

Filenames may start with "<protocol>:" to explicitly use a protocol like
nbd.  Filenames with unknown protocols are rejected in most of QEMU
except for bdrv_create_file().  Even if a file with an invalid filename
can be created, QEMU cannot use it since all the other relevant
functions reject such paths.  Make bdrv_create_file() consistent.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block.c b/block.c
index 63effd8..e7a986c 100644
--- a/block.c
+++ b/block.c
@@ -215,7 +215,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
 
     drv = bdrv_find_protocol(filename);
     if (drv == NULL) {
-        drv = bdrv_find_format("file");
+        return -ENOENT;
     }
 
     return bdrv_create(drv, filename, options);
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/3] block: Introduce path_has_protocol() function

[Stefan Hajnoczi]

The bdrv_find_protocol() function returns NULL if an unknown protocol
name is given.  It returns the "file" protocol when the filename
contains no protocol at all.  This makes it difficult to distinguish
between paths which contain a protocol and those which do not.

Factor out a helper function that tests whether or not a filename has a
protocol.  The next patch makes use of this function.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index e7a986c..59b69e4 100644
--- a/block.c
+++ b/block.c
@@ -70,6 +70,19 @@ static BlockDriverState *bs_snapshots;
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
+/* check if the path starts with "<protocol>:" */
+static int path_has_protocol(const char *path)
+{
+#ifdef _WIN32
+    if (is_windows_drive(path) ||
+        is_windows_drive_prefix(path)) {
+        return 0;
+    }
+#endif
+
+    return strchr(path, ':') != NULL;
+}
+
 int path_is_absolute(const char *path)
 {
     const char *p;
@@ -307,16 +320,11 @@ BlockDriver *bdrv_find_protocol(const char *filename)
         return drv1;
     }
 
-#ifdef _WIN32
-     if (is_windows_drive(filename) ||
-         is_windows_drive_prefix(filename))
-         return bdrv_find_format("file");
-#endif
-
-    p = strchr(filename, ':');
-    if (!p) {
+    if (!path_has_protocol(filename)) {
         return bdrv_find_format("file");
     }
+    p = strchr(filename, ':');
+    assert(p != NULL);
     len = p - filename;
     if (len > sizeof(protocol) - 1)
         len = sizeof(protocol) - 1;
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/3] block: Fix the use of protocols in backing files

[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);
+            }
+        }
 
         /* backing files always opened read-only */
         back_flags =
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 3/3] block: Fix the use of protocols in backing files

[Kevin Wolf]

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

[Qemu-devel] Re: [PATCH 3/3] block: Fix the use of protocols in backing files

[Stefan Hajnoczi]

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

[Qemu-devel] Re: [PATCH 2/3] block: Introduce path_has_protocol() function

[Kevin Wolf]

Am 30.11.2010 16:14, schrieb Stefan Hajnoczi:
> The bdrv_find_protocol() function returns NULL if an unknown protocol
> name is given.  It returns the "file" protocol when the filename
> contains no protocol at all.  This makes it difficult to distinguish
> between paths which contain a protocol and those which do not.
> 
> Factor out a helper function that tests whether or not a filename has a
> protocol.  The next patch makes use of this function.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

This breaks the mingw32 build:

/home/kwolf/tmp/win32/qemu/block.c: In function 'path_has_protocol':
/home/kwolf/tmp/win32/qemu/block.c:78: warning: implicit declaration of
function 'is_windows_drive_prefix'
/home/kwolf/tmp/win32/qemu/block.c:78: warning: nested extern
declaration of 'is_windows_drive_prefix'
/home/kwolf/tmp/win32/qemu/block.c: At top level:
/home/kwolf/tmp/win32/qemu/block.c:261: error: static declaration of
'is_windows_drive_prefix' follows non-static declaration
/home/kwolf/tmp/win32/qemu/block.c:78: note: previous implicit
declaration of 'is_windows_drive_prefix' was here

Kevin