The two image formats called qcow

The two image formats called qcow

[Kevin Wolf]

Hi,

the idea of the recently submitted tap:ioemu block driver was that it
would be a drop-in replacement for all the other tap:* so that in the
long term tapdisk could be abandoned. This should work quite well
because we're having a block driver for each format supported by tapdisk
in ioemu as well.

So far for the theory. In practice this doesn't prove to be true: There
is a blktap driver claiming to implement a format called qcow and there
is an ioemu driver for qcow - and both formats are not the same, of
course. (The reason is that the original qcow uses a big endian L1 table
whereas blktap uses little endian - I honestly cannot imagine how one
could change this unintentionally, but OTOH why would you want to break
compatibility for no clear benefit?)

This does not only mean that you cannot use qcow images created for
blktap with qemu, but also that PV and HVM qcow have always been
incompatible. Am I really the first one to notice this?

Now if we're going to use ioemu as the one and only block driver code,
this will be a problem. How should this be handled best? We could add
code to ioemu to deal with the broken blktap images using some
heuristics (and maybe convert endianess whenever you open such a broken
file). This would mean that we have to carry a fix for a bug in older
versions forever. The other possibility would be to let the user convert
the old broken image files manually (with a new tool) and keep ioemu clean.

Which solution would you prefer? Or maybe you have completely different,
better ideas how to deal with it?

Kevin

Re: The two image formats called qcow

[Otavio Salvador]

Kevin Wolf <kwolf@suse.de> writes:

> Which solution would you prefer? Or maybe you have completely different,
> better ideas how to deal with it?

If it was up to me, I'd choose to convert the image and keep the code
clean. Keeping workarounds forever shouldn't be an option.

-- 
Otavio Salvador                  O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854         http://projetos.ossystems.com.br

Re: The two image formats called qcow

[Keir Fraser]

On 25/3/08 18:58, "Otavio Salvador" <otavio@ossystems.com.br> wrote:

> Kevin Wolf <kwolf@suse.de> writes:
> 
>> Which solution would you prefer? Or maybe you have completely different,
>> better ideas how to deal with it?
> 
> If it was up to me, I'd choose to convert the image and keep the code
> clean. Keeping workarounds forever shouldn't be an option.

It's tricky where users' non-volatile storage is concerned though. Other
than that I would say the bug should be fixed immediately. Is there an easy
way to detect with reasonable reliability whether we have an old or new
image? Failing that we may have to provide a tool to convert old images to
new format.

 -- Keir

Re: The two image formats called qcow

[Kevin Wolf]

Keir Fraser schrieb:
> It's tricky where users' non-volatile storage is concerned though. Other
> than that I would say the bug should be fixed immediately. Is there an easy
> way to detect with reasonable reliability whether we have an old or new
> image? Failing that we may have to provide a tool to convert old images to
> new format.

Something like "that number looks too big, it be must little endian"
could easily turn into "that harddisk looks big, let's break the image",
I suspect.

However, I just noticed that the tapdisk qcow driver writes an extended
Xen-specific header to the image file. This should be reliable enough to
detect tapdisk images.

Is it an option to convert broken images to big endian when it is opened
for the first time in ioemu? In this case, the fix for older versions
could be in one place at least instead of being scattered over the whole
file. Then you wouldn't be able to open such a file with tapdisk again,
though.

Kevin

Re: The two image formats called qcow

[Otavio Salvador]

Kevin Wolf <kwolf@suse.de> writes:

> Is it an option to convert broken images to big endian when it is opened
> for the first time in ioemu? In this case, the fix for older versions
> could be in one place at least instead of being scattered over the whole
> file. Then you wouldn't be able to open such a file with tapdisk again,
> though.

If it could be done, a note in release notes would be required to warn
users that when it has been opened in a newer version using ioemu it
wouldn't be possible to be used again by tap driver anymore.

But yes, this would be an easy to use option if it's not too messy and
takes too long to convert the image (otherwise user can think that the
system has freeze).

-- 
Otavio Salvador                  O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854         http://projetos.ossystems.com.br

Re: The two image formats called qcow

[Daniel P. Berrange]

On Wed, Mar 26, 2008 at 09:50:06AM +0100, Kevin Wolf wrote:
> Keir Fraser schrieb:
> > It's tricky where users' non-volatile storage is concerned though. Other
> > than that I would say the bug should be fixed immediately. Is there an easy
> > way to detect with reasonable reliability whether we have an old or new
> > image? Failing that we may have to provide a tool to convert old images to
> > new format.
> 
> Something like "that number looks too big, it be must little endian"
> could easily turn into "that harddisk looks big, let's break the image",
> I suspect.
> 
> However, I just noticed that the tapdisk qcow driver writes an extended
> Xen-specific header to the image file. This should be reliable enough to
> detect tapdisk images.
> 
> Is it an option to convert broken images to big endian when it is opened
> for the first time in ioemu? In this case, the fix for older versions
> could be in one place at least instead of being scattered over the whole
> file. Then you wouldn't be able to open such a file with tapdisk again,
> though.

I don't like the idea of  secretly migrating them to  the fixed disk format
without admin interaction / confirmation.

If we can detect the old style disk format, then perhaps we could put a
check into the hotplug scripts. That way, when the user tries to start
a guest with the old broken format, we could prevent the guest starting
and show them a error message. Then can then run the conversion tool
to fix the format & start the guest again. 

Dan
-- 
|: Red Hat, Engineering, Boston   -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: The two image formats called qcow

[Kevin Wolf]

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

Otavio Salvador schrieb:
> Kevin Wolf <kwolf@suse.de> writes:
> 
>> Is it an option to convert broken images to big endian when it is opened
>> for the first time in ioemu? In this case, the fix for older versions
>> could be in one place at least instead of being scattered over the whole
>> file. Then you wouldn't be able to open such a file with tapdisk again,
>> though.
> 
> If it could be done, a note in release notes would be required to warn
> users that when it has been opened in a newer version using ioemu it
> wouldn't be possible to be used again by tap driver anymore.

I agree, a note wouldn't be wrong.

> But yes, this would be an easy to use option if it's not too messy and
> takes too long to convert the image (otherwise user can think that the
> system has freeze).

Converting the image means byte-swapping the L1 table which is some
kilobytes in size. This is done in no time, the user won't even notice
the delay. The attached patch implements this conversion.

What should we do with the tapdisk implementation? Leave it broken and
hope that it will disappear soon, add support for big endian L1 tables
or do a conversion the other way round? The latter doesn't feel right
(in fact it would be intentionally breaking a correct image), but adding
support for big endian is much more critical because we end up with
"mixed endian" if we miss one conversion...

Kevin


ioemu: Fix L1 table endianess of qcow images created by tapdisk

The qemu/ioemu implementation of the qcow format uses a big endian L1
table. tapdisk omits the necessary conversion, so qcow images have the
wrong endianess and cannot be read by correct implementations of qcow.

This patch detects broken tapdisk images and converts their L1 tables to
big endian when the image file is opened in ioemu for the first time.
The fixed image has a new flag EXTHDR_L1_BIG_ENDIAN set in the extended
header.

Note that a converted image cannot be opened by tapdisk again.

Signed-off-by: Kevin Wolf <kwolf@suse.de>

[-- Attachment #2: ioemu-qcow-fix-tapdisk-images.patch --]
[-- Type: text/x-patch, Size: 2506 bytes --]

diff -r 81147306041a tools/ioemu/block-qcow.c
--- a/tools/ioemu/block-qcow.c	Tue Mar 25 14:37:43 2008
+++ b/tools/ioemu/block-qcow.c	Wed Mar 26 15:25:37 2008
@@ -37,6 +37,11 @@
 
 #define QCOW_OFLAG_COMPRESSED (1LL << 63)
 
+#define XEN_MAGIC  (('X' << 24) | ('E' << 16) | ('N' << 8) | 0xfb)
+
+#define EXTHDR_SPARSE_FILE      0x01
+#define EXTHDR_L1_BIG_ENDIAN    0x02
+
 typedef struct QCowHeader {
     uint32_t magic;
     uint32_t version;
@@ -49,6 +54,14 @@
     uint32_t crypt_method;
     uint64_t l1_table_offset;
 } QCowHeader;
+
+/*Extended header for Xen enhancements*/
+typedef struct QCowHeader_ext {
+    uint32_t xmagic;
+    uint32_t cksum;
+    uint32_t min_cluster_alloc;
+    uint32_t flags;
+} QCowHeader_ext;
 
 #define L2_CACHE_SIZE 16
 
@@ -137,6 +150,51 @@
     if (bdrv_pread(s->hd, s->l1_table_offset, s->l1_table, s->l1_size * sizeof(uint64_t)) != 
         s->l1_size * sizeof(uint64_t))
         goto fail;
+
+    /* Try to detect old tapdisk images. They have to be fixed because they
+     * don't use big endian but native endianess for the L1 table */
+	if (header.backing_file_offset == 0 && s->l1_table_offset % 4096 == 0) {
+	
+        QCowHeader_ext exthdr;
+        uint64_t l1_bytes = s->l1_size * sizeof(uint64_t);
+
+        if (bdrv_pread(s->hd, sizeof(header), &exthdr, sizeof(exthdr)) 
+                != sizeof(exthdr))
+            goto end_xenhdr;
+
+		be32_to_cpus(&exthdr.xmagic);
+		if (exthdr.xmagic != XEN_MAGIC) 
+			goto end_xenhdr;
+
+		be32_to_cpus(&exthdr.flags);
+        if (exthdr.flags & EXTHDR_L1_BIG_ENDIAN)
+            goto end_xenhdr;
+
+        /* The image is broken. Fix it. */
+        fprintf(stderr, "qcow: Converting image to big endian L1 table\n");
+
+        for(i = 0;i < s->l1_size; i++) {
+            cpu_to_be64s(&s->l1_table[i]);
+        }
+    
+        if (bdrv_pwrite(s->hd, s->l1_table_offset, s->l1_table, 
+                l1_bytes) != l1_bytes) {
+            fprintf(stderr, "qcow: Failed to write new L1 table\n");
+            goto fail;
+        }
+
+        exthdr.flags |= EXTHDR_L1_BIG_ENDIAN;
+        cpu_to_be32s(&exthdr.flags);
+        
+        if (bdrv_pwrite(s->hd, sizeof(header), &exthdr, sizeof(exthdr)) 
+                != sizeof(exthdr)) {
+            fprintf(stderr, "qcow: Failed to write extended header\n");
+            goto fail;
+        }
+    }
+end_xenhdr:
+
+    /* L1 table is big endian now */
     for(i = 0;i < s->l1_size; i++) {
         be64_to_cpus(&s->l1_table[i]);
     }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: The two image formats called qcow

[Keir Fraser]

On 26/3/08 13:54, "Kevin Wolf" <kwolf@suse.de> wrote:

> ioemu: Fix L1 table endianess of qcow images created by tapdisk
> 
> The qemu/ioemu implementation of the qcow format uses a big endian L1
> table. tapdisk omits the necessary conversion, so qcow images have the
> wrong endianess and cannot be read by correct implementations of qcow.
> 
> This patch detects broken tapdisk images and converts their L1 tables to
> big endian when the image file is opened in ioemu for the first time.
> The fixed image has a new flag EXTHDR_L1_BIG_ENDIAN set in the extended
> header.
> 
> Note that a converted image cannot be opened by tapdisk again.

Can we really tack an 'extended header' into a public format like qcow?

 -- Keir

Re: The two image formats called qcow

[Kevin Wolf]

Keir Fraser schrieb:
> On 26/3/08 13:54, "Kevin Wolf" <kwolf@suse.de> wrote:
> 
>> ioemu: Fix L1 table endianess of qcow images created by tapdisk
>>
>> The qemu/ioemu implementation of the qcow format uses a big endian L1
>> table. tapdisk omits the necessary conversion, so qcow images have the
>> wrong endianess and cannot be read by correct implementations of qcow.
>>
>> This patch detects broken tapdisk images and converts their L1 tables to
>> big endian when the image file is opened in ioemu for the first time.
>> The fixed image has a new flag EXTHDR_L1_BIG_ENDIAN set in the extended
>> header.
>>
>> Note that a converted image cannot be opened by tapdisk again.
> 
> Can we really tack an 'extended header' into a public format like qcow?

I didn't introduce this, it was already there in tapdisk. I don't see a
problem with it as the start of the L1 table is referenced in the normal
qcow header. qemu-img sets this to something like 0x48 which is
immediately after the header, tapdisk uses 0x1000 and gains some unused
space for things like the extended header. This is compatible with the
qcow implementation of qemu/ioemu.

On the other hand, I could simply strip that extended header (i.e.
overwrite the magic with 0x0) after having fixed the image. Then it
wouldn't be detected as broken on the next start as well.

Kevin

Re: The two image formats called qcow

[Keir Fraser]

On 26/3/08 14:07, "Kevin Wolf" <kwolf@suse.de> wrote:

>> Can we really tack an 'extended header' into a public format like qcow?
> 
> I didn't introduce this, it was already there in tapdisk. I don't see a
> problem with it as the start of the L1 table is referenced in the normal
> qcow header. qemu-img sets this to something like 0x48 which is
> immediately after the header, tapdisk uses 0x1000 and gains some unused
> space for things like the extended header. This is compatible with the
> qcow implementation of qemu/ioemu.
> 
> On the other hand, I could simply strip that extended header (i.e.
> overwrite the magic with 0x0) after having fixed the image. Then it
> wouldn't be detected as broken on the next start as well.

Oh, I see. I think it's fine as it is then. Is there any reason not to paste
this fixup code into tapdisk too?

 -- Keir

Re: The two image formats called qcow

[Kevin Wolf]

Keir Fraser schrieb:
> On 26/3/08 14:07, "Kevin Wolf" <kwolf@suse.de> wrote:
> 
>>> Can we really tack an 'extended header' into a public format like qcow?
>> I didn't introduce this, it was already there in tapdisk. I don't see a
>> problem with it as the start of the L1 table is referenced in the normal
>> qcow header. qemu-img sets this to something like 0x48 which is
>> immediately after the header, tapdisk uses 0x1000 and gains some unused
>> space for things like the extended header. This is compatible with the
>> qcow implementation of qemu/ioemu.
>>
>> On the other hand, I could simply strip that extended header (i.e.
>> overwrite the magic with 0x0) after having fixed the image. Then it
>> wouldn't be detected as broken on the next start as well.
> 
> Oh, I see. I think it's fine as it is then. Is there any reason not to paste
> this fixup code into tapdisk too?

It's not done with the conversion in tapdisk. You would also need to
change all writes to the L1 table. Additionally, I noticed that this
glorious extended header contains a checksum over the L1 table. And I'm
not sure if there are other traps in that code.

As I explained in the mail containing the patch I really don't want to
end up with a "mixed endian" image by overlooking a needed change. You
could throw it away then. Better don't start the VM at all and let the
user specify tap:ioemu...

Kevin

[PATCH] tapdisk: Fix L1 table endianess of qcow images

[Kevin Wolf]

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

Kevin Wolf schrieb:
> What should we do with the tapdisk implementation? Leave it broken and
> hope that it will disappear soon, add support for big endian L1 tables
> or do a conversion the other way round? The latter doesn't feel right
> (in fact it would be intentionally breaking a correct image), but adding
> support for big endian is much more critical because we end up with
> "mixed endian" if we miss one conversion...

And another one for tapdisk. I'm taking the same approach as for ioemu
here, i.e. converting the endianess when the image is opened and
rewriting the tapdisk code to use big endian.

To avoid the mentioned "mixed endian" issue and thus data corruption,
please double check the patch before you check it in. I successfully
installed a VM with this patch, though, so I'm confident that it is correct.

Kevin


tapdisk: Fix L1 table endianess of qcow images

Fix tapdisk to use big endian L1 tables as used by qemu/ioemu. Old
tapdisk images with native endianess are automagically converted to big
endian when the image file is opened for the first time.

Signed-off-by: Kevin Wolf <kwolf@suse.de>

[-- Attachment #2: tapdisk-qcow-endianess.patch --]
[-- Type: text/x-patch, Size: 3200 bytes --]

diff -r 81147306041a tools/blktap/drivers/block-qcow.c
--- a/tools/blktap/drivers/block-qcow.c	Tue Mar 25 14:37:43 2008
+++ b/tools/blktap/drivers/block-qcow.c	Thu Mar 27 11:45:28 2008
@@ -76,6 +76,7 @@
 
 #define QCOW_OFLAG_COMPRESSED (1LL << 63)
 #define SPARSE_FILE 0x01
+#define EXTHDR_L1_BIG_ENDIAN 0x02
 
 #ifndef O_BINARY
 #define O_BINARY 0
@@ -147,19 +148,30 @@
 
 static uint32_t gen_cksum(char *ptr, int len)
 {
+	int i;
 	unsigned char *md;
 	uint32_t ret;
 
 	md = malloc(MD5_DIGEST_LENGTH);
 
 	if(!md) return 0;
-
-	if (MD5((unsigned char *)ptr, len, md) != md) {
-		free(md);
-		return 0;
-	}
-
-	memcpy(&ret, md, sizeof(uint32_t));
+	
+	/* Convert L1 table to big endian */
+	for(i = 0; i < len / sizeof(uint64_t); i++) {
+		cpu_to_be64s(&((uint64_t*) ptr)[i]);
+	}
+
+	/* Generate checksum */
+	if (MD5((unsigned char *)ptr, len, md) != md)
+		ret = 0;
+	else
+		memcpy(&ret, md, sizeof(uint32_t));
+
+	/* Convert L1 table back to native endianess */
+	for(i = 0; i < len / sizeof(uint64_t); i++) {
+		be64_to_cpus(&((uint64_t*) ptr)[i]);
+	}
+
 	free(md);
 	return ret;
 }
@@ -354,7 +366,8 @@
                                    int n_start, int n_end)
 {
 	int min_index, i, j, l1_index, l2_index, l2_sector, l1_sector;
-	char *tmp_ptr, *tmp_ptr2, *l2_ptr, *l1_ptr;
+	char *tmp_ptr2, *l2_ptr, *l1_ptr;
+	uint64_t *tmp_ptr;
 	uint64_t l2_offset, *l2_table, cluster_offset, tmp;
 	uint32_t min_count;
 	int new_l2_table;
@@ -400,6 +413,11 @@
 			DPRINTF("ERROR allocating memory for L1 table\n");
 		}
 		memcpy(tmp_ptr, l1_ptr, 4096);
+
+		/* Convert block to write to big endian */
+		for(i = 0; i < 4096 / sizeof(uint64_t); i++) {
+			cpu_to_be64s(&tmp_ptr[i]);
+		}
 
 		/*
 		 * Issue non-asynchronous L1 write.
@@ -777,7 +795,7 @@
 		goto fail;
 
 	for(i = 0; i < s->l1_size; i++) {
-		//be64_to_cpus(&s->l1_table[i]);
+		be64_to_cpus(&s->l1_table[i]);
 		//DPRINTF("L1[%d] => %llu\n", i, s->l1_table[i]);
 		if (s->l1_table[i] > final_cluster)
 			final_cluster = s->l1_table[i];
@@ -810,6 +828,38 @@
 		be32_to_cpus(&exthdr->xmagic);
 		if(exthdr->xmagic != XEN_MAGIC) 
 			goto end_xenhdr;
+    
+		/* Try to detect old tapdisk images. They have to be fixed because 
+		 * they don't use big endian but native endianess for the L1 table */
+		if ((exthdr->flags & EXTHDR_L1_BIG_ENDIAN) == 0) {
+
+			/* 
+			   The image is broken. Fix it. The L1 table has already been 
+			   byte-swapped, so we can write it to the image file as it is
+			   currently in memory. Then swap it back to native endianess
+			   for operation.
+			 */
+
+			DPRINTF("qcow: Converting image to big endian L1 table\n");
+
+			lseek(fd, s->l1_table_offset, SEEK_SET);
+			if (write(fd, s->l1_table, l1_table_size) != l1_table_size) {
+				DPRINTF("qcow: Failed to write new L1 table\n");
+				goto fail;
+			}
+
+			for(i = 0;i < s->l1_size; i++) {
+				cpu_to_be64s(&s->l1_table[i]);
+			}
+
+			/* Write the big endian flag to the extended header */
+			exthdr->flags |= EXTHDR_L1_BIG_ENDIAN;
+
+			if (write(fd, buf, 512) != 512) {
+				DPRINTF("qcow: Failed to write extended header\n");
+				goto fail;
+			}
+		}
 
 		/*Finally check the L1 table cksum*/
 		be32_to_cpus(&exthdr->cksum);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel