[libvirt] [Xen-devel] [PATCH] libxl: fix framebuffer port setting for HVM domains

[Ian Campbell]

On Tue, 2014-03-18 at 15:12 -0600, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> > On Tue, Mar 18, 2014 at 12:19:47PM -0600, Jim Fehlig wrote:
> >   
> >> libxl uses the libxl_vnc_info and libxl_sdl_info fields from the
> >> hvm union in libxl_domain_build_info struct when generating QEMU
> >> args for VNC or SDL.  These fields were left unset by the libxl
> >> driver, causing libxl to ignore any user settings.  E.g. with
> >>
> >>   <graphics type='vnc' port='5950'/>
> >>
> >> port would be ignored and QEMU would instead be invoked with
> >>
> >>   -vnc 127.0.0.1:0,to=99
> >>
> >> Unlike the libxl_domain_config struct, the libxl_domain_build_info
> >> contains only a single libxl_vnc_info and libxl_sdl_info, so
> >> populate these fields from the first vfb in
> >> libxl_domain_config->vfbs.
> >>
> >> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> >> Signed-off-by: David Kiarie <davidkiarie4 at gmail.com>
> >> ---
> >>  src/libxl/libxl_conf.c | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> >> index aa5d958..cfac847 100644
> >> --- a/src/libxl/libxl_conf.c
> >> +++ b/src/libxl/libxl_conf.c
> >> @@ -1022,6 +1022,20 @@ libxlMakeVfbList(libxlDriverPrivatePtr driver,
> >>      d_config->vkbs = x_vkbs;
> >>      d_config->num_vfbs = d_config->num_vkbs = nvfbs;
> >>  
> >> +    /*
> >> +     * VNC or SDL info must also be set in libxl_domain_build_info
> >> +     * for HVM domains.  Use the first vfb device.
> >> +     */
> >> +    if (STREQ(def->os.type, "hvm")) {
> >> +        libxl_domain_build_info *b_info = &d_config->b_info;
> >> +        libxl_device_vfb vfb = d_config->vfbs[0];
> >> +
> >> +        if (libxl_defbool_val(vfb.vnc.enable))
> >> +            memcpy(&b_info->u.hvm.vnc, &vfb.vnc, sizeof(libxl_vnc_info));
> >> +        else if (libxl_defbool_val(vfb.sdl.enable))
> >> +            memcpy(&b_info->u.hvm.sdl, &vfb.sdl, sizeof(libxl_sdl_info));
> >> +    }
> >> +
> >>      return 0;
> >>  
> >>  error:
> >>     
> >
> > ACK.
> >   
> 
> Thanks, I'll push this shortly.
> 
> > It sure would be nice if there were some kind of API in libxl which
> > took a 'libxl_domain_config*' and returned a char **argv string for
> > the QEMU command line args that it would later invoke. That would let
> > us create a test suite for the XML -> libxl_domain_config conversion
> > to spot these kind of problems.
> >   
> 
> Yes, agreed.

I don't think we would want to expose such a function to actual users of
the library, but I think the infrastructure Ian added in eb5cdc52756e
"libxl: events: Makefile builds internal unit tests" could be used to
expose such a function only to the test application which needs it.

>   I meant to CC xen-devel on this patch (done now) since it
> is rather cumbersome to supply the same info in two places for HVM
> guests.

TBH I thought the library did this copy already, but apparently not.
Adding this would need to be done carefully since it is at least in
principal possible today to have the emulated VGA vnc/sdl output
entirely separate from the first PV FB.

>   The improvement you suggest would also be very welcome.
> 
> The libxl driver in general needs some in-tree testing love.  Although I
> was a bit worried it would be too short for a GSoC project, I proposed
> this work as potential project through the openSUSE org
> 
> https://en.opensuse.org/openSUSE:GSOC_ideas#Implement_a_test_driver_for_the_libvirt_libxl_driver
> 
> I've only had one semi-serious inquiry thus far, so looks like I'll be
> doing this myself during free cycles.

Regardless of which org it ends up under It might be worth also adding
it to http://wiki.xen.org/wiki/GSoc_2014 where it might get more eyes
from people thinking about doing Xen stuff.

Ian.