Re: [Qemu-devel] [PATCH] qemu-img: Check post-truncation size

[Max Reitz <mreitz@redhat.com>]

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

On 2018-04-21 17:35, Eric Blake wrote:
> On 04/20/2018 05:53 PM, Max Reitz wrote:
>> Some block drivers (iscsi and file-posix when dealing with device files)
>> do not actually support truncation, even though they provide a
>> .bdrv_truncate() method and will happily return success when providing a
>> new size that does not exceed the current size.  This is because these
>> drivers expect the user to resize the image outside of qemu and then
>> provide qemu with that information through the block_resize command
>> (compare cb1b83e740384b4e0d950f3d7c81c02b8ce86c2e).
>>
>> Of course, anyone using qemu-img resize will find that behavior useless.
>> So we should check the actual size of the image after the supposedly
>> successful truncation took place, emit an error if nothing changed and
>> emit a warning if the target size was not met.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1523065
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> Testing this is not quite trivial.  Or, well, it is, but you need either
>> an iscsi test server or root access.
> 
> Or, you need NBD to document and implement NBD_CMD_RESIZE, and then the
> nbd driver will support .bdrv_truncate() but fail when talking to a
> server that doesn't actually resize after all.

I suppose the NBD client would recognize that, though, and return an
error code (and set *errp).  The issue in this case is that the drivers
in question pretend that everything went according to plan (they return
success) when actually nothing was resized at all.

>>
>> Because in my opinion iotests that require root access are never run, I
>> decided against writing such a test case.
> 
> So maybe when I get around to adding NBD resize support, I should add
> such a test ;)
> 
> 
>> +    if (new_size != total_size && new_size == current_size) {
>> +        error_report("Image was not resized. Resizing may not be supported "
>> +                     "for this image.");
> 
> error_report() generally does not have trailing dot, and generally has a
> single sentence.  Would this be better as:
> 
> Image was not resized; resizing may not be supported for this image

Yes, it would.  I just made this a qprintf() in the first version and
forgot to change it when making it an error_report().

>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>> +    if (new_size != total_size) {
>> +        warn_report("Image should have been resized to %" PRIi64
>> +                    " bytes, but was resized to %" PRIi64 " bytes.",
>> +                    total_size, new_size);
> 
> Trailing dot again.

Same here, yes.

>                      Also, PRId64 is much more common than PRIi64, even
> though the two are identical in behavior.

:-(

But I like my %i!

> But the idea makes sense to me.

OK, thanks.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]