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

[Fabio Fantoni <fabio.fantoni@xxxxxxx>]

Il 01/04/2014 17:54, Ian Campbell ha scritto:
> On Mon, 2014-03-31 at 16:39 +0200, Fabio Fantoni wrote:
> > This patch adds basic spice support for pv domUs.
> > The qemu parameters are the same as the hvm ones and they works.
> > Therefore xl cfg paramters are the same as the hvm ones except that
> > features not supported yet by pv domUs (vdagent and usbredirection)
> > are kept disabled by default.
> > It also enables vfb and vkb required to have basic spice working.
> >
> > Note: this patch is only a draft and needs to be improved.
> > Patch is tested and working, except for the pointer not
> > visible but working.
> > Any feedback is appreciated.
> >
> > Signed-off-by: Fabio Fantoni <fabio.fantoni@xxxxxxx>
> >
> > ---
> >
> > Changes in v2:
> > - xl_cmdimpl.c: always set vnc and sdl toplevel parameters in &vfb
> > with vnc or spice enabled on pv domUs otherwise in some cases it
> > would fail with error for one bool default value missing.
> > - libxl_dm.c: do not add -nographic if spice is enabled, even though
> > -nographic seems buggy in upstream qemu.
> > ---
> >   tools/libxl/libxl_create.c |   20 ++++++++++----------
> >   tools/libxl/libxl_dm.c     |   33 ++++++++++++++++-----------------
> >   tools/libxl/xl_cmdimpl.c   |   29 +++++++++++++++++------------
> No docs updates?
Added in v3 posted now.

> >   3 files changed, 43 insertions(+), 39 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index 3376b5c..f1a1d73 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -215,6 +215,16 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >       if (!b_info->event_channels)
> >           b_info->event_channels = 1023;
> >   
> > +    libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
> > +    if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
> Accessing b_info->u.hvm when b_info->type != LIBXL_DOMAIN_TYPE_HVM is
> most certainly wrong.
>
> b_info->u is a union, so you are actually changing b_info->u.pv.* here
> too.
>
> It will be tricky to make u.hvm.spice common without breaking the API.
> Probably the only good option is to add u.pv.spice and be very careful
> about using the correct one. How very tedious.
I think that is bad have (and use) double and unuseful spice xl 
parameters and variables as vnc.
In the v3 I added a spice struct out u.hvm.* and used only it, is good 
or not?
I also think that for now u.hvm.* should be maintained for compatibility 
of third part software that use it, right?
About u.hvm.* I think is needed only one copy of content  if received 
from external software right?
If yes where I should do it?

Thanks for any reply and sorry for my bad english.

> > +        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);
> > +    }
> > +
> >       switch (b_info->type) {
> >       case LIBXL_DOMAIN_TYPE_HVM:
> >           if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index e87f606..b72f23d 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -478,6 +478,21 @@ static char ** 
> > libxl__build_device_model_args_new(libxl__gc *gc,
> >           flexarray_vappend(dm_args, "-k", keymap, NULL);
> >       }
> >   
> > +    if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
> Also not safe. Actually this comment applies to the entire substance of
> this patch.
>
> Ian.

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