[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.