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

[Fabio Fantoni <fabio.fantoni@xxxxxxx>]

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?
> 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?

> >           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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel