Re: [libvirt] [PATCH] qemu: Fix -chardev udp if parameters are omitted
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
Attachments:
- 689761.patch (text/plain — 7.0 KB)