[libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary (rewrite)
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. Signed-off-by: John Ferlan <jferlan@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)) { + if ((ULLONG_MAX - size) < 512) + size = ULLONG_MAX; + else + size = VIR_ROUND_UP(size, 512); } if (virAsprintf(&device, "%s%s", QEMU_DRIVE_HOST_PREFIX, -- 1.9.0
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.
Signed-off-by: John Ferlan <jferlan@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
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@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
participants (2)
-
John Ferlan -
Ján Tomko