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

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.