[Qemu-devel] [PATCH for-2.6 0/7] block: VHD format fixes

[Qemu-devel] [PATCH for-2.6 0/7] block: VHD format fixes

[Jeff Cody]

Fixes for a regression in vpc_create(), as well as a few issues with VHD format
compatibility.


Jeff Cody (6):
  block/vpc: fix VPC 'qemu-img create' regression
  block/vpc: use current_size field for XenConverter VHD images
  block/vpc: Use the correct max sector count for VHD images
  block/vpc: make checks on max table size a bit more lax
  block/vpc: set errp in vpc_open
  block/vpc: update comments to be compliant w/coding guidelines

Stefan Hajnoczi (1):
  vpc: use current_size field for XenServer VHD images

 block/vpc.c | 106 ++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 60 insertions(+), 46 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH for-2.6 1/7] block/vpc: fix VPC 'qemu-img create' regression

[Jeff Cody]

Commit 'b8f45cdf7827e39f9a1e6cc446f5972cc6144237' switched VPC
over to using blk_pwrite() instead of bdrv_pwrite_sync().  The
return value of bdrv_pwrite_sync() was always 0 for success, and
create_dynamic_disk() in one instance checked for a non-zero return
value to indicate error.  However, blk_pwrite() may return positive
values for success.

This fails silently as well, since vpc_create() did not set errp
in this failuer case.  Set errp in all instances in vpc_create().

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index 8435205..bc3d1c6 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -774,7 +774,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);
 
     ret = blk_pwrite(blk, offset, buf, HEADER_SIZE);
-    if (ret) {
+    if (ret < 0) {
         goto fail;
     }
 
@@ -873,6 +873,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
         } else if (!strcmp(disk_type_param, "fixed")) {
             disk_type = VHD_FIXED;
         } else {
+            error_setg(errp, "Invalid disk type, %s", disk_type_param);
             ret = -EINVAL;
             goto out;
         }
@@ -924,6 +925,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
         total_sectors = total_size / BDRV_SECTOR_SIZE;
         /* Allow a maximum disk size of approximately 2 TB */
         if (total_sectors > VHD_MAX_SECTORS) {
+            error_setg(errp, "Disk size is too large, max size is 2040 GiB");
             ret = -EFBIG;
             goto out;
         }
@@ -974,6 +976,9 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
     } else {
         ret = create_fixed_disk(blk, buf, total_size);
     }
+    if (ret < 0) {
+        error_setg(errp, "Unable to create or write VHD header");
+    }
 
 out:
     blk_unref(blk);
-- 
1.9.3

[Qemu-devel] [PATCH for-2.6 2/7] vpc: use current_size field for XenServer VHD images

[Jeff Cody]

From: Stefan Hajnoczi <stefanha@redhat.com>

The vpc driver has two methods of determining virtual disk size.  The
correct one to use depends on the software that generated the image
file.  Add the XenServer creator_app signature so that image size is
correctly detected for those images.

Reported-by: Grant Wu <grantwwu@gmail.com>
Reported-by: Spencer Baugh <sbaugh@catern.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index bc3d1c6..8b8b9a7 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -298,6 +298,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
      *      'qem2'  :  current_size     QEMU (uses current_size)
      *      'win '  :  current_size     Hyper-V
      *      'd2v '  :  current_size     Disk2vhd
+     *      'tap\0' :  current_size     XenServer
      *
      *  The user can override the table values via drive options, however
      *  even with an override we will still use current_size for images
@@ -305,7 +306,8 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
      */
     use_chs = (!!strncmp(footer->creator_app, "win ", 4) &&
                !!strncmp(footer->creator_app, "qem2", 4) &&
-               !!strncmp(footer->creator_app, "d2v ", 4)) || s->force_use_chs;
+               !!strncmp(footer->creator_app, "d2v ", 4) &&
+               !!memcmp(footer->creator_app, "tap", 4)) || s->force_use_chs;
 
     if (!use_chs || bs->total_sectors == VHD_MAX_GEOMETRY || s->force_use_sz) {
         bs->total_sectors = be64_to_cpu(footer->current_size) /
-- 
1.9.3

[Qemu-devel] [PATCH for-2.6 3/7] block/vpc: use current_size field for XenConverter VHD images

[Jeff Cody]

XenConverter VHD images are another VHD image where current_size is
different from the CHS values in the the format header.  Use
current_size as the default, by looking at the creator_app signature
field.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/vpc.c b/block/vpc.c
index 8b8b9a7..6ad8406 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -299,6 +299,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
      *      'win '  :  current_size     Hyper-V
      *      'd2v '  :  current_size     Disk2vhd
      *      'tap\0' :  current_size     XenServer
+     *      'CTXS'  :  current_size     XenConverter
      *
      *  The user can override the table values via drive options, however
      *  even with an override we will still use current_size for images
@@ -307,6 +308,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     use_chs = (!!strncmp(footer->creator_app, "win ", 4) &&
                !!strncmp(footer->creator_app, "qem2", 4) &&
                !!strncmp(footer->creator_app, "d2v ", 4) &&
+               !!strncmp(footer->creator_app, "CTXS", 4) &&
                !!memcmp(footer->creator_app, "tap", 4)) || s->force_use_chs;
 
     if (!use_chs || bs->total_sectors == VHD_MAX_GEOMETRY || s->force_use_sz) {
-- 
1.9.3

[Qemu-devel] [PATCH for-2.6 4/7] block/vpc: Use the correct max sector count for VHD images

[Jeff Cody]

The old VHD_MAX_SECTORS value is incorrect, and is a throwback
to the CHS calculations.  The VHD specification allows images up to 2040
GiB, which (using 512 byte sectors) corresponds to a maximum number of
sectors of 0xff000000, rather than the old value of 0xfe0001ff.

Update VHD_MAX_SECTORS to reflect the correct value.

Also, update comment references to the actual size limit, and correct
one compare so that we can have sizes up to the limit.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 6ad8406..2e023d0 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -51,7 +51,7 @@ enum vhd_type {
 #define VHD_CHS_MAX_H   16
 #define VHD_CHS_MAX_S   255
 
-#define VHD_MAX_SECTORS       (65535LL * 255 * 255)
+#define VHD_MAX_SECTORS       0xff000000    /* 2040 GiB max image size */
 #define VHD_MAX_GEOMETRY      (VHD_CHS_MAX_C * VHD_CHS_MAX_H * VHD_CHS_MAX_S)
 
 #define VPC_OPT_FORCE_SIZE "force_size"
@@ -316,8 +316,8 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
                                         BDRV_SECTOR_SIZE;
     }
 
-    /* Allow a maximum disk size of approximately 2 TB */
-    if (bs->total_sectors >= VHD_MAX_SECTORS) {
+    /* Allow a maximum disk size of 2040 GiB */
+    if (bs->total_sectors > VHD_MAX_SECTORS) {
         ret = -EFBIG;
         goto fail;
     }
@@ -721,7 +721,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
  * Note that the geometry doesn't always exactly match total_sectors but
  * may round it down.
  *
- * Returns 0 on success, -EFBIG if the size is larger than ~2 TB. Override
+ * Returns 0 on success, -EFBIG if the size is larger than 2040 GiB. Override
  * the hardware EIDE and ATA-2 limit of 16 heads (max disk size of 127 GB)
  * and instead allow up to 255 heads.
  */
@@ -927,7 +927,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
 
     if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) {
         total_sectors = total_size / BDRV_SECTOR_SIZE;
-        /* Allow a maximum disk size of approximately 2 TB */
+        /* Allow a maximum disk size of 2040 GiB */
         if (total_sectors > VHD_MAX_SECTORS) {
             error_setg(errp, "Disk size is too large, max size is 2040 GiB");
             ret = -EFBIG;
-- 
1.9.3

[Qemu-devel] [PATCH for-2.6 5/7] block/vpc: make checks on max table size a bit more lax

[Jeff Cody]

The check on the max_table_size field not being larger than required is
valid, and in accordance with the VHD spec.  However, there have been
VHD images encountered in the wild that have an out-of-spec max table
size that is technically too large.

There is no issue in allowing this larger table size, as we also
later verify that the computed size (used for the pagetable) is
large enough to fit all sectors.  In addition, max_table_entries
is bounds checked against SIZE_MAX and INT_MAX.

Remove the strict check, so that we can accomodate these sorts of
images that are benignly out of spec.

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Reported-by: Grant Wu <grantwwu@gmail.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 2e023d0..67ab376 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -350,10 +350,6 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             ret = -EINVAL;
             goto fail;
         }
-        if (s->max_table_entries > (VHD_MAX_SECTORS * 512) / s->block_size) {
-            ret = -EINVAL;
-            goto fail;
-        }
 
         computed_size = (uint64_t) s->max_table_entries * s->block_size;
         if (computed_size < bs->total_sectors * 512) {
-- 
1.9.3

[Qemu-devel] [PATCH for-2.6 6/7] block/vpc: set errp in vpc_open

[Jeff Cody]

Add more useful error information to failure paths in vpc_open

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/vpc.c b/block/vpc.c
index 67ab376..5dd9950 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -237,6 +237,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 
     ret = bdrv_pread(bs->file->bs, 0, s->footer_buf, HEADER_SIZE);
     if (ret < 0) {
+        error_setg(errp, "Unable to read VHD header");
         goto fail;
     }
 
@@ -245,9 +246,11 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         int64_t offset = bdrv_getlength(bs->file->bs);
         if (offset < 0) {
             ret = offset;
+            error_setg(errp, "Invalid file size");
             goto fail;
         } else if (offset < HEADER_SIZE) {
             ret = -EINVAL;
+            error_setg(errp, "File too small for a VHD header");
             goto fail;
         }
 
@@ -326,12 +329,14 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         ret = bdrv_pread(bs->file->bs, be64_to_cpu(footer->data_offset), buf,
                          HEADER_SIZE);
         if (ret < 0) {
+            error_setg(errp, "Error reading dynamic VHD header");
             goto fail;
         }
 
         dyndisk_header = (VHDDynDiskHeader *) buf;
 
         if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
+            error_setg(errp, "Invalid header magic");
             ret = -EINVAL;
             goto fail;
         }
@@ -347,12 +352,14 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         s->max_table_entries = be32_to_cpu(dyndisk_header->max_table_entries);
 
         if ((bs->total_sectors * 512) / s->block_size > 0xffffffffU) {
+            error_setg(errp, "Too many blocks");
             ret = -EINVAL;
             goto fail;
         }
 
         computed_size = (uint64_t) s->max_table_entries * s->block_size;
         if (computed_size < bs->total_sectors * 512) {
+            error_setg(errp, "Page table too small");
             ret = -EINVAL;
             goto fail;
         }
@@ -369,6 +376,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 
         s->pagetable = qemu_try_blockalign(bs->file->bs, pagetable_size);
         if (s->pagetable == NULL) {
+            error_setg(errp, "Unable to allocate memory for page table");
             ret = -ENOMEM;
             goto fail;
         }
@@ -378,6 +386,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         ret = bdrv_pread(bs->file->bs, s->bat_offset, s->pagetable,
                          pagetable_size);
         if (ret < 0) {
+            error_setg(errp, "Error reading pagetable");
             goto fail;
         }
 
-- 
1.9.3

[Qemu-devel] [PATCH for-2.6 7/7] block/vpc: update comments to be compliant w/coding guidelines

[Jeff Cody]

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 70 ++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 5dd9950..0b48cf4 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -36,7 +36,7 @@
 
 #define HEADER_SIZE 512
 
-//#define CACHE
+/* #define CACHE */
 
 enum vhd_type {
     VHD_FIXED           = 2,
@@ -44,7 +44,7 @@ enum vhd_type {
     VHD_DIFFERENCING    = 4,
 };
 
-// Seconds since Jan 1, 2000 0:00:00 (UTC)
+/* Seconds since Jan 1, 2000 0:00:00 (UTC) */
 #define VHD_TIMESTAMP_BASE 946684800
 
 #define VHD_CHS_MAX_C   65535LL
@@ -56,22 +56,22 @@ enum vhd_type {
 
 #define VPC_OPT_FORCE_SIZE "force_size"
 
-// always big-endian
+/* always big-endian */
 typedef struct vhd_footer {
-    char        creator[8]; // "conectix"
+    char        creator[8]; /* "conectix" */
     uint32_t    features;
     uint32_t    version;
 
-    // Offset of next header structure, 0xFFFFFFFF if none
+    /* Offset of next header structure, 0xFFFFFFFF if none */
     uint64_t    data_offset;
 
-    // Seconds since Jan 1, 2000 0:00:00 (UTC)
+    /* Seconds since Jan 1, 2000 0:00:00 (UTC) */
     uint32_t    timestamp;
 
-    char        creator_app[4]; // "vpc "
+    char        creator_app[4]; /*  e.g., "vpc " */
     uint16_t    major;
     uint16_t    minor;
-    char        creator_os[4]; // "Wi2k"
+    char        creator_os[4]; /* "Wi2k" */
 
     uint64_t    orig_size;
     uint64_t    current_size;
@@ -82,29 +82,29 @@ typedef struct vhd_footer {
 
     uint32_t    type;
 
-    // Checksum of the Hard Disk Footer ("one's complement of the sum of all
-    // the bytes in the footer without the checksum field")
+    /* Checksum of the Hard Disk Footer ("one's complement of the sum of all
+       the bytes in the footer without the checksum field") */
     uint32_t    checksum;
 
-    // UUID used to identify a parent hard disk (backing file)
+    /* UUID used to identify a parent hard disk (backing file) */
     uint8_t     uuid[16];
 
     uint8_t     in_saved_state;
 } QEMU_PACKED VHDFooter;
 
 typedef struct vhd_dyndisk_header {
-    char        magic[8]; // "cxsparse"
+    char        magic[8]; /* "cxsparse" */
 
-    // Offset of next header structure, 0xFFFFFFFF if none
+    /* Offset of next header structure, 0xFFFFFFFF if none */
     uint64_t    data_offset;
 
-    // Offset of the Block Allocation Table (BAT)
+    /* Offset of the Block Allocation Table (BAT) */
     uint64_t    table_offset;
 
     uint32_t    version;
-    uint32_t    max_table_entries; // 32bit/entry
+    uint32_t    max_table_entries; /* 32bit/entry */
 
-    // 2 MB by default, must be a power of two
+    /* 2 MB by default, must be a power of two */
     uint32_t    block_size;
 
     uint32_t    checksum;
@@ -112,7 +112,7 @@ typedef struct vhd_dyndisk_header {
     uint32_t    parent_timestamp;
     uint32_t    reserved;
 
-    // Backing file name (in UTF-16)
+    /* Backing file name (in UTF-16) */
     uint8_t     parent_name[512];
 
     struct {
@@ -277,9 +277,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     /* Write 'checksum' back to footer, or else will leave it with zero. */
     footer->checksum = cpu_to_be32(checksum);
 
-    // The visible size of a image in Virtual PC depends on the geometry
-    // rather than on the size stored in the footer (the size in the footer
-    // is too large usually)
+    /* The visible size of a image in Virtual PC depends on the geometry
+       rather than on the size stored in the footer (the size in the footer
+       is too large usually) */
     bs->total_sectors = (int64_t)
         be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
 
@@ -465,16 +465,16 @@ static inline int64_t get_sector_offset(BlockDriverState *bs,
     pageentry_index = (offset % s->block_size) / 512;
 
     if (pagetable_index >= s->max_table_entries || s->pagetable[pagetable_index] == 0xffffffff)
-        return -1; // not allocated
+        return -1; /* not allocated */
 
     bitmap_offset = 512 * (uint64_t) s->pagetable[pagetable_index];
     block_offset = bitmap_offset + s->bitmap_size + (512 * pageentry_index);
 
-    // We must ensure that we don't write to any sectors which are marked as
-    // unused in the bitmap. We get away with setting all bits in the block
-    // bitmap each time we write to a new block. This might cause Virtual PC to
-    // miss sparse read optimization, but it's not a problem in terms of
-    // correctness.
+    /* We must ensure that we don't write to any sectors which are marked as
+       unused in the bitmap. We get away with setting all bits in the block
+       bitmap each time we write to a new block. This might cause Virtual PC to
+       miss sparse read optimization, but it's not a problem in terms of
+       correctness. */
     if (write && (s->last_bitmap_offset != bitmap_offset)) {
         uint8_t bitmap[s->bitmap_size];
 
@@ -520,18 +520,18 @@ static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num)
     int ret;
     uint8_t bitmap[s->bitmap_size];
 
-    // Check if sector_num is valid
+    /* Check if sector_num is valid */
     if ((sector_num < 0) || (sector_num > bs->total_sectors))
         return -1;
 
-    // Write entry into in-memory BAT
+    /* Write entry into in-memory BAT */
     index = (sector_num * 512) / s->block_size;
     if (s->pagetable[index] != 0xFFFFFFFF)
         return -1;
 
     s->pagetable[index] = s->free_data_block_offset / 512;
 
-    // Initialize the block's bitmap
+    /* Initialize the block's bitmap */
     memset(bitmap, 0xff, s->bitmap_size);
     ret = bdrv_pwrite_sync(bs->file->bs, s->free_data_block_offset, bitmap,
         s->bitmap_size);
@@ -539,13 +539,13 @@ static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num)
         return ret;
     }
 
-    // Write new footer (the old one will be overwritten)
+    /* Write new footer (the old one will be overwritten) */
     s->free_data_block_offset += s->block_size + s->bitmap_size;
     ret = rewrite_footer(bs);
     if (ret < 0)
         goto fail;
 
-    // Write BAT entry to disk
+    /* Write BAT entry to disk */
     bat_offset = s->bat_offset + (4 * index);
     bat_value = cpu_to_be32(s->pagetable[index]);
     ret = bdrv_pwrite_sync(bs->file->bs, bat_offset, &bat_value, 4);
@@ -778,7 +778,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
     int ret;
     int64_t offset = 0;
 
-    // Write the footer (twice: at the beginning and at the end)
+    /* 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);
 
@@ -793,7 +793,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
         goto fail;
     }
 
-    // Write the initial BAT
+    /* Write the initial BAT */
     offset = 3 * 512;
 
     memset(buf, 0xFF, 512);
@@ -805,7 +805,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
         offset += 512;
     }
 
-    // Prepare the Dynamic Disk Header
+    /* Prepare the Dynamic Disk Header */
     memset(buf, 0, 1024);
 
     memcpy(dyndisk_header->magic, "cxsparse", 8);
@@ -822,7 +822,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
 
     dyndisk_header->checksum = cpu_to_be32(vpc_checksum(buf, 1024));
 
-    // Write the header
+    /* Write the header */
     offset = 512;
 
     ret = blk_pwrite(blk, offset, buf, 1024);
-- 
1.9.3

Re: [Qemu-devel] [PATCH for-2.6 1/7] block/vpc: fix VPC 'qemu-img create' regression

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 260 bytes --]

On Tue, Mar 22, 2016 at 11:33:38PM -0400, Jeff Cody wrote:
> This fails silently as well, since vpc_create() did not set errp
> in this failuer case.  Set errp in all instances in vpc_create().

s/faileur/failure/

Can be done when merging, no need to resend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH for-2.6 0/7] block: VHD format fixes

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

On Tue, Mar 22, 2016 at 11:33:37PM -0400, Jeff Cody wrote:
> Fixes for a regression in vpc_create(), as well as a few issues with VHD format
> compatibility.
> 
> 
> Jeff Cody (6):
>   block/vpc: fix VPC 'qemu-img create' regression
>   block/vpc: use current_size field for XenConverter VHD images
>   block/vpc: Use the correct max sector count for VHD images
>   block/vpc: make checks on max table size a bit more lax
>   block/vpc: set errp in vpc_open
>   block/vpc: update comments to be compliant w/coding guidelines
> 
> Stefan Hajnoczi (1):
>   vpc: use current_size field for XenServer VHD images
> 
>  block/vpc.c | 106 ++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 60 insertions(+), 46 deletions(-)
> 
> -- 
> 1.9.3
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH for-2.6 1/7] block/vpc: fix VPC 'qemu-img create' regression

[Jeff Cody]

On Tue, Mar 22, 2016 at 11:33:38PM -0400, Jeff Cody wrote:
> Commit 'b8f45cdf7827e39f9a1e6cc446f5972cc6144237' switched VPC
> over to using blk_pwrite() instead of bdrv_pwrite_sync().  The
> return value of bdrv_pwrite_sync() was always 0 for success, and
> create_dynamic_disk() in one instance checked for a non-zero return
> value to indicate error.  However, blk_pwrite() may return positive
> values for success.
> 
> This fails silently as well, since vpc_create() did not set errp
> in this failuer case.  Set errp in all instances in vpc_create().
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>

Ping on this series - ideally the whole series for 2.6, but this patch
in particular is needed for 2.6 to prevent a regression from 2.5 (QEMU
can no longer create VPC/VHD images without this patch).

> ---
>  block/vpc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 8435205..bc3d1c6 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -774,7 +774,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t *buf,
>      num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);
>  
>      ret = blk_pwrite(blk, offset, buf, HEADER_SIZE);
> -    if (ret) {
> +    if (ret < 0) {
>          goto fail;
>      }
>  
> @@ -873,6 +873,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
>          } else if (!strcmp(disk_type_param, "fixed")) {
>              disk_type = VHD_FIXED;
>          } else {
> +            error_setg(errp, "Invalid disk type, %s", disk_type_param);
>              ret = -EINVAL;
>              goto out;
>          }
> @@ -924,6 +925,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
>          total_sectors = total_size / BDRV_SECTOR_SIZE;
>          /* Allow a maximum disk size of approximately 2 TB */
>          if (total_sectors > VHD_MAX_SECTORS) {
> +            error_setg(errp, "Disk size is too large, max size is 2040 GiB");
>              ret = -EFBIG;
>              goto out;
>          }
> @@ -974,6 +976,9 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
>      } else {
>          ret = create_fixed_disk(blk, buf, total_size);
>      }
> +    if (ret < 0) {
> +        error_setg(errp, "Unable to create or write VHD header");
> +    }
>  
>  out:
>      blk_unref(blk);
> -- 
> 1.9.3
>

Re: [Qemu-devel] [PATCH for-2.6 1/7] block/vpc: fix VPC 'qemu-img create' regression

[Kevin Wolf]

Am 13.04.2016 um 17:40 hat Jeff Cody geschrieben:
> On Tue, Mar 22, 2016 at 11:33:38PM -0400, Jeff Cody wrote:
> > Commit 'b8f45cdf7827e39f9a1e6cc446f5972cc6144237' switched VPC
> > over to using blk_pwrite() instead of bdrv_pwrite_sync().  The
> > return value of bdrv_pwrite_sync() was always 0 for success, and
> > create_dynamic_disk() in one instance checked for a non-zero return
> > value to indicate error.  However, blk_pwrite() may return positive
> > values for success.
> > 
> > This fails silently as well, since vpc_create() did not set errp
> > in this failuer case.  Set errp in all instances in vpc_create().
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> 
> Ping on this series - ideally the whole series for 2.6, but this patch
> in particular is needed for 2.6 to prevent a regression from 2.5 (QEMU
> can no longer create VPC/VHD images without this patch).

The regression was already fixed in commit 40a99aac. With some changes
in the commit message, the rest of this patch is still useful.

Kevin

Re: [Qemu-devel] [PATCH for-2.6 7/7] block/vpc: update comments to be compliant w/coding guidelines

[Kevin Wolf]

Am 23.03.2016 um 04:33 hat Jeff Cody geschrieben:
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/vpc.c | 70 ++++++++++++++++++++++++++++++-------------------------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 5dd9950..0b48cf4 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -36,7 +36,7 @@
>  
>  #define HEADER_SIZE 512
>  
> -//#define CACHE
> +/* #define CACHE */

I'll revert this one while applying, C99 style is much more convenient
when you want to enable debugging and we're doing the same in other
places.

The rest of them is good.

Kevin

Re: [Qemu-devel] [PATCH for-2.6 0/7] block: VHD format fixes

[Kevin Wolf]

Am 23.03.2016 um 04:33 hat Jeff Cody geschrieben:
> Fixes for a regression in vpc_create(), as well as a few issues with VHD format
> compatibility.

Thanks, applied to the block brannch.

Kevin