Re: [libvirt] [PATCH 5/7] conf: spice: Do more automation if autoport is requested

On 04/24/13 10:19, Daniel P. Berrange wrote: >On Tue, Apr 23, 2013 at 08:57:10PM +0200, Peter Krempa wrote: >>On 04/23/13 18:21, Daniel P. Berrange wrote: >>>On Tue, Apr 23, 2013 at 03:46:12PM +0200, Peter Krempa wrote: >>>>With autoport enabled, both ports were alocated. With enabling >>>>defaultMode or setting separate channel modes one of the ports may not >>>>be needed. This will allow later on doing this kind of change. >>>>--- >>>> docs/formatdomain.html.in | 2 +- >>>> src/conf/domain_conf.c | 5 ----- >>>> 2 files changed, 1 insertion(+), 6 deletions(-) >>>> >>>>diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >>>>index 888c005..bb75943 100644 >>>>--- a/docs/formatdomain.html.in >>>>+++ b/docs/formatdomain.html.in >>>>@@ -3470,7 +3470,7 @@ qemu-kvm -net nic,model=? /dev/null >>>> while <code>tlsPort</code> gives an alternative secure >>>> port number. The <code>autoport</code> attribute is the >>>> new preferred syntax for indicating autoallocation of >>>>- both port numbers. The <code>listen</code> attribute is >>>>+ needed port numbers. The <code>listen</code> attribute is >>>> an IP address for the server to listen >>>> on. The <code>passwd</code> attribute provides a SPICE >>>> password in clear text. The <code>keymap</code> >>>>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >>>>index dc0ecaa..86a444c 100644 >>>>--- a/src/conf/domain_conf.c >>>>+++ b/src/conf/domain_conf.c >>>>@@ -7595,11 +7595,6 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, >>>> VIR_FREE(defaultMode); >>>> } >>>> >>>>- if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) { >>>>- /* Legacy compat syntax, used -1 for auto-port */ >>>>- def->data.spice.autoport = true; >>>>- } >>> >>>I'm not clear why this is safe. The idea is that if the user sends XML >>> >>> <graphics port='-1' tlsPort='-1'/> >>> >>>then libvirt would turn it into >>> >>> <graphics port='-1' tlsPort='-1' autoport='yes'/> >>> >>>with this removed, won't we be instead outputting >>> >>> <graphics port='-1' tlsPort='-1' autoport='no'/> >>> >>>despite the fact that it is auto-allocating the ports? >> >>Later on this will slightly change semantics: >> >><graphics port='-1' tlsPort='-1' autoport='no'/> >> >>Will allocate both ports every time, even if one isn't needed >>because of other configuration (eg defaultMode="insecure") > >That is certainly not right. > >If we're allocating ports then we *must* be setting autoport='yes'. >Having port='1' and tlsPort='-1' and autoport='no' is a non-sensical >configuration. Okay, that is fair enough. In that case, is it okay not to allocate both ports if the configuration doesn't require it even if we did so before? Or do we need to have an option to force allocation of both TLS and non-tls port even if it's not needed?