[Qemu-devel][PATCH] Qemu image over raw devices

[Qemu-devel][PATCH] Qemu image over raw devices

[Shahar Frank]

[-- Attachment #1: Type: text/plain, Size: 3631 bytes --]

Hi,

The following patch enables QEMU to create and use images with any 
format on top of a raw device. Note that -f <format> is not enough for 
bcking files support.

The patch includes the following:

1. The check for block devices is weaken so you can override it by 
specifying a protocol
2. If a protocol exists but not found in the protocols list, the logic 
falls back to image type probing. This means use can write 
"probe:filename" or just ":filename"

Note that if regular file/device path names are used, the previous 
behavior is kept.

lvcreate -L 5G -n base store
dd bs=32k if=win.qcow2 of=/dev/store/base
./qemu-img info :/dev/store/base
lvcreate -L 2G -n l2 store
./qemu-img create -b :/dev/store/base -f qcow2 /dev/store/l2
./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l2 -L pc-bios/
lvcreate -L 2G -n l3 store
./qemu-img create -b :/dev/store/l2 -f qcow2 /dev/store/l3
./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l3 -L pc-bios/

Signed-off-by: Shahar Frank <sfrank@redhat.com>

diff --git a/block.c b/block.c
index 28d63d7..cbf9968 100644
--- a/block.c
+++ b/block.c
@@ -226,6 +226,17 @@ static int is_windows_drive(const char *filename)
  }
  #endif

+static const char *raw_filename(const char *filename)
+{
+    char *_filename;
+
+    _filename = strchr(filename, ':');
+    if (_filename)
+        return _filename + 1;
+    else
+        return filename;
+}
+
  static BlockDriver *find_protocol(const char *filename)
  {
      BlockDriver *drv1;
@@ -267,25 +278,28 @@ 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 == &bdrv_raw) {
  #ifdef _WIN32
-    if (is_windows_drive(filename))
-        return &bdrv_host_device;
+		if (is_windows_drive(filename))
+			return &bdrv_host_device;
  #else
-    {
-        struct stat st;
-        if (stat(filename, &st) >= 0 &&
-            (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
-            return &bdrv_host_device;
-        }
-    }
+		{
+			struct stat st;
+			if (stat(filename, &st) >= 0 &&
+				(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
+					return &bdrv_host_device;
+				}
+		}
  #endif
+    }

-    drv = find_protocol(filename);
      /* no need to test disk image formats for vvfat */
      if (drv == &bdrv_vvfat)
          return drv;

-    ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY);
+    ret = bdrv_file_open(&bs, raw_filename(filename), BDRV_O_RDONLY);
      if (ret < 0)
          return NULL;
      ret = bdrv_pread(bs, 0, buf, sizeof(buf));
@@ -335,6 +349,7 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
      int ret, open_flags;
      char tmp_filename[PATH_MAX];
      char backing_filename[PATH_MAX];
+    char const *_filename;

      bs->read_only = 0;
      bs->is_temporary = 0;
@@ -403,9 +418,12 @@ 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);
-    ret = drv->bdrv_open(bs, filename, open_flags);
+
+    _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);
+        ret = drv->bdrv_open(bs, _filename, open_flags & ~BDRV_O_RDWR);
          bs->read_only = 1;
      }
      if (ret < 0) {
diff --git a/qemu-img.c b/qemu-img.c



[-- Attachment #2: qemu-image-over-raw.patch --]
[-- Type: text/plain, Size: 2591 bytes --]

diff --git a/block.c b/block.c
index 28d63d7..cbf9968 100644
--- a/block.c
+++ b/block.c
@@ -226,6 +226,17 @@ static int is_windows_drive(const char *filename)
 }
 #endif
 
+static const char *raw_filename(const char *filename)
+{
+    char *_filename;
+
+    _filename = strchr(filename, ':');
+    if (_filename)
+        return _filename + 1;
+    else
+        return filename;
+}
+
 static BlockDriver *find_protocol(const char *filename)
 {
     BlockDriver *drv1;
@@ -267,25 +278,28 @@ 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 == &bdrv_raw) {
 #ifdef _WIN32
-    if (is_windows_drive(filename))
-        return &bdrv_host_device;
+		if (is_windows_drive(filename))
+			return &bdrv_host_device;
 #else
-    {
-        struct stat st;
-        if (stat(filename, &st) >= 0 &&
-            (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
-            return &bdrv_host_device;
-        }
-    }
+		{
+			struct stat st;
+			if (stat(filename, &st) >= 0 &&
+				(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
+					return &bdrv_host_device;
+				}
+		}
 #endif
+    }
 
-    drv = find_protocol(filename);
     /* no need to test disk image formats for vvfat */
     if (drv == &bdrv_vvfat)
         return drv;
 
-    ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY);
+    ret = bdrv_file_open(&bs, raw_filename(filename), BDRV_O_RDONLY);
     if (ret < 0)
         return NULL;
     ret = bdrv_pread(bs, 0, buf, sizeof(buf));
@@ -335,6 +349,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     int ret, open_flags;
     char tmp_filename[PATH_MAX];
     char backing_filename[PATH_MAX];
+    char const *_filename;
 
     bs->read_only = 0;
     bs->is_temporary = 0;
@@ -403,9 +418,12 @@ 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);
-    ret = drv->bdrv_open(bs, filename, open_flags);
+    
+    _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);
+        ret = drv->bdrv_open(bs, _filename, open_flags & ~BDRV_O_RDWR);
         bs->read_only = 1;
     }
     if (ret < 0) {
diff --git a/qemu-img.c b/qemu-img.c

Re: [Qemu-devel][PATCH] Qemu image over raw devices

[Kevin Wolf]

Shahar Frank schrieb:
> The following patch enables QEMU to create and use images with any
> format on top of a raw device. Note that -f <format> is not enough for
> bcking files support.

When would I need to explicitly specify the type of a backing file?

> The patch includes the following:
> 
> 1. The check for block devices is weaken so you can override it by
> specifying a protocol
> 2. If a protocol exists but not found in the protocols list, the logic
> falls back to image type probing. This means use can write
> "probe:filename" or just ":filename"

IIUC, on qemu side this is just another syntax for -drive format=xyz?
Wouldn't it be better to add a parameter to qemu-img then instead of
inventing new ways of specifying the format?

> Note that if regular file/device path names are used, the previous
> behavior is kept.
> 
> lvcreate -L 5G -n base store
> dd bs=32k if=win.qcow2 of=/dev/store/base
> ./qemu-img info :/dev/store/base
> lvcreate -L 2G -n l2 store
> ./qemu-img create -b :/dev/store/base -f qcow2 /dev/store/l2
> ./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l2 -L pc-bios/
> lvcreate -L 2G -n l3 store
> ./qemu-img create -b :/dev/store/l2 -f qcow2 /dev/store/l3
> ./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l3 -L pc-bios/

Does it even make sense to store qcow2 images on raw block devices?
qcow2 are usually growing whereas devices tend to not change their size.

Kevin

Re: [Qemu-devel][PATCH] Qemu image over raw devices

[Shahar Frank]

----- "Kevin Wolf" <kwolf@suse.de> wrote:

> Shahar Frank schrieb:
> > The following patch enables QEMU to create and use images with any
> > format on top of a raw device. Note that -f <format> is not enough
> for
> > bcking files support.
> 
> When would I need to explicitly specify the type of a backing file?

The patch doesn't allow you to specify a type (image format). It allows you to force probing. This is done to override the default block-device => raw semantics.

> 
> > The patch includes the following:
> >
> > 1. The check for block devices is weaken so you can override it by
> > specifying a protocol
> > 2. If a protocol exists but not found in the protocols list, the
> logic
> > falls back to image type probing. This means use can write
> > "probe:filename" or just ":filename"
> 
> IIUC, on qemu side this is just another syntax for -drive format=xyz?
> Wouldn't it be better to add a parameter to qemu-img then instead of
> inventing new ways of specifying the format?

The problem is with the backing file, this format does not apply to the backing file and this is the correct behavior - the backing file can be of a different format. Note that the new way is just forcing probing.

> 
> > Note that if regular file/device path names are used, the previous
> > behavior is kept.
> >
> > lvcreate -L 5G -n base store
> > dd bs=32k if=win.qcow2 of=/dev/store/base
> > ./qemu-img info :/dev/store/base
> > lvcreate -L 2G -n l2 store
> > ./qemu-img create -b :/dev/store/base -f qcow2 /dev/store/l2
> > ./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l2 -L pc-bios/
> > lvcreate -L 2G -n l3 store
> > ./qemu-img create -b :/dev/store/l2 -f qcow2 /dev/store/l3
> > ./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l3 -L pc-bios/
> 
> Does it even make sense to store qcow2 images on raw block devices?
> qcow2 are usually growing whereas devices tend to not change their
> size.
> 

The idea is to allow QCOW2 (or similar formats) capabilities in SAN only environment, where SAN-FS is not applicable (for example because it is too expensive or too complex). 

For the size issue, Logical volumes can be extended. In the near future some patches that allow monitoring the internal space usage and then extend the LV size are going to be posted to this list. Another issue that has to be handled is out of space (out of range) scenarios.

> Kevin

Shahar

Re: [Qemu-devel][PATCH] Qemu image over raw devices

[Kevin Wolf]

Shahar Frank schrieb:
> ----- "Kevin Wolf" <kwolf@suse.de> wrote:
> 
>> Shahar Frank schrieb:
>>> The following patch enables QEMU to create and use images with any
>>> format on top of a raw device. Note that -f <format> is not enough
>> for
>>> bcking files support.
>> When would I need to explicitly specify the type of a backing file?
> 
> The patch doesn't allow you to specify a type (image format). It allows you to force probing. This is done to override the default block-device => raw semantics.

Ok, I see. But didn't we want to get rid of the probing whenever
possible because you can't tell raw files from whatever other format
reliably?

>>> The patch includes the following:
>>>
>>> 1. The check for block devices is weaken so you can override it by
>>> specifying a protocol
>>> 2. If a protocol exists but not found in the protocols list, the
>> logic
>>> falls back to image type probing. This means use can write
>>> "probe:filename" or just ":filename"
>> IIUC, on qemu side this is just another syntax for -drive format=xyz?
>> Wouldn't it be better to add a parameter to qemu-img then instead of
>> inventing new ways of specifying the format?
> 
> The problem is with the backing file, this format does not apply to the backing file and this is the correct behavior - the backing file can be of a different format. Note that the new way is just forcing probing.

You don't specify the backing file on the command line, so that's not
what I meant. I really thought only of the parameters to qemu you
specify on the command line.

IMHO, the really correct behaviour would be that the image doesn't only
save the path but also the image format of the backing file - and then
add a parameter to qemu-img to specify that type.

This would need a change to qcow2 of course, but I think you can make it
a compatible change: Writing something like "image.qcow2\0qcow2" as
backing file to the image should do the trick. And then qcow2 would
check if the driver is specified and opens the file with exactly that
driver instead of guessing and possibly being fooled by a bad guest (ok,
it's not that bad with a backing file because the guest can't write to
it directly, but you can still commit).

>>> Note that if regular file/device path names are used, the previous
>>> behavior is kept.
>>>
>>> lvcreate -L 5G -n base store
>>> dd bs=32k if=win.qcow2 of=/dev/store/base
>>> ./qemu-img info :/dev/store/base
>>> lvcreate -L 2G -n l2 store
>>> ./qemu-img create -b :/dev/store/base -f qcow2 /dev/store/l2
>>> ./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l2 -L pc-bios/
>>> lvcreate -L 2G -n l3 store
>>> ./qemu-img create -b :/dev/store/l2 -f qcow2 /dev/store/l3
>>> ./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l3 -L pc-bios/
>> Does it even make sense to store qcow2 images on raw block devices?
>> qcow2 are usually growing whereas devices tend to not change their
>> size.
>>
> 
> The idea is to allow QCOW2 (or similar formats) capabilities in SAN only environment, where SAN-FS is not applicable (for example because it is too expensive or too complex). 
> 
> For the size issue, Logical volumes can be extended. In the near future some patches that allow monitoring the internal space usage and then extend the LV size are going to be posted to this list. Another issue that has to be handled is out of space (out of range) scenarios.

That sounds interesting. :-) How is growing LVs performance-wise? Just
about the same as growing a file or does it take noticably longer?

Kevin

Re: [Qemu-devel][PATCH] Qemu image over raw devices

[Shahar Frank]

Kevin Wolf wrote:
> Shahar Frank schrieb:
>> ----- "Kevin Wolf" <kwolf@suse.de> wrote:
>>
>>> Shahar Frank schrieb:
>>>> The following patch enables QEMU to create and use images with any
>>>> format on top of a raw device. Note that -f <format> is not enough
>>> for
>>>> bcking files support.
>>> When would I need to explicitly specify the type of a backing file?
>> The patch doesn't allow you to specify a type (image format). It allows you to force probing. This is done to override the default block-device => raw semantics.
> 
> Ok, I see. But didn't we want to get rid of the probing whenever
> possible because you can't tell raw files from whatever other format
> reliably?
> 
>>>> The patch includes the following:
>>>>
>>>> 1. The check for block devices is weaken so you can override it by
>>>> specifying a protocol
>>>> 2. If a protocol exists but not found in the protocols list, the
>>> logic
>>>> falls back to image type probing. This means use can write
>>>> "probe:filename" or just ":filename"
>>> IIUC, on qemu side this is just another syntax for -drive format=xyz?
>>> Wouldn't it be better to add a parameter to qemu-img then instead of
>>> inventing new ways of specifying the format?
>> The problem is with the backing file, this format does not apply to the backing file and this is the correct behavior - the backing file can be of a different format. Note that the new way is just forcing probing.
> 
> You don't specify the backing file on the command line, so that's not
> what I meant. I really thought only of the parameters to qemu you
> specify on the command line.
> 

You specify the backing file in the "qemu-img create" command line.

> IMHO, the really correct behaviour would be that the image doesn't only
> save the path but also the image format of the backing file - and then
> add a parameter to qemu-img to specify that type.
> 
> This would need a change to qcow2 of course, but I think you can make it
> a compatible change: Writing something like "image.qcow2\0qcow2" as
> backing file to the image should do the trick. And then qcow2 would
> check if the driver is specified and opens the file with exactly that
> driver instead of guessing and possibly being fooled by a bad guest (ok,
> it's not that bad with a backing file because the guest can't write to
> it directly, but you can still commit).
> 

This is not a bad idea, but I think it is too close to be a hack ;-). 
The danger with such a hack is that it may introduce new unexpected 
corner cases such as cluster overflow due that added bytes, code that 
will be confused due the added sting (non zero terminated logic), etc. 
If QCOW2 had a general way to include extensions, I would use that, but 
I think that for now, all I suggesting is a way that use the existing 
protocol notation to force probing.

In fact, what I do is just use the following logic (which was already 
partially used): if you can't find the specified protocol fall back to 
probing. Please note that probing is anyhow the default behavior in most 
cases, so the change is just a way to override the block device default 
behavior.

An additional way I considered is just declaring QCOW2 as a protocol. 
This would allow you to specify "qcow2:/dev/store/lv1". But this blurs 
the boundaries between protocols and image formats (a boundary that is 
not clear enough even today...).

>>>> Note that if regular file/device path names are used, the previous
>>>> behavior is kept.
>>>>
>>>> lvcreate -L 5G -n base store
>>>> dd bs=32k if=win.qcow2 of=/dev/store/base
>>>> ./qemu-img info :/dev/store/base
>>>> lvcreate -L 2G -n l2 store
>>>> ./qemu-img create -b :/dev/store/base -f qcow2 /dev/store/l2
>>>> ./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l2 -L pc-bios/
>>>> lvcreate -L 2G -n l3 store
>>>> ./qemu-img create -b :/dev/store/l2 -f qcow2 /dev/store/l3
>>>> ./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l3 -L pc-bios/
>>> Does it even make sense to store qcow2 images on raw block devices?
>>> qcow2 are usually growing whereas devices tend to not change their
>>> size.
>>>
>> The idea is to allow QCOW2 (or similar formats) capabilities in SAN only environment, where SAN-FS is not applicable (for example because it is too expensive or too complex). 
>>
>> For the size issue, Logical volumes can be extended. In the near future some patches that allow monitoring the internal space usage and then extend the LV size are going to be posted to this list. Another issue that has to be handled is out of space (out of range) scenarios.
> 
> That sounds interesting. :-) How is growing LVs performance-wise? Just
> about the same as growing a file or does it take noticably longer?
>

This depends on your block mapping mechanism and the extension unit 
size. For Linux and LVM2, the underlying mechanism is the device mapper, 
and as much as I could see, it is pretty scalable so you can program a 
table with large amount of mapping without significant performance 
overhead. There is an issue of the number of LVM side mappings, but if 
you work in large enough units (say 64MB, 128MB or larger) it should be 
OK. I didn't play with other logical volumes managers, but I guess that 
the status is pretty much the same. If anybody knows differently, I 
would glad to know about it.

Comparing to files (systems), the allocation/extension unit is typically 
much bigger (4MB comparing to 4KB in FS) and the mapping scheme is much 
simpler (in most cases plain linear, no holes support, no trees, etc.) 
so LVMs perform better but are much less flexible. For our specific case 
it is the correct trade off.

> Kevin
> 
> 
Shahar

Re: [Qemu-devel][PATCH] Qemu image over raw devices

[Daniel P. Berrange]

On Tue, Dec 16, 2008 at 10:40:03AM +0100, Kevin Wolf wrote:
> Shahar Frank schrieb:
> > ----- "Kevin Wolf" <kwolf@suse.de> wrote:
> > 
> >> Shahar Frank schrieb:
> >>> The following patch enables QEMU to create and use images with any
> >>> format on top of a raw device. Note that -f <format> is not enough
> >> for
> >>> bcking files support.
> >> When would I need to explicitly specify the type of a backing file?
> > 
> > The patch doesn't allow you to specify a type (image format). It allows you to force probing. This is done to override the default block-device => raw semantics.
> 
> Ok, I see. But didn't we want to get rid of the probing whenever
> possible because you can't tell raw files from whatever other format
> reliably?

Autoprobing of formats is usally a security flaw. ie, host admin configures
the guest with raw file, but autoprobing is enabled.  Guest admin now
writes magic into their disk to match the qcow header and reboots, qemu
now autoprobes the guest's disk as a grow on demand qcow format, letting
them basically create any size disk they like beyond the initial raw file
allocation. Even worse the guest could admin could have written a backing 
file location into the header and thus more or less get access to any file
they  like on the host. Autoprobing: just say no.

NB, I'm talking about context of qemu here, not qemu-img which is all
under host admin's control anyway so not an issue.

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][PATCH] Qemu image over raw devices

[Daniel P. Berrange]

On Mon, Dec 15, 2008 at 04:04:20PM +0200, Shahar Frank wrote:
> Hi,
> 
> The following patch enables QEMU to create and use images with any 
> format on top of a raw device. Note that -f <format> is not enough for 
> bcking files support.

> 1. The check for block devices is weaken so you can override it by 
> specifying a protocol
> 2. If a protocol exists but not found in the protocols list, the logic 
> falls back to image type probing. This means use can write 
> "probe:filename" or just ":filename"
>
> Note that if regular file/device path names are used, the previous 
> behavior is kept.
> 
> lvcreate -L 5G -n base store
> dd bs=32k if=win.qcow2 of=/dev/store/base
> ./qemu-img info :/dev/store/base
> lvcreate -L 2G -n l2 store
> ./qemu-img create -b :/dev/store/base -f qcow2 /dev/store/l2
> ./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l2 -L pc-bios/
> lvcreate -L 2G -n l3 store
> ./qemu-img create -b :/dev/store/l2 -f qcow2 /dev/store/l3
> ./x86_64-softmmu/qemu-system-x86_64 -hda :/dev/store/l3 -L pc-bios/

This embedded ':' syntax is really very disgusting. 

The -hda/b/c/d args are all legacy options defined to use autoprobing.
Since autoprobing is a potential security flaw these args should really 
never be used anymore where there's a possibility of using raw disk
formats. The new arg "-drive" already has support for explicitly
specifying the disk image format and so this doesn't need re-inventing
for block devices

   -drive file=/dev/store/l3,format=qcow2

It is already possible to create a qcow2 file on a block device giving
an explicit format

  # qemu-img  create -f qcow2 /dev/HostVG/Guest1 100M
  Formatting '/dev/HostVG/Guest1', fmt=qcow2, size=102400 kB
  # qemu-img info -f qcow2 /dev/HostVG/Guest1
  image: /dev/HostVG/Guest1
  file format: qcow2
  virtual size: 100M (104857600 bytes)
  disk size: 0
  cluster_size: 4096

So the missing bit at create time is being able to specify the format of
the backing store. Currently qemu-img always probes the backing store 
format which is not satisfactory. That's easily fixed though by adding 
an explicit  '-F backing_fmt' arg that can be used. 

  # qemu-img create -b /dev/HostVG/Backing -F qcow2 -f qcow2 /dev/HostVG/Guest1

The second problem is that the format of the backing file is never stored 
in the QCOw2 header so when the disk is used, it resorts to probing again.
The most desriable way to fix this would be to encode the format of the 
backing file in the  QCOw2 header, since we explicitly know this at time 
of the call which did 'qemu-img create -b backingfile'. That has compat
problems though, so we should extend the -drive arg to allow probing of 
backing file formats to be disabled (making it assume backing store is 
same format as child image), or to be fixed to a specific format (eg, 
fix the backing store as raw).

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][PATCH] Qemu image over raw devices

[Kevin Wolf]

Shahar Frank schrieb:
>> IMHO, the really correct behaviour would be that the image doesn't only
>> save the path but also the image format of the backing file - and then
>> add a parameter to qemu-img to specify that type.
>>
>> This would need a change to qcow2 of course, but I think you can make it
>> a compatible change: Writing something like "image.qcow2\0qcow2" as
>> backing file to the image should do the trick. And then qcow2 would
>> check if the driver is specified and opens the file with exactly that
>> driver instead of guessing and possibly being fooled by a bad guest (ok,
>> it's not that bad with a backing file because the guest can't write to
>> it directly, but you can still commit).
>>
> 
> This is not a bad idea, but I think it is too close to be a hack ;-).

Right. Extending a file format in a compatible way when the format is
not designed to be extended, is always hackish.

> The danger with such a hack is that it may introduce new unexpected
> corner cases such as cluster overflow due that added bytes, code that
> will be confused due the added sting (non zero terminated logic), etc.

In general yes, but I think in this case it's reasonable to make the
extension. The code that deals with this data is in two places: Creating
the image and opening it. When the image is opened, the path is copied
to a normal zero terminated string, so no problems there.

> If QCOW2 had a general way to include extensions, I would use that, but
> I think that for now, all I suggesting is a way that use the existing
> protocol notation to force probing.
> 
> In fact, what I do is just use the following logic (which was already
> partially used): if you can't find the specified protocol fall back to
> probing. Please note that probing is anyhow the default behavior in most
> cases, so the change is just a way to override the block device default
> behavior.

As Dan said, probing is error prone and likely to lead to security
problems. You should provide a way to specify the right format instead
of increasing security risks.

You really _can't_ tell raw images from other formats. So don't force
any user to do it to achieve what he wants.

> An additional way I considered is just declaring QCOW2 as a protocol.
> This would allow you to specify "qcow2:/dev/store/lv1". But this blurs
> the boundaries between protocols and image formats (a boundary that is
> not clear enough even today...).

That would be another possibility to specify the file format, yes.

With respect to blurring boundaries: Probably it should be defined first
what a protocol really is. There are people who believe that there is no
difference at all between formats and protocols. There are other people
who think it are two completely different things. And the code doesn't
say who's right.

> This depends on your block mapping mechanism and the extension unit
> size. For Linux and LVM2, the underlying mechanism is the device mapper,
> and as much as I could see, it is pretty scalable so you can program a
> table with large amount of mapping without significant performance
> overhead. There is an issue of the number of LVM side mappings, but if
> you work in large enough units (say 64MB, 128MB or larger) it should be
> OK. I didn't play with other logical volumes managers, but I guess that
> the status is pretty much the same. If anybody knows differently, I
> would glad to know about it.
> 
> Comparing to files (systems), the allocation/extension unit is typically
> much bigger (4MB comparing to 4KB in FS) and the mapping scheme is much
> simpler (in most cases plain linear, no holes support, no trees, etc.)
> so LVMs perform better but are much less flexible. For our specific case
> it is the correct trade off.

Sounds good. I'm looking forward to your upcoming patches.

Kevin

Re: [Qemu-devel][PATCH] Qemu image over raw devices

[Shahar Frank]

Kevin, Daniel,

What do you think is the right thing to do here?

Option one:

1. I can suggest a new extension mechanism for QCOW2 (it can be even 
backwards compatible) and store the backing file format there
2. I can add a flag to qemu-img create to force backing file format 
(that is stored in the above extension).

Or:

Option two:

I can register qcow2 as a protocol and allow users to explicitly force 
qcow2 image logic.
It is true that the ":" notation is still used, but as this notation is 
still required even for the drive option (vvfat, for example) it won't 
be the first to introduce it to the system.

I have a feeling you both prefer option one ;-)

Shahar

Re: [Qemu-devel][PATCH] Qemu image over raw devices

[Kevin Wolf]

Shahar Frank schrieb:
> Kevin, Daniel,
> 
> What do you think is the right thing to do here?
> 
> Option one:
> 
> 1. I can suggest a new extension mechanism for QCOW2 (it can be even
> backwards compatible) and store the backing file format there
> 2. I can add a flag to qemu-img create to force backing file format
> (that is stored in the above extension).
> 
> Or:
> 
> Option two:
> 
> I can register qcow2 as a protocol and allow users to explicitly force
> qcow2 image logic.
> It is true that the ":" notation is still used, but as this notation is
> still required even for the drive option (vvfat, for example) it won't
> be the first to introduce it to the system.
> 
> I have a feeling you both prefer option one ;-)

I think I'm not really decided on that one. For me, the most important
thing is that you don't force users to rely on guessing. So while I
slightly tend towards the first option, I also wouldn't be opposed to
the second one.

Kevin

Re: [Qemu-devel][PATCH] Qemu image over raw devices

[Shahar Frank]

Kevin Wolf wrote:
> Shahar Frank schrieb:
>> Kevin, Daniel,
>>
>> What do you think is the right thing to do here?
>>
>> Option one:
>>
>> 1. I can suggest a new extension mechanism for QCOW2 (it can be even
>> backwards compatible) and store the backing file format there
>> 2. I can add a flag to qemu-img create to force backing file format
>> (that is stored in the above extension).
>>
>> Or:
>>
>> Option two:
>>
>> I can register qcow2 as a protocol and allow users to explicitly force
>> qcow2 image logic.
>> It is true that the ":" notation is still used, but as this notation is
>> still required even for the drive option (vvfat, for example) it won't
>> be the first to introduce it to the system.
>>
>> I have a feeling you both prefer option one ;-)
> 
> I think I'm not really decided on that one. For me, the most important
> thing is that you don't force users to rely on guessing. So while I
> slightly tend towards the first option, I also wouldn't be opposed to
> the second one.
> 
> Kevin
> 
> 

Kevin, thanks for the input. I think I will try to suggest a patch to 
implement option 2.

My reasons are:
1. The raw image + probing is not caused by the current patch and not 
worsen by the current patch. It just allow you to use probing if you want.
2. I think it much more dangerous to mess with the qcow2 header than 
adding this probe option.
3. The security problem can be solved by various ways. For example, use 
probing just for non raw entities. If you know it is raw, force it to be 
  raw (using one of the current ways). This will solve the security 
problem. I have some other ideas to solve this issue, but none of them 
are related to the current patch - it is completely different issue. I 
hope to post such (additional) patch soon.

Shahar