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

On 05/07/2014 01:58 PM, John Ferlan wrote: > > > On 04/08/2014 12:26 PM, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1002813 >> >> If qemuDomainBlockResize() is passed a size not on a KiB boundary - that >> is passed a size based in bytes (VIR_DOMAIN_BLOCK_RESIZE_BYTES), then >> depending on the source format (qcow2 or qed), the value passed must >> be on a sector (or 512 byte) boundary. Since other libvirt code quietly >> adjusts the capacity values, then do so here as well - of course ensuring >> that adjustment still fits. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/qemu/qemu_driver.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> > > Although discussion was taken off this list - the changes here were > ACK'd and pushed today... I think ACKs should be on-list.

> > Essentially the following API's will round up the value as well: > > virStorageBackendCreateQcowCreate() > virStorageBackendLogicalCreateVol() > virStorageBackendCreateQemuImgCmd() > > For libvirt created volumes, virStorageBackendCreateQemuImgCmd() or > virStorageBackendCreateQcowCreate() is called - both will take the > capacity value and VIR_DIV_UP using 1024. For the vol-resize path (e.g. > non running vm case), virStorageBackendFilesystemResizeQemuImg() will > use ROUND_UP on 512 byte value because it knows (and comments) that > qemu-img will fail to resize on non sector boundaries. > > Additionally, it was noted that using "K" and "KiB" would produce 1024 > based results, it's libvirt's allowance of "KB" for sizes that results > in the nuance. Being strict KB shouldn't be used for storage, but rather > than penalize for not knowing the difference between KiB and KB the code > just assumes KiB should have been used. > > John > >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 4bb4819..3e407d7 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -9421,6 +9421,7 @@ qemuDomainBlockResize(virDomainPtr dom, >> virDomainObjPtr vm; >> qemuDomainObjPrivatePtr priv; >> int ret = -1, idx; >> + unsigned long long size_up; >> char *device = NULL; >> virDomainDiskDefPtr disk = NULL; >> >> @@ -9467,6 +9474,21 @@ 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 >> + */ >> + if (size != size_up && >> + (disk->src.format == VIR_STORAGE_FILE_QCOW2 || >> + disk->src.format == VIR_STORAGE_FILE_QED)) { >> + if (size_up > ULLONG_MAX) { This is always false.

>> + virReportError(VIR_ERR_OVERFLOW, >> + _("size must be less than %llu KiB"), >> + ULLONG_MAX / 1024); >> + goto endjob; >> + } >> + size = size_up; Just a nitpick: rounding it up unconditionally here would get rid of the temporary variable and have no effect on values specified without the BYTES flag.