[libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary (rewrite)

[John Ferlan]

On 05/12/2014 07:11 AM, Ján Tomko wrote:
> On 05/08/2014 06:02 PM, John Ferlan wrote:
>> A post commit id 'e3d66229' review (and followup):
>>
>> http://www.redhat.com/archives/libvir-list/2014-May/msg00268.html
>>
>> noted some issues with the code, so I have adjusted the code
>> accordingly. The difference between this and the commit prior
>> to the change (commit id 'f3be5f0c') will just be the check for
>> qcow2/qed using a non 512 block aligned size will result in a
>> round up of size. If the size is within the last 512 bytes to
>> ULLONG_MAX, then just set it there rather than erroring out.
> 
> This patch essentially reverts commit e3d66229.
> Reverting it explicitly using 'git revert' and basing this patch on that would
> make it smaller and maybe easier to backport to the -maint branches.
> 
> If you choose to do an explicit revert, please include the bugzilla link in
> the commit message.
> 

So with the revert and the changes below this essentially turns into:

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f0e82e9..63a9e77 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9459,6 +9459,13 @@ qemuDomainBlockResize(virDomainPtr dom,
     }
     disk = vm->def->disks[idx];

+    /* qcow2 and qed must be sized on 512 byte blocks/sectors,
+     * so adjust size if necessary to round up.
+     */
+    if (disk->src.format == VIR_STORAGE_FILE_QCOW2 ||
+        disk->src.format == VIR_STORAGE_FILE_QED)
+        size = VIR_ROUND_UP(size, 512);
+
     if (virAsprintf(&device, "%s%s", QEMU_DRIVE_HOST_PREFIX,
                     disk->info.alias) < 0)
         goto endjob;


The whole overflow issue for VIR_ROUND_UP and VIR_DIV_UP is something
that could/would/should be handled separately...

John

>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/qemu/qemu_driver.c | 28 +++++++++-------------------
>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 4ff8a2d..8771cae 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -9413,7 +9413,6 @@ qemuDomainBlockResize(virDomainPtr dom,
>>      virDomainObjPtr vm;
>>      qemuDomainObjPrivatePtr priv;
>>      int ret = -1, idx;
>> -    unsigned long long size_up;
>>      char *device = NULL;
>>      virDomainDiskDefPtr disk = NULL;
>>  
>> @@ -9434,12 +9433,6 @@ qemuDomainBlockResize(virDomainPtr dom,
>>              return -1;
>>          }
>>          size *= 1024;
>> -        size_up = size;
>> -    } else {
>> -        /* For 'qcow2' and 'qed', qemu resize blocks expects values
>> -         * on sector boundary, so round our value up to prepare
>> -         */
>> -        size_up = VIR_ROUND_UP(size, 512);
>>      }
>>  
>>      if (!(vm = qemuDomObjFromDomain(dom)))
>> @@ -9466,19 +9459,16 @@ qemuDomainBlockResize(virDomainPtr dom,
>>      }
>>      disk = vm->def->disks[idx];
>>  
>> -    /* qcow2 and qed must be sized appropriately, so be sure our value
>> -     * is sized appropriately and will fit
>> +    /* qcow2 and qed must be sized on 512 byte blocks/sectors,
>> +     * so adjust size if necessary to round up (if possible).
>>       */
>> -    if (size != size_up &&
>> -        (disk->src.format == VIR_STORAGE_FILE_QCOW2 ||
>> -         disk->src.format == VIR_STORAGE_FILE_QED)) {
>> -        if (size_up > ULLONG_MAX) {
>> -            virReportError(VIR_ERR_OVERFLOW,
>> -                           _("size must be less than %llu KiB"),
>> -                           ULLONG_MAX / 1024);
>> -            goto endjob;
>> -        }
>> -        size = size_up;
>> +    if ((disk->src.format == VIR_STORAGE_FILE_QCOW2 ||
>> +         disk->src.format == VIR_STORAGE_FILE_QED) &&
>> +        (size % 512)) {
> 
> The (size % 512) condition is not necessary - VIR_ROUND_UP won't change the
> value if the number is divisible by 512 if it does not overflow.
> 
>> +        if ((ULLONG_MAX - size) < 512)
>> +            size = ULLONG_MAX;
> 
> ULLONG_MAX is not divisible by 512 thus I don't think it's any better than
> what the user entered.
> 
>> +        else
>> +            size = VIR_ROUND_UP(size, 512);
>>      }
>>  
>>      if (virAsprintf(&device, "%s%s", QEMU_DRIVE_HOST_PREFIX,
>>
> 
> ACK with the ULLONG_MAX assignment removed - whether you do 'git revert' or not.
> 
> Jan
>