[Qemu-devel] [PATCH 00/10] RFC: spice: add -display app to launch external UI

[Qemu-devel] [PATCH 00/10] RFC: spice: add -display app to launch external UI

[Marc-André Lureau]

Hi,

One of the most featurefull UI that qemu has is the GTK one. Yet, it
doesn't provide many of the functionalities we can expect from a VM
desktop application (USB redirection, shared folders, drag and drop,
better multimonitor support etc.). Spice is able to export all the VM
details to a client, including QMP monitor, serial ports etc... It's
thus possible for a Spice client to provide a similar experience as
the current qemu -display gtk UI (while keeping similar performances).

In a related virt-viewer series, "[PATCH 00/22] Add QEMU-like UI: VT
console & basic VM state", I proposed to augment remote-viewer with
VTE consoles and basic QMP support to provide a UI similar to qemu
gtk.

With this series, starting qemu with "-display app" will setup a Spice
server in the background and launch the client application associated
with spice+unix:// uri handling to interact with qemu (the concept
could be extended to other protocols). The resulting user experience
should be very close to what qemu gtk display provides (it should be
possible to make it work on various platforms, although I only
tested/developped on Linux)

Marc-André Lureau (10):
  char/spice: trigger HUP event
  char/spice: discard write() if backend is disconnected
  configure: bump spice-server required version to 0.12.6
  spice: avoid spice runtime assert
  spice: merge options lists
  spice: do not stop spice if VM is paused
  char: move SpiceChardev and open_spice_port() to spice.h header
  char: register spice ports after spice started
  build-sys: add gio-2.0 check
  display: add -display app launching external application

 qapi/ui.json            |   3 +-
 hw/display/qxl.h        |   2 -
 include/chardev/spice.h |  27 ++++++
 include/ui/qemu-spice.h |   9 --
 chardev/spice.c         |  66 +++++++--------
 hw/display/qxl.c        |   8 --
 ui/app.c                | 179 ++++++++++++++++++++++++++++++++++++++++
 ui/spice-core.c         |  19 +++--
 chardev/trace-events    |   1 +
 configure               |  17 +++-
 qemu-options.hx         |   5 ++
 ui/Makefile.objs        |   5 ++
 12 files changed, 274 insertions(+), 67 deletions(-)
 create mode 100644 include/chardev/spice.h
 create mode 100644 ui/app.c

-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 01/10] char/spice: trigger HUP event

[Marc-André Lureau]

Inform the front-end of disconnected state (spice client
disconnected).

This will wakeup the source handler immediately, so it can detect the
disconnection asap.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/spice.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/chardev/spice.c b/chardev/spice.c
index e66e3ad568..fe06034d7f 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -152,15 +152,25 @@ static void vmc_unregister_interface(SpiceChardev *scd)
 static gboolean spice_char_source_prepare(GSource *source, gint *timeout)
 {
     SpiceCharSource *src = (SpiceCharSource *)source;
+    Chardev *chr = CHARDEV(src->scd);
 
     *timeout = -1;
 
+    if (!chr->be_open) {
+        return true;
+    }
+
     return !src->scd->blocked;
 }
 
 static gboolean spice_char_source_check(GSource *source)
 {
     SpiceCharSource *src = (SpiceCharSource *)source;
+    Chardev *chr = CHARDEV(src->scd);
+
+    if (!chr->be_open) {
+        return true;
+    }
 
     return !src->scd->blocked;
 }
@@ -168,9 +178,12 @@ static gboolean spice_char_source_check(GSource *source)
 static gboolean spice_char_source_dispatch(GSource *source,
     GSourceFunc callback, gpointer user_data)
 {
+    SpiceCharSource *src = (SpiceCharSource *)source;
+    Chardev *chr = CHARDEV(src->scd);
     GIOFunc func = (GIOFunc)callback;
+    GIOCondition cond = chr->be_open ? G_IO_OUT : G_IO_HUP;
 
-    return func(NULL, G_IO_OUT, user_data);
+    return func(NULL, cond, user_data);
 }
 
 static GSourceFuncs SpiceCharSourceFuncs = {
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 02/10] char/spice: discard write() if backend is disconnected

[Marc-André Lureau]

Most chardev backend handle write() as discarded data if underlying
system is disconnected. For unknown historical reasons, the Spice
backend has "reliable" write. It will wait until the client end is
reconnected to accept further write().

Let's review Spice chardev usage and handling of a disconnected
client:

 * spice vdagent
   The agent will reopen the virtio port on disconnect.

 * usb redirection
   A disconnect creates a device disconnection.

 * smartcard emulation
   Data is discarded in passthru_apdu_from_guest()

 * spice webdavd
   The daemon will restart the service, and reopen the virtio port.

 * spice ports (serial console, qemu monitor..)
   Depends on the associated device or usage.

   - 16550A serial does nothing special, and may block guest on write

   - QMP/HMP monitor have some CLOSED event handling, but want to
     flush the write, which will finish when a new client connects.

For all these use cases, it is better to discard pending write when
the client is disconnected, and expect the device/agent to behave
correctly on CHR_EVENT_CLOSED (to stop reading and writing from
chardev).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/spice.c      | 6 ++++++
 chardev/trace-events | 1 +
 2 files changed, 7 insertions(+)

diff --git a/chardev/spice.c b/chardev/spice.c
index fe06034d7f..6ad95ffe62 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -212,6 +212,12 @@ static int spice_chr_write(Chardev *chr, const uint8_t *buf, int len)
     int read_bytes;
 
     assert(s->datalen == 0);
+
+    if (!chr->be_open) {
+        trace_spice_chr_discard_write(len);
+        return len;
+    }
+
     s->datapos = buf;
     s->datalen = len;
     spice_server_char_device_wakeup(&s->sin);
diff --git a/chardev/trace-events b/chardev/trace-events
index d0e5f3bbc1..b8a7596344 100644
--- a/chardev/trace-events
+++ b/chardev/trace-events
@@ -10,6 +10,7 @@ wct_cmd_other(const char *cmd) "%s"
 wct_speed(int speed) "%d"
 
 # chardev/spice.c
+spice_chr_discard_write(int len) "spice chr write discarded %d"
 spice_vmc_write(ssize_t out, int len) "spice wrote %zd of requested %d"
 spice_vmc_read(int bytes, int len) "spice read %d of requested %d"
 spice_vmc_register_interface(void *scd) "spice vmc registered interface %p"
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 02/10] char/spice: discard write() if backend is disconnected

[Marc-André Lureau]

Hi

On Fri, Aug 3, 2018 at 7:36 PM, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> Most chardev backend handle write() as discarded data if underlying
> system is disconnected. For unknown historical reasons, the Spice
> backend has "reliable" write. It will wait until the client end is
> reconnected to accept further write().
>
> Let's review Spice chardev usage and handling of a disconnected
> client:
>
>  * spice vdagent
>    The agent will reopen the virtio port on disconnect.
>
>  * usb redirection
>    A disconnect creates a device disconnection.
>
>  * smartcard emulation
>    Data is discarded in passthru_apdu_from_guest()

Doing more testing, I realize this creates is a regression on Spice
smartcard. Sadly, the Spice smartcard channel isn't actually based on
spicevmc (as the chardev name may imply), and doesn't set the
connected sif->state(). I am sending a patch to spice-devel to fix
this. I'll update this patch to set the chardev opened at "smartcard"
spicevmc creation time when we have an old spice server.

>
>  * spice webdavd
>    The daemon will restart the service, and reopen the virtio port.
>
>  * spice ports (serial console, qemu monitor..)
>    Depends on the associated device or usage.
>
>    - 16550A serial does nothing special, and may block guest on write
>
>    - QMP/HMP monitor have some CLOSED event handling, but want to
>      flush the write, which will finish when a new client connects.
>
> For all these use cases, it is better to discard pending write when
> the client is disconnected, and expect the device/agent to behave
> correctly on CHR_EVENT_CLOSED (to stop reading and writing from
> chardev).
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/spice.c      | 6 ++++++
>  chardev/trace-events | 1 +
>  2 files changed, 7 insertions(+)
>
> diff --git a/chardev/spice.c b/chardev/spice.c
> index fe06034d7f..6ad95ffe62 100644
> --- a/chardev/spice.c
> +++ b/chardev/spice.c
> @@ -212,6 +212,12 @@ static int spice_chr_write(Chardev *chr, const uint8_t *buf, int len)
>      int read_bytes;
>
>      assert(s->datalen == 0);
> +
> +    if (!chr->be_open) {
> +        trace_spice_chr_discard_write(len);
> +        return len;
> +    }
> +
>      s->datapos = buf;
>      s->datalen = len;
>      spice_server_char_device_wakeup(&s->sin);
> diff --git a/chardev/trace-events b/chardev/trace-events
> index d0e5f3bbc1..b8a7596344 100644
> --- a/chardev/trace-events
> +++ b/chardev/trace-events
> @@ -10,6 +10,7 @@ wct_cmd_other(const char *cmd) "%s"
>  wct_speed(int speed) "%d"
>
>  # chardev/spice.c
> +spice_chr_discard_write(int len) "spice chr write discarded %d"
>  spice_vmc_write(ssize_t out, int len) "spice wrote %zd of requested %d"
>  spice_vmc_read(int bytes, int len) "spice read %d of requested %d"
>  spice_vmc_register_interface(void *scd) "spice vmc registered interface %p"
> --
> 2.18.0.547.g1d89318c48
>
>



-- 
Marc-André Lureau

[Qemu-devel] [PATCH 03/10] configure: bump spice-server required version to 0.12.6

[Marc-André Lureau]

Looking at chardev/spice.c code, I realize compilation was broken for
a while with spice-server < 0.12.3. I propose to bump required version
to 0.12.6, released 3y ago, instead of adding more #ifdef.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/qxl.h        |  2 --
 include/ui/qemu-spice.h |  9 ---------
 chardev/spice.c         | 12 ------------
 hw/display/qxl.c        |  8 --------
 ui/spice-core.c         |  8 --------
 configure               |  4 ++--
 6 files changed, 2 insertions(+), 41 deletions(-)

diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index 089696ef62..e4afab8c3a 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -100,9 +100,7 @@ typedef struct PCIQXLDevice {
     QXLModes           *modes;
     uint32_t           rom_size;
     MemoryRegion       rom_bar;
-#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
     uint16_t           max_outputs;
-#endif
 
     /* vram pci bar */
     uint64_t           vram_size;
diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index c6d50eb87a..f3e17612b1 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -41,18 +41,9 @@ int qemu_spice_set_pw_expire(time_t expires);
 int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
                             const char *subject);
 
-#if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
-#define SPICE_NEEDS_SET_MM_TIME 1
-#else
 #define SPICE_NEEDS_SET_MM_TIME 0
-#endif
 
-#if SPICE_SERVER_VERSION >= 0x000c02
 void qemu_spice_register_ports(void);
-#else
-static inline Chardev *qemu_chr_open_spice_port(const char *name)
-{ return NULL; }
-#endif
 
 #else  /* CONFIG_SPICE */
 
diff --git a/chardev/spice.c b/chardev/spice.c
index 6ad95ffe62..4d4bafe34e 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -77,7 +77,6 @@ static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t *buf, int len)
     return bytes;
 }
 
-#if SPICE_SERVER_VERSION >= 0x000c02
 static void vmc_event(SpiceCharDeviceInstance *sin, uint8_t event)
 {
     SpiceChardev *scd = container_of(sin, SpiceChardev, sin);
@@ -95,7 +94,6 @@ static void vmc_event(SpiceCharDeviceInstance *sin, uint8_t event)
     trace_spice_vmc_event(chr_event);
     qemu_chr_be_event(chr, chr_event);
 }
-#endif
 
 static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
 {
@@ -119,12 +117,8 @@ static SpiceCharDeviceInterface vmc_interface = {
     .state              = vmc_state,
     .write              = vmc_write,
     .read               = vmc_read,
-#if SPICE_SERVER_VERSION >= 0x000c02
     .event              = vmc_event,
-#endif
-#if SPICE_SERVER_VERSION >= 0x000c06
     .flags              = SPICE_CHAR_DEVICE_NOTIFY_WRITABLE,
-#endif
 };
 
 
@@ -242,9 +236,7 @@ static void char_spice_finalize(Object *obj)
     }
 
     g_free((char *)s->sin.subtype);
-#if SPICE_SERVER_VERSION >= 0x000c02
     g_free((char *)s->sin.portname);
-#endif
 }
 
 static void spice_vmc_set_fe_open(struct Chardev *chr, int fe_open)
@@ -259,7 +251,6 @@ static void spice_vmc_set_fe_open(struct Chardev *chr, int fe_open)
 
 static void spice_port_set_fe_open(struct Chardev *chr, int fe_open)
 {
-#if SPICE_SERVER_VERSION >= 0x000c02
     SpiceChardev *s = SPICE_CHARDEV(chr);
 
     if (fe_open) {
@@ -267,7 +258,6 @@ static void spice_port_set_fe_open(struct Chardev *chr, int fe_open)
     } else {
         spice_server_port_event(&s->sin, SPICE_PORT_EVENT_CLOSED);
     }
-#endif
 }
 
 static void spice_chr_accept_input(struct Chardev *chr)
@@ -317,7 +307,6 @@ static void qemu_chr_open_spice_vmc(Chardev *chr,
     chr_open(chr, type);
 }
 
-#if SPICE_SERVER_VERSION >= 0x000c02
 static void qemu_chr_open_spice_port(Chardev *chr,
                                      ChardevBackend *backend,
                                      bool *be_opened,
@@ -350,7 +339,6 @@ void qemu_spice_register_ports(void)
         vmc_register_interface(s);
     }
 }
-#endif
 
 static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
                                      Error **errp)
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 830c392c53..cdd884984d 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -274,11 +274,9 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
                     QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
                     0));
     } else {
-#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
         if (qxl->max_outputs) {
             spice_qxl_set_max_monitors(&qxl->ssd.qxl, qxl->max_outputs);
         }
-#endif
         qxl->guest_monitors_config = qxl->ram->monitors_config;
         spice_qxl_monitors_config_async(&qxl->ssd.qxl,
                 qxl->ram->monitors_config,
@@ -1094,12 +1092,10 @@ static int interface_client_monitors_config(QXLInstance *sin,
         return 1;
     }
 
-#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
     /* limit number of outputs based on setting limit */
     if (qxl->max_outputs && qxl->max_outputs <= max_outputs) {
         max_outputs = qxl->max_outputs;
     }
-#endif
 
     config_changed = qxl_rom_monitors_config_changed(rom,
                                                      monitors_config,
@@ -1177,9 +1173,7 @@ static void qxl_enter_vga_mode(PCIQXLDevice *d)
         return;
     }
     trace_qxl_enter_vga_mode(d->id);
-#if SPICE_SERVER_VERSION >= 0x000c03 /* release 0.12.3 */
     spice_qxl_driver_unload(&d->ssd.qxl);
-#endif
     graphic_console_set_hwops(d->ssd.dcl.con, d->vga.hw_ops, &d->vga);
     update_displaychangelistener(&d->ssd.dcl, GUI_REFRESH_INTERVAL_DEFAULT);
     qemu_spice_create_host_primary(&d->ssd);
@@ -2403,9 +2397,7 @@ static Property qxl_properties[] = {
         DEFINE_PROP_UINT32("vram64_size_mb", PCIQXLDevice, vram_size_mb, -1),
         DEFINE_PROP_UINT32("vgamem_mb", PCIQXLDevice, vgamem_size_mb, 16),
         DEFINE_PROP_INT32("surfaces", PCIQXLDevice, ssd.num_surfaces, 1024),
-#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
         DEFINE_PROP_UINT16("max_outputs", PCIQXLDevice, max_outputs, 0),
-#endif
         DEFINE_PROP_UINT32("xres", PCIQXLDevice, xres, 0),
         DEFINE_PROP_UINT32("yres", PCIQXLDevice, yres, 0),
         DEFINE_PROP_BOOL("global-vmstate", PCIQXLDevice, vga.global_vmstate, false),
diff --git a/ui/spice-core.c b/ui/spice-core.c
index f8c0878529..76896f7c7a 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -746,13 +746,7 @@ void qemu_spice_init(void)
     }
 
     if (qemu_opt_get_bool(opts, "disable-agent-file-xfer", 0)) {
-#if SPICE_SERVER_VERSION >= 0x000c04
         spice_server_set_agent_file_xfer(spice_server, false);
-#else
-        error_report("this qemu build does not support the "
-                     "\"disable-agent-file-xfer\" option");
-        exit(1);
-#endif
     }
 
     compression = SPICE_IMAGE_COMPRESS_AUTO_GLZ;
@@ -818,9 +812,7 @@ void qemu_spice_init(void)
     g_free(x509_cert_file);
     g_free(x509_cacert_file);
 
-#if SPICE_SERVER_VERSION >= 0x000c02
     qemu_spice_register_ports();
-#endif
 
 #ifdef HAVE_SPICE_GL
     if (qemu_opt_get_bool(opts, "gl", 0)) {
diff --git a/configure b/configure
index 8280ae7a8a..8a1371c55c 100755
--- a/configure
+++ b/configure
@@ -4521,7 +4521,7 @@ int main(void) { spice_server_new(); return 0; }
 EOF
   spice_cflags=$($pkg_config --cflags spice-protocol spice-server 2>/dev/null)
   spice_libs=$($pkg_config --libs spice-protocol spice-server 2>/dev/null)
-  if $pkg_config --atleast-version=0.12.0 spice-server && \
+  if $pkg_config --atleast-version=0.12.6 spice-server && \
      $pkg_config --atleast-version=0.12.3 spice-protocol && \
      compile_prog "$spice_cflags" "$spice_libs" ; then
     spice="yes"
@@ -4532,7 +4532,7 @@ EOF
   else
     if test "$spice" = "yes" ; then
       feature_not_found "spice" \
-          "Install spice-server(>=0.12.0) and spice-protocol(>=0.12.3) devel"
+          "Install spice-server(>=0.12.6) and spice-protocol(>=0.12.3) devel"
     fi
     spice="no"
   fi
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 03/10] configure: bump spice-server required version to 0.12.6

[Eric Blake]

On 08/03/2018 12:36 PM, Marc-André Lureau wrote:
> Looking at chardev/spice.c code, I realize compilation was broken for
> a while with spice-server < 0.12.3. I propose to bump required version
> to 0.12.6, released 3y ago, instead of adding more #ifdef.

Can you please also investigate which version is shipping in various 
distros? Look at commit e7b3af81 for a good example of justifying a 
minimum version bump, by calling out the versions in use according to 
the supported platforms doc.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 03/10] configure: bump spice-server required version to 0.12.6

[Daniel P. Berrangé]

On Fri, Aug 03, 2018 at 07:36:07PM +0200, Marc-André Lureau wrote:
> Looking at chardev/spice.c code, I realize compilation was broken for
> a while with spice-server < 0.12.3. I propose to bump required version
> to 0.12.6, released 3y ago, instead of adding more #ifdef.

Since we introduced our platform support policy[1], it is desired that
any min version changes consider what versions our target OS have
available. eg take a look at this commit message example:

  https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg03796.html

could you use repology[2] to do the same for spice to validate that
this proposed min version is compatible with distros we target.

Regards,
Daniel

[1] https://qemu.weilnetz.de/doc/qemu-doc.html#Supported-build-platforms

[2] https://repology.org/metapackage/spice/versions
    https://repology.org/metapackage/spice-server/versions
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH 04/10] spice: avoid spice runtime assert

[Marc-André Lureau]

The Spice server doesn't like to be started or stopped twice . It
aborts with:

(process:6191): Spice-ERROR **: 19:29:35.912: red-worker.c:623:handle_dev_start: assertion `!worker->running' failed

It's easy to avoid that situation since qemu spice_display_is_running
tracks the server state.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/spice-core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 76896f7c7a..13f1c11b61 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -923,12 +923,20 @@ int qemu_spice_display_add_client(int csock, int skipauth, int tls)
 
 void qemu_spice_display_start(void)
 {
+    if (spice_display_is_running) {
+        return;
+    }
+
     spice_display_is_running = true;
     spice_server_vm_start(spice_server);
 }
 
 void qemu_spice_display_stop(void)
 {
+    if (!spice_display_is_running) {
+        return;
+    }
+
     spice_server_vm_stop(spice_server);
     spice_display_is_running = false;
 }
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 04/10] spice: avoid spice runtime assert

[Gerd Hoffmann]

On Fri, Aug 03, 2018 at 07:36:08PM +0200, Marc-André Lureau wrote:
> The Spice server doesn't like to be started or stopped twice . It
> aborts with:
> 
> (process:6191): Spice-ERROR **: 19:29:35.912: red-worker.c:623:handle_dev_start: assertion `!worker->running' failed
> 
> It's easy to avoid that situation since qemu spice_display_is_running
> tracks the server state.

How do you trigger this?  qemu should not do that in the first place.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 04/10] spice: avoid spice runtime assert

[Marc-André Lureau]

Hi
On Tue, Aug 7, 2018 at 5:48 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Aug 03, 2018 at 07:36:08PM +0200, Marc-André Lureau wrote:
> > The Spice server doesn't like to be started or stopped twice . It
> > aborts with:
> >
> > (process:6191): Spice-ERROR **: 19:29:35.912: red-worker.c:623:handle_dev_start: assertion `!worker->running' failed
> >
> > It's easy to avoid that situation since qemu spice_display_is_running
> > tracks the server state.
>
> How do you trigger this?  qemu should not do that in the first place.

After "[PATCH 06/10] spice: do not stop spice if VM is paused ", you
can pause the VM and resume it (also through spice): it will call
qemu_spice_display_start() twice. The easiest seems to add a check for
spice_display_is_running, with this patch.

-- 
Marc-André Lureau

[Qemu-devel] [PATCH 05/10] spice: merge options lists

[Marc-André Lureau]

Passing several -spice options to qemu command line, or calling
several time qemu_opts_set() will ignore all but the first option
list. Since the spice server is a singleton, it makes sense to merge
all the options, the last value being the one taken into account.

This changes the behaviour from, for ex:
$ qemu... -spice port=5900 -spice port=5901 -> port: 5900
to:
$ qemu... -spice port=5900 -spice port=5901 -> port: 5901

(if necessary we could instead produce an error when an option is
given twice, although this makes handling default values and such more
complicated)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/spice-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 13f1c11b61..6bd107f0f9 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -400,6 +400,7 @@ static SpiceChannelList *qmp_query_spice_channels(void)
 static QemuOptsList qemu_spice_opts = {
     .name = "spice",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_spice_opts.head),
+    .merge_lists = true,
     .desc = {
         {
             .name = "port",
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 06/10] spice: do not stop spice if VM is paused

[Marc-André Lureau]

spice_server_vm_start/stop() was added to help migration state.

However, a paused VM could keep running the spice server. This will
allow a Spice client to keep sending commands to a spice chardev. This
allows to stop/cont a VM from a Spice monitor port.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/spice-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 6bd107f0f9..88f0cd7262 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -629,7 +629,7 @@ static void vm_change_state_handler(void *opaque, int running,
 {
     if (running) {
         qemu_spice_display_start();
-    } else {
+    } else if (state != RUN_STATE_PAUSED) {
         qemu_spice_display_stop();
     }
 }
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 07/10] char: move SpiceChardev and open_spice_port() to spice.h header

[Marc-André Lureau]

This will allow to subclass SpiceChardev easily.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/spice.h | 27 +++++++++++++++++++++++++++
 chardev/spice.c         | 28 +++++-----------------------
 2 files changed, 32 insertions(+), 23 deletions(-)
 create mode 100644 include/chardev/spice.h

diff --git a/include/chardev/spice.h b/include/chardev/spice.h
new file mode 100644
index 0000000000..6431da3205
--- /dev/null
+++ b/include/chardev/spice.h
@@ -0,0 +1,27 @@
+#ifndef CHARDEV_SPICE_H_
+#define CHARDEV_SPICE_H_
+
+#include <spice.h>
+#include "chardev/char-fe.h"
+
+typedef struct SpiceChardev {
+    Chardev               parent;
+
+    SpiceCharDeviceInstance sin;
+    bool                  active;
+    bool                  blocked;
+    const uint8_t         *datapos;
+    int                   datalen;
+    QLIST_ENTRY(SpiceChardev) next;
+} SpiceChardev;
+
+#define TYPE_CHARDEV_SPICE "chardev-spice"
+#define TYPE_CHARDEV_SPICEVMC "chardev-spicevmc"
+#define TYPE_CHARDEV_SPICEPORT "chardev-spiceport"
+
+#define SPICE_CHARDEV(obj) OBJECT_CHECK(SpiceChardev, (obj), TYPE_CHARDEV_SPICE)
+
+void qemu_chr_open_spice_port(Chardev *chr, ChardevBackend *backend,
+                              bool *be_opened, Error **errp);
+
+#endif
diff --git a/chardev/spice.c b/chardev/spice.c
index 4d4bafe34e..0112efb6d2 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -2,30 +2,12 @@
 #include "trace.h"
 #include "ui/qemu-spice.h"
 #include "chardev/char.h"
+#include "chardev/spice.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/option.h"
-#include <spice.h>
 #include <spice/protocol.h>
 
-
-typedef struct SpiceChardev {
-    Chardev               parent;
-
-    SpiceCharDeviceInstance sin;
-    bool                  active;
-    bool                  blocked;
-    const uint8_t         *datapos;
-    int                   datalen;
-    QLIST_ENTRY(SpiceChardev) next;
-} SpiceChardev;
-
-#define TYPE_CHARDEV_SPICE "chardev-spice"
-#define TYPE_CHARDEV_SPICEVMC "chardev-spicevmc"
-#define TYPE_CHARDEV_SPICEPORT "chardev-spiceport"
-
-#define SPICE_CHARDEV(obj) OBJECT_CHECK(SpiceChardev, (obj), TYPE_CHARDEV_SPICE)
-
 typedef struct SpiceCharSource {
     GSource               source;
     SpiceChardev       *scd;
@@ -307,10 +289,10 @@ static void qemu_chr_open_spice_vmc(Chardev *chr,
     chr_open(chr, type);
 }
 
-static void qemu_chr_open_spice_port(Chardev *chr,
-                                     ChardevBackend *backend,
-                                     bool *be_opened,
-                                     Error **errp)
+void qemu_chr_open_spice_port(Chardev *chr,
+                              ChardevBackend *backend,
+                              bool *be_opened,
+                              Error **errp)
 {
     ChardevSpicePort *spiceport = backend->u.spiceport.data;
     const char *name = spiceport->fqdn;
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 07/10] char: move SpiceChardev and open_spice_port() to spice.h header

[Eric Blake]

On 08/03/2018 12:36 PM, Marc-André Lureau wrote:
> This will allow to subclass SpiceChardev easily.

Grammar - 'allow to ${verb}' is not idiomatic; better is 'allow 
${verb}ing' or 'allow ${subject} to ${verb}'. Here, I'd write:

This will allow easier subclassing of SpiceChardev.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [PATCH 08/10] char: register spice ports after spice started

[Marc-André Lureau]

Spice port registration is delayed until the server is started. But
ports created after are not being registered. If the server is already
started, do vmc_register_interface() to register it from
qemu_chr_open_spice_port().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/spice.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/chardev/spice.c b/chardev/spice.c
index 0112efb6d2..1deb0cedef 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -308,6 +308,11 @@ void qemu_chr_open_spice_port(Chardev *chr,
     *be_opened = false;
     s = SPICE_CHARDEV(chr);
     s->sin.portname = g_strdup(name);
+
+    if (using_spice) {
+        /* spice server already created */
+        vmc_register_interface(s);
+    }
 }
 
 void qemu_spice_register_ports(void)
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 09/10] build-sys: add gio-2.0 check

[Marc-André Lureau]

GIO is required for the -display app backend.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/configure b/configure
index 8a1371c55c..9f91752289 100755
--- a/configure
+++ b/configure
@@ -3483,6 +3483,14 @@ for i in $glib_modules; do
     fi
 done
 
+if $pkg_config --atleast-version=$glib_req_ver gio-2.0; then
+    gio=yes
+    gio_cflags=$($pkg_config --cflags gio-2.0)
+    gio_libs=$($pkg_config --libs gio-2.0)
+else
+    gio=no
+fi
+
 # Sanity check that the current size_t matches the
 # size that glib thinks it should be. This catches
 # problems on multi-arch where people try to build
@@ -6344,6 +6352,11 @@ if test "$gtk" = "yes" ; then
     echo "CONFIG_GTK_GL=y" >> $config_host_mak
   fi
 fi
+if test "$gio" = "yes" ; then
+    echo "CONFIG_GIO=y" >> $config_host_mak
+    echo "GIO_CFLAGS=$gio_cflags" >> $config_host_mak
+    echo "GIO_LIBS=$gio_libs" >> $config_host_mak
+fi
 echo "CONFIG_TLS_PRIORITY=\"$tls_priority\"" >> $config_host_mak
 if test "$gnutls" = "yes" ; then
   echo "CONFIG_GNUTLS=y" >> $config_host_mak
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Marc-André Lureau]

Add a new display backend that will configure Spice to allow a remote
client to control QEMU in a similar fashion as other display backend
like GTK.

For this to work, we set up Spice server with a unix socket, and
register a VC chardev that will be exposed as Spice ports. A QMP
monitor is also exposed as a Spice port, this allows the remote client
fuller qemu control and state handling.

- doesn't handle VC set_echo() - this doesn't seem a strong
  requirement, very few front-end use it
- spice options can be tweaked with other -spice arguments
- Windows support shouldn't be hard to do, but will probably use a TCP
  port instead
- we may want to watch the child process to quit automatically if it
  crashed

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qapi/ui.json     |   3 +-
 ui/app.c         | 179 +++++++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx  |   5 ++
 ui/Makefile.objs |   5 ++
 4 files changed, 191 insertions(+), 1 deletion(-)
 create mode 100644 ui/app.c

diff --git a/qapi/ui.json b/qapi/ui.json
index 4ca91bb45a..9b96f1f9d7 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1057,7 +1057,8 @@
 ##
 { 'enum'    : 'DisplayType',
   'data'    : [ 'default', 'none', 'gtk', 'sdl',
-                'egl-headless', 'curses', 'cocoa' ] }
+                'egl-headless', 'curses', 'cocoa',
+                'app'] }
 
 ##
 # @DisplayOptions:
diff --git a/ui/app.c b/ui/app.c
new file mode 100644
index 0000000000..d29f630a70
--- /dev/null
+++ b/ui/app.c
@@ -0,0 +1,179 @@
+/*
+ * QEMU external app display driver
+ *
+ * Copyright (c) 2018 Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+
+#include <gio/gio.h>
+
+#include "qemu-common.h"
+#include "ui/console.h"
+#include "qemu/config-file.h"
+#include "qemu/option.h"
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+#include "io/channel-command.h"
+#include "chardev/spice.h"
+#include "sysemu/sysemu.h"
+
+static char *tmp_dir;
+static char *sock_path;
+
+typedef struct VCChardev {
+    SpiceChardev parent;
+} VCChardev;
+
+#define TYPE_CHARDEV_VC "chardev-vc"
+#define VC_CHARDEV(obj) OBJECT_CHECK(VCChardev, (obj), TYPE_CHARDEV_VC)
+
+static ChardevBackend *
+chr_spice_backend_new(void)
+{
+    ChardevBackend *be = g_new0(ChardevBackend, 1);
+
+    be->type = CHARDEV_BACKEND_KIND_SPICEPORT;
+    be->u.spiceport.data = g_new0(ChardevSpicePort, 1);
+
+    return be;
+}
+
+static void vc_chr_open(Chardev *chr,
+                        ChardevBackend *backend,
+                        bool *be_opened,
+                        Error **errp)
+{
+    ChardevBackend *be;
+    const char *fqdn = NULL;
+
+    if (strstart(chr->label, "serial", NULL)) {
+        fqdn = "org.qemu.console.serial.0";
+    } else if (strstart(chr->label, "parallel", NULL)) {
+        fqdn = "org.qemu.console.parallel.0";
+    } else if (strstart(chr->label, "compat_monitor", NULL)) {
+        fqdn = "org.qemu.monitor.hmp.0";
+    }
+
+    be = chr_spice_backend_new();
+    be->u.spiceport.data->fqdn = fqdn ?
+        g_strdup(fqdn) : g_strdup_printf("org.qemu.console.%s", chr->label);
+    qemu_chr_open_spice_port(chr, be, be_opened, errp);
+    qapi_free_ChardevBackend(be);
+}
+
+static void vc_chr_set_echo(Chardev *chr, bool echo)
+{
+    /* TODO: set echo for frontends QMP and qtest */
+}
+
+static void char_vc_class_init(ObjectClass *oc, void *data)
+{
+    ChardevClass *cc = CHARDEV_CLASS(oc);
+
+    cc->parse = qemu_chr_parse_vc;
+    cc->open = vc_chr_open;
+    cc->chr_set_echo = vc_chr_set_echo;
+}
+
+static const TypeInfo char_vc_type_info = {
+    .name = TYPE_CHARDEV_VC,
+    .parent = TYPE_CHARDEV_SPICEPORT,
+    .instance_size = sizeof(VCChardev),
+    .class_init = char_vc_class_init,
+};
+
+static void app_atexit(void)
+{
+    if (sock_path) {
+        unlink(sock_path);
+    }
+    if (tmp_dir) {
+        rmdir(tmp_dir);
+    }
+    g_free(sock_path);
+    g_free(tmp_dir);
+}
+
+static void app_display_early_init(DisplayOptions *opts)
+{
+    QemuOpts *qopts;
+    ChardevBackend *be = chr_spice_backend_new();
+    GError *err = NULL;
+
+    atexit(app_atexit);
+    tmp_dir = g_dir_make_tmp(NULL, &err);
+    if (err) {
+        error_report("Failed to create temporary directory: %s", err->message);
+        abort();
+    }
+
+    type_register(&char_vc_type_info);
+
+    sock_path = g_strjoin("", tmp_dir, "/", "spice.sock", NULL);
+    qopts = qemu_opts_create(qemu_find_opts("spice"), NULL, 0, &error_abort);
+    qemu_opt_set(qopts, "disable-ticketing", "on", &error_abort);
+    qemu_opt_set(qopts, "unix", "on", &error_abort);
+    qemu_opt_set(qopts, "addr", sock_path, &error_abort);
+    qemu_opt_set(qopts, "image-compression", "off", &error_abort);
+    qemu_opt_set(qopts, "streaming-video", "off", &error_abort);
+    qemu_opt_set(qopts, "gl", opts->has_gl ? "on" : "off", &error_abort);
+    display_opengl = opts->has_gl;
+
+    be->u.spiceport.data->fqdn = g_strdup("org.qemu.monitor.qmp.0");
+    qemu_chardev_new("org.qemu.monitor.qmp", TYPE_CHARDEV_SPICEPORT,
+                     be, &error_abort);
+    qopts = qemu_opts_create(qemu_find_opts("mon"),
+                             NULL, 0, &error_fatal);
+    qemu_opt_set(qopts, "chardev", "org.qemu.monitor.qmp", &error_abort);
+    qemu_opt_set(qopts, "mode", "control", &error_abort);
+
+    if (!qemu_name) {
+        qemu_name = "QEMU";
+    }
+    qapi_free_ChardevBackend(be);
+}
+
+static void app_display_init(DisplayState *ds, DisplayOptions *opts)
+{
+    GError *err = NULL;
+    gchar *uri;
+
+    uri = g_strjoin("", "spice+unix://", tmp_dir, "/", "spice.sock", NULL);
+    g_app_info_launch_default_for_uri(uri, NULL, &err);
+    if (err) {
+        error_report("Failed to launch %s URI: %s", uri, err->message);
+        exit(1);
+    }
+    g_free(uri);
+}
+
+static QemuDisplay qemu_display_app = {
+    .type       = DISPLAY_TYPE_APP,
+    .early_init = app_display_early_init,
+    .init       = app_display_init,
+};
+
+static void register_app(void)
+{
+    qemu_display_register(&qemu_display_app);
+}
+
+type_init(register_app);
diff --git a/qemu-options.hx b/qemu-options.hx
index b1bf0f485f..846e0b5c65 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1239,6 +1239,7 @@ STEXI
 ETEXI
 
 DEF("display", HAS_ARG, QEMU_OPTION_display,
+    "-display app[,gl=on|off]\n"
     "-display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]\n"
     "            [,window_close=on|off][,gl=on|core|es|off]\n"
     "-display gtk[,grab_on_hover=on|off][,gl=on|off]|\n"
@@ -1286,6 +1287,10 @@ menus and other UI elements to configure and control the VM during
 runtime.
 @item vnc
 Start a VNC server on display <arg>
+@item app
+Start QEMU as a Spice server and launch the default Spice client
+application. The Spice server will redirect the serial consoles and
+QEMU monitors.
 @end table
 ETEXI
 
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 00f6976c30..11178fbe24 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -54,6 +54,11 @@ curses.mo-objs := curses.o
 curses.mo-cflags := $(CURSES_CFLAGS)
 curses.mo-libs := $(CURSES_LIBS)
 
+common-obj-$(call land,$(CONFIG_SPICE),$(CONFIG_GIO)) += app.mo
+app.mo-objs := app.o
+app.mo-cflags := $(GIO_CFLAGS)
+app.mo-libs := $(GIO_LIBS)
+
 common-obj-$(CONFIG_OPENGL) += shader.o
 common-obj-$(CONFIG_OPENGL) += console-gl.o
 common-obj-$(CONFIG_OPENGL) += egl-helpers.o
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Eric Blake]

On 08/03/2018 12:36 PM, Marc-André Lureau wrote:
> Add a new display backend that will configure Spice to allow a remote
> client to control QEMU in a similar fashion as other display backend
> like GTK.
> 
> For this to work, we set up Spice server with a unix socket, and
> register a VC chardev that will be exposed as Spice ports. A QMP
> monitor is also exposed as a Spice port, this allows the remote client
> fuller qemu control and state handling.
> 
> - doesn't handle VC set_echo() - this doesn't seem a strong
>    requirement, very few front-end use it
> - spice options can be tweaked with other -spice arguments
> - Windows support shouldn't be hard to do, but will probably use a TCP
>    port instead
> - we may want to watch the child process to quit automatically if it
>    crashed
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   qapi/ui.json     |   3 +-
>   ui/app.c         | 179 +++++++++++++++++++++++++++++++++++++++++++++++
>   qemu-options.hx  |   5 ++
>   ui/Makefile.objs |   5 ++
>   4 files changed, 191 insertions(+), 1 deletion(-)
>   create mode 100644 ui/app.c
> 
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 4ca91bb45a..9b96f1f9d7 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1057,7 +1057,8 @@
>   ##
>   { 'enum'    : 'DisplayType',
>     'data'    : [ 'default', 'none', 'gtk', 'sdl',
> -                'egl-headless', 'curses', 'cocoa' ] }
> +                'egl-headless', 'curses', 'cocoa',
> +                'app'] }

Missing documentation for 'app' (at a minimum, it should mention since 3.1)
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Daniel P. Berrangé]

On Fri, Aug 03, 2018 at 07:36:14PM +0200, Marc-André Lureau wrote:
> Add a new display backend that will configure Spice to allow a remote
> client to control QEMU in a similar fashion as other display backend
> like GTK.
> 
> For this to work, we set up Spice server with a unix socket, and
> register a VC chardev that will be exposed as Spice ports. A QMP
> monitor is also exposed as a Spice port, this allows the remote client
> fuller qemu control and state handling.
> 
> - doesn't handle VC set_echo() - this doesn't seem a strong
>   requirement, very few front-end use it
> - spice options can be tweaked with other -spice arguments
> - Windows support shouldn't be hard to do, but will probably use a TCP
>   port instead
> - we may want to watch the child process to quit automatically if it
>   crashed
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> +    type_register(&char_vc_type_info);
> +
> +    sock_path = g_strjoin("", tmp_dir, "/", "spice.sock", NULL);
> +    qopts = qemu_opts_create(qemu_find_opts("spice"), NULL, 0, &error_abort);
> +    qemu_opt_set(qopts, "disable-ticketing", "on", &error_abort);
> +    qemu_opt_set(qopts, "unix", "on", &error_abort);
> +    qemu_opt_set(qopts, "addr", sock_path, &error_abort);
> +    qemu_opt_set(qopts, "image-compression", "off", &error_abort);
> +    qemu_opt_set(qopts, "streaming-video", "off", &error_abort);
> +    qemu_opt_set(qopts, "gl", opts->has_gl ? "on" : "off", &error_abort);

Hmm, so ultimately "-display app" is just syntactic sugar for a hardcoded
set of "-display spice" arguments, plus automatic launching of the client
app.  I'm thinking users may well ask for ability to control some of those
spice arguments over time.  So if we want auto-launching of a remote app,
I think it is preferrable to do it via extra args to the existing
"-display spice" format. eg we could add a "client=yes|no" to control
launching the client

   -display spice,client=yes

would do what your "-display app" proposes, and still let users tweak
all the other spice args as desired.  Another option "autoquit=yes|no"
could control whether QEMU automatically exits when the client quits,
vs whether it stays alive.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Marc-André Lureau]

Hi

On Tue, Aug 7, 2018 at 12:15 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Fri, Aug 03, 2018 at 07:36:14PM +0200, Marc-André Lureau wrote:
>> Add a new display backend that will configure Spice to allow a remote
>> client to control QEMU in a similar fashion as other display backend
>> like GTK.
>>
>> For this to work, we set up Spice server with a unix socket, and
>> register a VC chardev that will be exposed as Spice ports. A QMP
>> monitor is also exposed as a Spice port, this allows the remote client
>> fuller qemu control and state handling.
>>
>> - doesn't handle VC set_echo() - this doesn't seem a strong
>>   requirement, very few front-end use it
>> - spice options can be tweaked with other -spice arguments
>> - Windows support shouldn't be hard to do, but will probably use a TCP
>>   port instead
>> - we may want to watch the child process to quit automatically if it
>>   crashed
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>> +    type_register(&char_vc_type_info);
>> +
>> +    sock_path = g_strjoin("", tmp_dir, "/", "spice.sock", NULL);
>> +    qopts = qemu_opts_create(qemu_find_opts("spice"), NULL, 0, &error_abort);
>> +    qemu_opt_set(qopts, "disable-ticketing", "on", &error_abort);
>> +    qemu_opt_set(qopts, "unix", "on", &error_abort);
>> +    qemu_opt_set(qopts, "addr", sock_path, &error_abort);
>> +    qemu_opt_set(qopts, "image-compression", "off", &error_abort);
>> +    qemu_opt_set(qopts, "streaming-video", "off", &error_abort);
>> +    qemu_opt_set(qopts, "gl", opts->has_gl ? "on" : "off", &error_abort);
>
> Hmm, so ultimately "-display app" is just syntactic sugar for a hardcoded
> set of "-display spice" arguments, plus automatic launching of the client
> app.  I'm thinking users may well ask for ability to control some of those

You can do it via additional -spice arguments.

> spice arguments over time.  So if we want auto-launching of a remote app,
> I think it is preferrable to do it via extra args to the existing
> "-display spice" format. eg we could add a "client=yes|no" to control
> launching the client
>
>    -display spice,client=yes

There is no -display spice, atm.

However there is a -display vnc.

It's a bit unclear to me the relation between -display and
-vnc/-spice/-curses etc. In the end, I tend to think of -display foo
as a shortcut for a longer -foo configuration.

So -display spice,client=yes is a reasonable proposal to me, making it
clear that it will run spice. (client=yes is less clear to me but
fine)

I wanted a simple and short replacement for "gtk", "app" was generic
enough, and could be also further tweaked if needed (specify vnc,
spice, auto-quit etc)


>
> would do what your "-display app" proposes, and still let users tweak
> all the other spice args as desired.  Another option "autoquit=yes|no"
> could control whether QEMU automatically exits when the client quits,
> vs whether it stays alive.

Yes, I haven't looked at that in details, but we could either have
extra URI args, use a .vv file with extra config keys, use a dedicated
spice message to tell the client, or have a global client config. Too
many options to chose from :)

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Gerd Hoffmann]

  Hi,

> > spice arguments over time.  So if we want auto-launching of a remote app,
> > I think it is preferrable to do it via extra args to the existing
> > "-display spice" format. eg we could add a "client=yes|no" to control
> > launching the client
> >
> >    -display spice,client=yes
> 
> There is no -display spice, atm.
> 
> However there is a -display vnc.

That should not be there.  Now that we have a deprecation process
I should probably actually deprecate it in favor of -vnc.

> It's a bit unclear to me the relation between -display and
> -vnc/-spice/-curses etc. In the end, I tend to think of -display foo
> as a shortcut for a longer -foo configuration.

-display is for builtin UIs.  You can have exactly one of these.

-spice and -vnc is for remote protocols.  They can be used together with
builtin UIs (even though that isn't a typical use case).  Configuring
both spice and vnc works too.

-sdl and -curses are shortcuts for -display sdl and -display curses.

> So -display spice,client=yes is a reasonable proposal to me, making it
> clear that it will run spice. (client=yes is less clear to me but
> fine)

Hmm, this will both configure some standard stuff and start an external
application.  Doesn't really fit with "-spice ...".

Adding "-display remote-client" doesn't really fit either.  But still
looks better to me.  Or we just create a new -remote-client top level
switch.  Adding higher-level config options (protocol=spice/vnc,
monitor=on/off, serial=on/off, ...) is less confusing then (especially
vnc support :) ), compared to have a bunch of more -spice options which
only have an effect with client=yes.

Also:  remote-viewer accepts config files.  I'd suggest to write one, so
it is easy to restart remote-viewer.  Also I would not use a temp dir
for the files and sockets, but some fixed location.
/run/user/$uid/qemu/$vmname for example (where $vmname is whatever you
passed to qemu using -name).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Paolo Bonzini]

On 07/08/2018 16:30, Gerd Hoffmann wrote:
>>>
>>>    -display spice,client=yes
>> There is no -display spice, atm.
>>
>> However there is a -display vnc.
> That should not be there.  Now that we have a deprecation process
> I should probably actually deprecate it in favor of -vnc.
> 
>> It's a bit unclear to me the relation between -display and
>> -vnc/-spice/-curses etc. In the end, I tend to think of -display foo
>> as a shortcut for a longer -foo configuration.
> -display is for builtin UIs.  You can have exactly one of these.
> 
> -spice and -vnc is for remote protocols.  They can be used together with
> builtin UIs (even though that isn't a typical use case).  Configuring
> both spice and vnc works too.
> 
> -sdl and -curses are shortcuts for -display sdl and -display curses.
> 

Why not allow multiple -display options, but give an error if more than
one builtin UI is specified?

Paolo

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Marc-André Lureau]

Hi

On Tue, Aug 7, 2018 at 6:30 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > spice arguments over time.  So if we want auto-launching of a remote app,
> > > I think it is preferrable to do it via extra args to the existing
> > > "-display spice" format. eg we could add a "client=yes|no" to control
> > > launching the client
> > >
> > >    -display spice,client=yes
> >
> > There is no -display spice, atm.
> >
> > However there is a -display vnc.
>
> That should not be there.  Now that we have a deprecation process
> I should probably actually deprecate it in favor of -vnc.
>
> > It's a bit unclear to me the relation between -display and
> > -vnc/-spice/-curses etc. In the end, I tend to think of -display foo
> > as a shortcut for a longer -foo configuration.
>
> -display is for builtin UIs.  You can have exactly one of these.
>
> -spice and -vnc is for remote protocols.  They can be used together with
> builtin UIs (even though that isn't a typical use case).  Configuring
> both spice and vnc works too.
>
> -sdl and -curses are shortcuts for -display sdl and -display curses.
>
> > So -display spice,client=yes is a reasonable proposal to me, making it
> > clear that it will run spice. (client=yes is less clear to me but
> > fine)
>
> Hmm, this will both configure some standard stuff and start an external
> application.  Doesn't really fit with "-spice ...".
>
> Adding "-display remote-client" doesn't really fit either.  But still

I would rather keep the shorter "-display app" version. If you feel
strongly about "remote-client", I'll rename it.

> looks better to me.  Or we just create a new -remote-client top level
> switch.  Adding higher-level config options (protocol=spice/vnc,
> monitor=on/off, serial=on/off, ...) is less confusing then (especially
> vnc support :) ), compared to have a bunch of more -spice options which
> only have an effect with client=yes.
>
> Also:  remote-viewer accepts config files.  I'd suggest to write one, so
> it is easy to restart remote-viewer.  Also I would not use a temp dir

Where should it be written? Should the location be printed on stdout,
or given by the user, on command line? What should be the content of
the .vv file? Should it use a template to avoid proxying all the
options?

I think this can be considered for a later improvement.

> for the files and sockets, but some fixed location.
> /run/user/$uid/qemu/$vmname for example (where $vmname is whatever you
> passed to qemu using -name).

That's easy enough, although if no -name is given, I suppose a
temporary location is still better than a fixed location. I'll update
the patch.

thanks



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Gerd Hoffmann]

  Hi,

> > Also:  remote-viewer accepts config files.  I'd suggest to write one, so
> > it is easy to restart remote-viewer.  Also I would not use a temp dir
> 
> Where should it be written?

/run/user/$uid/qemu/$vmname/remote-viewer.vv ?

> What should be the content of
> the .vv file?

Everything needed, so launching remote-viewer is
just "remote-viewer /path/to/config.vv".

We might consider writing such a config file
unconditionally, even without -display app.

> Should it use a template to avoid proxying all the
> options?

Is this needed?  virt-viewer has a user settings file,
shouldn't user preferences go there instead?

> > for the files and sockets, but some fixed location.
> > /run/user/$uid/qemu/$vmname for example (where $vmname is whatever you
> > passed to qemu using -name).
> 
> That's easy enough, although if no -name is given, I suppose a
> temporary location is still better than a fixed location.

Yes, just replace $vmname with `mktemp --directory` then.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Marc-André Lureau]

On Wed, Dec 19, 2018 at 11:13 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > Also:  remote-viewer accepts config files.  I'd suggest to write one, so
> > > it is easy to restart remote-viewer.  Also I would not use a temp dir
> >
> > Where should it be written?
>
> /run/user/$uid/qemu/$vmname/remote-viewer.vv ?
>
> > What should be the content of
> > the .vv file?
>
> Everything needed, so launching remote-viewer is
> just "remote-viewer /path/to/config.vv".

If it's just what is needed to launch the viewer, why not stick to the
url syntax?

.vv files are quite specific to virt-viewer/remote-viewer.

>
> We might consider writing such a config file
> unconditionally, even without -display app.
>
> > Should it use a template to avoid proxying all the
> > options?
>
> Is this needed?  virt-viewer has a user settings file,
> shouldn't user preferences go there instead?

User settings files are quite limited at this point. Afaik, it's only
used for the "ask-quit" preference, and "monitor-mapping". Other
options are either from command line or .vv files.

>
> > > for the files and sockets, but some fixed location.
> > > /run/user/$uid/qemu/$vmname for example (where $vmname is whatever you
> > > passed to qemu using -name).
> >
> > That's easy enough, although if no -name is given, I suppose a
> > temporary location is still better than a fixed location.
>
> Yes, just replace $vmname with `mktemp --directory` then.
>
> cheers,
>   Gerd
>

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Gerd Hoffmann]

On Wed, Dec 19, 2018 at 11:44:07AM +0400, Marc-André Lureau wrote:
> On Wed, Dec 19, 2018 at 11:13 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > > Also:  remote-viewer accepts config files.  I'd suggest to write one, so
> > > > it is easy to restart remote-viewer.  Also I would not use a temp dir
> > >
> > > Where should it be written?
> >
> > /run/user/$uid/qemu/$vmname/remote-viewer.vv ?
> >
> > > What should be the content of
> > > the .vv file?
> >
> > Everything needed, so launching remote-viewer is
> > just "remote-viewer /path/to/config.vv".
> 
> If it's just what is needed to launch the viewer, why not stick to the
> url syntax?

First, the info needed to launch is located on a fixed & well known
place on disk then, so it is easy to restart remote-viewer.

Second, with a vv file you can do a bit more, for example setting "ca"
in case TLS is configured, or set enable-{smartcard,usbredir} depending
on guest configuration.

> .vv files are quite specific to virt-viewer/remote-viewer.

The same is true for the URL syntax.  Trying to pass a URL to vncviewer
doesn't work ...

> > > Should it use a template to avoid proxying all the
> > > options?
> >
> > Is this needed?  virt-viewer has a user settings file,
> > shouldn't user preferences go there instead?
> 
> User settings files are quite limited at this point. Afaik, it's only
> used for the "ask-quit" preference, and "monitor-mapping". Other
> options are either from command line or .vv files.

Ok.

I think it is more useful to extend that instead of passing through
options from the qemu command line to virt-viewer.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Marc-André Lureau]

Hi

On Wed, Dec 19, 2018 at 1:54 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Dec 19, 2018 at 11:44:07AM +0400, Marc-André Lureau wrote:
> > On Wed, Dec 19, 2018 at 11:13 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >   Hi,
> > >
> > > > > Also:  remote-viewer accepts config files.  I'd suggest to write one, so
> > > > > it is easy to restart remote-viewer.  Also I would not use a temp dir
> > > >
> > > > Where should it be written?
> > >
> > > /run/user/$uid/qemu/$vmname/remote-viewer.vv ?
> > >
> > > > What should be the content of
> > > > the .vv file?
> > >
> > > Everything needed, so launching remote-viewer is
> > > just "remote-viewer /path/to/config.vv".
> >
> > If it's just what is needed to launch the viewer, why not stick to the
> > url syntax?
>
> First, the info needed to launch is located on a fixed & well known
> place on disk then, so it is easy to restart remote-viewer.

It would also be a fixed URI, if we use the spice+unix:// syntax.

Fwiw, yes it's not registered in IANA/IETF, or with a RFC, but it is
considered "standard" withing the spice project. So we could formally
document it, as part of the protocol perhaps?, if it helps.

>
> Second, with a vv file you can do a bit more, for example setting "ca"
> in case TLS is configured, or set enable-{smartcard,usbredir} depending
> on guest configuration.
>
> > .vv files are quite specific to virt-viewer/remote-viewer.
>
> The same is true for the URL syntax.  Trying to pass a URL to vncviewer
> doesn't work ...

For vnc://
https://tools.ietf.org/html/rfc7869

(I recall some docs abourt rdp:// as well)

>
> > > > Should it use a template to avoid proxying all the
> > > > options?
> > >
> > > Is this needed?  virt-viewer has a user settings file,
> > > shouldn't user preferences go there instead?
> >
> > User settings files are quite limited at this point. Afaik, it's only
> > used for the "ask-quit" preference, and "monitor-mapping". Other
> > options are either from command line or .vv files.
>
> Ok.
>
> I think it is more useful to extend that instead of passing through
> options from the qemu command line to virt-viewer.

Hmm, although it limits what you can easily configure per instance as well..

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 10/10] display: add -display app launching external application

[Gerd Hoffmann]

> > > > Everything needed, so launching remote-viewer is
> > > > just "remote-viewer /path/to/config.vv".
> > >
> > > If it's just what is needed to launch the viewer, why not stick to the
> > > url syntax?
> >
> > First, the info needed to launch is located on a fixed & well known
> > place on disk then, so it is easy to restart remote-viewer.
> 
> It would also be a fixed URI, if we use the spice+unix:// syntax.

Ah, good.  Fine with me then.

> > I think it is more useful to extend that instead of passing through
> > options from the qemu command line to virt-viewer.
> 
> Hmm, although it limits what you can easily configure per instance as well..

We might decide on a case-by-vase basis, which make sense as
per-instance and which not.  For example: Hotkeys is more a user
preference and I don't think we need them be configurable per-vm.
For fullscreen it is probably more useful to have that as vm-specific
option.

cheers,
  Gerd