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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org