[libvirt] [PATCH v3 05/14] graphics: move port definition to listen element
Pavel Hrdina
phrdina at redhat.com
Tue May 17 10:27:31 UTC 2016
On Mon, May 16, 2016 at 01:57:13PM -0400, Cole Robinson wrote:
> On 05/12/2016 11:15 AM, Pavel Hrdina wrote:
> > So far we have only two listen types that supports only address:port
> > method, but in the future we may want to add a new different listen
> > type, for example socket.
> >
> > This patch moves the ports values out of graphics unions into listen
> > element. The domain XML will now duplicate the ports from first listen
> > element into the graphics element as we do also for address.
> >
> > This allows us to make part of the graphics code as listen-driven and
> > prepare the code for new listen types.
> >
> > To support migration back to older versions the new attributes from
> > listen elements are not written into migratable XML.
> >
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>
> Since we are breaking from the past, instead I suggest we do:
>
> <listen>
> <port type='XXX' autoport='on|off' value=XXXX/>
> <port .../>
> </listen>
>
> More future proof WRT future port additions, and gives us a natural place to
> fix the websocket autoport issue without adding a websocket_autoport=on|off
> parameter:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1235846
Ok, this probably makes sense and it also allow us to drop the legacy '-1'
syntax for autoport. I guess that we can go this way.
>
> This is a very large patch with several different aspects to it. It should
> probably be a series unto itself. This is off the cuff, but I suggest
> structuring the series like
Yes it's very large patch and my intention was to keep those changes together,
but I agree that splitting it into multiple patches would be easier for review.
I was thinking about this and decided to go with one large patch.
> - add port bits to listen structure, but no XML parsing/formatting. fill them
> with the pre-existing port attributes, maybe on demand at GetListen time
> - convert the code to grab port values from gListen. that way we can verify
> that the test output doesn't change. this patch should not be changing
> functionality AFAICT. code that actually updates the port values, like during
> port allocation, likely needs to continue to use the old values
> - make gListen values the canonical store, and convert the remaining users
>
> If you don't want to separate that series from the later listen=none/socket
> bits, if the conflict is too high, I imagine you can just do the above patches
> to make the code 'listen driven'. But then the extra patches would be
I'll see what I can do about this and how hard it would be.
>
> - parse/format the XML, regenerate the test suite
> - add new targeted test cases
> - docs
>
> But ideally the port XML rework doesn't block the listen=none/spice bits which
> are interesting for spice GL
>
> Some review on the doc bits below
>
> >
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index fd2dd33..10a9e8d 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -5090,7 +5090,7 @@ qemu-kvm -net nic,model=? /dev/null
> > <devices>
> > <graphics type='sdl' display=':0.0'/>
> > <graphics type='vnc' port='5904' sharePolicy='allow-exclusive'>
> > - <listen type='address' address='1.2.3.4'/>
> > + <listen type='address' address='1.2.3.4' port='5904'/>
> > </graphics>
> > <graphics type='rdp' autoport='yes' multiUser='yes' />
> > <graphics type='desktop' fullscreen='yes'/>
> > @@ -5122,16 +5122,13 @@ qemu-kvm -net nic,model=? /dev/null
> > <dt><code>vnc</code></dt>
> > <dd>
> > <p>
> > - Starts a VNC server. The <code>port</code> attribute specifies
> > - the TCP port number (with -1 as legacy syntax indicating that it
> > - should be auto-allocated). The <code>autoport</code> attribute is
> > - the new preferred syntax for indicating auto-allocation of the TCP
> > - port to use. The <code>passwd</code> attribute provides a VNC
> > - password in clear text. The <code>keymap</code> attribute specifies
> > - the keymap to use. It is possible to set a limit on the validity of
> > + Starts a VNC server. To set port or address use <code>listen</code>
>
> *'use the' or 'use a'
>
> > + element. The <code>passwd</code> attribute provides a VNC password
> > + in clear text. The <code>keymap</code> attribute specifies the
> > + keymap to use. It is possible to set a limit on the validity of
> > the password by giving an timestamp
> > - <code>passwdValidTo='2010-04-09T15:51:00'</code> assumed to be
> > - in UTC. The <code>connected</code> attribute allows control of
> > + <code>passwdValidTo='2010-04-09T15:51:00'</code> assumed to be in
> > + UTC. The <code>connected</code> attribute allows control of
> > connected client during password changes. VNC accepts
> > <code>keep</code> value only <span class="since">since 0.9.3</span>.
> > NB, this may not be supported by all hypervisors.
> > @@ -5148,26 +5145,16 @@ qemu-kvm -net nic,model=? /dev/null
> > unconditionally <span class="since">since 1.0.6</span>.
> > </p>
> > <p>
> > - Rather than using listen/port, QEMU supports a <code>socket</code>
> > + Rather than using listen element, QEMU supports a <code>socket</code>
>
> *'using the' or 'using a'
>
> > attribute for listening on a unix domain socket path
> > <span class="since">Since 0.8.8</span>.
> > </p>
> > - <p>
> > - For VNC WebSocket functionality, <code>websocket</code> attribute
> > - may be used to specify port to listen on (with -1 meaning
> > - auto-allocation and <code>autoport</code> having no effect due to
> > - security reasons) <span class="since">Since 1.0.6</span>.
>
> The 'Since 1.0.6' is lost for websocket.
>
> > - </p>
> > </dd>
> > <dt><code>spice</code> <span class="since">Since 0.8.6</span></dt>
> > <dd>
> > <p>
> > - Starts a SPICE server. The <code>port</code> attribute specifies
> > - the TCP port number (with -1 as legacy syntax indicating that it
> > - should be auto-allocated), while <code>tlsPort</code> gives
> > - an alternative secure port number. The <code>autoport</code>
> > - attribute is the new preferred syntax for indicating
> > - auto-allocation of needed port numbers. The <code>passwd</code>
> > + Starts a SPICE server. To set port or address use
> > + <code>listen</code> element. The <code>passwd</code>
> > attribute provides a SPICE password in clear text. The
> > <code>keymap</code> attribute specifies the keymap to use. It is
> > possible to set a limit on the validity of the password by giving
> > @@ -5209,6 +5196,7 @@ qemu-kvm -net nic,model=? /dev/null
> > </p>
> > <pre>
> > <graphics type='spice' port='-1' tlsPort='-1' autoport='yes'>
> > + <listen type='address' autoport='yes'/>
> > <channel name='main' mode='secure'/>
> > <channel name='record' mode='insecure'/>
> > <image compression='auto_glz'/>
> > @@ -5266,16 +5254,13 @@ qemu-kvm -net nic,model=? /dev/null
> > <dt><code>rdp</code></dt>
> > <dd>
> > <p>
> > - Starts a RDP server. The <code>port</code> attribute specifies the
> > - TCP port number (with -1 as legacy syntax indicating that it should
> > - be auto-allocated). The <code>autoport</code> attribute is the new
> > - preferred syntax for indicating auto-allocation of the TCP port to
> > - use. The <code>replaceUser</code> attribute is a boolean deciding
> > - whether multiple simultaneous connections to the VM are permitted.
> > - The <code>multiUser</code> attribute is a boolean deciding whether
> > - the existing connection must be dropped and a new connection must
> > - be established by the VRDP server, when a new client connects in
> > - single connection mode.
> > + Starts a RDP server. To set port or address use <code>listen</code>
> > + element. The <code>replaceUser</code> attribute is
> > + a boolean deciding whether multiple simultaneous connections to
> > + the VM are permitted. The <code>multiUser</code> attribute is
> > + a boolean deciding whether the existing connection must be dropped
> > + and a new connection must be established by the VRDP server, when
> > + a new client connects in single connection mode.
> > </p>
> > </dd>
> > <dt><code>desktop</code></dt>
> > @@ -5318,6 +5303,40 @@ qemu-kvm -net nic,model=? /dev/null
> > attribute in <code>graphics</code> element for backward compatibility.
> > If both are provided they must be equal.
> > </p>
> > + <p>
> > + With address it's also possible to specify ports to listen on and
>
> <code>address</code>
>
> > + whether those ports should be auto-generated or not
> > + <span class="since">Since 1.3.5</span>. Depending on graphics type
>
> Since capitalization is weird here. I think move the Since block to the start
> of the line.
>
> > + those attributes are available:
> > + </p>
> > + <ul>
> > + <li>
> > + <code>port</code> TCP port number (<code>vnc</code>,
> > + <code>spice</code>, <code>rdp</code>),
> > + </li>
> > + <li>
> > + <code>tlsPort</code> secure TCP port number (<code>spice</code>),
> > + </li>
> > + <li>
> > + <code>websocket</code> TCP port number (<code>vnc</code>),
> > + </li>
> > + <li>
> > + <code>autoport</code> TCP port number (<code>vnc</code>,
> > + <code>spice</code>, <code>rdp</code>).
> > + </li>
> > + </ul>
>
> I think consolidating the ports configuration is a good idea, maybe even give
> it its own subsection. This should probably be a <dd> list though which is
> more common.
>
> But I think the list items should describe when the original attribute was
> added, and a bit about what the attribute does. Then add the bit about 'since
> 1.3.5 specify these in the <listen> element...' should come afterwards. That
> way there's a natural place to preserve the websocket 'Since' bit, the
> Websocket warning, etc.
>
> > + <p>
> > + If <code>autoport='yes'</code> the <code>port</code> and
> > + <code>tlsPort</code> are auto-generated. It's also possible to use
> > + <code>-1</code> as legacy syntax to tell that the port should be
> > + auto-generated. The <code>websocket</code> has an exception for
> > + security reasons that it can be only auto-generated using the legacy
>
> s/be only/only be/g
>
> > + syntax <code>websocket='-1'</code>.
> > + </p>
> > + <p>
> > + This ports configuration is duplicated into <code>graphics</code>
> > + element for backwards compatibility.
> > + </p>
> > </dd>
> > <dt><code>network</code></dt>
> > <dd>
> > @@ -5335,6 +5354,10 @@ qemu-kvm -net nic,model=? /dev/null
> > describing one of the 'direct' (macvtap) modes, the first IPv4 address
> > of the first forward dev will be used.
> > </p>
> > + <p>
> > + Specifying ports uses the same rules and syntax as listen type
> > + <code>address</code>.
> > + </p>
> > </dd>
> > </dl>
> >
>
> Granted the docs will likely be different if the <port> format is used
So I'll prepare another version and we'll see.
Thanks,
Pavel
More information about the libvir-list
mailing list