| Message ID | 0cf3d19bbc32776d12b46cd471f82c20ea9ce182.1458702790.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 >
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
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);
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(-)