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 <[email protected]>

Am 11.01.2010 14:06, schrieb Christoph Hellwig:

Quoted 7 lines

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 <[email protected]>

Acked-by Kevin Wolf <[email protected]>

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

Quoted 9 lines

Am 11.01.2010 14:06, schrieb Christoph Hellwig:

Quoted 7 lines

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 <[email protected]>

Quoted 7 lines

Acked-by Kevin Wolf <[email protected]>

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

Quoted 3 lines

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.

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.

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:

Quoted 6 lines

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?

Quoted 3 lines · Kevin Wolf

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.

Quoted 3 lines

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.

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<[email protected]>

Index: qemu/block/dmg.c
===================================================================

block/dmg.c +14 -14

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

Quoted 11 lines

Quoted 6 lines

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.

Quoted 2 lines

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.