On Tue, Mar 22, 2011 at 04:08:10PM -0600, Eric Blake wrote:
On 03/22/2011 02:36 PM, Cole Robinson wrote:
The following XML:
<serial type='udp'> <source mode='connect' service='9999'/> </serial>
is accepted by domain_conf.c but maps to the qemu command line:
-chardev udp,host=127.0.0.1,port=2222,localaddr=(null),localport=(null)
qemu can cope with everything omitting except the connection port, which seems to also be the intent of domain_conf validation, so let's not generate bogus command lines for that case.
In the past, I've been told that the qemu command line should be as full-featured as possible - that is, if we know what qemu will be using as a default, then we should explicitly provide that argument rather than relying on the default (it helps us future-proof if not all versions of qemu take the same defaults).
I agree that something needs to be done here to avoid the NULL dereference (glibc is too kind by printing "(null)" instead of crashing); but am not sure that making the qemu command line parameters optional is right, if we instead know what qemu does when those parameters are missing.
I'm resurecting that thread because we ended up not carrying the patch and I think we should. I looked and this is done in qemu inet_dgram_opts() function of qemu-sockets.c, basically it does: /* lookup local addr */ memset(&ai,0, sizeof(ai)); ai.ai_flags = AI_PASSIVE; ai.ai_family = peer->ai_family; ai.ai_socktype = SOCK_DGRAM; addr = qemu_opt_get(opts, "localaddr"); port = qemu_opt_get(opts, "localport"); if (addr == NULL || strlen(addr) == 0) { addr = NULL; } if (!port || strlen(port) == 0) port = "0"; if (0 != (rc = getaddrinfo(addr, port, &ai, &local))) { fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, gai_strerror(rc)); return -1; } so it relies on the behaviour of getaddrinfo() in that case, and NULL for addr is fine since the service port then default to "0", if we want to force behaviour on QEmu then I think the best is to use, localaddr=,localport=0 since we can't pass "NULL" addr we can pass an empty addr and the qemu behaviour is that it's mapped to NULL, I doubt they would change this. The patch has been rebased, I use temp variables instead as I find this more readable (but slightly more verbose I admit) and keeps the arguments order. There is a small tweak to the original patch on qemuParseCommandLineChr bindService to avoid taking 0 as anything but the default. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/