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@xxxxxxxxxx> > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list