[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qemu-img: Check post-truncation size
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH] qemu-img: Check post-truncation size |
Date: |
Sat, 21 Apr 2018 17:39:36 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
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 <address@hidden>
>> ---
>> 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
signature.asc
Description: OpenPGP digital signature