[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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
> >    &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

So I'll prepare another version and we'll see.

Thanks,

Pavel


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]