Re: [Qemu-devel] [PATCH] dmg: fix -open failure
Am 11.01.2010 18:47, schrieb Christoph Hellwig: On Mon, Jan 11, 2010 at 03:11:52PM +0100, Kevin Wolf wrote: Ok, if you start talking about layering, we can have a fundamental discussion on this topic and why the layering is broken anyway. Logically, we have image formats like qcow2, VMDK and raw, and they are stored in files, on CD-ROMs or general block devices. From a layering perspective, it is wrong to include the latter in the raw format driver in the first place. Actually, I think the differentiation between raw files and host_* is at the same level as protocols are. Probably they should be implemented very similarly. Do you think it's possible/worth the effort to try putting things straight here? So what you want is basically: - hdev_* and file as protocols in addition to nbd/ftp/http/.. - a raw image format that can be used ontop of any protocol instead of an image format Yes, this is pretty much what I was thinking of. That would indeed be a much better, not to say actually logical layering. The raw image format would be more or less a no-op just stacking ontop of the protocol. If we can find a way to implement this efficiently it might be the way to go. Right, getting it done for raw without losing performance was more or less my only concern. I mean, remapping directly to the protocol in bdrv_open is exactly what we want to avoid. The question is if stubs to pass requests down to the protocol are really that expensive. It shouldn't be much more than two additional function calls. Kevin
[Qemu-devel] [PATCH] dmg: fix -open failure
Currently the dmg image format driver simply opens the images as raw
if any kind of failure happens. This is contrarty to the behaviour
of all other image formats which just return an error and let the
block core deal with it.
Signed-off-by: Christoph Hellwig h...@lst.de
Index: qemu/block/dmg.c
===
--- qemu.orig/block/dmg.c 2010-01-11 14:00:25.945021645 +0100
+++ qemu/block/dmg.c2010-01-11 14:03:03.006036707 +0100
@@ -90,24 +90,21 @@ static int dmg_open(BlockDriverState *bs
/* read offset of info blocks */
if(lseek(s-fd,-0x1d8,SEEK_END)0) {
-dmg_close:
- close(s-fd);
- /* open raw instead */
- bs-drv=bdrv_find_format(raw);
- return bs-drv-bdrv_open(bs, filename, flags);
+goto fail;
}
+
info_begin=read_off(s-fd);
if(info_begin==0)
- goto dmg_close;
+ goto fail;
if(lseek(s-fd,info_begin,SEEK_SET)0)
- goto dmg_close;
+ goto fail;
if(read_uint32(s-fd)!=0x100)
- goto dmg_close;
+ goto fail;
if((count = read_uint32(s-fd))==0)
- goto dmg_close;
+ goto fail;
info_end = info_begin+count;
if(lseek(s-fd,0xf8,SEEK_CUR)0)
- goto dmg_close;
+ goto fail;
/* read offsets */
last_in_offset = last_out_offset = 0;
@@ -116,14 +113,14 @@ dmg_close:
count = read_uint32(s-fd);
if(count==0)
- goto dmg_close;
+ goto fail;
type = read_uint32(s-fd);
if(type!=0x6d697368 || count244)
lseek(s-fd,count-4,SEEK_CUR);
else {
int new_size, chunk_count;
if(lseek(s-fd,200,SEEK_CUR)0)
- goto dmg_close;
+ goto fail;
chunk_count = (count-204)/40;
new_size = sizeof(uint64_t) * (s-n_chunks + chunk_count);
s-types = qemu_realloc(s-types, new_size/2);
@@ -142,7 +139,7 @@ dmg_close:
chunk_count--;
i--;
if(lseek(s-fd,36,SEEK_CUR)0)
- goto dmg_close;
+ goto fail;
continue;
}
read_uint32(s-fd);
@@ -163,11 +160,14 @@ dmg_close:
s-compressed_chunk = qemu_malloc(max_compressed_size+1);
s-uncompressed_chunk = qemu_malloc(512*max_sectors_per_chunk);
if(inflateInit(s-zstream) != Z_OK)
- goto dmg_close;
+ goto fail;
s-current_chunk = s-n_chunks;
return 0;
+fail:
+close(s-fd);
+return -1;
}
static inline int is_sector_in_chunk(BDRVDMGState* s,
Re: [Qemu-devel] [PATCH] dmg: fix -open failure
Am 11.01.2010 14:06, schrieb Christoph Hellwig: Currently the dmg image format driver simply opens the images as raw if any kind of failure happens. This is contrarty to the behaviour of all other image formats which just return an error and let the block core deal with it. Signed-off-by: Christoph Hellwig h...@lst.de Acked-by: Kevin Wolf kw...@redhat.com I mean looking at the patched code I see lots of things that are wrong, but they are all unrelated to your change: There are error cases where memory is leaked, and it should use bdrv_* functions instead of the native open/read/etc. And obviously coding style is completely off (most annoying: tabs!) Kevin
Re: [Qemu-devel] [PATCH] dmg: fix -open failure
On Mon, Jan 11, 2010 at 02:43:59PM +0100, Kevin Wolf wrote: Am 11.01.2010 14:06, schrieb Christoph Hellwig: Currently the dmg image format driver simply opens the images as raw if any kind of failure happens. This is contrarty to the behaviour of all other image formats which just return an error and let the block core deal with it. Signed-off-by: Christoph Hellwig h...@lst.de Acked-by: Kevin Wolf kw...@redhat.com I mean looking at the patched code I see lots of things that are wrong, but they are all unrelated to your change: There are error cases where memory is leaked, and it should use bdrv_* functions instead of the native open/read/etc. And obviously coding style is completely off (most annoying: tabs!) Yes, the code pretty much is a mess, but I didn't really want to touch it. I just looked into picking up your search host_ for raw patches and was looking for all the block image driver functionality in the tree. Kevin ---end quoted text---
Re: [Qemu-devel] [PATCH] dmg: fix -open failure
Am 11.01.2010 14:46, schrieb Christoph Hellwig: On Mon, Jan 11, 2010 at 02:43:59PM +0100, Kevin Wolf wrote: Am 11.01.2010 14:06, schrieb Christoph Hellwig: Currently the dmg image format driver simply opens the images as raw if any kind of failure happens. This is contrarty to the behaviour of all other image formats which just return an error and let the block core deal with it. Signed-off-by: Christoph Hellwig h...@lst.de Acked-by: Kevin Wolf kw...@redhat.com I mean looking at the patched code I see lots of things that are wrong, but they are all unrelated to your change: There are error cases where memory is leaked, and it should use bdrv_* functions instead of the native open/read/etc. And obviously coding style is completely off (most annoying: tabs!) Yes, the code pretty much is a mess, but I didn't really want to touch it. I just looked into picking up your search host_ for raw patches and was looking for all the block image driver functionality in the tree. Are you going to propose a cleaner patch? I have currently some other bugs to do first, but I was certainly planning to do so. However, I'll happily leave it to you if you have the time right now. For dmg, I'm not even sure if it's worth fixing. Does anybody use this driver? But I guess it's good enough for another lowest priority task on my todo list. Right after vvfat or so... Kevin
Re: [Qemu-devel] [PATCH] dmg: fix -open failure
On Mon, Jan 11, 2010 at 02:56:16PM +0100, Kevin Wolf wrote: Are you going to propose a cleaner patch? I have currently some other bugs to do first, but I was certainly planning to do so. However, I'll happily leave it to you if you have the time right now. I'm looking into doing it in the generic block layer, yes. For dmg, I'm not even sure if it's worth fixing. Does anybody use this driver? But I guess it's good enough for another lowest priority task on my todo list. Right after vvfat or so... Hehe. All these odd image formats are extremly low in my todo list either. I'd be really interested if there are any users around at all.
Re: [Qemu-devel] [PATCH] dmg: fix -open failure
Am 11.01.2010 15:00, schrieb Christoph Hellwig: On Mon, Jan 11, 2010 at 02:56:16PM +0100, Kevin Wolf wrote: Are you going to propose a cleaner patch? I have currently some other bugs to do first, but I was certainly planning to do so. However, I'll happily leave it to you if you have the time right now. I'm looking into doing it in the generic block layer, yes. More or less the same hack, just in cleaner? Or trying to fundamentally change things? I think you haven't answered yet to what I said in the thread of my original hack. I'm quoting it here for convenience: Ok, if you start talking about layering, we can have a fundamental discussion on this topic and why the layering is broken anyway. Logically, we have image formats like qcow2, VMDK and raw, and they are stored in files, on CD-ROMs or general block devices. From a layering perspective, it is wrong to include the latter in the raw format driver in the first place. Actually, I think the differentiation between raw files and host_* is at the same level as protocols are. Probably they should be implemented very similarly. Do you think it's possible/worth the effort to try putting things straight here? Kevin
Re: [Qemu-devel] [PATCH] dmg: fix -open failure
On Mon, Jan 11, 2010 at 03:11:52PM +0100, Kevin Wolf wrote: More or less the same hack, just in cleaner? Or trying to fundamentally change things? I think you haven't answered yet to what I said in the thread of my original hack. I'm quoting it here for convenience: Well, not dealing with the format list in raw, but rather in block.c Ok, if you start talking about layering, we can have a fundamental discussion on this topic and why the layering is broken anyway. Logically, we have image formats like qcow2, VMDK and raw, and they are stored in files, on CD-ROMs or general block devices. From a layering perspective, it is wrong to include the latter in the raw format driver in the first place. Actually, I think the differentiation between raw files and host_* is at the same level as protocols are. Probably they should be implemented very similarly. Do you think it's possible/worth the effort to try putting things straight here? So what you want is basically: - hdev_* and file as protocols in addition to nbd/ftp/http/.. - a raw image format that can be used ontop of any protocol instead of an image format That would indeed be a much better, not to say actually logical layering. The raw image format would be more or less a no-op just stacking ontop of the protocol. If we can find a way to implement this efficiently it might be the way to go.
Re: [Qemu-devel] [PATCH] dmg: fix -open failure
On Mon, 11 Jan 2010, Christoph Hellwig wrote: On Mon, Jan 11, 2010 at 02:56:16PM +0100, Kevin Wolf wrote: Are you going to propose a cleaner patch? I have currently some other bugs to do first, but I was certainly planning to do so. However, I'll happily leave it to you if you have the time right now. I'm looking into doing it in the generic block layer, yes. For dmg, I'm not even sure if it's worth fixing. Does anybody use this driver? But I guess it's good enough for another lowest priority task on my todo list. Right after vvfat or so... Hehe. All these odd image formats are extremly low in my todo list either. I'd be really interested if there are any users around at all. I use vvfat. -- mailto:av1...@comtv.ru
Re: [Qemu-devel] [PATCH] dmg: fix -open failure
On 01/11/2010 07:06 AM, Christoph Hellwig wrote:
Currently the dmg image format driver simply opens the images as raw
if any kind of failure happens. This is contrarty to the behaviour
of all other image formats which just return an error and let the
block core deal with it.
Signed-off-by: Christoph Hellwigh...@lst.de
Applied. Thanks.
Regards,
Anthony Liguori
Index: qemu/block/dmg.c
===
--- qemu.orig/block/dmg.c 2010-01-11 14:00:25.945021645 +0100
+++ qemu/block/dmg.c2010-01-11 14:03:03.006036707 +0100
@@ -90,24 +90,21 @@ static int dmg_open(BlockDriverState *bs
/* read offset of info blocks */
if(lseek(s-fd,-0x1d8,SEEK_END)0) {
-dmg_close:
- close(s-fd);
- /* open raw instead */
- bs-drv=bdrv_find_format(raw);
- return bs-drv-bdrv_open(bs, filename, flags);
+goto fail;
}
+
info_begin=read_off(s-fd);
if(info_begin==0)
- goto dmg_close;
+ goto fail;
if(lseek(s-fd,info_begin,SEEK_SET)0)
- goto dmg_close;
+ goto fail;
if(read_uint32(s-fd)!=0x100)
- goto dmg_close;
+ goto fail;
if((count = read_uint32(s-fd))==0)
- goto dmg_close;
+ goto fail;
info_end = info_begin+count;
if(lseek(s-fd,0xf8,SEEK_CUR)0)
- goto dmg_close;
+ goto fail;
/* read offsets */
last_in_offset = last_out_offset = 0;
@@ -116,14 +113,14 @@ dmg_close:
count = read_uint32(s-fd);
if(count==0)
- goto dmg_close;
+ goto fail;
type = read_uint32(s-fd);
if(type!=0x6d697368 || count244)
lseek(s-fd,count-4,SEEK_CUR);
else {
int new_size, chunk_count;
if(lseek(s-fd,200,SEEK_CUR)0)
- goto dmg_close;
+ goto fail;
chunk_count = (count-204)/40;
new_size = sizeof(uint64_t) * (s-n_chunks + chunk_count);
s-types = qemu_realloc(s-types, new_size/2);
@@ -142,7 +139,7 @@ dmg_close:
chunk_count--;
i--;
if(lseek(s-fd,36,SEEK_CUR)0)
- goto dmg_close;
+ goto fail;
continue;
}
read_uint32(s-fd);
@@ -163,11 +160,14 @@ dmg_close:
s-compressed_chunk = qemu_malloc(max_compressed_size+1);
s-uncompressed_chunk = qemu_malloc(512*max_sectors_per_chunk);
if(inflateInit(s-zstream) != Z_OK)
- goto dmg_close;
+ goto fail;
s-current_chunk = s-n_chunks;
return 0;
+fail:
+close(s-fd);
+return -1;
}
static inline int is_sector_in_chunk(BDRVDMGState* s,
