Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Olaf Hering]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Jim Fehlig]

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) {
>

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Jim Fehlig]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Olaf Hering]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

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.

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Jim Fehlig]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

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.

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Olaf Hering]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

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.

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

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.

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Olaf Hering]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Jim Fehlig]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Olaf Hering]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

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.

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Olaf Hering]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

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.

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Olaf Hering]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

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.

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Olaf Hering]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

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

Re: [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Jim Fehlig]

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

[PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Olaf Hering]

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) {