On 05/07/2014 10:34 AM, Ján Tomko wrote:
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@redhat.com> --- 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.
Understood - Eric's CC/query to qemu-devel on his initial response got no response other than my followup... So a more direct question was asked to (I assume) someone more involved in the blockdev layer. We got a direct response from that, but nothing else in general from qemu. So, after a couple more weeks of receiving no feedback - I queried whether the change was OK. Eric's response indicated to go ahead with this patch - all these are attached to this response if interested. While the 3 letters weren't used, I took the response as implicitly being OK with the change.
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.
An OVERLy cautious check - I cannot remember what I was thinking about a month ago... I think this was the check can VIR_ROUND_UP provide an incorrect value. I can send a follow-up patch to remove those lines if that's desired.
+ 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.
Only qcow2 and qed have this issue regarding needing to be on a 512 byte boundary. Since this is a generic routine I was limiting the rounding to the two types from the bz rather than taking a chance that some generic round up would cause some other issue. Or am I misinterpreting your comment? John