[Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Christophe Fergeau]

Currently, virgl support has to go through a local unix socket, trying
to connect to a VM using -spice gl through spice://localhost:5900 will
only result in a black screen.
This commit errors out when the user tries to start a VM with both GL
support and a port/tls-port set.
This would fit better in spice-server, but currently QEMU does not call
into spice-server when parsing 'gl' on its command line, so we have to
do this check in QEMU instead.

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
---
 ui/spice-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 7987a4e..94f3236 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -844,6 +844,10 @@ void qemu_spice_init(void)
 
 #ifdef HAVE_SPICE_GL
     if (qemu_opt_get_bool(opts, "gl", 0)) {
+        if ((port != 0) || (tls_port != 0)) {
+            error_report("SPICE GL support is local-only for now and incompatible with -spice port/tls-port");
+            exit(1);
+        }
         if (egl_rendernode_init() == 0) {
             display_opengl = 1;
         }
-- 
2.5.0

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Marc-André Lureau]

On Mon, Mar 14, 2016 at 12:41 PM, Christophe Fergeau
<cfergeau@redhat.com> wrote:
> Currently, virgl support has to go through a local unix socket, trying
> to connect to a VM using -spice gl through spice://localhost:5900 will
> only result in a black screen.
> This commit errors out when the user tries to start a VM with both GL
> support and a port/tls-port set.
> This would fit better in spice-server, but currently QEMU does not call
> into spice-server when parsing 'gl' on its command line, so we have to
> do this check in QEMU instead.
>
> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>

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


> ---
>  ui/spice-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 7987a4e..94f3236 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -844,6 +844,10 @@ void qemu_spice_init(void)
>
>  #ifdef HAVE_SPICE_GL
>      if (qemu_opt_get_bool(opts, "gl", 0)) {
> +        if ((port != 0) || (tls_port != 0)) {
> +            error_report("SPICE GL support is local-only for now and incompatible with -spice port/tls-port");
> +            exit(1);
> +        }
>          if (egl_rendernode_init() == 0) {
>              display_opengl = 1;
>          }
> --
> 2.5.0
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Gerd Hoffmann]

On Mo, 2016-03-14 at 12:41 +0100, Christophe Fergeau wrote:
> Currently, virgl support has to go through a local unix socket, trying
> to connect to a VM using -spice gl through spice://localhost:5900 will
> only result in a black screen.
> This commit errors out when the user tries to start a VM with both GL
> support and a port/tls-port set.
> This would fit better in spice-server, but currently QEMU does not call
> into spice-server when parsing 'gl' on its command line, so we have to
> do this check in QEMU instead.

Hmm.  It's something which we want support long-term though, by encoding
those dma-bufs as video stream and send them off over tcp.

I don't think this is a good idea long-term.

And even as temporary stopgap:  Can libvirt + virt-manager handle this?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Christophe Fergeau]

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

Hi,

On Mon, Mar 14, 2016 at 04:00:11PM +0100, Gerd Hoffmann wrote:
> On Mo, 2016-03-14 at 12:41 +0100, Christophe Fergeau wrote:
> > Currently, virgl support has to go through a local unix socket, trying
> > to connect to a VM using -spice gl through spice://localhost:5900 will
> > only result in a black screen.
> > This commit errors out when the user tries to start a VM with both GL
> > support and a port/tls-port set.
> > This would fit better in spice-server, but currently QEMU does not call
> > into spice-server when parsing 'gl' on its command line, so we have to
> > do this check in QEMU instead.
> 
> Hmm.  It's something which we want support long-term though, by encoding
> those dma-bufs as video stream and send them off over tcp.
> 
> I don't think this is a good idea long-term.

Yes, long-term we will want to remove this once spice gets support for
this. Currently however, this imo makes it a bit easier to understand
how everything is setup.
Otherwise I expect people are going to just take their existing SPICE
libvirt configuration listening on the network, add <gl enable="yes"/>
to it, try remote-viewer spice://localhost:5900 and wonder why they get
a black screen, and not know whether this is because their guest does
not support virgl, or their host, or because of some other issue, ...

Having this as a stopgap ensures that they at least be informed this is
not a valid usecase.

> And even as temporary stopgap:  Can libvirt + virt-manager handle this?

libvirt can, I haven't tried virt-manager (I believe all the patches
from Marc-André haven't been pushed yet)
<spice autoport="no"/> starts QEMU with -spice port=0

Christophe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Gerd Hoffmann]

  Hi,

> Yes, long-term we will want to remove this once spice gets support for
> this.

How can qemu figure whenever spice supports gl+tcp or not?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Christophe Fergeau]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On Tue, Mar 15, 2016 at 02:09:47PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > Yes, long-term we will want to remove this once spice gets support for
> > this.
> 
> How can qemu figure whenever spice supports gl+tcp or not?

gl support is already enabled through a spice-server version check

#if defined(CONFIG_OPENGL_DMABUF)
# if SPICE_SERVER_VERSION >= 0x000d01 /* release 0.13.1 */
#  define HAVE_SPICE_GL 1
#  include "ui/egl-helpers.h"
#  include "ui/egl-context.h"
# endif
#endif

We can do something similar once gl+tcp is available.

Christophe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Gerd Hoffmann]

On Di, 2016-03-15 at 15:17 +0100, Christophe Fergeau wrote:
> On Tue, Mar 15, 2016 at 02:09:47PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > Yes, long-term we will want to remove this once spice gets support for
> > > this.
> > 
> > How can qemu figure whenever spice supports gl+tcp or not?
> 
> gl support is already enabled through a spice-server version check
> 
> #if defined(CONFIG_OPENGL_DMABUF)
> # if SPICE_SERVER_VERSION >= 0x000d01 /* release 0.13.1 */
> #  define HAVE_SPICE_GL 1
> #  include "ui/egl-helpers.h"
> #  include "ui/egl-context.h"
> # endif
> #endif

Sure, qemu needed to figure whenever spice-server provides the gl API.

> We can do something similar once gl+tcp is available.

I don't expect adding gl+tcp support to spice needs changes in the
spice-server API and qemu.  So ifdef'ing on the spice-server version is
bogous, especially as the version you compiled qemu against can be newer
or older as the version used to run qemu.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Christophe Fergeau]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On Tue, Mar 15, 2016 at 03:32:31PM +0100, Gerd Hoffmann wrote:
> > We can do something similar once gl+tcp is available.
> 
> I don't expect adding gl+tcp support to spice needs changes in the
> spice-server API and qemu.  So ifdef'ing on the spice-server version is
> bogous,

Hmm, I expected some changes, at least wrt options if the user needs to
tweak the format of the video stream, hence the suggestion :)
Things get trickier then. Adding a runtime spice_get_version() would not
be that great either as the check would have to be updated with each
spice-server release.

I'd still like to have some failure when people try such configurations,
gathering all the pieces is complicated enough, better to let people
know when they try doing something that won't work.

Christophe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

On 03/14/2016 05:41 AM, Christophe Fergeau wrote:
> Currently, virgl support has to go through a local unix socket, trying
> to connect to a VM using -spice gl through spice://localhost:5900 will
> only result in a black screen.
> This commit errors out when the user tries to start a VM with both GL
> support and a port/tls-port set.
> This would fit better in spice-server, but currently QEMU does not call
> into spice-server when parsing 'gl' on its command line, so we have to
> do this check in QEMU instead.
> 
> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
> ---
>  ui/spice-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 7987a4e..94f3236 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -844,6 +844,10 @@ void qemu_spice_init(void)
>  
>  #ifdef HAVE_SPICE_GL
>      if (qemu_opt_get_bool(opts, "gl", 0)) {
> +        if ((port != 0) || (tls_port != 0)) {

Overparenthesized; you could write:

if (port || tls_port) {

for the same effect with less typing.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Christophe Fergeau]

[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]

On Mon, Mar 14, 2016 at 09:41:34AM -0600, Eric Blake wrote:
> On 03/14/2016 05:41 AM, Christophe Fergeau wrote:
> > Currently, virgl support has to go through a local unix socket, trying
> > to connect to a VM using -spice gl through spice://localhost:5900 will
> > only result in a black screen.
> > This commit errors out when the user tries to start a VM with both GL
> > support and a port/tls-port set.
> > This would fit better in spice-server, but currently QEMU does not call
> > into spice-server when parsing 'gl' on its command line, so we have to
> > do this check in QEMU instead.
> > 
> > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
> > ---
> >  ui/spice-core.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > index 7987a4e..94f3236 100644
> > --- a/ui/spice-core.c
> > +++ b/ui/spice-core.c
> > @@ -844,6 +844,10 @@ void qemu_spice_init(void)
> >  
> >  #ifdef HAVE_SPICE_GL
> >      if (qemu_opt_get_bool(opts, "gl", 0)) {
> > +        if ((port != 0) || (tls_port != 0)) {
> 
> Overparenthesized; you could write:
> 
> if (port || tls_port) {
> 
> for the same effect with less typing.

Yeah I know I'm overly verbose with these tests, the parentheses make it
explicit that there are no operator priority issues, the != 0 emphasize
it's an integer type which is being handled.

I'll change it to your recommendation before sending a v2.

Christophe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Gerd Hoffmann]

On Mo, 2016-03-14 at 12:41 +0100, Christophe Fergeau wrote:
> Currently, virgl support has to go through a local unix socket, trying
> to connect to a VM using -spice gl through spice://localhost:5900 will
> only result in a black screen.
> This commit errors out when the user tries to start a VM with both GL
> support and a port/tls-port set.
> This would fit better in spice-server, but currently QEMU does not call
> into spice-server when parsing 'gl' on its command line, so we have to
> do this check in QEMU instead.
> 
> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>

Picked up for ui patch queue.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH] spice: Disallow use of gl + TCP port

[Christophe Fergeau]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

On Fri, Mar 18, 2016 at 09:17:53AM +0100, Gerd Hoffmann wrote:
> On Mo, 2016-03-14 at 12:41 +0100, Christophe Fergeau wrote:
> > Currently, virgl support has to go through a local unix socket, trying
> > to connect to a VM using -spice gl through spice://localhost:5900 will
> > only result in a black screen.
> > This commit errors out when the user tries to start a VM with both GL
> > support and a port/tls-port set.
> > This would fit better in spice-server, but currently QEMU does not call
> > into spice-server when parsing 'gl' on its command line, so we have to
> > do this check in QEMU instead.
> > 
> > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
> 
> Picked up for ui patch queue.

Great thanks!

Fwiw, I've changed it locally to address Eric's comments, but did not
send it as v2 since a discussion was ongoing, here is what it looks in
my local clone now (but I'm fine with either version):


commit c0e10fb17a45a9da14d068a58af3a00e78f82403
Author: Christophe Fergeau <cfergeau@redhat.com>
Date:   Mon Mar 14 12:37:50 2016 +0100

    spice: Disallow use of gl + TCP port

    Currently, virgl support has to go through a local unix socket, trying
    to connect to a VM using -spice gl through spice://localhost:5900 will
    only result in a black screen.
    This commit errors out when the user tries to start a VM with both GL
    support and a port/tls-port set.
    This would fit better in spice-server, but currently QEMU does not call
    into spice-server when parsing 'gl' on its command line, so we have to
    do this check in QEMU instead.

    Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 7987a4e..83950fe 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -844,6 +844,10 @@ void qemu_spice_init(void)

 #ifdef HAVE_SPICE_GL
     if (qemu_opt_get_bool(opts, "gl", 0)) {
+        if (port || tls_port) {
+            error_report("SPICE GL support is local-only for now and incompatible with -spice port/tls-port");
+            exit(1);
+        }
         if (egl_rendernode_init() == 0) {
             display_opengl = 1;
         }


Christophe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]