[libvirt] [PATCH v2 06/12] graphics: introduce listen type=socket and use it for VNC

[Pavel Hrdina]

On Thu, May 12, 2016 at 01:06:41PM +0200, Christophe Fergeau wrote:
> Hey,
> 
> On Wed, May 11, 2016 at 05:08:25PM +0200, Pavel Hrdina wrote:
> > Introduce a new listen type that will be used to tell a graphics device
> > to listen on unix socket and use it for VNC graphics instead of socket
> > attribute.  The socket attribute will remain in the XML for backward
> > compatibility.
> > 
> > Since old libvirt supports 'socket' attribute inside 'graphics' element
> > for socket path provided by user libvirt will generate migratable XML
> > without that listen type='socket' but only with 'socket' attribute in
> > order to be able to migrate back to old libvirt.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > ---
> >  docs/formatdomain.html.in                          |  16 +++
> >  docs/schemas/domaincommon.rng                      |  10 ++
> >  src/conf/domain_conf.c                             | 119 ++++++++++++++++-----
> >  src/conf/domain_conf.h                             |   8 +-
> >  src/libvirt_private.syms                           |   1 +
> >  src/qemu/qemu_command.c                            |  45 ++++----
> >  src/qemu/qemu_domain.c                             |  19 ++--
> >  src/qemu/qemu_hotplug.c                            |   9 ++
> >  src/qemu/qemu_parse_command.c                      |   2 +-
> >  src/qemu/qemu_process.c                            |  47 ++++++--
> >  src/security/virt-aa-helper.c                      |  15 ++-
> >  .../generic-graphics-vnc-socket-listen.xml         |   4 +-
> >  .../generic-graphics-vnc-socket.xml                |   4 +-
> >  .../qemuargv2xml-graphics-vnc-socket.xml           |   4 +-
> >  .../qemuxml2argv-graphics-vnc-auto-socket.args     |  20 ++++
> >  .../qemuxml2argv-graphics-vnc-auto-socket.xml      |  30 ++++++
> >  .../qemuxml2argv-graphics-vnc-socket.args          |   4 +-
> >  .../qemuxml2argv-graphics-vnc-socket.xml           |  10 +-
> >  tests/qemuxml2argvtest.c                           |   2 +
> >  .../qemuxml2xmlout-graphics-vnc-auto-socket.xml    |  35 ++++++
> >  .../qemuxml2xmlout-graphics-vnc-autosocket.xml     |   4 +-
> >  .../qemuxml2xmlout-graphics-vnc-socket.xml         |  35 ++++++
> >  tests/qemuxml2xmltest.c                            |   2 +
> >  23 files changed, 361 insertions(+), 84 deletions(-)
> >  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-auto-socket.args
> >  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-auto-socket.xml
> >  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-auto-socket.xml
> >  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-socket.xml
> > 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index b0847b7..f67076d 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -5359,6 +5359,22 @@ qemu-kvm -net nic,model=? /dev/null
> >            <code>address</code>.
> >          </p>
> >        </dd>
> > +      <dt><code>socket</code> <span class="since">since 1.3.5</span></dt>
> > +      <dd>
> > +        <p>
> > +          This listen type tells a graphics server to listen on unix socket.
> > +          Attribute <code>socket</code> contains a path to unix socket. If this
> > +          attribute is omitted libvirt will generate this path for you.
> > +          Supported by graphics type <code>vnc</code>.
> > +        </p>
> > +        <p>
> > +          For <code>vnc</code> graphics be backward compatible
> > +          the <code>socket</code> attribute of first <code>listen</code> element
> > +          is duplicated as <code>socket</code> attribute in <code>graphics</code>
> > +          element. If <code>graphics</code> element contains a <code>socket</code>
> > +          attribute all <code>listen</code> elements are ignored.
> > +        </p>
> 
> 
> If both a socket attribute and a listen type="socket" node are present,
> shouldn't this check if they are using the same path? This is what is
> done for 'listen' attribute and listen type="address", but I could not
> find the same thing in this patch.

I know, that I had that code somewhere but I cannot remember why I didn't use
it.  I can probably add this check in to the code.  Right know I can think of
only one case, where it could break things: if some application uses socket and
tries to change it.  But in that case it's probably OK to print an error.

> 
> > +      </dd>
> >      </dl>
> >  
> >      <h4><a name="elementsVideo">Video devices</a></h4>
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> > index e7eda77..e3dbcc6 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -3014,6 +3014,16 @@
> >                </attribute>
> >              </optional>
> >            </group>
> > +          <group>
> > +            <attribute name="type">
> > +              <value>socket</value>
> > +            </attribute>
> > +            <optional>
> > +              <attribute name="socket">
> > +                <ref name="absFilePath"/>
> > +              </attribute>
> > +            </optional>
> > +          </group>
> 
> Imo this would be better as 
> <listen type="unix" socket="/some/path"/>
> 
> This would be more consistent with /disk/source/host
> vhost-user also uses type="unix" but with a path attribute rather than
> socket
> There is also <channel type="unix"> (again with 'path' rather than
> 'socket').

Yes, it would be better to use unix and path, but I've tired to follow the same
logic based on current listen types address and network.  I actually don't care
about the type name and the attribute name, so if someone else has any idea,
please share it :).

Thanks,

Pavel

> 
> Christophe