[Qemu-devel] [PATCH v2 0/5] Allow VPC and VDI to be created over protocols

[Qemu-devel] [PATCH v2 0/5] Allow VPC and VDI to be created over protocols

[Jeff Cody]

Changes from v1 -> v2:
    * Patch 2: Use'bs' instead of 'bs->file' (Max)
    * Patch 3: Same as patch 2 (ripple through)
    * Patch 5: Update VDI test for static image (Kevin)
    * Added Max's R-b to patches 1,3,4



This allows VPC and VDI to be created over protocols; currently, they use
posix calls directly to open, seek, and write into new image files.  This
obviously precludes them from being able to be created over a protocol, like
glusterfs.

Jeff Cody (5):
  block: allow bdrv_unref() to be passed NULL pointers
  block: vdi - use block layer ops in vdi_create, instead of posix calls
  block: use the standard 'ret' instead of 'result'
  block: vpc - use block layer ops in vpc_create, instead of posix calls
  block: iotest - update 084 to test static VDI image creation

 block.c                    |  3 ++
 block/vdi.c                | 82 +++++++++++++++++---------------------
 block/vpc.c                | 99 ++++++++++++++++++++--------------------------
 tests/qemu-iotests/084     | 16 +++++++-
 tests/qemu-iotests/084.out | 15 +++++++
 5 files changed, 111 insertions(+), 104 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/5] block: allow bdrv_unref() to be passed NULL pointers

[Jeff Cody]

If bdrv_unref() is passed a NULL BDS pointer, it is safe to
exit with no operation.  This will allow cleanup code to blindly
call bdrv_unref() on a BDS that has been initialized to NULL.

Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block.c b/block.c
index 23f366d..f79efc8 100644
--- a/block.c
+++ b/block.c
@@ -5385,6 +5385,9 @@ void bdrv_ref(BlockDriverState *bs)
  * deleted. */
 void bdrv_unref(BlockDriverState *bs)
 {
+    if (!bs) {
+        return;
+    }
     assert(bs->refcnt > 0);
     if (--bs->refcnt == 0) {
         bdrv_delete(bs);
-- 
1.9.3

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

[Jeff Cody]

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 hacks such as NOCOW are not needed in the image format driver, and
the underlying file protocol appropriate for the host OS can be relied
upon.

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(-)

diff --git a/block/vdi.c b/block/vdi.c
index 197bd77..5fd9d5f 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, offset + blocks * block_size);
+        if (result < 0) {
+            error_setg(errp, "Failed to statically allocate %s", filename);
+            goto exit;
         }
     }
 
-close_and_exit:
-    if ((close(fd) < 0) && !result) {
-        result = -errno;
-    }
-
 exit:
+    bdrv_unref(bs);
+    g_free(bmap);
     return result;
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 3/5] block: use the standard 'ret' instead of 'result'

[Jeff Cody]

Most QEMU code uses 'ret' for function return values. The VDI driver
uses a mix of 'result' and 'ret'.  This cleans that up, switching over
to the standard 'ret' usage.

Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vdi.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 5fd9d5f..cb347a7 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -357,23 +357,23 @@ static int vdi_make_empty(BlockDriverState *bs)
 static int vdi_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
     const VdiHeader *header = (const VdiHeader *)buf;
-    int result = 0;
+    int ret = 0;
 
     logout("\n");
 
     if (buf_size < sizeof(*header)) {
         /* Header too small, no VDI. */
     } else if (le32_to_cpu(header->signature) == VDI_SIGNATURE) {
-        result = 100;
+        ret = 100;
     }
 
-    if (result == 0) {
+    if (ret == 0) {
         logout("no vdi image\n");
     } else {
         logout("%s", header->text);
     }
 
-    return result;
+    return ret;
 }
 
 static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
@@ -681,7 +681,7 @@ static int vdi_co_write(BlockDriverState *bs,
 
 static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
 {
-    int result = 0;
+    int ret = 0;
     uint64_t bytes = 0;
     uint32_t blocks;
     size_t block_size = DEFAULT_CLUSTER_SIZE;
@@ -711,21 +711,21 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
 #endif
 
     if (bytes > VDI_DISK_SIZE_MAX) {
-        result = -ENOTSUP;
+        ret = -ENOTSUP;
         error_setg(errp, "Unsupported VDI image size (size is 0x%" PRIx64
                           ", max supported is 0x%" PRIx64 ")",
                           bytes, VDI_DISK_SIZE_MAX);
         goto exit;
     }
 
-    result = bdrv_create_file(filename, opts, &local_err);
-    if (result < 0) {
+    ret = bdrv_create_file(filename, opts, &local_err);
+    if (ret < 0) {
         error_propagate(errp, local_err);
         goto exit;
     }
-    result = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
-                       NULL, &local_err);
-    if (result < 0) {
+    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                    NULL, &local_err);
+    if (ret < 0) {
         error_propagate(errp, local_err);
         goto exit;
     }
@@ -759,8 +759,8 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
     vdi_header_print(&header);
 #endif
     vdi_header_to_le(&header);
-    result = bdrv_pwrite_sync(bs, offset, &header, sizeof(header));
-    if (result < 0) {
+    ret = bdrv_pwrite_sync(bs, offset, &header, sizeof(header));
+    if (ret < 0) {
         error_setg(errp, "Error writing header to %s", filename);
         goto exit;
     }
@@ -775,8 +775,8 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
                 bmap[i] = VDI_UNALLOCATED;
             }
         }
-        result = bdrv_pwrite_sync(bs, offset, bmap, bmap_size);
-        if (result < 0) {
+        ret = bdrv_pwrite_sync(bs, offset, bmap, bmap_size);
+        if (ret < 0) {
             error_setg(errp, "Error writing bmap to %s", filename);
             goto exit;
         }
@@ -784,8 +784,8 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
     }
 
     if (image_type == VDI_TYPE_STATIC) {
-        result = bdrv_truncate(bs, offset + blocks * block_size);
-        if (result < 0) {
+        ret = bdrv_truncate(bs, offset + blocks * block_size);
+        if (ret < 0) {
             error_setg(errp, "Failed to statically allocate %s", filename);
             goto exit;
         }
@@ -794,7 +794,7 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
 exit:
     bdrv_unref(bs);
     g_free(bmap);
-    return result;
+    return ret;
 }
 
 static void vdi_close(BlockDriverState *bs)
-- 
1.9.3

[Qemu-devel] [PATCH v2 4/5] block: vpc - use block layer ops in vpc_create, instead of posix calls

[Jeff Cody]

Use the block layer to create, and write to, the image file in the VPC
.bdrv_create() operation.

This has a couple of benefits: Images can now be created over protocols,
and hacks such as NOCOW are not needed in the image format driver, and
the underlying file protocol appropriate for the host OS can be relied
upon.

Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 99 +++++++++++++++++++++++++++----------------------------------
 1 file changed, 43 insertions(+), 56 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 8b376a4..c418c23 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -656,39 +656,41 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
     return 0;
 }
 
-static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
+static int create_dynamic_disk(BlockDriverState *bs, uint8_t *buf,
+                               int64_t total_sectors)
 {
     VHDDynDiskHeader *dyndisk_header =
         (VHDDynDiskHeader *) buf;
     size_t block_size, num_bat_entries;
     int i;
-    int ret = -EIO;
+    int ret;
+    int64_t offset = 0;
 
     // Write the footer (twice: at the beginning and at the end)
     block_size = 0x200000;
     num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);
 
-    if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
+    ret = bdrv_pwrite_sync(bs, offset, buf, HEADER_SIZE);
+    if (ret) {
         goto fail;
     }
 
-    if (lseek(fd, 1536 + ((num_bat_entries * 4 + 511) & ~511), SEEK_SET) < 0) {
-        goto fail;
-    }
-    if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
+    offset = 1536 + ((num_bat_entries * 4 + 511) & ~511);
+    ret = bdrv_pwrite_sync(bs, offset, buf, HEADER_SIZE);
+    if (ret < 0) {
         goto fail;
     }
 
     // Write the initial BAT
-    if (lseek(fd, 3 * 512, SEEK_SET) < 0) {
-        goto fail;
-    }
+    offset = 3 * 512;
 
     memset(buf, 0xFF, 512);
     for (i = 0; i < (num_bat_entries * 4 + 511) / 512; i++) {
-        if (write(fd, buf, 512) != 512) {
+        ret = bdrv_pwrite_sync(bs, offset, buf, 512);
+        if (ret < 0) {
             goto fail;
         }
+        offset += 512;
     }
 
     // Prepare the Dynamic Disk Header
@@ -709,39 +711,35 @@ static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
     dyndisk_header->checksum = be32_to_cpu(vpc_checksum(buf, 1024));
 
     // Write the header
-    if (lseek(fd, 512, SEEK_SET) < 0) {
-        goto fail;
-    }
+    offset = 512;
 
-    if (write(fd, buf, 1024) != 1024) {
+    ret = bdrv_pwrite_sync(bs, offset, buf, 1024);
+    if (ret < 0) {
         goto fail;
     }
-    ret = 0;
 
  fail:
     return ret;
 }
 
-static int create_fixed_disk(int fd, uint8_t *buf, int64_t total_size)
+static int create_fixed_disk(BlockDriverState *bs, uint8_t *buf,
+                             int64_t total_size)
 {
-    int ret = -EIO;
+    int ret;
 
     /* Add footer to total size */
-    total_size += 512;
-    if (ftruncate(fd, total_size) != 0) {
-        ret = -errno;
-        goto fail;
-    }
-    if (lseek(fd, -512, SEEK_END) < 0) {
-        goto fail;
-    }
-    if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
-        goto fail;
+    total_size += HEADER_SIZE;
+
+    ret = bdrv_truncate(bs, total_size);
+    if (ret < 0) {
+        return ret;
     }
 
-    ret = 0;
+    ret = bdrv_pwrite_sync(bs, total_size - HEADER_SIZE, buf, HEADER_SIZE);
+    if (ret < 0) {
+        return ret;
+    }
 
- fail:
     return ret;
 }
 
@@ -750,7 +748,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
     uint8_t buf[1024];
     VHDFooter *footer = (VHDFooter *) buf;
     char *disk_type_param;
-    int fd, i;
+    int i;
     uint16_t cyls = 0;
     uint8_t heads = 0;
     uint8_t secs_per_cyl = 0;
@@ -758,7 +756,8 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
     int64_t total_size;
     int disk_type;
     int ret = -EIO;
-    bool nocow = false;
+    Error *local_err = NULL;
+    BlockDriverState *bs = NULL;
 
     /* Read out options */
     total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
@@ -775,28 +774,17 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
     } else {
         disk_type = VHD_DYNAMIC;
     }
-    nocow = qemu_opt_get_bool_del(opts, BLOCK_OPT_NOCOW, false);
 
-    /* Create the file */
-    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
-    if (fd < 0) {
-        ret = -EIO;
+    ret = bdrv_create_file(filename, opts, &local_err);
+    if (ret < 0) {
+        error_propagate(errp, local_err);
         goto out;
     }
-
-    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
+    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                    NULL, &local_err);
+    if (ret < 0) {
+        error_propagate(errp, local_err);
+        goto out;
     }
 
     /*
@@ -810,7 +798,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
                                &secs_per_cyl))
         {
             ret = -EFBIG;
-            goto fail;
+            goto out;
         }
     }
 
@@ -856,14 +844,13 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
     footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
 
     if (disk_type == VHD_DYNAMIC) {
-        ret = create_dynamic_disk(fd, buf, total_sectors);
+        ret = create_dynamic_disk(bs, buf, total_sectors);
     } else {
-        ret = create_fixed_disk(fd, buf, total_size);
+        ret = create_fixed_disk(bs, buf, total_size);
     }
 
-fail:
-    qemu_close(fd);
 out:
+    bdrv_unref(bs);
     g_free(disk_type_param);
     return ret;
 }
-- 
1.9.3

[Qemu-devel] [PATCH v2 5/5] block: iotest - update 084 to test static VDI image creation

[Jeff Cody]

This updates the VDI corruption test to also test static VDI image
creation, as well as the default dynamic image creation.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/084     | 16 ++++++++++++++--
 tests/qemu-iotests/084.out | 15 +++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/084 b/tests/qemu-iotests/084
index cb4d7b7..ae33c2c 100755
--- a/tests/qemu-iotests/084
+++ b/tests/qemu-iotests/084
@@ -1,6 +1,7 @@
 #!/bin/bash
 #
-# Test case for VDI header corruption; image too large, and too many blocks
+# Test case for VDI header corruption; image too large, and too many blocks.
+# Also simple test for creating dynamic and static VDI images.
 #
 # Copyright (C) 2013 Red Hat, Inc.
 #
@@ -43,14 +44,25 @@ _supported_fmt vdi
 _supported_proto generic
 _supported_os Linux
 
+size=64M
 ds_offset=368  # disk image size field offset
 bs_offset=376  # block size field offset
 bii_offset=384 # block in image field offset
 
 echo
+echo "=== Statically allocated image creation ==="
+echo
+_make_test_img $size -o static
+_img_info
+stat -c"disk image file size in bytes: %s" "${TEST_IMG}"
+_cleanup_test_img
+
+echo
 echo "=== Testing image size bounds ==="
 echo
-_make_test_img 64M
+_make_test_img $size
+_img_info
+stat -c"disk image file size in bytes: %s" "${TEST_IMG}"
 
 # check for image size too large
 # poke max image size, and appropriate blocks_in_image value
diff --git a/tests/qemu-iotests/084.out b/tests/qemu-iotests/084.out
index c7120d9..2dfafb7 100644
--- a/tests/qemu-iotests/084.out
+++ b/tests/qemu-iotests/084.out
@@ -1,8 +1,23 @@
 QA output created by 084
 
+=== Statically allocated image creation ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+statically allocating 67109888 bytes
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+cluster_size: 1048576
+disk image file size in bytes: 67109888
+
 === Testing image size bounds ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+cluster_size: 1048576
+disk image file size in bytes: 1024
 Test 1: Maximum size (1024 TB):
 qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument
 
-- 
1.9.3

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

[Max Reitz]

On 21.07.2014 21:52, 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 hacks such as NOCOW are not needed in the image format driver, and
> the underlying file protocol appropriate for the host OS can be relied

This sounds a bit strange to me, but I don't know if it's wrong.

> upon.
>
> 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(-)
>
> diff --git a/block/vdi.c b/block/vdi.c
> index 197bd77..5fd9d5f 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -681,7 +681,6 @@ static int vdi_co_write(BlockDriverState *bs,
>   

Anyway:

Reviewed-by: Max Reitz <mreitz@redhat.com>

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

[Jeff Cody]

On Tue, Jul 22, 2014 at 10:14:58PM +0200, Max Reitz wrote:
> On 21.07.2014 21:52, 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 hacks such as NOCOW are not needed in the image format driver, and
> >the underlying file protocol appropriate for the host OS can be relied
> 
> This sounds a bit strange to me, but I don't know if it's wrong.
>

You find the wording strange, or the logic itself?  If the latter, the
use of the posix calls (open(), write()) means that we cannot create
images over protocols such as gluster - so using the qemu block layer
operations instead means protocols are now supported.  And NOCOW is
not needed in the image format driver now, since raw-posix does the
same thing (if the bs->file protocol driver is a host file on a posix
system).

> >upon.
> >
> >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(-)
> >
> >diff --git a/block/vdi.c b/block/vdi.c
> >index 197bd77..5fd9d5f 100644
> >--- a/block/vdi.c
> >+++ b/block/vdi.c
> >@@ -681,7 +681,6 @@ static int vdi_co_write(BlockDriverState *bs,
> 
> Anyway:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

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

[Max Reitz]

On 22.07.2014 22:19, Jeff Cody wrote:
> On Tue, Jul 22, 2014 at 10:14:58PM +0200, Max Reitz wrote:
>> On 21.07.2014 21:52, 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 hacks such as NOCOW are not needed in the image format driver, and
>>> the underlying file protocol appropriate for the host OS can be relied
>> This sounds a bit strange to me, but I don't know if it's wrong.
>>
> You find the wording strange, or the logic itself?  If the latter, the

Just the wording. :-)

> use of the posix calls (open(), write()) means that we cannot create
> images over protocols such as gluster - so using the qemu block layer
> operations instead means protocols are now supported.  And NOCOW is
> not needed in the image format driver now, since raw-posix does the
> same thing (if the bs->file protocol driver is a host file on a posix
> system).
>
>>> upon.
>>>
>>> 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(-)
>>>
>>> diff --git a/block/vdi.c b/block/vdi.c
>>> index 197bd77..5fd9d5f 100644
>>> --- a/block/vdi.c
>>> +++ b/block/vdi.c
>>> @@ -681,7 +681,6 @@ static int vdi_co_write(BlockDriverState *bs,
>> Anyway:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 5/5] block: iotest - update 084 to test static VDI image creation

[Max Reitz]

On 21.07.2014 21:52, Jeff Cody wrote:
> This updates the VDI corruption test to also test static VDI image
> creation, as well as the default dynamic image creation.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   tests/qemu-iotests/084     | 16 ++++++++++++++--
>   tests/qemu-iotests/084.out | 15 +++++++++++++++
>   2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/084 b/tests/qemu-iotests/084
> index cb4d7b7..ae33c2c 100755
> --- a/tests/qemu-iotests/084
> +++ b/tests/qemu-iotests/084
> @@ -1,6 +1,7 @@
>   #!/bin/bash
>   #
> -# Test case for VDI header corruption; image too large, and too many blocks
> +# Test case for VDI header corruption; image too large, and too many blocks.
> +# Also simple test for creating dynamic and static VDI images.
>   #
>   # Copyright (C) 2013 Red Hat, Inc.
>   #
> @@ -43,14 +44,25 @@ _supported_fmt vdi
>   _supported_proto generic
>   _supported_os Linux
>   
> +size=64M
>   ds_offset=368  # disk image size field offset
>   bs_offset=376  # block size field offset
>   bii_offset=384 # block in image field offset
>   
>   echo
> +echo "=== Statically allocated image creation ==="
> +echo
> +_make_test_img $size -o static
> +_img_info
> +stat -c"disk image file size in bytes: %s" "${TEST_IMG}"
> +_cleanup_test_img
> +
> +echo
>   echo "=== Testing image size bounds ==="
>   echo
> -_make_test_img 64M
> +_make_test_img $size
> +_img_info
> +stat -c"disk image file size in bytes: %s" "${TEST_IMG}"
>   
>   # check for image size too large
>   # poke max image size, and appropriate blocks_in_image value
> diff --git a/tests/qemu-iotests/084.out b/tests/qemu-iotests/084.out
> index c7120d9..2dfafb7 100644
> --- a/tests/qemu-iotests/084.out
> +++ b/tests/qemu-iotests/084.out
> @@ -1,8 +1,23 @@
>   QA output created by 084
>   
> +=== Statically allocated image creation ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +statically allocating 67109888 bytes

This seems like left-over debug output to me (at least it doesn't show 
up for me and I don't find it in block/vdi.c).

With this line removed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +image: TEST_DIR/t.IMGFMT
> +file format: IMGFMT
> +virtual size: 64M (67108864 bytes)
> +cluster_size: 1048576
> +disk image file size in bytes: 67109888
> +
>   === Testing image size bounds ===
>   
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +image: TEST_DIR/t.IMGFMT
> +file format: IMGFMT
> +virtual size: 64M (67108864 bytes)
> +cluster_size: 1048576
> +disk image file size in bytes: 1024
>   Test 1: Maximum size (1024 TB):
>   qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument
>   

Re: [Qemu-devel] [PATCH v2 5/5] block: iotest - update 084 to test static VDI image creation

[Jeff Cody]

On Tue, Jul 22, 2014 at 10:21:48PM +0200, Max Reitz wrote:
> On 21.07.2014 21:52, Jeff Cody wrote:
> >This updates the VDI corruption test to also test static VDI image
> >creation, as well as the default dynamic image creation.
> >
> >Signed-off-by: Jeff Cody <jcody@redhat.com>
> >---
> >  tests/qemu-iotests/084     | 16 ++++++++++++++--
> >  tests/qemu-iotests/084.out | 15 +++++++++++++++
> >  2 files changed, 29 insertions(+), 2 deletions(-)
> >
> >diff --git a/tests/qemu-iotests/084 b/tests/qemu-iotests/084
> >index cb4d7b7..ae33c2c 100755
> >--- a/tests/qemu-iotests/084
> >+++ b/tests/qemu-iotests/084
> >@@ -1,6 +1,7 @@
> >  #!/bin/bash
> >  #
> >-# Test case for VDI header corruption; image too large, and too many blocks
> >+# Test case for VDI header corruption; image too large, and too many blocks.
> >+# Also simple test for creating dynamic and static VDI images.
> >  #
> >  # Copyright (C) 2013 Red Hat, Inc.
> >  #
> >@@ -43,14 +44,25 @@ _supported_fmt vdi
> >  _supported_proto generic
> >  _supported_os Linux
> >+size=64M
> >  ds_offset=368  # disk image size field offset
> >  bs_offset=376  # block size field offset
> >  bii_offset=384 # block in image field offset
> >  echo
> >+echo "=== Statically allocated image creation ==="
> >+echo
> >+_make_test_img $size -o static
> >+_img_info
> >+stat -c"disk image file size in bytes: %s" "${TEST_IMG}"
> >+_cleanup_test_img
> >+
> >+echo
> >  echo "=== Testing image size bounds ==="
> >  echo
> >-_make_test_img 64M
> >+_make_test_img $size
> >+_img_info
> >+stat -c"disk image file size in bytes: %s" "${TEST_IMG}"
> >  # check for image size too large
> >  # poke max image size, and appropriate blocks_in_image value
> >diff --git a/tests/qemu-iotests/084.out b/tests/qemu-iotests/084.out
> >index c7120d9..2dfafb7 100644
> >--- a/tests/qemu-iotests/084.out
> >+++ b/tests/qemu-iotests/084.out
> >@@ -1,8 +1,23 @@
> >  QA output created by 084
> >+=== Statically allocated image creation ===
> >+
> >+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> >+statically allocating 67109888 bytes
> 
> This seems like left-over debug output to me (at least it doesn't
> show up for me and I don't find it in block/vdi.c).
>

Indeed, it is.  I'll respin & remove it, and add your R-b.

Thanks,
Jeff


> With this line removed:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> >+image: TEST_DIR/t.IMGFMT
> >+file format: IMGFMT
> >+virtual size: 64M (67108864 bytes)
> >+cluster_size: 1048576
> >+disk image file size in bytes: 67109888
> >+
> >  === Testing image size bounds ===
> >  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> >+image: TEST_DIR/t.IMGFMT
> >+file format: IMGFMT
> >+virtual size: 64M (67108864 bytes)
> >+cluster_size: 1048576
> >+disk image file size in bytes: 1024
> >  Test 1: Maximum size (1024 TB):
> >  qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument
>