Re: [Xen-devel] [PATCH v3] libxl: add basic spice support for pv domUs

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-04-29 at 10:45 +0200, Fabio Fantoni wrote:
> Il 23/04/2014 12:46, Ian Campbell ha scritto:
> > On Fri, 2014-04-11 at 10:45 +0200, Fabio Fantoni wrote:
> >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> >> index 3376b5c..dc54c9b 100644
> >> --- a/tools/libxl/libxl_create.c
> >> +++ b/tools/libxl/libxl_create.c
> >> @@ -215,6 +215,14 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >>       if (!b_info->event_channels)
> >>           b_info->event_channels = 1023;
> >>   
> >> +    libxl_defbool_setdefault(&b_info->spice.enable, false);
> >> +    if (libxl_defbool_val(b_info->spice.enable)) {
> >> +        libxl_defbool_setdefault(&b_info->spice.disable_ticketing, false);
> >> +        libxl_defbool_setdefault(&b_info->spice.agent_mouse, true);
> >> +        libxl_defbool_setdefault(&b_info->spice.vdagent, false);
> >> +        libxl_defbool_setdefault(&b_info->spice.clipboard_sharing, false);
> >> +    }
> >> +
> >>       switch (b_info->type) {
> >>       case LIBXL_DOMAIN_TYPE_HVM:
> >>           if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
> >> @@ -306,10 +314,10 @@ int libxl__domain_build_info_setdefault(libxl__gc 
> >> *gc,
> >>           libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   
> >> true);
> >>   
> >>           if (!b_info->u.hvm.usbversion &&
> >> -            (b_info->u.hvm.spice.usbredirection > 0) )
> >> +            (b_info->spice.usbredirection > 0) )
> >>               b_info->u.hvm.usbversion = 2;
> >>   
> >> -        if ((b_info->u.hvm.usbversion || 
> >> b_info->u.hvm.spice.usbredirection) &&
> >> +        if ((b_info->u.hvm.usbversion || b_info->spice.usbredirection) &&
> >>               ( libxl_defbool_val(b_info->u.hvm.usb)
> >>               || b_info->u.hvm.usbdevice_list
> >>               || b_info->u.hvm.usbdevice) ){
> >> @@ -337,16 +345,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >>               libxl_defbool_setdefault(&b_info->u.hvm.sdl.opengl, false);
> >>           }
> >>   
> >> -        libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
> >> -        if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
> >> -            
> >> libxl_defbool_setdefault(&b_info->u.hvm.spice.disable_ticketing,
> >> -                                     false);
> >> -            libxl_defbool_setdefault(&b_info->u.hvm.spice.agent_mouse, 
> >> true);
> >> -            libxl_defbool_setdefault(&b_info->u.hvm.spice.vdagent, false);
> >> -            
> >> libxl_defbool_setdefault(&b_info->u.hvm.spice.clipboard_sharing,
> >> -                                     false);
> >> -        }
> > libxl appear to now ignore b_info->u.hvm.spice completely. Unfortunately
> > this will break backwards compatibility.
> >
> > It should be possible to build a application written against old libxl
> > against this new libxl and have everything continue to work.
> > Unfortunately this means you need to jump through a few hoops in the
> > library code. You should define some sane precedence between
> > b_info->spice and b_info->u.hvm.spice and copy things from u.hvm.spice
> > to spice as necessary. I think it makes sense to say that ->spice takes
> > precedence over ->u.hvm.spice. IOW if ->spice.enable == false and
> > ->u.hvm.spice.enable is true then copy ->u.hvm.spice into ->spice. You
> > should think through the consequences of this though in case I am
> > confused.
> 
> Where is the correct place to check for ->u.hvm.spice and copy it in 
> ->spice if enabled?

In the relevant libxl__._setdefault I think.

> >
> > You also need to announce the presence of this new field via a
> > LIBXL_HAVE #define in libxl.h (there are plenty of existing examples).
> > You should document the precedence which you decide on as part of the
> > associated comment.
> 
> Sorry to forgot it, I'll add it on next version.
> May I mark ->u.hvm.spice as deprecated?

You can say that ->spice is preferred for sure.

> >>           xlu_cfg_get_defbool(config, "spicevdagent",
> >> -                            &b_info->u.hvm.spice.vdagent, 0);
> >> +                            &b_info->spice.vdagent, 0);
> >>           xlu_cfg_get_defbool(config, "spice_clipboard_sharing",
> >> -                            &b_info->u.hvm.spice.clipboard_sharing, 0);
> >> +                            &b_info->spice.clipboard_sharing, 0);
> >>           if (!xlu_cfg_get_long (config, "spiceusbredirection", &l, 0))
> >> -            b_info->u.hvm.spice.usbredirection = l;
> >> +            b_info->spice.usbredirection = l;
> > So it appears that some members of b_info->spice are not relevant to PV
> > guests?
> >
> >
> 
> At the moment the pv domUs don't support virtio and emulated usb, so I 
> leave such features disabled on them.

"At the moment" implies that this might change, have you given any
thought to the API stability implications of adding this. Perhaps there
are steps you can take now while introducing the basic level of support
which will simplify the future work?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel