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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

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

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