[Qemu-devel] [PATCH][RFC] Handling ':' on filenames

[Qemu-devel] [PATCH][RFC] Handling ':' on filenames

[Eduardo Habkost]

This patch fixes this issue:

  $ qemu-img create -f qcow2 /tmp/a:b 1G
  $ qemu-system-x86_64 -hda qcow2:/tmp/a:b
  qemu: could not open disk image /tmp/a:b
  $

Based on a suggestion by Daniel Berrange.

However, this is still just a workaround. The semantics of filenames
containing colon characters (and how this can be escaped, avoided,
or worked around) are not very clear.

Going further, what if we stop using "protocol:filename" strings
internally, except where the user interface or external data really
requires this format?

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 7c744c7..04488d6 100644
--- a/block.c
+++ b/block.c
@@ -236,6 +236,14 @@ static BlockDriver *find_protocol(const char *filename)
         is_windows_drive_prefix(filename))
         return &bdrv_raw;
 #endif
+
+    /* Protocol name will never start with a slash.
+     * This allows the user to specify absolute filenames
+     * containing a ":" character.
+     */
+    if (*filename == '/')
+        return &bdrv_raw;
+
     p = strchr(filename, ':');
     if (!p)
         return &bdrv_raw;
-- 
1.6.1

-- 
Eduardo

Re: [Qemu-devel] [PATCH][RFC] Handling ':' on filenames

[Uri Lublin]

Eduardo Habkost wrote:
> This patch fixes this issue:
> 
>   $ qemu-img create -f qcow2 /tmp/a:b 1G
>   $ qemu-system-x86_64 -hda qcow2:/tmp/a:b
>   qemu: could not open disk image /tmp/a:b
>   $

This patch looks good to me with the following comments:

1. I think the example above is wrong.
    a. There is no protocol "qcow2"
    b. We do not want to use "qcow2:/tmp/a:b" as filenames (been there).
       We want to use -drive file=/tmp/a:b,format=qcow2 (and
       similar "qemu-img info -f ").
    c. But, without this patch, even when using appropriate -drive, we'd
       still get the same result (could not open disk image).
       Qemu considers "/tmp/a" to be a protocol, which does not exist,
       so Qemu fails to open the image.
       With this patch, Qemu opens the file successfully.

> 
> Based on a suggestion by Daniel Berrange.
> 
> However, this is still just a workaround. The semantics of filenames
> containing colon characters (and how this can be escaped, avoided,
> or worked around) are not very clear.
> 
> Going further, what if we stop using "protocol:filename" strings
> internally, except where the user interface or external data really
> requires this format?

2. Do you mean something like adding a '-drive protocol=xxx' option ?

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  block.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 7c744c7..04488d6 100644
> --- a/block.c
> +++ b/block.c
> @@ -236,6 +236,14 @@ static BlockDriver *find_protocol(const char *filename)
>          is_windows_drive_prefix(filename))
>          return &bdrv_raw;
>  #endif
> +
> +    /* Protocol name will never start with a slash.
> +     * This allows the user to specify absolute filenames
> +     * containing a ":" character.
> +     */
> +    if (*filename == '/')
> +        return &bdrv_raw;

3. The patch limits protocols names to not start with '/' (full paths).
    I think we should apply the same logic to relative paths, so
    protocol names would not start with '.' as well (no protocol
    starts with '.' today):

  +   if ((*filename == '/') || (*filename == '.'))
  +       return &bdrv_raw;

4. Note that this patch does not limit image formats to raw, it
    just says "use no protocols for full/relative paths".

> +
>      p = strchr(filename, ':');
>      if (!p)
>          return &bdrv_raw;


Regards,
     Uri.

Re: [Qemu-devel] [PATCH][RFC] Handling ':' on filenames

[Stuart Brady]

On Sun, Mar 08, 2009 at 01:28:31PM +0200, Uri Lublin wrote:
>    b. We do not want to use "qcow2:/tmp/a:b" as filenames (been there).
>       We want to use -drive file=/tmp/a:b,format=qcow2 (and
>       similar "qemu-img info -f ").

I can't help but feel that any approach to extracting filenames from the
middle of arguments will always have problems.

I suppose a syntax like this:

   -drive file=/tmp/file,with,commas:and:colons ,format=qcow2

(i.e. with the next parameter to -drive starting in a new argument)...
wouldn't win many supporters though?

Cheers,
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH][RFC] Handling ':' on filenames

[Eduardo Habkost]

Excerpts from Stuart Brady's message of Dom Mar 08 08:49:41 -0300 2009:
> On Sun, Mar 08, 2009 at 01:28:31PM +0200, Uri Lublin wrote:
> >    b. We do not want to use "qcow2:/tmp/a:b" as filenames (been there).
> >       We want to use -drive file=/tmp/a:b,format=qcow2 (and
> >       similar "qemu-img info -f ").
> 
> I can't help but feel that any approach to extracting filenames from the
> middle of arguments will always have problems.
> 
> I suppose a syntax like this:
> 
>    -drive file=/tmp/file,with,commas:and:colons ,format=qcow2
> 
> (i.e. with the next parameter to -drive starting in a new argument)...
> wouldn't win many supporters though?

The -drive option has a way to escape colons. For the filename
"/tmp/a,b,c", you can use:

 -drive file=/tmp/a,,b,,c,format=qcow2


Not pretty, but it seems to work.
-- 
Eduardo

Re: [Qemu-devel] [PATCH][RFC] Handling ':' on filenames

[Jamie Lokier]

Eduardo Habkost wrote:
> The -drive option has a way to escape colons. For the filename
> "/tmp/a,b,c", you can use:
> 
>  -drive file=/tmp/a,,b,,c,format=qcow2
> 
> Not pretty, but it seems to work.

I wonder how you quote filenames from the monitor.

In my own management scripts I've taken to making a symlink with a
"safe" name to the real image file, and passing the safe name to KVM,
so that it's unambiguous.  Causes a bit of fun with qcow2 files, which
refer to their backing files relative to the symlink not the target
file.

-- Jamie

Re: [Qemu-devel] [PATCH][RFC] Handling ':' on filenames

[Eduardo Habkost]

Excerpts from kvm_autotest's message of Dom Mar 08 08:28:31 -0300 2009:
> Eduardo Habkost wrote:
> > This patch fixes this issue:
> > 
> >   $ qemu-img create -f qcow2 /tmp/a:b 1G
> >   $ qemu-system-x86_64 -hda qcow2:/tmp/a:b
> >   qemu: could not open disk image /tmp/a:b
> >   $
> 
> This patch looks good to me with the following comments:
> 
> 1. I think the example above is wrong.
>     a. There is no protocol "qcow2"
>     b. We do not want to use "qcow2:/tmp/a:b" as filenames (been there).
>        We want to use -drive file=/tmp/a:b,format=qcow2 (and
>        similar "qemu-img info -f ").

Oops. Ignore that, I've pasted the wrong command-line. That was when I
was testing the behaviour of qemu when I specified other things before
the colon. The error appears when I do this:

 $ qemu-system-x86_64 -hda /tmp/a:b

>     c. But, without this patch, even when using appropriate -drive, we'd
>        still get the same result (could not open disk image).
>        Qemu considers "/tmp/a" to be a protocol, which does not exist,
>        so Qemu fails to open the image.
>        With this patch, Qemu opens the file successfully.
> 

Yes.

> > 
> > Based on a suggestion by Daniel Berrange.
> > 
> > However, this is still just a workaround. The semantics of filenames
> > containing colon characters (and how this can be escaped, avoided,
> > or worked around) are not very clear.
> > 
> > Going further, what if we stop using "protocol:filename" strings
> > internally, except where the user interface or external data really
> > requires this format?
> 
> 2. Do you mean something like adding a '-drive protocol=xxx' option ?

Where possible, yes.
-- 
Eduardo

Re: [Qemu-devel] [PATCH][RFC] Handling ':' on filenames

[Eduardo Habkost]

Excerpts from kvm_autotest's message of Dom Mar 08 08:28:31 -0300 2009:
<snip>
> > ---
> >  block.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 7c744c7..04488d6 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -236,6 +236,14 @@ static BlockDriver *find_protocol(const char *filename)
> >          is_windows_drive_prefix(filename))
> >          return &bdrv_raw;
> >  #endif
> > +
> > +    /* Protocol name will never start with a slash.
> > +     * This allows the user to specify absolute filenames
> > +     * containing a ":" character.
> > +     */
> > +    if (*filename == '/')
> > +        return &bdrv_raw;
> 
> 3. The patch limits protocols names to not start with '/' (full paths).
>     I think we should apply the same logic to relative paths, so
>     protocol names would not start with '.' as well (no protocol
>     starts with '.' today):
> 
>   +   if ((*filename == '/') || (*filename == '.'))
>   +       return &bdrv_raw;

Is ':' a valid character for Windows filenames? In this case, we
may want to check for backslashes also.

The "./filename" approach for relative paths seems nice to complete the fix,
but I still see this is a workaround to a more fundamental problem.


> 
> 4. Note that this patch does not limit image formats to raw, it
>     just says "use no protocols for full/relative paths".
> 
> > +
> >      p = strchr(filename, ':');
> >      if (!p)
> >          return &bdrv_raw;
> 
> 
> Regards,
>      Uri.
-- 
Eduardo

Re: [Qemu-devel] [PATCH][RFC] Handling ':' on filenames

[Amit Shah]

On (Mon) Mar 09 2009 [11:27:53], Eduardo Habkost wrote:
> > > +
> > > +    /* Protocol name will never start with a slash.
> > > +     * This allows the user to specify absolute filenames
> > > +     * containing a ":" character.
> > > +     */
> > > +    if (*filename == '/')
> > > +        return &bdrv_raw;
> > 
> > 3. The patch limits protocols names to not start with '/' (full paths).
> >     I think we should apply the same logic to relative paths, so
> >     protocol names would not start with '.' as well (no protocol
> >     starts with '.' today):
> > 
> >   +   if ((*filename == '/') || (*filename == '.'))
> >   +       return &bdrv_raw;
> 
> Is ':' a valid character for Windows filenames? In this case, we
> may want to check for backslashes also.

I guess so; full paths are (at least used to be) something like
"C:\some-dir\some-file"

Amit