Re: [Xen-devel] [PATCH v2] libxl: Spice vdagent support for upstream qemu

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-05-01 at 12:28 +0100, Stefano Stabellini wrote:
> On Tue, 30 Apr 2013, fantonifabio@xxxxxxxxxx wrote:
> > Usage: spicevdagent=1|0 (default=0)
> > Enables spice vdagent. The default is 0.
> > 
> > Signed-off-by: Fabio Fantoni <fabio.fantoni@xxxxxxx>
> 
> The patch looks reasonable but I am unable to test it because it
> requires a spice-enabled QEMU.
> 
> Trusting Fabio that this patch makes it work for him, I think that I
> would accept it.

For 4.3? It's hard for me to determine if it is worth the risk, it's not
clear what the wider implications of enabling this seemingly innocuous
option are.

It would be useful, both in making this determination and for our users,
if the changelog and/or documentation included some details on what the
spice vdagent actually is, what it does, what features it enables and
what is required from the guest side in order to use it.

> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index d10a58f..bc605e4 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -465,6 +465,12 @@ static char ** 
> > libxl__build_device_model_args_new(libxl__gc *gc,
> >  
> >              flexarray_append(dm_args, "-spice");
> >              flexarray_append(dm_args, spiceoptions);
> > +            if (libxl_defbool_val(b_info->u.hvm.spice.vdagent)) {

Something somewhere needs to call libxl_defbool_setdefault on this,
otherwise users who don't ask for something explicit will get an
abort().

libxl__domain_build_info_setdefault looks like the right place for that.

> > +                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) {
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index ecf1f0b..8a33444 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -172,6 +172,7 @@ libxl_spice_info = Struct("spice_info", [
> >      ("disable_ticketing", libxl_defbool),
> >      ("passwd",      string),
> >      ("agent_mouse", libxl_defbool),
> > +    ("vdagent", libxl_defbool),

Please indent to match the others.

> >      ])
> >  
> >  libxl_sdl_info = Struct("sdl_info", [
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index c1a969b..44a632c 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -1491,6 +1491,8 @@ skip_vfb:
> >                                  &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",
> > +                            &b_info->u.hvm.spice.vdagent, 0);
> >          xlu_cfg_get_defbool(config, "nographic", &b_info->u.hvm.nographic, 
> > 0);
> >          xlu_cfg_get_defbool(config, "gfx_passthru", 
> >                              &b_info->u.hvm.gfx_passthru, 0);
> > -- 
> > 1.7.9.5
> > 



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