From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59304) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X9DER-0005bd-T7 for qemu-devel@nongnu.org; Mon, 21 Jul 2014 09:02:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X9DEL-0001Zo-Nc for qemu-devel@nongnu.org; Mon, 21 Jul 2014 09:02:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41026) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X9DEL-0001Zb-FW for qemu-devel@nongnu.org; Mon, 21 Jul 2014 09:02:13 -0400 Date: Mon, 21 Jul 2014 15:02:09 +0200 From: Kevin Wolf Message-ID: <20140721130209.GD3168@noname.str.redhat.com> References: <53CA6F0A.8040804@redhat.com> <20140721125849.GA3550@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140721125849.GA3550@localhost.localdomain> Subject: Re: [Qemu-devel] [PATCH 2/4] block: vdi - use block layer ops in vdi_create, instead of posix calls List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: sw@weilnetz.de, qemu-devel@nongnu.org, stefanha@redhat.com, Max Reitz Am 21.07.2014 um 14:58 hat Jeff Cody geschrieben: > On Sat, Jul 19, 2014 at 03:13:46PM +0200, Max Reitz wrote: > > On 18.07.2014 22:53, Jeff Cody wrote: > > >Use the block layer to create, and write to, the image file in the > > >VDI .bdrv_create() operation. > > > > > >This has a couple of benefits: Images can now be created over protocols, > > >and host raw file optimizations (such as nocow) do not need to be > > >handled in the image format driver. > > > > > >Also some minor cleanup for error handling. > > > > > >Signed-off-by: Jeff Cody > > >--- > > > block/vdi.c | 68 ++++++++++++++++++++++++++----------------------------------- > > > 1 file changed, 29 insertions(+), 39 deletions(-) > > > > With this, I think you could have dropped the #ifdef __linux__ block > > from the top of block/vdi.c, too. > > > > >diff --git a/block/vdi.c b/block/vdi.c > > >index 197bd77..a74ba85 100644 > > >--- a/block/vdi.c > > >+++ b/block/vdi.c > > >@@ -681,7 +681,6 @@ static int vdi_co_write(BlockDriverState *bs, > > > static int vdi_create(const char *filename, QemuOpts *opts, Error **errp) > > > { > > >- int fd; > > > int result = 0; > > > uint64_t bytes = 0; > > > uint32_t blocks; > > >@@ -690,7 +689,10 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp) > > > VdiHeader header; > > > size_t i; > > > size_t bmap_size; > > >- bool nocow = false; > > >+ int64_t offset = 0; > > >+ Error *local_err = NULL; > > >+ BlockDriverState *bs = NULL; > > >+ uint32_t *bmap = NULL; > > > logout("\n"); > > >@@ -707,7 +709,6 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp) > > > image_type = VDI_TYPE_STATIC; > > > } > > > #endif > > >- nocow = qemu_opt_get_bool_del(opts, BLOCK_OPT_NOCOW, false); > > > if (bytes > VDI_DISK_SIZE_MAX) { > > > result = -ENOTSUP; > > >@@ -717,27 +718,16 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp) > > > goto exit; > > > } > > >- fd = qemu_open(filename, > > >- O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, > > >- 0644); > > >- if (fd < 0) { > > >- result = -errno; > > >+ result = bdrv_create_file(filename, opts, &local_err); > > >+ if (result < 0) { > > >+ error_propagate(errp, local_err); > > > goto exit; > > > } > > >- > > >- if (nocow) { > > >-#ifdef __linux__ > > >- /* Set NOCOW flag to solve performance issue on fs like btrfs. > > >- * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value will > > >- * be ignored since any failure of this operation should not block the > > >- * left work. > > >- */ > > >- int attr; > > >- if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) { > > >- attr |= FS_NOCOW_FL; > > >- ioctl(fd, FS_IOC_SETFLAGS, &attr); > > >- } > > >-#endif > > >+ result = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, > > >+ NULL, &local_err); > > >+ if (result < 0) { > > >+ error_propagate(errp, local_err); > > >+ goto exit; > > > } > > > /* We need enough blocks to store the given disk size, > > >@@ -769,13 +759,15 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp) > > > vdi_header_print(&header); > > > #endif > > > vdi_header_to_le(&header); > > >- if (write(fd, &header, sizeof(header)) < 0) { > > >- result = -errno; > > >- goto close_and_exit; > > >+ result = bdrv_pwrite_sync(bs, offset, &header, sizeof(header)); > > >+ if (result < 0) { > > >+ error_setg(errp, "Error writing header to %s", filename); > > >+ goto exit; > > > } > > >+ offset += sizeof(header); > > > if (bmap_size > 0) { > > >- uint32_t *bmap = g_malloc0(bmap_size); > > >+ bmap = g_malloc0(bmap_size); > > > for (i = 0; i < blocks; i++) { > > > if (image_type == VDI_TYPE_STATIC) { > > > bmap[i] = i; > > >@@ -783,27 +775,25 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp) > > > bmap[i] = VDI_UNALLOCATED; > > > } > > > } > > >- if (write(fd, bmap, bmap_size) < 0) { > > >- result = -errno; > > >- g_free(bmap); > > >- goto close_and_exit; > > >+ result = bdrv_pwrite_sync(bs, offset, bmap, bmap_size); > > >+ if (result < 0) { > > >+ error_setg(errp, "Error writing bmap to %s", filename); > > >+ goto exit; > > > } > > >- g_free(bmap); > > >+ offset += bmap_size; > > > } > > > if (image_type == VDI_TYPE_STATIC) { > > >- if (ftruncate(fd, sizeof(header) + bmap_size + blocks * block_size)) { > > >- result = -errno; > > >- goto close_and_exit; > > >+ result = bdrv_truncate(bs->file, offset + blocks * block_size); > > > > bs actually *is* your file, so this should be just bs and not > > bs->file (like this, "qemu-img create -f vdi -o static=on foo.vdi > > 128K" segfaults). > > Oops, yes that is what I meant. Sending v2. If qemu-iotests doesn't catch this yet, can you add a case? Kevin