Re: [Qemu-devel] [PATCH 2/4] block: vdi - use block layer ops in vdi_create, instead of posix calls

[Jeff Cody <jcody@redhat.com>]

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 <jcody@redhat.com>
> >---
> >  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.