[virt-tools-list] [PATCH 3/5] virtinst: add listen=none graphics option

[Pavel Hrdina]

On Thu, Apr 28, 2016 at 10:14:27AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Thu, Apr 28, 2016 at 09:39:40AM -0400, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > ----- Original Message -----
> > > > On Thu, Apr 28, 2016 at 02:22:09PM +0200, Marc-André Lureau wrote:
> > > > > Add a special listen value to disable any extra display server
> > > > > listening
> > > > > socket. This is necessary now that qemu prevents starting a spice+virgl
> > > > > VM with listening sockets (until spice allows remoting with virgl).
> > > > > 
> > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > > > ---
> > > > >  man/virt-install.pod                                     |  9
> > > > >  +++++++--
> > > > >  tests/cli-test-xml/compare/virt-install-many-devices.xml |  4 ++++
> > > > >  tests/clitest.py                                         |  1 +
> > > > >  virtinst/cli.py                                          | 11
> > > > >  ++++++++++-
> > > > >  4 files changed, 22 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/man/virt-install.pod b/man/virt-install.pod
> > > > > index 0bc3d8c..0537693 100644
> > > > > --- a/man/virt-install.pod
> > > > > +++ b/man/virt-install.pod
> > > > > @@ -964,8 +964,13 @@ Specify the spice tlsport.
> > > > >  Address to listen on for VNC/Spice connections. Default is typically
> > > > >  127.0.0.1
> > > > >  (localhost only), but some hypervisors allow changing this globally
> > > > >  (for
> > > > >  example, the qemu driver default can be changed in
> > > > >  /etc/libvirt/qemu.conf).
> > > > > -Use 0.0.0.0 to allow access from other machines. This is use by 'vnc'
> > > > > and
> > > > > -'spice'
> > > > > +Use 0.0.0.0 to allow access from other machines.
> > > > > +
> > > > > +Use 'none' to specify that the display server should not listen on any
> > > > > +port. The display server can be accessed only locally through
> > > > > +libvirt unix socket (virt-viewer with --attach for instance).
> > > > > +
> > > > > +This is used by 'vnc' and 'spice'
> > > > >  
> > > > >  =item B<keymap>
> > > > >  
> > > > > diff --git a/tests/cli-test-xml/compare/virt-install-many-devices.xml
> > > > > b/tests/cli-test-xml/compare/virt-install-many-devices.xml
> > > > > index e771cd3..d268cd9 100644
> > > > > --- a/tests/cli-test-xml/compare/virt-install-many-devices.xml
> > > > > +++ b/tests/cli-test-xml/compare/virt-install-many-devices.xml
> > > > > @@ -214,6 +214,10 @@
> > > > >        <gl enable="yes"/>
> > > > >        <image compression="off"/>
> > > > >      </graphics>
> > > > > +    <graphics type="spice" autoport="no">
> > > > > +      <gl enable="yes"/>
> > > > > +      <image compression="off"/>
> > > > > +    </graphics>
> > > > 
> > > > NACK, please don't do this ... it's not intuitive and it may confuse
> > > > users.
> > > > This wasn't designed in libvirt to start a domain with spice to not
> > > > listen
> > > > anywhere.  Even though it works, we shouldn't use it.  I'm working on
> > > > patches,
> > > > that will introduce new <listen type='fd'/> which will tell qemu that
> > > > spice
> > > > shouldn't listen and that a client have to pass FD in order to connect.
> > > > I'll
> > > > also block this particular case in libvirt while starting a domain, that
> > > > no
> > > > port
> > > > was specified.  It's not even documented anywhere.
> > > 
> > > Isn't it simply a documentation issue then? I don't think listen type='fd'
> > > is more intuitive either. Also changing this now will break existing
> > > configuration relying on this behaviour.
> > 
> > The 'fd' is a working name, I'll probably change it to 'none' or something
> > else.
> > Well one shouldn't rely on anything that isn't mentioned anywhere and is base
> > on
> > testing or reviewing code.  Instead of erroring out I'll probably convert
> > this
> > case to <listen type='none'/> but you should wait with those patches.  I
> 
> Ok, please keep me in CC. Are you working on this for the next release? if not, I can help.
> 
> > don't
> > like similar configurations anywhere, where for some values it listens on
> > address but with one exception that if you set port=0 and autoport=no that it
> > won't listen anywhere (even if live XML has a listen=127.0.0.1).  It's
> > ambiguous
> > and like I said it may confuse users.
> > 
> > For that XML you are using in this patch live XML looks like this:
> > 
> > ...
> >     <graphics type='spice' autoport='no' listen='127.0.0.1'>
> >       <listen type='address' address='127.0.0.1/>
> >       <gl enable='yes'/>
> >       <image compression='off'/>
> >     </graphics>
> > ...
> > 
> > which basically means that spice should listen on address 127.0.0.1 but no
> > port
> > was specified so does it really listen anywhere or what?
> 
> Isn't that a live XML "bug" instead?

Well, the code for graphics has more that one bug.

> 
> > 
> > I'm working on making this clear and to always represent a current state in
> > the
> > XML so everyone that uses libvirt knows exactly what's the configuration.
> 
> Sounds good
> 
> > 
> > So in this case it should looks like this:
> > 
> > ...
> >     <graphics type='spice'>
> >       <listen type='none'/>
> >       <gl enable='yes'/>
> >       <image compression='off'/>
> >     </graphics>
> > ...
> 
> I like it, let me know how I can help. I hope we can have it in next libvirt release, since it's not possible to setup a spice+virgl VM anymore since qemu "Disallow use of gl + TCP port" now.

Sure, I'll add you to CC.  I'm finishing those patches and I'll send them to
libvirt in a few days.  Let's hope it will get in 1.3.5 release :).