8:59 p.m.
Suspending a VM which contains shell meta characters doesn't work with
libvirt-0.8.7:
/var/log/libvirt/qemu/andreas_231-ne\ doch\ nicht.log:
sh: -c: line 0: syntax error near unexpected token `doch'
sh: -c: line 0: `cat | { dd bs=4096 seek=1 if=/dev/null && dd bs=1048576; }
Although target="andreas_231-ne doch nicht" contains shell meta
characters (here: blanks), they are not properly escaped by
src/qemu/qemu_monitor_{json,text}.c#qemuMonitor{JSON,Text}MigrateToFile()
First, the filename needs to be properly escaped for the shell, than
this command line has to be properly escaped for qemu again.
For this to work, remove the old qemuMonitorEscapeArg() wrapper, rename
qemuMonitorEscape() to it removing the handling for shell=TRUE, and
implement a new qemuMonitorEscapeShell() returning strings using single
quotes.
Using double quotes or escaping special shell characters with backslashes
would also be possible, but the set of special characters heavily
depends on the concrete shell (dsh, bash, zsh) and its setting (history
expansion, interactive use, ...)
Signed-off-by: Philipp Hahn <hahn(a)univention.de>
---
src/qemu/qemu_monitor.c | 65 +++++++++++++++++++++++++++-------------------
1 files changed, 38 insertions(+), 27 deletions(-)
3:19 a.m.
On 02/11/2011 05:59 AM, Philipp Hahn wrote:
Suspending a VM which contains shell meta characters doesn't work with
libvirt-0.8.7:
/var/log/libvirt/qemu/andreas_231-ne\ doch\ nicht.log:
sh: -c: line 0: syntax error near unexpected token `doch'
sh: -c: line 0: `cat | { dd bs=4096 seek=1 if=/dev/null && dd bs=1048576; }
Although target="andreas_231-ne doch nicht" contains shell meta
characters (here: blanks), they are not properly escaped by
src/qemu/qemu_monitor_{json,text}.c#qemuMonitor{JSON,Text}MigrateToFile()
Good catch.
It doesn't help that virsh support for such domain names is spotty (that
is, on RHEL 6.0, virt-manager won't let me create a domain named "a b",
but 'virsh define a.xml' did; then I had the problem that 'virsh
undefine "a b"' from RHEL 6.0 virsh (0.8.1) won't nuke it - I had to
upgrade virsh first). At least virsh in v0.8.5 and later better handles
such domain names.
First, the filename needs to be properly escaped for the shell, than
this command line has to be properly escaped for qemu again.
For this to work, remove the old qemuMonitorEscapeArg() wrapper, rename
qemuMonitorEscape() to it removing the handling for shell=TRUE, and
implement a new qemuMonitorEscapeShell() returning strings using single
quotes.
Using double quotes or escaping special shell characters with backslashes
would also be possible, but the set of special characters heavily
depends on the concrete shell (dsh, bash, zsh) and its setting (history
expansion, interactive use, ...)
Not _that_ heavily - we invoke the shell non-interactively as sh, and
pretty much all shells when invoked in that manner behave sanely on the
set of characters needing \ escaping within "" (namely: ", `, $, \).
But I agree that '' is easier to implement than "", and virsh also uses
that quoting for 'virsh echo --shell'.
In fact, post-0.8.8, I'd like to patch things so that src/util provides
a convenience function for shell quoting an argument, so that this code,
virsh, and also virCommandToString can all share that common utility.
Then make virCommandToString output a shell-quoted output into the log,
and teach domxml-{from,to}-native for qemu to better handle shell quoting.
For this particular problem, I'd also like to avoid it completely for
newer qemu by using fd: rather than exec: migration, at which point
there's no file name to pass through the monitor or the shell in the
first place; also post-0.8.8.
char *qemuMonitorEscapeShell(const char *in)
{
- return qemuMonitorEscape(in, 1);
+ int len = 2; /* leading and trailing single quote */
Technically, we only need add leading and trailing '' if the string
contains any metacharacters, and could just use the string as-is
otherwise (and virsh echo does this); but that's something for the
common utility routine. For now, this patch fixes the bug, even if it
adds more quoting than strictly necessary for some cases.
So, ACK to your patch, and it's a bug fix, so I've pushed it.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5010
days inactive
5010
days old
1 comments
2 participants
participants (2)
-
Eric Blake -
Philipp Hahn