Re: [libvirt] [PATCH 4/5] conf: make sure that chardev reconnect is formatted only for connect mode

On Wed, Aug 30, 2017 at 02:32:50PM +0200, Michal Privoznik wrote: > On 08/30/2017 01:40 PM, Pavel Hrdina wrote: >> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; >> --- >> src/conf/domain_conf.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index f7574d77b6..7f443e5b4d 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -23257,8 +23257,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf, >> virBufferAsprintf(&attrBuf, " tlsFromConfig='%d'", >> def->data.tcp.tlsFromConfig); >> >> - virDomainChrSourceReconnectDefFormat(&childBuf, >> - &def->data.tcp.reconnect); >> + if (!def->data.tcp.listen) >> + virDomainChrSourceReconnectDefFormat(&childBuf, >> + &def->data.tcp.reconnect); >> >> if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) >> goto error; >> @@ -23276,8 +23277,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf, >> virDomainSourceDefFormatSeclabel(&childBuf, def->nseclabels, >> def->seclabels, flags); >> >> - virDomainChrSourceReconnectDefFormat(&childBuf, >> - &def->data.nix.reconnect); >> + if (!def->data.nix.listen) >> + virDomainChrSourceReconnectDefFormat(&childBuf, >> + &def->data.nix.reconnect); >> >> if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) >> goto error; >> > > This looks like a workaround. Because def->data.tcp.listen shouldn't be > set if reconnect is enabled and vice versa. And > virDomainChrSourceReconnectDefFormat() shortcuts out. Or you want the > following: > > <channel type='tcp'> > <source mode='bind' host='localhost' service='5678'> > <reconnect enabled='no'/> > </source> > <target type='virtio' name='test2'/> > </channel> > > to be turned into: > > <channel type='tcp'> > <source mode='bind' host='localhost' service='5678'/> > <target type='virtio' name='test2'/> > </channel> > > Michal Yes, it's kind of workaround for the case where you will set <channel type='unix'> <source mode='connect' path='/var/lib/libvirt/qemu/channel/target/domain-test/test2'> <reconnect enabled='no'/> </source> <target type='virtio' name='test2'/> </channel> Without this patch it would lead to: <channel type='unix'> <source mode='bind' path='/var/lib/libvirt/qemu/channel/target/domain-5-test/test2'> <reconnect enabled='no'/> </source> <target type='virtio' name='test2'/> </channel> because we remove the auto-generated path and while starting a guest we generate a new one and sets the mode to "bind". This patch makes sure that in this case the XML of live guest is correct. The proper fix would be to update _virDomainChrSourceDef by changing 'bool listen' into 'virDomainChrSourceModeType mode' and the parse would properly store three different values: default, connect, bind. This would require rewrite a lot of code which is not suitable for a freeze, therefore this workaround. I'm planning to fix it properly so that workaround is not required anymore.