On 07.03.2014 00:47, Eric Blake wrote:
While investigating https://bugzilla.redhat.com/show_bug.cgi?id=1061827 I noticed that we pass user input unscathed for block-pull, but always pass a canonical absolute name through for block-commit. [Note that we probably _ought_ to validate that the user's request for block-pull actually matches the backing chain, the way we already do for block-commit - but that's a separate issue. Further note that the ability to pass user input through unscathed allows backdoors such as specifying a backing image that is a network URI such as a gluster disk, instead of forcing things to the local file system; which is an area still under active investigation on whether libvirt needs to behave differently for network disks.] Since qemu may write the name that the user passed in as the backing file, a user may have a reason to want a relative file name passed through to qemu, and always munging things to absolute prevents that. Put another way, if you have the backing chain: [A] <- [B(back=./A)] <- [C(back=./B)] and commit B into A (virsh blockcommit $dom vda --base A --top B), the metadata of C will have to be re-written. But should it be rewritten as [C(back=./A)] or as [C(back=/path/to/A)]? Still up in the air is whether qemu's decision should be based on whether B and/or C had relative paths, or on whether the --base and/or --top arguments to the command were relative paths; but if we always pass a canonical name, we've prevented the spelling of the command arguments from being part of the hueristics that qemu uses. I also audited the code, and verified that we never call qemuMonitorBlockCommit() with a NULL base, either before or after the change to qemu_driver.c. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Preserve user's spelling, since absolute vs. relative matters to qemu. * src/qemu/qemu_monitor.h (qemuMonitorBlockCommit): Base is never null. * src/qemu/qemu_monitor.c (qemuMonitorBlockCommit): Likewise. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockCommit): Likewise. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockCommit): Likewise. Signed-off-by: Eric Blake <eblake@xxxxxxxxxx> --- I was _hoping_ that this would solve the mentioned bugzilla. But even with this applied, qemu still ended up writing an absolute backing file name into the active file in the backing chain, so that bug has been reassigned back to qemu - it probably has to do with the fact that libvirt always spawns qemu with -drive pointing to an absolute name, which is unrelated to what this patch fixes. Therefore, I'm a little bit hesitant to apply this patch, but wanted to post it for review anyway. src/qemu/qemu_driver.c | 11 ++++++++--- src/qemu/qemu_monitor.c | 4 ++-- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_monitor_json.h | 5 +++-- 5 files changed, 16 insertions(+), 9 deletions(-)
ACK Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list