[libvirt] [PATCH] virsh: blockcopy: force an absolute path
virsh doesn't reject or absolutify a passed relative path, which can give unexpected results. Convert a relative path to an absolute one before calling the API https://bugzilla.redhat.com/show_bug.cgi?id=1300177 --- tools/virsh-domain.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a1d4a75..7074ded 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2256,8 +2256,9 @@ static bool cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; - const char *dest = NULL; + const char *dest_cli = NULL; const char *format = NULL; + char *dest = NULL; unsigned long bandwidth = 0; unsigned int granularity = 0; unsigned long long buf_size = 0; @@ -2281,7 +2282,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) return false; - if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0) + if (vshCommandOptStringReq(ctl, cmd, "dest", &dest_cli) < 0) return false; if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0) return false; @@ -2308,6 +2309,11 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (async) abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; + if (dest_cli && virFileAbsPath(dest_cli, &dest) < 0) { + vshError(ctl, _("failed to build absolute path for %s"), dest_cli); + return false; + } + VSH_EXCLUSIVE_OPTIONS_VAR(dest, xml); VSH_EXCLUSIVE_OPTIONS_VAR(format, xml); VSH_EXCLUSIVE_OPTIONS_VAR(blockdev, xml); @@ -2462,6 +2468,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + VIR_FREE(dest); VIR_FREE(xmlstr); virTypedParamsFree(params, nparams); virDomainFree(dom); -- 2.7.3
On Wed, Apr 20, 2016 at 01:33:15PM -0400, Cole Robinson wrote:
virsh doesn't reject or absolutify a passed relative path, which can give unexpected results. Convert a relative path to an absolute one before calling the API
https://bugzilla.redhat.com/show_bug.cgi?id=1300177 --- tools/virsh-domain.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
This is a nasty bug, that's for sure. However, shouldn't our API be fixed rather than just virsh? There's still a bug there. Moreover, IIUC the path you give it should be relative to the volume (or something specific) and not the path you're calling it with. Well, it could make sense to make it relative to CWD, but what if it's called from a remote client? That path might not exist at all. I think it should be relative to something else and the volume seems the most sensible (after thinking about it for roughly 14 seconds). Martin
On Thu, Apr 21, 2016 at 08:35:43 +0200, Martin Kletzander wrote:
On Wed, Apr 20, 2016 at 01:33:15PM -0400, Cole Robinson wrote:
virsh doesn't reject or absolutify a passed relative path, which can give unexpected results. Convert a relative path to an absolute one before calling the API
https://bugzilla.redhat.com/show_bug.cgi?id=1300177 --- tools/virsh-domain.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
This is a nasty bug, that's for sure. However, shouldn't our API be fixed rather than just virsh? There's still a bug there.
Moreover, IIUC the path you give it should be relative to the volume (or something specific) and not the path you're calling it with. Well, it could make sense to make it relative to CWD, but what if it's called from a remote client? That path might not exist at all. I think it should be relative to something else and the volume seems the most sensible (after thinking about it for roughly 14 seconds).
Yes this is entirely true. We also forbid relative paths for the top level disks and since the target of block copy can eventually become a top level disks the approach chosen by the patch is wrong. NACK
On Wed, Apr 20, 2016 at 13:33:15 -0400, Cole Robinson wrote:
virsh doesn't reject or absolutify a passed relative path, which can give unexpected results. Convert a relative path to an absolute one before calling the API
virsh should never try to convert relative path to absolute. If we want to support relative paths, libvirtd should convert them, but I don't think we really want to do that. I think the right fix for this bug is the bug summary says, i.e., just forbid using relative paths. Jirka
participants (4)
-
Cole Robinson -
Jiri Denemark -
Martin Kletzander -
Peter Krempa