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@xxxxxxxxxx> >> --- >> 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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list