Re: [Qemu-devel] [PATCH V9 2/4] raw, qcow2: don't convert file size to sector size

[Eric Blake]

On 05/27/2014 02:22 AM, Chen Fan wrote:
> From: Hu Tao <address@hidden>
> 
> and avoid converting it back later. And round up file size to nearest
> sector.

The fact that you started a sentence with "And" makes me wonder if this
should be two patches.

> 
> Signed-off-by: Hu Tao <address@hidden>
> ---
>  block/qcow2.c     | 8 ++++----
>  block/raw-posix.c | 5 +++--
>  block/raw-win32.c | 5 +++--
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 

> @@ -1779,7 +1779,7 @@ static int qcow2_create(const char *filename, 
> QEMUOptionParameter *options,
>      /* Read out options */
>      while (options && options->name) {
>          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            sectors = options->value.n / 512;
> +            size = (options->value.n + BDRV_SECTOR_SIZE) & BDRV_SECTOR_MASK;

Probably better to use DIV_ROUND_UP for this.

Also, I have mentioned several times that it appears that the qcow2 file
format is perfectly capable of representing a file size that is not a
sector or cluster multiple; it's just that the rest of the code base is
probably not well prepared to deal with that.  While I think that
rounding up instead of truncating is the RIGHT thing to do (and
therefore your patch is fixing a valid bug), I wonder if it is further
worth trying to represent the EXACT size requested by the user.  There's
probably a lot more followups.  If nothing else, I think that this
change (which corresponds to the sentence starting with "And" in your
commit message) deserves to be in its own patch, and accompanied by a
testsuite addition to prove we don't regress (whether we allow exact
size or always round up, the testsuite should at least prove that
'qemu-img create -f qcow2 foo.img 1234' should not truncate to a 1024
byte file, but should be at least 1234 bytes visible to the guest).

> +++ b/block/raw-posix.c
> @@ -1252,7 +1252,8 @@ static int raw_create(const char *filename, 
> QEMUOptionParameter *options,
>      /* Read out options */
>      while (options && options->name) {
>          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            total_size = options->value.n / BDRV_SECTOR_SIZE;
> +            total_size = (options->value.n + BDRV_SECTOR_SIZE) &
> +                BDRV_SECTOR_MASK;

Another candidate for DIV_ROUND_UP

> +++ b/block/raw-win32.c
> @@ -489,7 +489,8 @@ static int raw_create(const char *filename, 
> QEMUOptionParameter *options,
>      /* Read out options */
>      while (options && options->name) {
>          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            total_size = options->value.n / 512;
> +            total_size = (options->value.n + BDRV_SECTOR_SIZE) &
> +                BDRV_SECTOR_MASK;

And again.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature