[Qemu-devel] [PATCH] dmg: fix ->open failure

[Qemu-devel] [PATCH] dmg: fix ->open failure

[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>

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,

Re: [Qemu-devel] [PATCH] dmg: fix ->open failure

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] dmg: fix ->open failure

[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.

> 
> Kevin
---end quoted text---

Re: [Qemu-devel] [PATCH] dmg: fix ->open failure

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] dmg: fix ->open failure

[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.

> 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

[Kevin Wolf]

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

[Christoph Hellwig]

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

[malc]

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:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] dmg: fix ->open failure

[Anthony Liguori]

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,
>
>
>
>    

Re: [Qemu-devel] [PATCH] dmg: fix ->open failure

[Kevin Wolf]

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