[Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Uri Lublin]

From: Uri Lublin <uril@redhat.com>

Changes from my last post:
   * rebase for qemu.
   * if find_image_format() returns NULL set "raw" drv instead of returning
     with error. This fixes breakage of '-cdrom isofile'

The purpose of this prefix is to
1. Provide a way to know the backing file format without probing
    it (setting the format upon creation time).
2. Enable using qcow2 format (and others) over host block devices.
    (only if the user specifically asks for it).

If no fmt:FMT: is provided we go back to probing.

Based on a similar patch from Shahar Frank.
http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg01083.html

Also fixes a security flaw found by Daniel P. Berrange on the
above thread which summarizes: "Autoprobing: just say no."

Examples:

backing file format is qcow2 (even though it's on a host block device)
$ qemu-img create -b fmt:qcow2:/dev/loop0 -f qcow2 /tmp/uuu.qcow2

force backing file format to raw (no probing)
$ qemu-img create -f raw /tmp/image1.raw 10G
$ qemu-img create -b fmt:raw:/tmp/image1.raw -f qcow2 /tmp/image1.qcow2

Use together with other protocols, e.g. nbd
$ qemu-nbd -v -n --snapshot -t -k /tmp/uuu.socket fmt:qcow2:/tmp/images/uuu.qcow2 &
$ qemu-img info nbd:unix:/tmp/uuu.socket
$ qemu-system-x86_64 -snapshot -hda nbd:unix:/tmp/uuu.socket

Or fat
$ qemu-system-x86_64 -hda fmt:qcow2:/tmp/uuu.qcow2 -hdb fat:floppy:/tmp/images

Signed-off-by: Uri Lublin <uril@redhat.com>
---
  block.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++-----------
  1 files changed, 61 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 28d63d7..82959d6 100644
--- a/block.c
+++ b/block.c
@@ -71,7 +71,7 @@ int path_is_absolute(const char *path)
      if (*path == '/' || *path == '\\')
          return 1;
  #endif
-    p = strchr(path, ':');
+    p = strrchr(path, ':');
      if (p)
          p++;
      else
@@ -95,10 +95,23 @@ void path_combine(char *dest, int dest_size,

      if (dest_size <= 0)
          return;
+
+    /* copy "fmt:" prefix of filename if exists */
+    p = strrchr(filename, ':');
+    if (p) {
+        len = p - filename + 1;
+        if (dest_size <= len)
+            return;
+        strncpy(dest, filename, len);
+        filename  += len;
+        dest      += len;
+        dest_size -= len;
+    }
+
      if (path_is_absolute(filename)) {
          pstrcpy(dest, dest_size, filename);
      } else {
-        p = strchr(base_path, ':');
+        p = strrchr(base_path, ':');
          if (p)
              p++;
          else
@@ -226,30 +239,51 @@ static int is_windows_drive(const char *filename)
  }
  #endif

+static const char *raw_filename(const char *filename)
+{
+    char *_filename;
+
+    _filename = strrchr(filename, ':');
+    if (_filename)
+        return _filename + 1;
+    else
+        return filename;
+}
+
  static BlockDriver *find_protocol(const char *filename)
  {
      BlockDriver *drv1;
      char protocol[128];
-    int len;
+    int len, is_fmt = 0;
      const char *p;

+    if (!strncmp(filename, "fmt:", 4)) {
+        filename += 4;
+        is_fmt = 1;
+    }
+
  #ifdef _WIN32
      if (is_windows_drive(filename) ||
          is_windows_drive_prefix(filename))
-        return &bdrv_raw;
+        return NULL;
  #endif
      p = strchr(filename, ':');
      if (!p)
-        return &bdrv_raw;
+        return NULL;
      len = p - filename;
      if (len > sizeof(protocol) - 1)
          len = sizeof(protocol) - 1;
      memcpy(protocol, filename, len);
      protocol[len] = '\0';
      for(drv1 = first_drv; drv1 != NULL; drv1 = drv1->next) {
-        if (drv1->protocol_name &&
-            !strcmp(drv1->protocol_name, protocol))
-            return drv1;
+        if (is_fmt) {
+            if (!strcmp(drv1->format_name, protocol))
+                return drv1;
+        } else {
+            if (drv1->protocol_name &&
+                !strcmp(drv1->protocol_name, protocol))
+                return drv1;
+        }
      }
      return NULL;
  }
@@ -267,6 +301,14 @@ static BlockDriver *find_image_format(const char *filename)
         recognized as a host CDROM */
      if (strstart(filename, "/dev/cdrom", NULL))
          return &bdrv_host_device;
+
+    drv = find_protocol(filename);
+    if ((drv != NULL) && (drv->protocol_name == NULL))
+        return drv;
+
+    if (drv == NULL)
+        filename = raw_filename(filename);
+
  #ifdef _WIN32
      if (is_windows_drive(filename))
          return &bdrv_host_device;
@@ -280,7 +322,6 @@ static BlockDriver *find_image_format(const char *filename)
      }
  #endif

-    drv = find_protocol(filename);
      /* no need to test disk image formats for vvfat */
      if (drv == &bdrv_vvfat)
          return drv;
@@ -370,8 +411,11 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
          if (is_protocol)
              snprintf(backing_filename, sizeof(backing_filename),
                       "%s", filename);
-        else
-            realpath(filename, backing_filename);
+        else {
+            const char *p;
+            p = realpath(raw_filename(filename), backing_filename);
+            path_combine(backing_filename, sizeof(backing_filename), p, filename);
+        }

          if (bdrv_create(&bdrv_qcow2, tmp_filename,
                          total_size, backing_filename, 0) < 0) {
@@ -385,12 +429,12 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
      if (flags & BDRV_O_FILE) {
          drv = find_protocol(filename);
          if (!drv)
-            return -ENOENT;
+            drv = &bdrv_raw;
      } else {
          if (!drv) {
              drv = find_image_format(filename);
              if (!drv)
-                return -1;
+                drv = &bdrv_raw;
          }
      }
      bs->drv = drv;
@@ -403,6 +447,10 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
          open_flags = BDRV_O_RDWR | (flags & BDRV_O_CACHE_MASK);
      else
          open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
+
+    if (drv && !drv->protocol_name)
+        filename = raw_filename(filename);
+
      ret = drv->bdrv_open(bs, filename, open_flags);
      if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) {
          ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);
-- 
1.6.0.6

Re: [Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Daniel P. Berrange]

On Thu, Jan 08, 2009 at 08:15:25PM +0200, Uri Lublin wrote:
> The purpose of this prefix is to
> 1. Provide a way to know the backing file format without probing
>    it (setting the format upon creation time).
> 2. Enable using qcow2 format (and others) over host block devices.
>    (only if the user specifically asks for it).
> 
> If no fmt:FMT: is provided we go back to probing.

I still don't like the fact that this is inventing a second syntax
for specifying format that's different to the syntax used for  the 
existing -drive parameter, which is

  -drive file=/some/path,format=qcow2,....other disk options...

> backing file format is qcow2 (even though it's on a host block device)
> $ qemu-img create -b fmt:qcow2:/dev/loop0 -f qcow2 /tmp/uuu.qcow2

I'd prefer to see a '-F' flag to specify format of backing file and
leave syntax of existing -b arg alone

 $ qemu-img create -F qcow2 -b /dev/loop0 -f qcow2 /tmp/uuu.qcow2

> force backing file format to raw (no probing)
> $ qemu-img create -f raw /tmp/image1.raw 10G
> $ qemu-img create -b fmt:raw:/tmp/image1.raw -f qcow2 /tmp/image1.qcow2

  $ qemu-img create -F raw -b /tmp/image1.raw -f qcow2 /tmp/image1.qcow2

> Use together with other protocols, e.g. nbd
> $ qemu-nbd -v -n --snapshot -t -k /tmp/uuu.socket 
> fmt:qcow2:/tmp/images/uuu.qcow2 &

Should just add  a -f arg to qemu-nbd to specify format so it
follows qemu-img style, eg

 $ qemu-nbd -v -n --snapshot -t -k /tmp/uuu.socket \
    -f qcow2 /tmp/images/uuu.qcow2 &

> $ qemu-img info nbd:unix:/tmp/uuu.socket
> $ qemu-system-x86_64 -snapshot -hda nbd:unix:/tmp/uuu.socket
> 
> Or fat
> $ qemu-system-x86_64 -hda fmt:qcow2:/tmp/uuu.qcow2 -hdb 
> fat:floppy:/tmp/images

This is unneccessary, since -hda is deprecated, and there's a 
new -drive arg that already has ability to set format explicitly,
as well as many other flags that you need when setting up disks.

   $ qemu-system-x86_64 \ 
         -drive index=0,format=qcow2,file=/tmp/uuu.qcow2 \
         -drive index=1,format=fat:floppy,file=/tmp/images

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Thu, Jan 08, 2009 at 08:15:25PM +0200, Uri Lublin wrote:
>   
>> The purpose of this prefix is to
>> 1. Provide a way to know the backing file format without probing
>>    it (setting the format upon creation time).
>> 2. Enable using qcow2 format (and others) over host block devices.
>>    (only if the user specifically asks for it).
>>
>> If no fmt:FMT: is provided we go back to probing.
>>     
>
> I still don't like the fact that this is inventing a second syntax
> for specifying format that's different to the syntax used for  the 
> existing -drive parameter, which is
>
>   -drive file=/some/path,format=qcow2,....other disk options...
>
>   
>> backing file format is qcow2 (even though it's on a host block device)
>> $ qemu-img create -b fmt:qcow2:/dev/loop0 -f qcow2 /tmp/uuu.qcow2
>>     
>
> I'd prefer to see a '-F' flag to specify format of backing file and
> leave syntax of existing -b arg alone
>
>  $ qemu-img create -F qcow2 -b /dev/loop0 -f qcow2 /tmp/uuu.qcow2
>   

You may have multiple backing images.  I don't see how you can do fix 
this without encoding the information in the path, or changing qcow2.

Regards,

Anthony Liguori

>> force backing file format to raw (no probing)
>> $ qemu-img create -f raw /tmp/image1.raw 10G
>> $ qemu-img create -b fmt:raw:/tmp/image1.raw -f qcow2 /tmp/image1.qcow2
>>     
>
>   $ qemu-img create -F raw -b /tmp/image1.raw -f qcow2 /tmp/image1.qcow2
>
>   
>> Use together with other protocols, e.g. nbd
>> $ qemu-nbd -v -n --snapshot -t -k /tmp/uuu.socket 
>> fmt:qcow2:/tmp/images/uuu.qcow2 &
>>     
>
> Should just add  a -f arg to qemu-nbd to specify format so it
> follows qemu-img style, eg
>
>  $ qemu-nbd -v -n --snapshot -t -k /tmp/uuu.socket \
>     -f qcow2 /tmp/images/uuu.qcow2 &
>
>   
>> $ qemu-img info nbd:unix:/tmp/uuu.socket
>> $ qemu-system-x86_64 -snapshot -hda nbd:unix:/tmp/uuu.socket
>>
>> Or fat
>> $ qemu-system-x86_64 -hda fmt:qcow2:/tmp/uuu.qcow2 -hdb 
>> fat:floppy:/tmp/images
>>     
>
> This is unneccessary, since -hda is deprecated, and there's a 
> new -drive arg that already has ability to set format explicitly,
> as well as many other flags that you need when setting up disks.
>
>    $ qemu-system-x86_64 \ 
>          -drive index=0,format=qcow2,file=/tmp/uuu.qcow2 \
>          -drive index=1,format=fat:floppy,file=/tmp/images
>
> Regards,
> Daniel
>   

Re: [Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Uri Lublin]

Daniel P. Berrange wrote:
> On Thu, Jan 08, 2009 at 08:15:25PM +0200, Uri Lublin wrote:
>> The purpose of this prefix is to
>> 1. Provide a way to know the backing file format without probing
>>    it (setting the format upon creation time).
>> 2. Enable using qcow2 format (and others) over host block devices.
>>    (only if the user specifically asks for it).
>>
>> If no fmt:FMT: is provided we go back to probing.
> 
> I still don't like the fact that this is inventing a second syntax
> for specifying format that's different to the syntax used for  the 
> existing -drive parameter, which is
> 
>   -drive file=/some/path,format=qcow2,....other disk options...
> 
>> backing file format is qcow2 (even though it's on a host block device)
>> $ qemu-img create -b fmt:qcow2:/dev/loop0 -f qcow2 /tmp/uuu.qcow2
> 
> I'd prefer to see a '-F' flag to specify format of backing file and
> leave syntax of existing -b arg alone
> 
>  $ qemu-img create -F qcow2 -b /dev/loop0 -f qcow2 /tmp/uuu.qcow2

I can easily modify qemu-img to support -F.
The question is how to represent it such that when we run qemu it "knows" the 
backing file format. In other words what will '-F qcow2' do ? In this solution 
it will add 'fmt:qcow2:' as a prefix to the backing-file filename.

> 
>> force backing file format to raw (no probing)
>> $ qemu-img create -f raw /tmp/image1.raw 10G
>> $ qemu-img create -b fmt:raw:/tmp/image1.raw -f qcow2 /tmp/image1.qcow2
> 
>   $ qemu-img create -F raw -b /tmp/image1.raw -f qcow2 /tmp/image1.qcow2
> 
>>
>> Or fat
>> $ qemu-system-x86_64 -hda fmt:qcow2:/tmp/uuu.qcow2 -hdb 
>> fat:floppy:/tmp/images
> 
> This is unneccessary, since -hda is deprecated, and there's a 
> new -drive arg that already has ability to set format explicitly,
> as well as many other flags that you need when setting up disks.
> 
>    $ qemu-system-x86_64 \ 
>          -drive index=0,format=qcow2,file=/tmp/uuu.qcow2 \
>          -drive index=1,format=fat:floppy,file=/tmp/images

Again that's true only for the leaf (writeable) images.

Maybe the examples I've shown were confusing.
The main issues we are trying to solve with this format are:
1. When opening a backing file, know what the format is to prevent probing.
2. Enable using qcow2 over host-devices (e.g qcow2 format image on an LVM lv 
such as /dev/mapper/mygroup-myvm)

Regards,
    Uri.

Re: [Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Daniel P. Berrange]

On Thu, Jan 08, 2009 at 09:09:08PM +0200, Uri Lublin wrote:
> Daniel P. Berrange wrote:
> >On Thu, Jan 08, 2009 at 08:15:25PM +0200, Uri Lublin wrote:
> >>The purpose of this prefix is to
> >>1. Provide a way to know the backing file format without probing
> >>   it (setting the format upon creation time).
> >>2. Enable using qcow2 format (and others) over host block devices.
> >>   (only if the user specifically asks for it).
> >>
> >>If no fmt:FMT: is provided we go back to probing.
> >
> >I still don't like the fact that this is inventing a second syntax
> >for specifying format that's different to the syntax used for  the 
> >existing -drive parameter, which is
> >
> >  -drive file=/some/path,format=qcow2,....other disk options...
> >
> >>backing file format is qcow2 (even though it's on a host block device)
> >>$ qemu-img create -b fmt:qcow2:/dev/loop0 -f qcow2 /tmp/uuu.qcow2
> >
> >I'd prefer to see a '-F' flag to specify format of backing file and
> >leave syntax of existing -b arg alone
> >
> > $ qemu-img create -F qcow2 -b /dev/loop0 -f qcow2 /tmp/uuu.qcow2
> 
> I can easily modify qemu-img to support -F.
> The question is how to represent it such that when we run qemu it "knows" 
> the backing file format. In other words what will '-F qcow2' do ? In this 
> solution it will add 'fmt:qcow2:' as a prefix to the backing-file filename.

Yes the format you write into the qcow2 header can still use the 
fmt:qcow2: prefix - just no need to expose that particular qcow2
specific implementation detail on the CLI. 

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Uri Lublin]

Daniel P. Berrange wrote:
> On Thu, Jan 08, 2009 at 09:09:08PM +0200, Uri Lublin wrote:
>> Daniel P. Berrange wrote:
>>> On Thu, Jan 08, 2009 at 08:15:25PM +0200, Uri Lublin wrote:
>>>> The purpose of this prefix is to
>>>> 1. Provide a way to know the backing file format without probing
>>>>   it (setting the format upon creation time).
>>>> 2. Enable using qcow2 format (and others) over host block devices.
>>>>   (only if the user specifically asks for it).
>>>>
>>>> If no fmt:FMT: is provided we go back to probing.
>>> I still don't like the fact that this is inventing a second syntax
>>> for specifying format that's different to the syntax used for  the 
>>> existing -drive parameter, which is
>>>
>>>  -drive file=/some/path,format=qcow2,....other disk options...
>>>
>>>> backing file format is qcow2 (even though it's on a host block device)
>>>> $ qemu-img create -b fmt:qcow2:/dev/loop0 -f qcow2 /tmp/uuu.qcow2
>>> I'd prefer to see a '-F' flag to specify format of backing file and
>>> leave syntax of existing -b arg alone
>>>
>>> $ qemu-img create -F qcow2 -b /dev/loop0 -f qcow2 /tmp/uuu.qcow2
>> I can easily modify qemu-img to support -F.
>> The question is how to represent it such that when we run qemu it "knows" 
>> the backing file format. In other words what will '-F qcow2' do ? In this 
>> solution it will add 'fmt:qcow2:' as a prefix to the backing-file filename.
> 
> Yes the format you write into the qcow2 header can still use the 
> fmt:qcow2: prefix - just no need to expose that particular qcow2
> specific implementation detail on the CLI. 

OK. I'll prepare a patch that adds '-F fmt' to 'qemu-img create -b ...'

Thanks,
    Uri.

Re: [Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Jamie Lokier]

Uri Lublin wrote:
> Daniel P. Berrange wrote:
> >Yes the format you write into the qcow2 header can still use the 
> >fmt:qcow2: prefix - just no need to expose that particular qcow2
> >specific implementation detail on the CLI. 
> 
> OK. I'll prepare a patch that adds '-F fmt' to 'qemu-img create -b ...'

Will it break backing files whose filename begins with "fmt:"?

Just because I hate hidden "some filenames work, some filenames break
mysteriously, and of course it's not documented" dodgy hacks.

-- Jamie

Re: [Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Anthony Liguori]

Jamie Lokier wrote:
> Uri Lublin wrote:
>   
>> Daniel P. Berrange wrote:
>>     
>>> Yes the format you write into the qcow2 header can still use the 
>>> fmt:qcow2: prefix - just no need to expose that particular qcow2
>>> specific implementation detail on the CLI. 
>>>       
>> OK. I'll prepare a patch that adds '-F fmt' to 'qemu-img create -b ...'
>>     
>
> Will it break backing files whose filename begins with "fmt:"?
>
> Just because I hate hidden "some filenames work, some filenames break
> mysteriously, and of course it's not documented" dodgy hacks.
>   

I'm pretty certain that you couldn't do that today in QEMU.  The colon 
has always been used for protocol checking and if the protocol is 
invalid, I think we throw an error.

Regards,

Anthony Liguori

> -- Jamie
>
>
>   

Re: [Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Jamie Lokier]

Anthony Liguori wrote:
> >Just because I hate hidden "some filenames work, some filenames break
> >mysteriously, and of course it's not documented" dodgy hacks.
> >  
> 
> I'm pretty certain that you couldn't do that today in QEMU.  The colon 
> has always been used for protocol checking and if the protocol is 
> invalid, I think we throw an error.

That sounds like a prime example of "some filenames break today
already and of course it's not documented" :-)

My own kvm/qemu management scripts create symlinks to the disk images
and pass the symlink names to kvm/qemu instead of the real names for
this reason...  And also because what you can enter in the monitor
"change" command isn't quite the same as what you can put on the
command line.

-- Jamie

Re: [Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Kevin Wolf]

Jamie Lokier schrieb:
> Uri Lublin wrote:
>> Daniel P. Berrange wrote:
>>> Yes the format you write into the qcow2 header can still use the 
>>> fmt:qcow2: prefix - just no need to expose that particular qcow2
>>> specific implementation detail on the CLI. 
>> OK. I'll prepare a patch that adds '-F fmt' to 'qemu-img create -b ...'
> 
> Will it break backing files whose filename begins with "fmt:"?
> 
> Just because I hate hidden "some filenames work, some filenames break
> mysteriously, and of course it's not documented" dodgy hacks.

Uri, now that you are making the change in qcow2 instead of block.c,
have you thought about using the earlier suggested "filename.img\0qcow2"
hack? This shouldn't break filenames.

Kevin

Re: [Qemu-devel] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Uri Lublin]

Kevin Wolf wrote:
> Jamie Lokier schrieb:
>> Uri Lublin wrote:
>>> Daniel P. Berrange wrote:
>>>> Yes the format you write into the qcow2 header can still use the 
>>>> fmt:qcow2: prefix - just no need to expose that particular qcow2
>>>> specific implementation detail on the CLI. 
>>> OK. I'll prepare a patch that adds '-F fmt' to 'qemu-img create -b ...'
>> Will it break backing files whose filename begins with "fmt:"?
>>
>> Just because I hate hidden "some filenames work, some filenames break
>> mysteriously, and of course it's not documented" dodgy hacks.
> 
> Uri, now that you are making the change in qcow2 instead of block.c,
> have you thought about using the earlier suggested "filename.img\0qcow2"
> hack? This shouldn't break filenames.
> 

Hi Kevin,

1. The change is still in block.c, as the backing file is opened in 
bdrv_open2(), and also we must support raw format as a backing_file format (and 
basically any format).
2. I thought everyone agreed that "filename.img\0qcow2" would work, but that we 
should try a different solution.
3. This patch does not make things worse (with regards to "unsupported 
filename"). Without it too, qemu will refuse to run a filename that begins with 
"fmt:"

Thanks,
     Uri.