Xen devel - May 2013 - [PATCH v3] libxl: Spice vdagent support for upstream qemu

Usage: spicevdagent=1|0 (default=0)
Enables spice vdagent. The Spice vdagent is an optional component for
enhancing user experience and performing guest-oriented management
tasks. Its features includes: client mouse mode (no need to grab mouse
by client, no mouse lag), automatic adjustment of screen resolution,
copy and paste (text and image) between client and domU. It also
requires vdagent service installed on domU o.s. to work.

Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
---
 docs/man/xl.cfg.pod.5       |    9 +++++++++
 tools/libxl/libxl_create.c  |    1 +
 tools/libxl/libxl_dm.c      |    6 ++++++
 tools/libxl/libxl_types.idl |    1 +
 tools/libxl/xl_cmdimpl.c    |    2 ++
 5 files changed, 19 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index f8b4576..766862d 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1123,6 +1123,15 @@ Specify the ticket password which is used by a client for
connection.
 Whether SPICE agent is used for client mouse mode. The default is true
 (turn on)
 
+=item B<spicevdagent=BOOLEAN>
+
+Enables spice vdagent. The Spice vdagent is an optional component for
+enhancing user experience and performing guest-oriented management
+tasks. Its features includes: client mouse mode (no need to grab mouse
+by client, no mouse lag), automatic adjustment of screen resolution,
+copy and paste (text and image) between client and domU. It also
+requires vdagent service installed on domU o.s. to work. The default is 0.
+
 =back
 
 =head3 Miscellaneous Emulated Hardware
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index cb9c822..8db5460 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -288,6 +288,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
            
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.nographic, false);
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)) {
+                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..14425d1 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),
     ])
 
 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

On Mon, May 06, 2013 at 03:15:44PM +0100, fantonifabio@tiscali.it
wrote:
> Usage: spicevdagent=1|0 (default=0)
> Enables spice vdagent. The Spice vdagent is an optional component for
> enhancing user experience and performing guest-oriented management
> tasks. Its features includes: client mouse mode (no need to grab mouse
> by client, no mouse lag), automatic adjustment of screen resolution,
> copy and paste (text and image) between client and domU. It also
> requires vdagent service installed on domU o.s. to work.
> 
> Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> ---
>  docs/man/xl.cfg.pod.5       |    9 +++++++++
>  tools/libxl/libxl_create.c  |    1 +
>  tools/libxl/libxl_dm.c      |    6 ++++++
>  tools/libxl/libxl_types.idl |    1 +
>  tools/libxl/xl_cmdimpl.c    |    2 ++
>  5 files changed, 19 insertions(+)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index f8b4576..766862d 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1123,6 +1123,15 @@ Specify the ticket password which is used by a
client for connection.
>  Whether SPICE agent is used for client mouse mode. The default is true
>  (turn on)
>  
> +=item B<spicevdagent=BOOLEAN>
> +
> +Enables spice vdagent. The Spice vdagent is an optional component for
> +enhancing user experience and performing guest-oriented management
> +tasks. Its features includes: client mouse mode (no need to grab mouse
> +by client, no mouse lag), automatic adjustment of screen resolution,
> +copy and paste (text and image) between client and domU. It also
> +requires vdagent service installed on domU o.s. to work. The default is 0.
> +
For a boolean option, I think we should represent the value in
true/false not 1/0.

And as you said it is an optional component, what happens if QEMU
doesn''t have that component compiled in but user specifies this option
to be true?
>  =back
>  
>  =head3 Miscellaneous Emulated Hardware
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index cb9c822..8db5460 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -288,6 +288,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>             
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.nographic, false);
> 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)) {
> +                flexarray_vappend(dm_args, "-device",
"virtio-serial",
Dependency on VirtIO? Should this be documented as well? Does this mean
that vdagent can only work with HVM? AFAICT we don''t support VirtIO in
PV.


Wei.

On Mon, May 06, 2013 at 04:17:00PM +0100, Fabio Fantoni
wrote:
> >>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))
{
> >>+                flexarray_vappend(dm_args, "-device",
"virtio-serial",
> >Dependency on VirtIO? Should this be documented as well? Does this mean
> >that vdagent can only work with HVM? AFAICT we don''t support
VirtIO in
> >PV.
> >
> >
> >Wei.
> >
> Spice vdagent is available only if spice is available, and spice is
> available only on hvm domU for now. There is no way for libxl to
> check if the qemu build has all the required components before
> running it. For example xen build qemu-xen without all the features
> supported by libxl (not only the ones insert by me) and debian
> experimental qemu build support all libxl features. If there isn''t
> spice enabled nothing happens, and if spice is enabled but the qemu
> build doesn''t have it, qemu doesn''t start.
> 
We cannot check QEMU features in libxl, but we can control build options
for QEMU.

Do we need to modify tools/Makefile to make QEMU include spice?
Presumably the answer is "no" because you didn''t have it in
your patch
but it is better to be clear.


Wei.

On Mon, 2013-05-06 at 15:44 +0100, Wei Liu wrote:
> On Mon, May 06, 2013 at 03:15:44PM +0100, fantonifabio@tiscali.it wrote:
> > Usage: spicevdagent=1|0 (default=0)
> > Enables spice vdagent. The Spice vdagent is an optional component for
> > enhancing user experience and performing guest-oriented management
> > tasks. Its features includes: client mouse mode (no need to grab mouse
> > by client, no mouse lag), automatic adjustment of screen resolution,
> > copy and paste (text and image) between client and domU. It also
> > requires vdagent service installed on domU o.s. to work.
> > 
> > Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> > ---
> >  docs/man/xl.cfg.pod.5       |    9 +++++++++
> >  tools/libxl/libxl_create.c  |    1 +
> >  tools/libxl/libxl_dm.c      |    6 ++++++
> >  tools/libxl/libxl_types.idl |    1 +
> >  tools/libxl/xl_cmdimpl.c    |    2 ++
> >  5 files changed, 19 insertions(+)
> > 
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index f8b4576..766862d 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -1123,6 +1123,15 @@ Specify the ticket password which is used by a
client for connection.
> >  Whether SPICE agent is used for client mouse mode. The default is
true
> >  (turn on)
> >  
> > +=item B<spicevdagent=BOOLEAN>
> > +
> > +Enables spice vdagent. The Spice vdagent is an optional component for
> > +enhancing user experience and performing guest-oriented management
> > +tasks. Its features includes: client mouse mode (no need to grab
mouse
> > +by client, no mouse lag), automatic adjustment of screen resolution,
> > +copy and paste (text and image) between client and domU. It also
> > +requires vdagent service installed on domU o.s. to work. The default
is 0.
> > +
> 
> For a boolean option, I think we should represent the value in
> true/false not 1/0.
The xl cfg file parser doesn''t handle true/false so this option is just
following all the existing boolean options in the xl configuration file.

Supporting true/false might be a nice general enhancement to make, not
sure what the implications are in terms of compatibility with xm config
files, but I think it is OK for xl to move in, so long as moving from xm
to xl continues to work.
> > 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)) {
> > +                flexarray_vappend(dm_args, "-device",
"virtio-serial",
> 
> Dependency on VirtIO? Should this be documented as well?
The docs are in the HVM only part of the man page.

Ian.

On Tue, May 07, 2013 at 10:37:19AM +0100, Ian Campbell
wrote:
> On Mon, 2013-05-06 at 15:44 +0100, Wei Liu wrote:
> > On Mon, May 06, 2013 at 03:15:44PM +0100, fantonifabio@tiscali.it
wrote:
> > > Usage: spicevdagent=1|0 (default=0)
> > > Enables spice vdagent. The Spice vdagent is an optional component
for
> > > enhancing user experience and performing guest-oriented
management
> > > tasks. Its features includes: client mouse mode (no need to grab
mouse
> > > by client, no mouse lag), automatic adjustment of screen
resolution,
> > > copy and paste (text and image) between client and domU. It also
> > > requires vdagent service installed on domU o.s. to work.
> > > 
> > > Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> > > ---
> > >  docs/man/xl.cfg.pod.5       |    9 +++++++++
> > >  tools/libxl/libxl_create.c  |    1 +
> > >  tools/libxl/libxl_dm.c      |    6 ++++++
> > >  tools/libxl/libxl_types.idl |    1 +
> > >  tools/libxl/xl_cmdimpl.c    |    2 ++
> > >  5 files changed, 19 insertions(+)
> > > 
> > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > > index f8b4576..766862d 100644
> > > --- a/docs/man/xl.cfg.pod.5
> > > +++ b/docs/man/xl.cfg.pod.5
> > > @@ -1123,6 +1123,15 @@ Specify the ticket password which is used
by a client for connection.
> > >  Whether SPICE agent is used for client mouse mode. The default
is true
> > >  (turn on)
> > >  
> > > +=item B<spicevdagent=BOOLEAN>
> > > +
> > > +Enables spice vdagent. The Spice vdagent is an optional
component for
> > > +enhancing user experience and performing guest-oriented
management
> > > +tasks. Its features includes: client mouse mode (no need to grab
mouse
> > > +by client, no mouse lag), automatic adjustment of screen
resolution,
> > > +copy and paste (text and image) between client and domU. It also
> > > +requires vdagent service installed on domU o.s. to work. The
default is 0.
> > > +
> > 
> > For a boolean option, I think we should represent the value in
> > true/false not 1/0.
> 
> The xl cfg file parser doesn''t handle true/false so this option is
just
> following all the existing boolean options in the xl configuration file.
> 
> Supporting true/false might be a nice general enhancement to make, not
> sure what the implications are in terms of compatibility with xm config
> files, but I think it is OK for xl to move in, so long as moving from xm
> to xl continues to work.
> 
What I meant is in the documentation we should use true / false instead
of 1 / 0. Other options use true / false as well, like e820_host and
spiceagent_mouse.


Wei.

On Tue, 2013-05-07 at 10:58 +0100, Wei Liu wrote:
> On Tue, May 07, 2013 at 10:37:19AM +0100, Ian Campbell wrote:
> > On Mon, 2013-05-06 at 15:44 +0100, Wei Liu wrote:
> > > On Mon, May 06, 2013 at 03:15:44PM +0100, fantonifabio@tiscali.it
wrote:
> > > > Usage: spicevdagent=1|0 (default=0)
> > > > Enables spice vdagent. The Spice vdagent is an optional
component for
> > > > enhancing user experience and performing guest-oriented
management
> > > > tasks. Its features includes: client mouse mode (no need to
grab mouse
> > > > by client, no mouse lag), automatic adjustment of screen
resolution,
> > > > copy and paste (text and image) between client and domU. It
also
> > > > requires vdagent service installed on domU o.s. to work.
> > > > 
> > > > Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> > > > ---
> > > >  docs/man/xl.cfg.pod.5       |    9 +++++++++
> > > >  tools/libxl/libxl_create.c  |    1 +
> > > >  tools/libxl/libxl_dm.c      |    6 ++++++
> > > >  tools/libxl/libxl_types.idl |    1 +
> > > >  tools/libxl/xl_cmdimpl.c    |    2 ++
> > > >  5 files changed, 19 insertions(+)
> > > > 
> > > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > > > index f8b4576..766862d 100644
> > > > --- a/docs/man/xl.cfg.pod.5
> > > > +++ b/docs/man/xl.cfg.pod.5
> > > > @@ -1123,6 +1123,15 @@ Specify the ticket password which is
used by a client for connection.
> > > >  Whether SPICE agent is used for client mouse mode. The
default is true
> > > >  (turn on)
> > > >  
> > > > +=item B<spicevdagent=BOOLEAN>
> > > > +
> > > > +Enables spice vdagent. The Spice vdagent is an optional
component for
> > > > +enhancing user experience and performing guest-oriented
management
> > > > +tasks. Its features includes: client mouse mode (no need to
grab mouse
> > > > +by client, no mouse lag), automatic adjustment of screen
resolution,
> > > > +copy and paste (text and image) between client and domU. It
also
> > > > +requires vdagent service installed on domU o.s. to work.
The default is 0.
> > > > +
> > > 
> > > For a boolean option, I think we should represent the value in
> > > true/false not 1/0.
> > 
> > The xl cfg file parser doesn''t handle true/false so this
option is just
> > following all the existing boolean options in the xl configuration
file.
> > 
> > Supporting true/false might be a nice general enhancement to make, not
> > sure what the implications are in terms of compatibility with xm
config
> > files, but I think it is OK for xl to move in, so long as moving from
xm
> > to xl continues to work.
> > 
> 
> What I meant is in the documentation we should use true / false instead
> of 1 / 0. Other options use true / false as well, like e820_host and
> spiceagent_mouse.
Or right, we do seem to be rather inconsistent about that.

Given that the parser (currently) only understands 0 and 1 (or other
integers I guess) I think the most consistent usage would be to write
"true (1)" or "false (0)" everywhere.

Do you fancy putting together a patch to make this so?

Ian.

On Tue, May 07, 2013 at 11:18:43AM +0100, Ian Campbell wrote:
[...]
> > 
> > What I meant is in the documentation we should use true / false
instead
> > of 1 / 0. Other options use true / false as well, like e820_host and
> > spiceagent_mouse.
> 
> Or right, we do seem to be rather inconsistent about that.
> 
> Given that the parser (currently) only understands 0 and 1 (or other
> integers I guess) I think the most consistent usage would be to write
> "true (1)" or "false (0)" everywhere.
> 
> Do you fancy putting together a patch to make this so?
> 
We already have clear definition of BOOLEAN type around line 37 in the same
file. IMHO using "true" / "false" is sufficent.


Wei.
> Ian.
>

On Tue, 2013-05-07 at 11:38 +0100, Wei Liu wrote:
> On Tue, May 07, 2013 at 11:18:43AM +0100, Ian Campbell wrote:
> [...]
> > > 
> > > What I meant is in the documentation we should use true / false
instead
> > > of 1 / 0. Other options use true / false as well, like e820_host
and
> > > spiceagent_mouse.
> > 
> > Or right, we do seem to be rather inconsistent about that.
> > 
> > Given that the parser (currently) only understands 0 and 1 (or other
> > integers I guess) I think the most consistent usage would be to write
> > "true (1)" or "false (0)" everywhere.
> > 
> > Do you fancy putting together a patch to make this so?
> > 
> 
> We already have clear definition of BOOLEAN type around line 37 in the same
> file. IMHO using "true" / "false" is sufficent.
People never read this stuff properly.

In any case the current docs use 0/1 and true/false in approximately
equal measure...

Ian.

On Tue, May 07, 2013 at 11:41:25AM +0100, Ian Campbell
wrote:
> On Tue, 2013-05-07 at 11:38 +0100, Wei Liu wrote:
> > On Tue, May 07, 2013 at 11:18:43AM +0100, Ian Campbell wrote:
> > [...]
> > > > 
> > > > What I meant is in the documentation we should use true /
false instead
> > > > of 1 / 0. Other options use true / false as well, like
e820_host and
> > > > spiceagent_mouse.
> > > 
> > > Or right, we do seem to be rather inconsistent about that.
> > > 
> > > Given that the parser (currently) only understands 0 and 1 (or
other
> > > integers I guess) I think the most consistent usage would be to
write
> > > "true (1)" or "false (0)" everywhere.
> > > 
> > > Do you fancy putting together a patch to make this so?
> > > 
> > 
> > We already have clear definition of BOOLEAN type around line 37 in the
same
> > file. IMHO using "true" / "false" is sufficent.
> 
> People never read this stuff properly.
> 
> In any case the current docs use 0/1 and true/false in approximately
> equal measure...
> 
Haha, patch on the way.


Wei.
> Ian.
>

fantonifabio@tiscali.it writes ("[PATCH v3] libxl: Spice vdagent support
for upstream qemu"):
> Usage: spicevdagent=1|0 (default=0)
> Enables spice vdagent. The Spice vdagent is an optional component for
> enhancing user experience and performing guest-oriented management
> tasks. Its features includes: client mouse mode (no need to grab mouse
> by client, no mouse lag), automatic adjustment of screen resolution,
> copy and paste (text and image) between client and domU. It also
> requires vdagent service installed on domU o.s. to work.
Thanks, that''s helpful.  What are the security implications ?

In particular, does it mean that when the user has a spice client
connected to a guest, the guest can spy on the user''s clipboard all
the time ?

Thanks,
Ian.

On Tue, May 7, 2013 at 6:06 PM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> fantonifabio@tiscali.it writes ("[PATCH v3] libxl: Spice vdagent
support for upstream qemu"):
>> Usage: spicevdagent=1|0 (default=0)
>> Enables spice vdagent. The Spice vdagent is an optional component for
>> enhancing user experience and performing guest-oriented management
>> tasks. Its features includes: client mouse mode (no need to grab mouse
>> by client, no mouse lag), automatic adjustment of screen resolution,
>> copy and paste (text and image) between client and domU. It also
>> requires vdagent service installed on domU o.s. to work.
>
> Thanks, that''s helpful.  What are the security implications ?
>
> In particular, does it mean that when the user has a spice client
> connected to a guest, the guest can spy on the user''s clipboard
all
> the time ?
If it did, how would that affect your views on the patch?

It seems overall like the decision to run a piece of software on a
local computer which may send data (such as the clipboard) off to a
remote computer is the user''s problem.  She has no way of knowing for
sure that what she''s connecting to has vdagent enabled or not.  Even
if we don''t accept this patch, the administrator can probably enable
it by using the "extra" args in the config file.

 -George