| Message ID | 1385546829-3839-8-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 27, 2013 at 11:07:07AM +0100, Peter Lieven wrote: > @@ -1397,19 +1396,21 @@ static int img_convert(int argc, char **argv) > } > } > > + cluster_sectors = 0; > + ret = bdrv_get_info(out_bs, &bdi); > + if (ret < 0 && compress) { > + error_report("could not get block driver info"); > + goto out; > + } else { > + cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE; > + } Why do we only report error if 'compress' is set? cluster_sectors must be valid and we cannot guarantee that if bdrv_get_info() failed.
Am 04.12.2013 16:49, schrieb Stefan Hajnoczi: > On Wed, Nov 27, 2013 at 11:07:07AM +0100, Peter Lieven wrote: >> @@ -1397,19 +1396,21 @@ static int img_convert(int argc, char **argv) >> } >> } >> >> + cluster_sectors = 0; >> + ret = bdrv_get_info(out_bs, &bdi); >> + if (ret < 0 && compress) { >> + error_report("could not get block driver info"); >> + goto out; >> + } else { >> + cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE; >> + } > Why do we only report error if 'compress' is set? cluster_sectors must > be valid and we cannot guarantee that if bdrv_get_info() failed. You mean this should be: + if (ret < 0) { + if (compress) { + error_report("could not get block driver info"); + goto out; + } + } else { + cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE; + } if cluster_sectors is 0 the alignment logic is skipped, but we cannot guarantee that bdi is zero and stays zero if the call fails. can you fix that when you pick up the patch? Peter
On Wed, Dec 04, 2013 at 04:56:19PM +0100, Peter Lieven wrote: > Am 04.12.2013 16:49, schrieb Stefan Hajnoczi: > > On Wed, Nov 27, 2013 at 11:07:07AM +0100, Peter Lieven wrote: > >> @@ -1397,19 +1396,21 @@ static int img_convert(int argc, char **argv) > >> } > >> } > >> > >> + cluster_sectors = 0; > >> + ret = bdrv_get_info(out_bs, &bdi); > >> + if (ret < 0 && compress) { > >> + error_report("could not get block driver info"); > >> + goto out; > >> + } else { > >> + cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE; > >> + } > > Why do we only report error if 'compress' is set? cluster_sectors must > > be valid and we cannot guarantee that if bdrv_get_info() failed. > You mean this should be: > > + if (ret < 0) { > + if (compress) { > + error_report("could not get block driver info"); > + goto out; > + } > + } else { > + cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE; > + } > > > if cluster_sectors is 0 the alignment logic is skipped, but we cannot > guarantee that bdi is zero and stays zero if the call fails. > > can you fix that when you pick up the patch? Sure. Stefan
diff --git a/qemu-img.c b/qemu-img.c index 15f423b..9bb1f6f 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1124,8 +1124,7 @@ out3: static int img_convert(int argc, char **argv) { - int c, n, n1, bs_n, bs_i, compress, cluster_size, - cluster_sectors, skip_create; + int c, n, n1, bs_n, bs_i, compress, cluster_sectors, skip_create; int64_t ret = 0; int progress = 0, flags; const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename; @@ -1397,19 +1396,21 @@ static int img_convert(int argc, char **argv) } } + cluster_sectors = 0; + ret = bdrv_get_info(out_bs, &bdi); + if (ret < 0 && compress) { + error_report("could not get block driver info"); + goto out; + } else { + cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE; + } + if (compress) { - ret = bdrv_get_info(out_bs, &bdi); - if (ret < 0) { - error_report("could not get block driver info"); - goto out; - } - cluster_size = bdi.cluster_size; - if (cluster_size <= 0 || cluster_size > bufsectors * BDRV_SECTOR_SIZE) { + if (cluster_sectors <= 0 || cluster_sectors > bufsectors) { error_report("invalid cluster size"); ret = -1; goto out; } - cluster_sectors = cluster_size >> 9; sector_num = 0; nb_sectors = total_sectors; @@ -1542,6 +1543,19 @@ static int img_convert(int argc, char **argv) } n = MIN(nb_sectors, bufsectors); + + /* round down request length to an aligned sector, but + * do not bother doing this on short requests. They happen + * when we found an all-zero area, and the next sector to + * write will not be sector_num + n. */ + if (cluster_sectors > 0 && n >= cluster_sectors) { + int64_t next_aligned_sector = (sector_num + n); + next_aligned_sector -= next_aligned_sector % cluster_sectors; + if (sector_num + n > next_aligned_sector) { + n = next_aligned_sector - sector_num; + } + } + n = MIN(n, bs_sectors - (sector_num - bs_offset)); n1 = n;