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

[Kevin Wolf]

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 <address@hidden>
> > >---
> > >  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