[libvirt] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed. Initialize the relevant defbool variables in libxl_device_vfb. Signed-off-by: Olaf Hering <olaf@aepfle.de> Cc: Jim Fehlig <jfehlig@suse.com> --- Seen in 1.2.14. src/libxl/libxl_conf.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 9b3c949..6feb7d9 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1232,6 +1232,7 @@ libxlMakeVfb(virPortAllocatorPtr graphicsports, switch (l_vfb->type) { case VIR_DOMAIN_GRAPHICS_TYPE_SDL: libxl_defbool_set(&x_vfb->sdl.enable, 1); + libxl_defbool_set(&x_vfb->vnc.enable, 0); if (VIR_STRDUP(x_vfb->sdl.display, l_vfb->data.sdl.display) < 0) return -1; if (VIR_STRDUP(x_vfb->sdl.xauthority, l_vfb->data.sdl.xauth) < 0) @@ -1239,6 +1240,7 @@ libxlMakeVfb(virPortAllocatorPtr graphicsports, break; case VIR_DOMAIN_GRAPHICS_TYPE_VNC: libxl_defbool_set(&x_vfb->vnc.enable, 1); + libxl_defbool_set(&x_vfb->sdl.enable, 0); /* driver handles selection of free port */ libxl_defbool_set(&x_vfb->vnc.findunused, 0); if (l_vfb->data.vnc.autoport) {
On Fri, Apr 17, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
Initialize the relevant defbool variables in libxl_device_vfb.
Fix one crash, find another: Does libvirt have a representation for this one? libxl_defbool_val(vfb.sdl.opengl)); If not, it should be initialized to false in libxlMakeVfb. Olaf
On 04/17/2015 11:59 AM, Olaf Hering wrote:
On Fri, Apr 17, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
Initialize the relevant defbool variables in libxl_device_vfb. Fix one crash, find another:
Does libvirt have a representation for this one?
libxl_defbool_val(vfb.sdl.opengl));
I'm not aware of any way to specify OpenGL in libvirt domXML.
If not, it should be initialized to false in libxlMakeVfb.
As before, seems like the init function should handle this. Regards, Jim
On Fri, Apr 17, Jim Fehlig wrote:
On 04/17/2015 11:59 AM, Olaf Hering wrote:
On Fri, Apr 17, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
Initialize the relevant defbool variables in libxl_device_vfb. Fix one crash, find another:
Does libvirt have a representation for this one?
libxl_defbool_val(vfb.sdl.opengl));
I'm not aware of any way to specify OpenGL in libvirt domXML.
The qemu-dm process runs without DISPLAY=0:0, which leads to a failure if sdl is enabled. Once I will try to find the time I will see if setting DISPLAY= will actually help. xl.cfg(5) states that display= and xauthority= are currently not handled anyway. And libvirt lacks such functionality as well if I understand the docs correctly. Olaf
On Fri, 2015-04-17 at 13:57 -0600, Jim Fehlig wrote:
On 04/17/2015 11:59 AM, Olaf Hering wrote:
On Fri, Apr 17, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
Initialize the relevant defbool variables in libxl_device_vfb. Fix one crash, find another:
Does libvirt have a representation for this one?
libxl_defbool_val(vfb.sdl.opengl));
I'm not aware of any way to specify OpenGL in libvirt domXML.
If not, it should be initialized to false in libxlMakeVfb.
As before, seems like the init function should handle this.
As before, _not_ the init function, the setdefault within libxl should ensure that this is set to some value _before_ it is used. These changes are just hiding libxl bugs. Olaf, please can you use gdb to capture the stack trace so we can fix this (and the other issue) properly in libxl instead of just hacking around it in libvirt (which might also be appropriate for compat with old libxl but shouldn't be done without also fixing libxl IMHO). Ian.
Ian Campbell wrote:
On Fri, 2015-04-17 at 13:57 -0600, Jim Fehlig wrote:
On 04/17/2015 11:59 AM, Olaf Hering wrote:
On Fri, Apr 17, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
Initialize the relevant defbool variables in libxl_device_vfb.
Fix one crash, find another:
Does libvirt have a representation for this one?
libxl_defbool_val(vfb.sdl.opengl));
I'm not aware of any way to specify OpenGL in libvirt domXML.
If not, it should be initialized to false in libxlMakeVfb.
As before, seems like the init function should handle this.
As before, _not_ the init function, the setdefault within libxl should ensure that this is set to some value _before_ it is used. These changes are just hiding libxl bugs.
Yes, I agree.
Olaf, please can you use gdb to capture the stack trace so we can fix this (and the other issue) properly in libxl instead of just hacking around it in libvirt (which might also be appropriate for compat with old libxl but shouldn't be done without also fixing libxl IMHO).
I was torn on committing Olaf's changes to the libvirt libxl driver, but in the end did so for the compatibility you noted. Regards, Jim
On Fri, 2015-05-01 at 09:10 -0600, Jim Fehlig wrote:
Ian Campbell wrote:
On Fri, 2015-04-17 at 13:57 -0600, Jim Fehlig wrote:
On 04/17/2015 11:59 AM, Olaf Hering wrote:
On Fri, Apr 17, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
Initialize the relevant defbool variables in libxl_device_vfb.
Fix one crash, find another:
Does libvirt have a representation for this one?
libxl_defbool_val(vfb.sdl.opengl));
I'm not aware of any way to specify OpenGL in libvirt domXML.
If not, it should be initialized to false in libxlMakeVfb.
As before, seems like the init function should handle this.
As before, _not_ the init function, the setdefault within libxl should ensure that this is set to some value _before_ it is used. These changes are just hiding libxl bugs.
Yes, I agree.
Olaf, please can you use gdb to capture the stack trace so we can fix this (and the other issue) properly in libxl instead of just hacking around it in libvirt (which might also be appropriate for compat with old libxl but shouldn't be done without also fixing libxl IMHO).
I was torn on committing Olaf's changes to the libvirt libxl driver, but in the end did so for the compatibility you noted.
I just fear that the underlying issue will now not be diagnosed. Ian.
On Fri, May 01, Ian Campbell wrote:
Olaf, please can you use gdb to capture the stack trace so we can fix this (and the other issue) properly in libxl instead of just hacking around it in libvirt (which might also be appropriate for compat with old libxl but shouldn't be done without also fixing libxl IMHO).
The code flow was essentially like this: libxl_device_vfb_init(libxl); switch(libvirt->type) { case SDL: libxl_defbool_set(libxl->sdl.enable, 1); break; case VNC: libxl_defbool_set(libxl->vnc.enable, 1); break; } if (libvirt->os.type == HVM) { if (libxl_defbool_val(libxl->vnc.enable)) { /* do VNC things */ } else if (libxl_defbool_val(libxl->sdl.enable)) { /* do SDL things */ if (libxl_defbool_val(libxl->opengl.enable)) /* do openGL things */ } } The first crash was because I had SDL enabled, and the SDL case did not initialize the defbool for VNC. Once it did the next crash was the openGL part which was not initialized either. I see nothing wrong with libxl in such usage. Olaf
On Wed, 2015-05-06 at 10:24 +0200, Olaf Hering wrote:
On Fri, May 01, Ian Campbell wrote:
Olaf, please can you use gdb to capture the stack trace so we can fix this (and the other issue) properly in libxl instead of just hacking around it in libvirt (which might also be appropriate for compat with old libxl but shouldn't be done without also fixing libxl IMHO).
The code flow was essentially like this:
libxl_device_vfb_init(libxl); switch(libvirt->type) { case SDL: libxl_defbool_set(libxl->sdl.enable, 1); break; case VNC: libxl_defbool_set(libxl->vnc.enable, 1); break; }
if (libvirt->os.type == HVM) { if (libxl_defbool_val(libxl->vnc.enable)) { /* do VNC things */ } else if (libxl_defbool_val(libxl->sdl.enable)) { /* do SDL things */ if (libxl_defbool_val(libxl->opengl.enable)) /* do openGL things */ } }
The first crash was because I had SDL enabled, and the SDL case did not initialize the defbool for VNC. Once it did the next crash was the openGL part which was not initialized either.
I see nothing wrong with libxl in such usage.
I've explained this repeatedly now, it is a bug in libxl if the above ends up crashing in libxl. It is always a *libxl bug* for a libxl code path to lead a use of libxl_defbool_val on a value which has not had the appropriate setdefault called on it *by libxl*. It is not required for the user of libxl to initialise any defbool (other than via the libxl_TYPE_init function, which should set it to the explicit "default" value). It is therefore a bug if libxl reaches this code without having ensured that libxl_defbool_setdefault has been called *by libxl* on libxl->opengl.enable (perhaps on if libxl->sdl.enable is true). I see no code in libxl which matches what you have above, the only call to libxl_device_vfb_init has no switch statement or if == HVM anywhere near it. Please provide the actual stack trace as requested so we can see which code path in libxl is failing to properly initialise the defbools. Ian.
On Wed, 2015-05-06 at 10:08 +0100, Ian Campbell wrote:
On Wed, 2015-05-06 at 10:24 +0200, Olaf Hering wrote:
On Fri, May 01, Ian Campbell wrote:
Olaf, please can you use gdb to capture the stack trace so we can fix this (and the other issue) properly in libxl instead of just hacking around it in libvirt (which might also be appropriate for compat with old libxl but shouldn't be done without also fixing libxl IMHO).
The code flow was essentially like this:
libxl_device_vfb_init(libxl); switch(libvirt->type) { case SDL: libxl_defbool_set(libxl->sdl.enable, 1); break; case VNC: libxl_defbool_set(libxl->vnc.enable, 1); break; }
if (libvirt->os.type == HVM) { if (libxl_defbool_val(libxl->vnc.enable)) { /* do VNC things */ } else if (libxl_defbool_val(libxl->sdl.enable)) { /* do SDL things */ if (libxl_defbool_val(libxl->opengl.enable)) /* do openGL things */ } }
The first crash was because I had SDL enabled, and the SDL case did not initialize the defbool for VNC. Once it did the next crash was the openGL part which was not initialized either.
I see nothing wrong with libxl in such usage.
Please provide the actual stack trace as requested so we can see which code path in libxl is failing to properly initialise the defbools.
Ah, one moment, are you saying that all this code is within libvirt and not in libxl and that the "libxl->" object here hasn't yet been passed to a libxl function? If so then yes it is a libvirt bug to use libxl_defbool_val(libxl->opengl.enable) without having called set on it first. If you passed such a thing to libxl and it crashed then that would be a libxl bug, but it seems you aren't getting that far. I was confused by the initial message because the assert says libxl.c, but that's just because it is out of line, which lead to a confusing error message. Sorry for leading the conversation down the wrong alley way. Ian.
On Wed, May 06, Ian Campbell wrote:
On Wed, 2015-05-06 at 10:24 +0200, Olaf Hering wrote:
The code flow was essentially like this:
libxl_device_vfb_init(libxl); switch(libvirt->type) { case SDL: libxl_defbool_set(libxl->sdl.enable, 1); break; case VNC: libxl_defbool_set(libxl->vnc.enable, 1); break; }
if (libvirt->os.type == HVM) { if (libxl_defbool_val(libxl->vnc.enable)) { /* do VNC things */ } else if (libxl_defbool_val(libxl->sdl.enable)) { /* do SDL things */ if (libxl_defbool_val(libxl->opengl.enable)) /* do openGL things */ } } I see no code in libxl which matches what you have above, the only call to libxl_device_vfb_init has no switch statement or if == HVM anywhere near it.
This is libvirt code, no libxl involved other than using the defbool API. Olaf
Olaf Hering wrote:
On Fri, Apr 17, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
Initialize the relevant defbool variables in libxl_device_vfb.
Fix one crash, find another:
Does libvirt have a representation for this one?
libxl_defbool_val(vfb.sdl.opengl));
If not, it should be initialized to false in libxlMakeVfb.
Hrm, missed adding this before pushing your original patch. I'm going to push a trivial follow-up. Regards, Jim
On 04/17/2015 11:19 AM, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
The assertion seems harsh considering the offense...
Initialize the relevant defbool variables in libxl_device_vfb.
Signed-off-by: Olaf Hering <olaf@aepfle.de> Cc: Jim Fehlig <jfehlig@suse.com> ---
Seen in 1.2.14.
src/libxl/libxl_conf.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 9b3c949..6feb7d9 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1232,6 +1232,7 @@ libxlMakeVfb(virPortAllocatorPtr graphicsports, switch (l_vfb->type) { case VIR_DOMAIN_GRAPHICS_TYPE_SDL: libxl_defbool_set(&x_vfb->sdl.enable, 1); + libxl_defbool_set(&x_vfb->vnc.enable, 0);
Not shown here, but just before the switch is libxl_device_vfb_init(x_vfb); which IIUC (looking at the impl in $xensrc/tools/libxl/_libxl_types.c) should initialize the vfb struct with default values. Regards, Jim
if (VIR_STRDUP(x_vfb->sdl.display, l_vfb->data.sdl.display) < 0) return -1; if (VIR_STRDUP(x_vfb->sdl.xauthority, l_vfb->data.sdl.xauth) < 0) @@ -1239,6 +1240,7 @@ libxlMakeVfb(virPortAllocatorPtr graphicsports, break; case VIR_DOMAIN_GRAPHICS_TYPE_VNC: libxl_defbool_set(&x_vfb->vnc.enable, 1); + libxl_defbool_set(&x_vfb->sdl.enable, 0); /* driver handles selection of free port */ libxl_defbool_set(&x_vfb->vnc.findunused, 0); if (l_vfb->data.vnc.autoport) {
On Fri, Apr 17, Jim Fehlig wrote:
On 04/17/2015 11:19 AM, Olaf Hering wrote:
+ libxl_defbool_set(&x_vfb->vnc.enable, 0); Not shown here, but just before the switch is
libxl_device_vfb_init(x_vfb);
which IIUC (looking at the impl in $xensrc/tools/libxl/_libxl_types.c) should initialize the vfb struct with default values.
Yes, but the default values lead to the assert. Olaf
Olaf Hering wrote:
On Fri, Apr 17, Jim Fehlig wrote:
On 04/17/2015 11:19 AM, Olaf Hering wrote:
+ libxl_defbool_set(&x_vfb->vnc.enable, 0);
Not shown here, but just before the switch is
libxl_device_vfb_init(x_vfb);
which IIUC (looking at the impl in $xensrc/tools/libxl/_libxl_types.c) should initialize the vfb struct with default values.
Yes, but the default values lead to the assert.
Ok, best to be a bit defensive. I've pushed the patch now. Regards, Jim
On Fri, 2015-04-17 at 13:40 -0600, Jim Fehlig wrote:
On 04/17/2015 11:19 AM, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
The assertion seems harsh considering the offense...
libxl should be setting a default for this value if the application hasn't, by calling libxl__device_vfb_setdefault at some correct point. Not to say the application shouldn't also set it if it has a different default preference, but there is a libxl bug here somewhere too. Ian.
On Mon, Apr 20, Ian Campbell wrote:
On Fri, 2015-04-17 at 13:40 -0600, Jim Fehlig wrote:
On 04/17/2015 11:19 AM, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
The assertion seems harsh considering the offense...
libxl should be setting a default for this value if the application hasn't, by calling libxl__device_vfb_setdefault at some correct point.
This is an internal function. Its up to the caller of libxl_device_vfb_init to initialize the defbool members. Olaf
On Mon, 2015-04-20 at 11:32 +0200, Olaf Hering wrote:
On Mon, Apr 20, Ian Campbell wrote:
On Fri, 2015-04-17 at 13:40 -0600, Jim Fehlig wrote:
On 04/17/2015 11:19 AM, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes: libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
The assertion seems harsh considering the offense...
libxl should be setting a default for this value if the application hasn't, by calling libxl__device_vfb_setdefault at some correct point.
This is an internal function. Its up to the caller of libxl_device_vfb_init to initialize the defbool members.
No, it isn't. It's up to libxl to call libxl__device_vfb_setdefault somewhere on the code path between entering the library and actually using this defbool. (Possibly indirectly via the containing struct in this case) We normally do this as close to entry into the library as we can, e.g. in the libxl_device_FOO_add or e.g. in libxl_domain_create. It is always a libxl bug if a defbool gets used without having had a defaulted value applied by the library first, hence the assert. If what you said were true then an assert would be a rather harsh overreaction to an application coding error. Ian.
On Mon, Apr 20, Ian Campbell wrote:
If what you said were true then an assert would be a rather harsh overreaction to an application coding error.
Currently both libxl and libvirt are coded that way. Since the sdl code path in libvirt was never executed the crash in libxlMakeVfbList was not noticed. So you are saying that an external user of libxl_device_vfb_init has to call yet another function to initialize all defbool to give them either true of false? Or should libxl_device_vfb_init call libxl__device_vfb_setdefault directly? Olaf
On Mon, 2015-04-20 at 12:20 +0200, Olaf Hering wrote:
On Mon, Apr 20, Ian Campbell wrote:
If what you said were true then an assert would be a rather harsh overreaction to an application coding error.
Currently both libxl and libvirt are coded that way. Since the sdl code path in libvirt was never executed the crash in libxlMakeVfbList was not noticed.
So you are saying that an external user of libxl_device_vfb_init has to call yet another function to initialize all defbool to give them either true of false? Or should libxl_device_vfb_init call libxl__device_vfb_setdefault directly?
Neither. Any code within libxl which consumes a libxl_device_vfb must, at some point before touching the defbools therein, have arranged to call the appropriate setdefaults function. It makes no sense to do that at init time, the whole purpose of a defbool is to allow the calling application to choose a value or to explicitly leave it as a request to for the default (which might vary depending on other selections). It is up to *libxl* to convert such requests for default behaviour into a specific value before trying to use it, that is the purpose of the internal setdefaults functions and for the assert when reading a defbool. Ian.
On Mon, Apr 20, Ian Campbell wrote:
It makes no sense to do that at init time, the whole purpose of a defbool is to allow the calling application to choose a value or to explicitly leave it as a request to for the default (which might vary depending on other selections).
Yes, and thats why my patch does? libxl_device_vfb_init(x_vfb); +libxl_defbool_set(&x_vfb->sdl.opengl, 0); if (libxl_defbool_val(vfb.sdl.opengl)) ; Olaf
On Mon, 2015-04-20 at 12:32 +0200, Olaf Hering wrote:
On Mon, Apr 20, Ian Campbell wrote:
It makes no sense to do that at init time, the whole purpose of a defbool is to allow the calling application to choose a value or to explicitly leave it as a request to for the default (which might vary depending on other selections).
Yes, and thats why my patch does?
This is a libvirt patch. It might be correct in its own right, if libvirt has different ideas about the defaults to libxl, but it is also masking an underlying libxl bug. Ian.
libxl_device_vfb_init(x_vfb); +libxl_defbool_set(&x_vfb->sdl.opengl, 0); if (libxl_defbool_val(vfb.sdl.opengl)) ;
Olaf
participants (3)
-
Ian Campbell -
Jim Fehlig -
Olaf Hering