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

[Max Reitz]

On 27.05.2014 21:13, Eric Blake wrote:
> 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.

Actually, these are not only candidates, but either *need* to use 
DIV_ROUND_UP or "options->value.n + BDRV_SECTOR_SIZE - 1" to be correct. 
Without "- 1", they are not rounding up, but rounding up and sometimes 
adding BDRV_SECTOR_SIZE on top of that.

Max

> 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.