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

[Fabio Fantoni <fabio.fantoni@xxxxxxx>]

Il 24/03/2014 17:05, Fabio Fantoni ha scritto:
> This patch adds basic spice support for pv domUs, qemu parameters
> are the same as hvm ones and working.
> Therefore xl cfg paramters are the same as 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.
> Any feedback is appreciated.
Testing this patch I found that enabling spice on pv domU also add vnc 
without it is setted on vfb and vnc xl parameters.
I found that vnc is enabled with libxl_defbool_setdefault in 
libxl__device_vfb_setfault function.
Then I set vnc=0, with spice enabled also vnc remain enabled even if vnc=0.
With both spice and vnc disabled on xl cfg file show this error on create:
> xl -vvv create /etc/xen/wheezy.cfg
> Parsing config from /etc/xen/wheezy.cfg
> xl: libxl.c:262: libxl_defbool_val: Assertion 
> `!libxl_defbool_is_default(db)' failed.
> Annullato
I could not find exactly which variable give this error.
I only found that setting vnc=0 and vfb=[vnc=0] solves the problem and 
disable vnc, but probably one or more improvements are needed in libxl.
Can someone advise me what improvements do about it?

Thanks for any reply and sorry for my bad english.

> Signed-off-by: Fabio Fantoni <fabio.fantoni@xxxxxxx>
> ---
>   tools/libxl/libxl_create.c |   20 ++++++++++----------
>   tools/libxl/libxl_dm.c     |   31 +++++++++++++++----------------
>   tools/libxl/xl_cmdimpl.c   |   35 +++++++++++++++++++++--------------
>   3 files changed, 46 insertions(+), 40 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)) {
> +        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)
> @@ -337,16 +347,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_defbool_setdefault(&b_info->u.hvm.nographic, false);
>   
>           libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru, false);
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index e87f606..d5febc0 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)) {
> +        const libxl_spice_info *spice = &b_info->u.hvm.spice;
> +        char *spiceoptions = dm_spice_options(gc, spice);
> +        if (!spiceoptions)
> +            return NULL;
> +
> +        flexarray_append(dm_args, "-spice");
> +        flexarray_append(dm_args, spiceoptions);
> +        if (libxl_defbool_val(b_info->u.hvm.spice.vdagent)) {
> +            flexarray_vappend(dm_args, "-device", "virtio-serial",
> +                "-chardev", "spicevmc,id=vdagent,name=vdagent", "-device",
> +                "virtserialport,chardev=vdagent,name=com.redhat.spice.0", 
> NULL);
> +        }
> +    }
> +
>       if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>           int ioemu_nics = 0;
>   
> @@ -489,22 +504,6 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>               flexarray_append(dm_args, "-nographic");
>           }
>   
> -        if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
> -            const libxl_spice_info *spice = &b_info->u.hvm.spice;
> -            char *spiceoptions = dm_spice_options(gc, spice);
> -            if (!spiceoptions)
> -                return NULL;
> -
> -            flexarray_append(dm_args, "-spice");
> -            flexarray_append(dm_args, spiceoptions);
> -            if (libxl_defbool_val(b_info->u.hvm.spice.vdagent)) {
> -                flexarray_vappend(dm_args, "-device", "virtio-serial",
> -                    "-chardev", "spicevmc,id=vdagent,name=vdagent", "-device",
> -                    "virtserialport,chardev=vdagent,name=com.redhat.spice.0",
> -                    NULL);
> -            }
> -        }
> -
>           switch (b_info->u.hvm.vga.kind) {
>           case LIBXL_VGA_INTERFACE_TYPE_STD:
>               flexarray_append_pair(dm_args, "-device", "VGA");
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index e856e25..0c65beb 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1647,6 +1647,8 @@ skip_vfb:
>   
>   #undef parse_extra_args
>   
> +    xlu_cfg_get_defbool (config, "spice", &b_info->u.hvm.spice.enable, 0);
> +
>       /* If we've already got vfb=[] for PV guest then ignore top level
>        * VNC config. */
>       if (c_info->type == LIBXL_DOMAIN_TYPE_PV && !d_config->num_vfbs) {
> @@ -1655,7 +1657,7 @@ skip_vfb:
>           if (!xlu_cfg_get_long (config, "vnc", &l, 0))
>               vnc_enabled = l;
>   
> -        if (vnc_enabled) {
> +        if (vnc_enabled || libxl_defbool_val(b_info->u.hvm.spice.enable)) {
>               libxl_device_vfb *vfb;
>               libxl_device_vkb *vkb;
>   
> @@ -1665,15 +1667,30 @@ skip_vfb:
>               vkb = ARRAY_EXTEND_INIT(d_config->vkbs, d_config->num_vkbs,
>                                       libxl_device_vkb_init);
>   
> -            parse_top_level_vnc_options(config, &vfb->vnc);
> -            parse_top_level_sdl_options(config, &vfb->sdl);
>               xlu_cfg_replace_string (config, "keymap", &vfb->keymap, 0);
> +            if (vnc_enabled) {
> +                parse_top_level_vnc_options(config, &vfb->vnc);
> +                parse_top_level_sdl_options(config, &vfb->sdl);
> +            }
>           }
>       } else {
>           parse_top_level_vnc_options(config, &b_info->u.hvm.vnc);
>           parse_top_level_sdl_options(config, &b_info->u.hvm.sdl);
>       }
>   
> +    if (!xlu_cfg_get_long (config, "spiceport", &l, 0))
> +        b_info->u.hvm.spice.port = l;
> +    if (!xlu_cfg_get_long (config, "spicetls_port", &l, 0))
> +        b_info->u.hvm.spice.tls_port = l;
> +    xlu_cfg_replace_string (config, "spicehost",
> +                            &b_info->u.hvm.spice.host, 0);
> +    xlu_cfg_get_defbool(config, "spicedisable_ticketing",
> +                        &b_info->u.hvm.spice.disable_ticketing, 0);
> +    xlu_cfg_replace_string (config, "spicepasswd",
> +                            &b_info->u.hvm.spice.passwd, 0);
> +    /* Some spice features are missed because not supported by domU pv now */
> +    b_info->u.hvm.spice.usbredirection = 0;
> +
>       if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>           if (!xlu_cfg_get_string (config, "vga", &buf, 0)) {
>               if (!strcmp(buf, "stdvga")) {
> @@ -1693,17 +1710,7 @@ skip_vfb:
>                                            LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
>   
>           xlu_cfg_replace_string (config, "keymap", &b_info->u.hvm.keymap, 0);
> -        xlu_cfg_get_defbool (config, "spice", &b_info->u.hvm.spice.enable, 0);
> -        if (!xlu_cfg_get_long (config, "spiceport", &l, 0))
> -            b_info->u.hvm.spice.port = l;
> -        if (!xlu_cfg_get_long (config, "spicetls_port", &l, 0))
> -            b_info->u.hvm.spice.tls_port = l;
> -        xlu_cfg_replace_string (config, "spicehost",
> -                                &b_info->u.hvm.spice.host, 0);
> -        xlu_cfg_get_defbool(config, "spicedisable_ticketing",
> -                            &b_info->u.hvm.spice.disable_ticketing, 0);
> -        xlu_cfg_replace_string (config, "spicepasswd",
> -                                &b_info->u.hvm.spice.passwd, 0);
> +
>           xlu_cfg_get_defbool(config, "spiceagent_mouse",
>                               &b_info->u.hvm.spice.agent_mouse, 0);
>           xlu_cfg_get_defbool(config, "spicevdagent",

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