10:42 a.m.
Hi all,
I've hit a couple bugs in the qemu driver with the recent
domain xml refactoring. I've debugged them but in both
cases I'm not sure what the optimal solutions are, so I'm
just laying them out here:
1) Previously defining a qemu guest without a 'listen'
address specified in the graphics tag would default to
127.0.0.1 (hardcoded in qemu_driver->vncListen). Current
xml doesn't set this default, and will build a qemu
command line with an entry like '-vnc (null):1'. Not
sure if the default should be set at the parsing level
or the driver level.
2) If a cpuset isn't specified, def->cpumask is null.
However qemudInitCpus from qemu_driver.c doesn't know
how to handle this, causing libvirtd to crash. This
function is also using QEMUD_CPUMASK_LEN which I think
should be replaced with VIR_DOMAIN_CPUMASK_LEN. I tried
the obvious fix of just skipping the dependent code
if cpumask is null, but there still seemed to be problems
and I didn't dig much deeper.
Thanks,
Cole
10:53 a.m.
On Tue, Jul 22, 2008 at 11:42:03AM -0400, Cole Robinson wrote:
Hi all,
I've hit a couple bugs in the qemu driver with the recent
domain xml refactoring. I've debugged them but in both
cases I'm not sure what the optimal solutions are, so I'm
just laying them out here:
1) Previously defining a qemu guest without a 'listen'
address specified in the graphics tag would default to
127.0.0.1 (hardcoded in qemu_driver->vncListen). Current
xml doesn't set this default, and will build a qemu
command line with an entry like '-vnc (null):1'. Not
sure if the default should be set at the parsing level
or the driver level.
There was a good reason for removing the 127.0.0.1 from the XML parsing
stage, but i can't remember what it is :-) Anyway this should really be
handled at the point where we build the command line in the qemu driver
code
2) If a cpuset isn't specified, def->cpumask is null.
However qemudInitCpus from qemu_driver.c doesn't know
how to handle this, causing libvirtd to crash. This
function is also using QEMUD_CPUMASK_LEN which I think
should be replaced with VIR_DOMAIN_CPUMASK_LEN. I tried
the obvious fix of just skipping the dependent code
if cpumask is null, but there still seemed to be problems
and I didn't dig much deeper.
I'm surprised that doesn't work - we should be able to just
skip the bit of code within the HAVE_SCHED_GETAFFINITY
conditional, and go straight to the part when it issues the
'cont' command to the monitor to start execution.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
11:23 a.m.
On Tue, Jul 22, 2008 at 04:53:19PM +0100, Daniel P. Berrange wrote:
On Tue, Jul 22, 2008 at 11:42:03AM -0400, Cole Robinson wrote:
> Hi all,
>
> I've hit a couple bugs in the qemu driver with the recent
> domain xml refactoring. I've debugged them but in both
> cases I'm not sure what the optimal solutions are, so I'm
> just laying them out here:
>
> 1) Previously defining a qemu guest without a 'listen'
> address specified in the graphics tag would default to
> 127.0.0.1 (hardcoded in qemu_driver->vncListen). Current
> xml doesn't set this default, and will build a qemu
> command line with an entry like '-vnc (null):1'. Not
> sure if the default should be set at the parsing level
> or the driver level.
There was a good reason for removing the 127.0.0.1 from the XML parsing
stage, but i can't remember what it is :-) Anyway this should really be
handled at the point where we build the command line in the qemu driver
code
Oh I remember now - both Xen and QEMU have a global default setting
for VNC listen (/etc/libvirt/qemu.conf and /etc/xen/xend-config.sxp).
So, if we default to '127.0.0.1' in the parser, we'll always override
the global hypervisor default setting. Hence we need to apply any
defaults in the individual drivers at time of guest creation (if it
is applicable).
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
3:39 a.m.
On Tue, Jul 22, 2008 at 05:23:00PM +0100, Daniel P. Berrange wrote:
On Tue, Jul 22, 2008 at 04:53:19PM +0100, Daniel P. Berrange wrote:
> On Tue, Jul 22, 2008 at 11:42:03AM -0400, Cole Robinson wrote:
> > Hi all,
> >
> > I've hit a couple bugs in the qemu driver with the recent
> > domain xml refactoring. I've debugged them but in both
> > cases I'm not sure what the optimal solutions are, so I'm
> > just laying them out here:
> >
> > 1) Previously defining a qemu guest without a 'listen'
> > address specified in the graphics tag would default to
> > 127.0.0.1 (hardcoded in qemu_driver->vncListen). Current
> > xml doesn't set this default, and will build a qemu
> > command line with an entry like '-vnc (null):1'. Not
> > sure if the default should be set at the parsing level
> > or the driver level.
>
> There was a good reason for removing the 127.0.0.1 from the XML parsing
> stage, but i can't remember what it is :-) Anyway this should really be
> handled at the point where we build the command line in the qemu driver
> code
Oh I remember now - both Xen and QEMU have a global default setting
for VNC listen (/etc/libvirt/qemu.conf and /etc/xen/xend-config.sxp).
So, if we default to '127.0.0.1' in the parser, we'll always override
the global hypervisor default setting. Hence we need to apply any
defaults in the individual drivers at time of guest creation (if it
is applicable).
Here's a suggested patch for this:
diff -r 59140de4e7a9 src/qemu_driver.c
--- a/src/qemu_driver.c Mon Jul 21 18:27:29 2008 +0100
+++ b/src/qemu_driver.c Tue Jul 22 22:12:22 2008 +0100
@@ -769,9 +769,14 @@
maxcpu = nodeinfo.cpus;
CPU_ZERO(&mask);
- for (i = 0 ; i < maxcpu ; i++)
- if (vm->def->cpumask[i])
+ if (vm->def->cpumask) {
+ for (i = 0 ; i < maxcpu ; i++)
+ if (vm->def->cpumask[i])
+ CPU_SET(i, &mask);
+ } else {
+ for (i = 0 ; i < maxcpu ; i++)
CPU_SET(i, &mask);
+ }
for (i = 0 ; i < vm->nvcpupids ; i++) {
if (sched_setaffinity(vm->vcpupids[i],
NB, I explicitly set the affinity if none is specified - this makes sure
the children don't inherit any bogus affinity from the libvirtd daemon
itself
Daniel.
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
4:05 a.m.
On Thu, Jul 24, 2008 at 09:39:36AM +0100, Daniel P. Berrange wrote:
On Tue, Jul 22, 2008 at 05:23:00PM +0100, Daniel P. Berrange wrote:
> On Tue, Jul 22, 2008 at 04:53:19PM +0100, Daniel P. Berrange wrote:
> > There was a good reason for removing the 127.0.0.1 from the XML parsing
> > stage, but i can't remember what it is :-) Anyway this should really be
> > handled at the point where we build the command line in the qemu driver
> > code
>
> Oh I remember now - both Xen and QEMU have a global default setting
> for VNC listen (/etc/libvirt/qemu.conf and /etc/xen/xend-config.sxp).
> So, if we default to '127.0.0.1' in the parser, we'll always override
> the global hypervisor default setting. Hence we need to apply any
> defaults in the individual drivers at time of guest creation (if it
> is applicable).
Here's a suggested patch for this:
diff -r 59140de4e7a9 src/qemu_driver.c
--- a/src/qemu_driver.c Mon Jul 21 18:27:29 2008 +0100
+++ b/src/qemu_driver.c Tue Jul 22 22:12:22 2008 +0100
@@ -769,9 +769,14 @@
maxcpu = nodeinfo.cpus;
CPU_ZERO(&mask);
- for (i = 0 ; i < maxcpu ; i++)
- if (vm->def->cpumask[i])
+ if (vm->def->cpumask) {
+ for (i = 0 ; i < maxcpu ; i++)
+ if (vm->def->cpumask[i])
+ CPU_SET(i, &mask);
+ } else {
+ for (i = 0 ; i < maxcpu ; i++)
CPU_SET(i, &mask);
+ }
for (i = 0 ; i < vm->nvcpupids ; i++) {
if (sched_setaffinity(vm->vcpupids[i],
NB, I explicitly set the affinity if none is specified - this makes sure
the children don't inherit any bogus affinity from the libvirtd daemon
itself
I don't see how this relates to VNC listening parameters, but this still
sounds a good idea :-)
+1
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
2:23 p.m.
On Tue, Jul 22, 2008 at 04:53:19PM +0100, Daniel P. Berrange wrote:
On Tue, Jul 22, 2008 at 11:42:03AM -0400, Cole Robinson wrote:
[..snip..]
> 2) If a cpuset isn't specified, def->cpumask is null.
> However qemudInitCpus from qemu_driver.c doesn't know
> how to handle this, causing libvirtd to crash. This
> function is also using QEMUD_CPUMASK_LEN which I think
> should be replaced with VIR_DOMAIN_CPUMASK_LEN. I tried
> the obvious fix of just skipping the dependent code
> if cpumask is null, but there still seemed to be problems
> and I didn't dig much deeper.
I'm surprised that doesn't work - we should be able to just
skip the bit of code within the HAVE_SCHED_GETAFFINITY
conditional, and go straight to the part when it issues the
'cont' command to the monitor to start execution.
What you suggested works
here.
-- Guido
6:05 a.m.
On Tue, Jul 22, 2008 at 04:53:19PM +0100, Daniel P. Berrange wrote:
On Tue, Jul 22, 2008 at 11:42:03AM -0400, Cole Robinson wrote:
> Hi all,
>
> I've hit a couple bugs in the qemu driver with the recent
> domain xml refactoring. I've debugged them but in both
> cases I'm not sure what the optimal solutions are, so I'm
> just laying them out here:
>
> 1) Previously defining a qemu guest without a 'listen'
> address specified in the graphics tag would default to
> 127.0.0.1 (hardcoded in qemu_driver->vncListen). Current
> xml doesn't set this default, and will build a qemu
> command line with an entry like '-vnc (null):1'. Not
> sure if the default should be set at the parsing level
> or the driver level.
There was a good reason for removing the 127.0.0.1 from the XML parsing
stage, but i can't remember what it is :-) Anyway this should really be
handled at the point where we build the command line in the qemu driver
code
Here's a patch for this bit
diff -r 59140de4e7a9 src/qemu_conf.c
--- a/src/qemu_conf.c Mon Jul 21 18:27:29 2008 +0100
+++ b/src/qemu_conf.c Tue Jul 22 22:12:22 2008 +0100
@@ -1106,7 +1106,9 @@
options[sizeof(options)-1] = '\0';
}
ret = snprintf(vncdisplay, sizeof(vncdisplay), "%s:%d%s",
- vm->def->graphics->data.vnc.listenAddr,
+ (vm->def->graphics->data.vnc.listenAddr ?
+ vm->def->graphics->data.vnc.listenAddr :
+ (driver->vncListen ? driver->vncListen :
"")),
vm->def->graphics->data.vnc.port - 5900,
options);
} else {
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
5964
days inactive
5967
days old
6 comments
4 participants
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard -
Guido Günther