qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] simple block driver cleanups
@ 2010-05-04 10:43 Christoph Hellwig
  2010-05-04 10:44 ` [Qemu-devel] [PATCH 2/6] bochs: use qemu block API Christoph Hellwig
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Christoph Hellwig @ 2010-05-04 10:43 UTC (permalink / raw)
  To: qemu-devel

This series cleans up the simple read-only block drivers to use the
qemu block device API to access their backing devices, making the code
simpler and usable over nbd/curl.  I've not touched dmg yet as it's even
more bitrot than usual and deserves it's own series.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH 2/6] bochs: use qemu block API
  2010-05-04 10:43 [Qemu-devel] simple block driver cleanups Christoph Hellwig
@ 2010-05-04 10:44 ` Christoph Hellwig
  2010-05-04 10:44 ` [Qemu-devel] [PATCH 3/6] cloop: use pread Christoph Hellwig
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2010-05-04 10:44 UTC (permalink / raw)
  To: qemu-devel

Use bdrv_pwrite to access the backing device instead of pread, and
convert the driver to implementing the bdrv_open method which gives
it an already opened BlockDriverState for the underlying device.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: qemu-kevin/block/bochs.c
===================================================================
--- qemu-kevin.orig/block/bochs.c	2010-05-03 12:42:19.385274599 +0200
+++ qemu-kevin/block/bochs.c	2010-05-03 12:48:54.434007033 +0200
@@ -80,8 +80,6 @@ struct bochs_header {
 };
 
 typedef struct BDRVBochsState {
-    int fd;
-
     uint32_t *catalog_bitmap;
     int catalog_size;
 
@@ -109,23 +107,16 @@ static int bochs_probe(const uint8_t *bu
     return 0;
 }
 
-static int bochs_open(BlockDriverState *bs, const char *filename, int flags)
+static int bochs_open(BlockDriverState *bs, int flags)
 {
     BDRVBochsState *s = bs->opaque;
-    int fd, i;
+    int i;
     struct bochs_header bochs;
     struct bochs_header_v1 header_v1;
 
-    fd = open(filename, O_RDONLY | O_BINARY);
-    if (fd < 0) {
-        return -1;
-    }
-
     bs->read_only = 1; // no write support yet
 
-    s->fd = fd;
-
-    if (pread(fd, &bochs, sizeof(bochs), 0) != sizeof(bochs)) {
+    if (bdrv_pread(bs->file, 0, &bochs, sizeof(bochs)) != sizeof(bochs)) {
         goto fail;
     }
 
@@ -146,8 +137,8 @@ static int bochs_open(BlockDriverState *
 
     s->catalog_size = le32_to_cpu(bochs.extra.redolog.catalog);
     s->catalog_bitmap = qemu_malloc(s->catalog_size * 4);
-    if (pread(s->fd, s->catalog_bitmap, s->catalog_size * 4,
-              le32_to_cpu(bochs.header)) != s->catalog_size * 4)
+    if (bdrv_pread(bs->file, le32_to_cpu(bochs.header), s->catalog_bitmap,
+                   s->catalog_size * 4) != s->catalog_size * 4)
 	goto fail;
     for (i = 0; i < s->catalog_size; i++)
 	le32_to_cpus(&s->catalog_bitmap[i]);
@@ -161,7 +152,6 @@ static int bochs_open(BlockDriverState *
 
     return 0;
  fail:
-    close(fd);
     return -1;
 }
 
@@ -184,8 +174,8 @@ static int64_t seek_to_sector(BlockDrive
 	(s->extent_blocks + s->bitmap_blocks));
 
     /* read in bitmap for current extent */
-    if (pread(s->fd, &bitmap_entry, 1, bitmap_offset + (extent_offset / 8))
-            != 1) {
+    if (bdrv_pread(bs->file, bitmap_offset + (extent_offset / 8),
+                   &bitmap_entry, 1) != 1) {
         return -1;
     }
 
@@ -199,13 +189,12 @@ static int64_t seek_to_sector(BlockDrive
 static int bochs_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
-    BDRVBochsState *s = bs->opaque;
     int ret;
 
     while (nb_sectors > 0) {
         int64_t block_offset = seek_to_sector(bs, sector_num);
         if (block_offset >= 0) {
-            ret = pread(s->fd, buf, 512, block_offset);
+            ret = bdrv_pread(bs->file, block_offset, buf, 512);
             if (ret != 512) {
                 return -1;
             }
@@ -222,14 +211,13 @@ static void bochs_close(BlockDriverState
 {
     BDRVBochsState *s = bs->opaque;
     qemu_free(s->catalog_bitmap);
-    close(s->fd);
 }
 
 static BlockDriver bdrv_bochs = {
     .format_name	= "bochs",
     .instance_size	= sizeof(BDRVBochsState),
     .bdrv_probe		= bochs_probe,
-    .bdrv_file_open	= bochs_open,
+    .bdrv_open		= bochs_open,
     .bdrv_read		= bochs_read,
     .bdrv_close		= bochs_close,
 };

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH 3/6] cloop: use pread
  2010-05-04 10:43 [Qemu-devel] simple block driver cleanups Christoph Hellwig
  2010-05-04 10:44 ` [Qemu-devel] [PATCH 2/6] bochs: use qemu block API Christoph Hellwig
@ 2010-05-04 10:44 ` Christoph Hellwig
  2010-05-04 10:44 ` [Qemu-devel] [PATCH 4/6] cloop: use qemu block API Christoph Hellwig
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2010-05-04 10:44 UTC (permalink / raw)
  To: qemu-devel

Use pread instead of lseek + read in preparation of using the qemu
block API.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: qemu-kevin/block/cloop.c
===================================================================
--- qemu-kevin.orig/block/cloop.c	2010-05-03 13:01:09.035025542 +0200
+++ qemu-kevin/block/cloop.c	2010-05-03 13:01:57.918284307 +0200
@@ -62,23 +62,22 @@ static int cloop_open(BlockDriverState *
     bs->read_only = 1;
 
     /* read header */
-    if(lseek(s->fd,128,SEEK_SET)<0) {
-cloop_close:
-	close(s->fd);
-	return -1;
+    if (pread(s->fd, &s->block_size, 4, 128) < 4) {
+        goto cloop_close;
     }
-    if(read(s->fd,&s->block_size,4)<4)
-	goto cloop_close;
-    s->block_size=be32_to_cpu(s->block_size);
-    if(read(s->fd,&s->n_blocks,4)<4)
-	goto cloop_close;
-    s->n_blocks=be32_to_cpu(s->n_blocks);
+    s->block_size = be32_to_cpu(s->block_size);
+
+    if (pread(s->fd, &s->n_blocks, 4, 128 + 4) < 4) {
+        goto cloop_close;
+    }
+    s->n_blocks = be32_to_cpu(s->n_blocks);
 
     /* read offsets */
-    offsets_size=s->n_blocks*sizeof(uint64_t);
-    s->offsets=(uint64_t*)qemu_malloc(offsets_size);
-    if(read(s->fd,s->offsets,offsets_size)<offsets_size)
+    offsets_size = s->n_blocks * sizeof(uint64_t);
+    s->offsets = qemu_malloc(offsets_size);
+    if (pread(s->fd, s->offsets, offsets_size, 128 + 4 + 4) < offsets_size) {
 	goto cloop_close;
+    }
     for(i=0;i<s->n_blocks;i++) {
 	s->offsets[i]=be64_to_cpu(s->offsets[i]);
 	if(i>0) {
@@ -98,6 +97,10 @@ cloop_close:
     s->sectors_per_block = s->block_size/512;
     bs->total_sectors = s->n_blocks*s->sectors_per_block;
     return 0;
+
+cloop_close:
+    close(s->fd);
+    return -1;
 }
 
 static inline int cloop_read_block(BDRVCloopState *s,int block_num)
@@ -106,8 +109,7 @@ static inline int cloop_read_block(BDRVC
 	int ret;
         uint32_t bytes = s->offsets[block_num+1]-s->offsets[block_num];
 
-	lseek(s->fd, s->offsets[block_num], SEEK_SET);
-        ret = read(s->fd, s->compressed_block, bytes);
+        ret = pread(s->fd, s->compressed_block, bytes, s->offsets[block_num]);
         if (ret != bytes)
             return -1;
 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH 4/6] cloop: use qemu block API
  2010-05-04 10:43 [Qemu-devel] simple block driver cleanups Christoph Hellwig
  2010-05-04 10:44 ` [Qemu-devel] [PATCH 2/6] bochs: use qemu block API Christoph Hellwig
  2010-05-04 10:44 ` [Qemu-devel] [PATCH 3/6] cloop: use pread Christoph Hellwig
@ 2010-05-04 10:44 ` Christoph Hellwig
  2010-05-04 10:45 ` [Qemu-devel] [PATCH 5/6] parallels: use pread Christoph Hellwig
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2010-05-04 10:44 UTC (permalink / raw)
  To: qemu-devel

Use bdrv_pwrite to access the backing device instead of pread, and
convert the driver to implementing the bdrv_open method which gives
it an already opened BlockDriverState for the underlying device.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: qemu-kevin/block/cloop.c
===================================================================
--- qemu-kevin.orig/block/cloop.c	2010-05-03 13:01:57.918284307 +0200
+++ qemu-kevin/block/cloop.c	2010-05-03 13:02:27.836318598 +0200
@@ -27,7 +27,6 @@
 #include <zlib.h>
 
 typedef struct BDRVCloopState {
-    int fd;
     uint32_t block_size;
     uint32_t n_blocks;
     uint64_t* offsets;
@@ -51,23 +50,20 @@ static int cloop_probe(const uint8_t *bu
     return 0;
 }
 
-static int cloop_open(BlockDriverState *bs, const char *filename, int flags)
+static int cloop_open(BlockDriverState *bs, int flags)
 {
     BDRVCloopState *s = bs->opaque;
     uint32_t offsets_size,max_compressed_block_size=1,i;
 
-    s->fd = open(filename, O_RDONLY | O_BINARY);
-    if (s->fd < 0)
-        return -errno;
     bs->read_only = 1;
 
     /* read header */
-    if (pread(s->fd, &s->block_size, 4, 128) < 4) {
+    if (bdrv_pread(bs->file, 128, &s->block_size, 4) < 4) {
         goto cloop_close;
     }
     s->block_size = be32_to_cpu(s->block_size);
 
-    if (pread(s->fd, &s->n_blocks, 4, 128 + 4) < 4) {
+    if (bdrv_pread(bs->file, 128 + 4, &s->n_blocks, 4) < 4) {
         goto cloop_close;
     }
     s->n_blocks = be32_to_cpu(s->n_blocks);
@@ -75,7 +71,8 @@ static int cloop_open(BlockDriverState *
     /* read offsets */
     offsets_size = s->n_blocks * sizeof(uint64_t);
     s->offsets = qemu_malloc(offsets_size);
-    if (pread(s->fd, s->offsets, offsets_size, 128 + 4 + 4) < offsets_size) {
+    if (bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size) <
+            offsets_size) {
 	goto cloop_close;
     }
     for(i=0;i<s->n_blocks;i++) {
@@ -99,17 +96,19 @@ static int cloop_open(BlockDriverState *
     return 0;
 
 cloop_close:
-    close(s->fd);
     return -1;
 }
 
-static inline int cloop_read_block(BDRVCloopState *s,int block_num)
+static inline int cloop_read_block(BlockDriverState *bs, int block_num)
 {
+    BDRVCloopState *s = bs->opaque;
+
     if(s->current_block != block_num) {
 	int ret;
         uint32_t bytes = s->offsets[block_num+1]-s->offsets[block_num];
 
-        ret = pread(s->fd, s->compressed_block, bytes, s->offsets[block_num]);
+        ret = bdrv_pread(bs->file, s->offsets[block_num], s->compressed_block,
+                         bytes);
         if (ret != bytes)
             return -1;
 
@@ -138,7 +137,7 @@ static int cloop_read(BlockDriverState *
     for(i=0;i<nb_sectors;i++) {
 	uint32_t sector_offset_in_block=((sector_num+i)%s->sectors_per_block),
 	    block_num=(sector_num+i)/s->sectors_per_block;
-	if(cloop_read_block(s, block_num) != 0)
+	if(cloop_read_block(bs, block_num) != 0)
 	    return -1;
 	memcpy(buf+i*512,s->uncompressed_block+sector_offset_in_block*512,512);
     }
@@ -148,7 +147,6 @@ static int cloop_read(BlockDriverState *
 static void cloop_close(BlockDriverState *bs)
 {
     BDRVCloopState *s = bs->opaque;
-    close(s->fd);
     if(s->n_blocks>0)
 	free(s->offsets);
     free(s->compressed_block);
@@ -160,7 +158,7 @@ static BlockDriver bdrv_cloop = {
     .format_name	= "cloop",
     .instance_size	= sizeof(BDRVCloopState),
     .bdrv_probe		= cloop_probe,
-    .bdrv_file_open	= cloop_open,
+    .bdrv_open		= cloop_open,
     .bdrv_read		= cloop_read,
     .bdrv_close		= cloop_close,
 };

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH 5/6] parallels: use pread
  2010-05-04 10:43 [Qemu-devel] simple block driver cleanups Christoph Hellwig
                   ` (2 preceding siblings ...)
  2010-05-04 10:44 ` [Qemu-devel] [PATCH 4/6] cloop: use qemu block API Christoph Hellwig
@ 2010-05-04 10:45 ` Christoph Hellwig
  2010-05-05 14:19   ` Kevin Wolf
  2010-05-04 10:45 ` [Qemu-devel] [PATCH 6/6] parallels: use qemu block API Christoph Hellwig
  2010-05-04 12:38 ` [Qemu-devel] simple block driver cleanups Kevin Wolf
  5 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2010-05-04 10:45 UTC (permalink / raw)
  To: qemu-devel

Use pread instead of lseek + read in preparation of using the qemu
block API.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: qemu-kevin/block/parallels.c
===================================================================
--- qemu-kevin.orig/block/parallels.c	2010-05-03 13:00:09.711253925 +0200
+++ qemu-kevin/block/parallels.c	2010-05-03 13:04:15.686033993 +0200
@@ -83,7 +83,7 @@ static int parallels_open(BlockDriverSta
 
     s->fd = fd;
 
-    if (read(fd, &ph, sizeof(ph)) != sizeof(ph))
+    if (pread(fd, &ph, sizeof(ph), 0) != sizeof(ph))
         goto fail;
 
     if (memcmp(ph.magic, HEADER_MAGIC, 16) ||
@@ -93,14 +93,11 @@ static int parallels_open(BlockDriverSta
 
     bs->total_sectors = le32_to_cpu(ph.nb_sectors);
 
-    if (lseek(s->fd, 64, SEEK_SET) != 64)
-	goto fail;
-
     s->tracks = le32_to_cpu(ph.tracks);
 
     s->catalog_size = le32_to_cpu(ph.catalog_entries);
     s->catalog_bitmap = qemu_malloc(s->catalog_size * 4);
-    if (read(s->fd, s->catalog_bitmap, s->catalog_size * 4) !=
+    if (pread(s->fd, s->catalog_bitmap, s->catalog_size * 4, 64) !=
 	s->catalog_size * 4)
 	goto fail;
     for (i = 0; i < s->catalog_size; i++)
@@ -114,28 +111,18 @@ fail:
     return -1;
 }
 
-static inline int seek_to_sector(BlockDriverState *bs, int64_t sector_num)
+static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
 {
     BDRVParallelsState *s = bs->opaque;
     uint32_t index, offset;
-    uint64_t position;
 
     index = sector_num / s->tracks;
     offset = sector_num % s->tracks;
 
-    // not allocated
+    /* not allocated */
     if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
 	return -1;
-
-    position = (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
-
-//    fprintf(stderr, "sector: %llx index=%x offset=%x pointer=%x position=%x\n",
-//	sector_num, index, offset, s->catalog_bitmap[index], position);
-
-    if (lseek(s->fd, position, SEEK_SET) != position)
-	return -1;
-
-    return 0;
+    return (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
 }
 
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
@@ -144,11 +131,13 @@ static int parallels_read(BlockDriverSta
     BDRVParallelsState *s = bs->opaque;
 
     while (nb_sectors > 0) {
-	if (!seek_to_sector(bs, sector_num)) {
-	    if (read(s->fd, buf, 512) != 512)
-		return -1;
-	} else
+        uint64_t position = seek_to_sector(bs, sector_num);
+        if (position >= 0) {
+            if (pread(s->fd, buf, 512, position) != 512)
+                return -1;
+        } else {
             memset(buf, 0, 512);
+        }
         nb_sectors--;
         sector_num++;
         buf += 512;

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH 6/6] parallels: use qemu block API
  2010-05-04 10:43 [Qemu-devel] simple block driver cleanups Christoph Hellwig
                   ` (3 preceding siblings ...)
  2010-05-04 10:45 ` [Qemu-devel] [PATCH 5/6] parallels: use pread Christoph Hellwig
@ 2010-05-04 10:45 ` Christoph Hellwig
  2010-05-04 12:38 ` [Qemu-devel] simple block driver cleanups Kevin Wolf
  5 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2010-05-04 10:45 UTC (permalink / raw)
  To: qemu-devel

Use bdrv_pwrite to access the backing device instead of pread, and
convert the driver to implementing the bdrv_open method which gives
it an already opened BlockDriverState for the underlying device.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: qemu-kevin/block/parallels.c
===================================================================
--- qemu-kevin.orig/block/parallels.c	2010-05-03 13:04:37.494261748 +0200
+++ qemu-kevin/block/parallels.c	2010-05-03 13:05:55.781255810 +0200
@@ -46,7 +46,6 @@ struct parallels_header {
 } __attribute__((packed));
 
 typedef struct BDRVParallelsState {
-    int fd;
 
     uint32_t *catalog_bitmap;
     int catalog_size;
@@ -68,22 +67,15 @@ static int parallels_probe(const uint8_t
     return 0;
 }
 
-static int parallels_open(BlockDriverState *bs, const char *filename, int flags)
+static int parallels_open(BlockDriverState *bs, int flags)
 {
     BDRVParallelsState *s = bs->opaque;
-    int fd, i;
+    int i;
     struct parallels_header ph;
 
-    fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
-    if (fd < 0) {
-        return -1;
-    }
-
     bs->read_only = 1; // no write support yet
 
-    s->fd = fd;
-
-    if (pread(fd, &ph, sizeof(ph), 0) != sizeof(ph))
+    if (bdrv_pread(bs->file, 0, &ph, sizeof(ph)) != sizeof(ph))
         goto fail;
 
     if (memcmp(ph.magic, HEADER_MAGIC, 16) ||
@@ -97,7 +89,7 @@ static int parallels_open(BlockDriverSta
 
     s->catalog_size = le32_to_cpu(ph.catalog_entries);
     s->catalog_bitmap = qemu_malloc(s->catalog_size * 4);
-    if (pread(s->fd, s->catalog_bitmap, s->catalog_size * 4, 64) !=
+    if (bdrv_pread(bs->file, 64, s->catalog_bitmap, s->catalog_size * 4) !=
 	s->catalog_size * 4)
 	goto fail;
     for (i = 0; i < s->catalog_size; i++)
@@ -107,7 +99,6 @@ static int parallels_open(BlockDriverSta
 fail:
     if (s->catalog_bitmap)
 	qemu_free(s->catalog_bitmap);
-    close(fd);
     return -1;
 }
 
@@ -128,12 +119,10 @@ static int64_t seek_to_sector(BlockDrive
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
-    BDRVParallelsState *s = bs->opaque;
-
     while (nb_sectors > 0) {
         uint64_t position = seek_to_sector(bs, sector_num);
         if (position >= 0) {
-            if (pread(s->fd, buf, 512, position) != 512)
+            if (bdrv_pread(bs->file, position, buf, 512) != 512)
                 return -1;
         } else {
             memset(buf, 0, 512);
@@ -149,14 +138,13 @@ static void parallels_close(BlockDriverS
 {
     BDRVParallelsState *s = bs->opaque;
     qemu_free(s->catalog_bitmap);
-    close(s->fd);
 }
 
 static BlockDriver bdrv_parallels = {
     .format_name	= "parallels",
     .instance_size	= sizeof(BDRVParallelsState),
     .bdrv_probe		= parallels_probe,
-    .bdrv_file_open	= parallels_open,
+    .bdrv_open		= parallels_open,
     .bdrv_read		= parallels_read,
     .bdrv_close		= parallels_close,
 };

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] simple block driver cleanups
  2010-05-04 10:43 [Qemu-devel] simple block driver cleanups Christoph Hellwig
                   ` (4 preceding siblings ...)
  2010-05-04 10:45 ` [Qemu-devel] [PATCH 6/6] parallels: use qemu block API Christoph Hellwig
@ 2010-05-04 12:38 ` Kevin Wolf
  2010-05-04 13:22   ` Christoph Hellwig
  5 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2010-05-04 12:38 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: qemu-devel

Am 04.05.2010 12:43, schrieb Christoph Hellwig:
> This series cleans up the simple read-only block drivers to use the
> qemu block device API to access their backing devices, making the code
> simpler and usable over nbd/curl.  I've not touched dmg yet as it's even
> more bitrot than usual and deserves it's own series.

Have you already added something to qemu-iotests locally to test this or
did you test manually?

Kevin

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] simple block driver cleanups
  2010-05-04 12:38 ` [Qemu-devel] simple block driver cleanups Kevin Wolf
@ 2010-05-04 13:22   ` Christoph Hellwig
  2010-05-05 14:28     ` Kevin Wolf
  0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2010-05-04 13:22 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Christoph Hellwig, qemu-devel

On Tue, May 04, 2010 at 02:38:07PM +0200, Kevin Wolf wrote:
> Am 04.05.2010 12:43, schrieb Christoph Hellwig:
> > This series cleans up the simple read-only block drivers to use the
> > qemu block device API to access their backing devices, making the code
> > simpler and usable over nbd/curl.  I've not touched dmg yet as it's even
> > more bitrot than usual and deserves it's own series.
> 
> Have you already added something to qemu-iotests locally to test this or
> did you test manually?

I tested cloop manually, I haven't found usable creation tools for the
others yet.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH 5/6] parallels: use pread
  2010-05-04 10:45 ` [Qemu-devel] [PATCH 5/6] parallels: use pread Christoph Hellwig
@ 2010-05-05 14:19   ` Kevin Wolf
  2010-05-05 18:11     ` Christoph Hellwig
  0 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2010-05-05 14:19 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: qemu-devel

Am 04.05.2010 12:45, schrieb Christoph Hellwig:
> Use pread instead of lseek + read in preparation of using the qemu
> block API.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: qemu-kevin/block/parallels.c
> ===================================================================
> --- qemu-kevin.orig/block/parallels.c	2010-05-03 13:00:09.711253925 +0200
> +++ qemu-kevin/block/parallels.c	2010-05-03 13:04:15.686033993 +0200
> @@ -83,7 +83,7 @@ static int parallels_open(BlockDriverSta
>  
>      s->fd = fd;
>  
> -    if (read(fd, &ph, sizeof(ph)) != sizeof(ph))
> +    if (pread(fd, &ph, sizeof(ph), 0) != sizeof(ph))
>          goto fail;
>  
>      if (memcmp(ph.magic, HEADER_MAGIC, 16) ||
> @@ -93,14 +93,11 @@ static int parallels_open(BlockDriverSta
>  
>      bs->total_sectors = le32_to_cpu(ph.nb_sectors);
>  
> -    if (lseek(s->fd, 64, SEEK_SET) != 64)
> -	goto fail;
> -
>      s->tracks = le32_to_cpu(ph.tracks);
>  
>      s->catalog_size = le32_to_cpu(ph.catalog_entries);
>      s->catalog_bitmap = qemu_malloc(s->catalog_size * 4);
> -    if (read(s->fd, s->catalog_bitmap, s->catalog_size * 4) !=
> +    if (pread(s->fd, s->catalog_bitmap, s->catalog_size * 4, 64) !=
>  	s->catalog_size * 4)
>  	goto fail;
>      for (i = 0; i < s->catalog_size; i++)
> @@ -114,28 +111,18 @@ fail:
>      return -1;
>  }
>  
> -static inline int seek_to_sector(BlockDriverState *bs, int64_t sector_num)
> +static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
>  {
>      BDRVParallelsState *s = bs->opaque;
>      uint32_t index, offset;
> -    uint64_t position;
>  
>      index = sector_num / s->tracks;
>      offset = sector_num % s->tracks;
>  
> -    // not allocated
> +    /* not allocated */
>      if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
>  	return -1;
> -
> -    position = (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
> -
> -//    fprintf(stderr, "sector: %llx index=%x offset=%x pointer=%x position=%x\n",
> -//	sector_num, index, offset, s->catalog_bitmap[index], position);
> -
> -    if (lseek(s->fd, position, SEEK_SET) != position)
> -	return -1;
> -
> -    return 0;
> +    return (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
>  }
>  
>  static int parallels_read(BlockDriverState *bs, int64_t sector_num,
> @@ -144,11 +131,13 @@ static int parallels_read(BlockDriverSta
>      BDRVParallelsState *s = bs->opaque;
>  
>      while (nb_sectors > 0) {
> -	if (!seek_to_sector(bs, sector_num)) {
> -	    if (read(s->fd, buf, 512) != 512)
> -		return -1;
> -	} else
> +        uint64_t position = seek_to_sector(bs, sector_num);
> +        if (position >= 0) {

position should be a signed int64_t, otherwise the condition is always true.

Kevin

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] simple block driver cleanups
  2010-05-04 13:22   ` Christoph Hellwig
@ 2010-05-05 14:28     ` Kevin Wolf
  2010-05-05 18:10       ` Christoph Hellwig
  0 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2010-05-05 14:28 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: qemu-devel

Am 04.05.2010 15:22, schrieb Christoph Hellwig:
> On Tue, May 04, 2010 at 02:38:07PM +0200, Kevin Wolf wrote:
>> Am 04.05.2010 12:43, schrieb Christoph Hellwig:
>>> This series cleans up the simple read-only block drivers to use the
>>> qemu block device API to access their backing devices, making the code
>>> simpler and usable over nbd/curl.  I've not touched dmg yet as it's even
>>> more bitrot than usual and deserves it's own series.
>>
>> Have you already added something to qemu-iotests locally to test this or
>> did you test manually?
> 
> I tested cloop manually, I haven't found usable creation tools for the
> others yet.

Okay, so I have reviewed the patches now and except for that one thing
in parallels they look good.

For cloop I trust your test, and for bochs I did a very basic test
myself (however, I doubt that anyone uses this driver, considering how
hard it is to create such an image...). For parallels I still need to
find out how to create an image.

I've applied cloop and bochs to the block branch, and I'll wait for a v2
with the parallels patches.

Kevin

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] simple block driver cleanups
  2010-05-05 14:28     ` Kevin Wolf
@ 2010-05-05 18:10       ` Christoph Hellwig
  0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2010-05-05 18:10 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Christoph Hellwig, qemu-devel

On Wed, May 05, 2010 at 04:28:08PM +0200, Kevin Wolf wrote:
> For cloop I trust your test, and for bochs I did a very basic test
> myself (however, I doubt that anyone uses this driver, considering how
> hard it is to create such an image...). For parallels I still need to
> find out how to create an image.

Btw, I did a bit more testing than my trivial qemu-io testing before,
and it seem that cloop is utterly broken in qemu mainline already.
Just booting with a small cloop compressed filesystem image attached
is enough to give me tons of I/O errors.  When using ide I can even
crash qemu by trying to mount it.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH 5/6] parallels: use pread
  2010-05-05 14:19   ` Kevin Wolf
@ 2010-05-05 18:11     ` Christoph Hellwig
  0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2010-05-05 18:11 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel

On Wed, May 05, 2010 at 04:19:30PM +0200, Kevin Wolf wrote:
> position should be a signed int64_t, otherwise the condition is always true.

Indeed, I'll update the patch.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2010-05-05 18:11 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-04 10:43 [Qemu-devel] simple block driver cleanups Christoph Hellwig
2010-05-04 10:44 ` [Qemu-devel] [PATCH 2/6] bochs: use qemu block API Christoph Hellwig
2010-05-04 10:44 ` [Qemu-devel] [PATCH 3/6] cloop: use pread Christoph Hellwig
2010-05-04 10:44 ` [Qemu-devel] [PATCH 4/6] cloop: use qemu block API Christoph Hellwig
2010-05-04 10:45 ` [Qemu-devel] [PATCH 5/6] parallels: use pread Christoph Hellwig
2010-05-05 14:19   ` Kevin Wolf
2010-05-05 18:11     ` Christoph Hellwig
2010-05-04 10:45 ` [Qemu-devel] [PATCH 6/6] parallels: use qemu block API Christoph Hellwig
2010-05-04 12:38 ` [Qemu-devel] simple block driver cleanups Kevin Wolf
2010-05-04 13:22   ` Christoph Hellwig
2010-05-05 14:28     ` Kevin Wolf
2010-05-05 18:10       ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).