Re: [libvirt] [PATCH v3 05/14] graphics: move port definition to listen element

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(a)redhat.com&gt; 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

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

- 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

- 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 > &lt;devices&gt; > &lt;graphics type='sdl' display=':0.0'/&gt; > &lt;graphics type='vnc' port='5904' sharePolicy='allow-exclusive'&gt; > - &lt;listen type='address' address='1.2.3.4'/&gt; > + &lt;listen type='address' address='1.2.3.4' port='5904'/&gt; > &lt;/graphics&gt; > &lt;graphics type='rdp' autoport='yes' multiUser='yes' /&gt; > &lt;graphics type='desktop' fullscreen='yes'/&gt; > @@ -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> > &lt;graphics type='spice' port='-1' tlsPort='-1' autoport='yes'&gt; > + &lt;listen type='address' autoport='yes'/&gt; > &lt;channel name='main' mode='secure'/&gt; > &lt;channel name='record' mode='insecure'/&gt; > &lt;image compression='auto_glz'/&gt; > @@ -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