| Message ID | 20100111130654.GA24241@lst.de |
|---|---|
| State | New |
| Headers | show |
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 <hch@lst.de> Acked-by: Kevin Wolf <kwolf@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
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 <hch@lst.de> > > Acked-by: Kevin Wolf <kwolf@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---
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 <hch@lst.de> >> >> Acked-by: Kevin Wolf <kwolf@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
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.
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
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.
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.
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 Hellwig<hch@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.c 2010-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 || count<244) > 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, > > > >
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
Index: qemu/block/dmg.c =================================================================== --- qemu.orig/block/dmg.c 2010-01-11 14:00:25.945021645 +0100 +++ qemu/block/dmg.c 2010-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 || count<244) 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,
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 <hch@lst.de>