From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36656) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X8USA-0000BM-69 for qemu-devel@nongnu.org; Sat, 19 Jul 2014 09:13:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X8US4-0007rT-06 for qemu-devel@nongnu.org; Sat, 19 Jul 2014 09:13:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8136) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X8US3-0007rJ-O3 for qemu-devel@nongnu.org; Sat, 19 Jul 2014 09:13:23 -0400 Message-ID: <53CA6F0A.8040804@redhat.com> Date: Sat, 19 Jul 2014 15:13:46 +0200 From: Max Reitz MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 , qemu-devel@nongnu.org Cc: kwolf@redhat.com, sw@weilnetz.de, stefanha@redhat.com 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). Other than that, the patch looks good. Max