The test was introduced by 60135b22db6d. The auto-generated path is removed by post-parse callback which also changes the mode from "connect" to "bind" since the auto-generated path makes sense only for "bind" mode. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.args | 6 ++---- tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml b/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml index e0664b2a95..41ee248db3 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml @@ -18,7 +18,7 @@ </source> </redirdev> <smartcard mode='passthrough' type='unix'> - <source mode='connect' path='/tmp/channel/domain-oldname/asdf'> + <source mode='connect' path='/tmp/channel/asdf'> <reconnect enabled='yes' timeout='20'/> </source> </smartcard> @@ -29,7 +29,7 @@ <target type='virtio' name='asdf'/> </channel> <channel type='unix'> - <source mode='connect' path='/tmp/channel/domain-oldname/fdsa'> + <source mode='connect' path='/tmp/channel/fdsa'> <reconnect enabled='no'/> </source> <target type='virtio' name='fdsa'/>

On Wed, Aug 30, 2017 at 02:33:00PM +0200, Michal Privoznik wrote: > On 08/30/2017 01:40 PM, Pavel Hrdina wrote: >> The test was introduced by 60135b22db6d. >> >> The auto-generated path is removed by post-parse callback which >> also changes the mode from "connect" to "bind" since the auto-generated >> path makes sense only for "bind" mode. >> >> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; >> --- >> tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.args | 6 ++---- >> tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml | 4 ++-- >> 2 files changed, 4 insertions(+), 6 deletions(-) >> > >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml b/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml >> index e0664b2a95..41ee248db3 100644 >> --- a/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.xml >> @@ -18,7 +18,7 @@ >> </source> >> </redirdev> >> <smartcard mode='passthrough' type='unix'> >> - <source mode='connect' path='/tmp/channel/domain-oldname/asdf'> >> + <source mode='connect' path='/tmp/channel/asdf'> >> <reconnect enabled='yes' timeout='20'/> >> </source> >> </smartcard> >> @@ -29,7 +29,7 @@ >> <target type='virtio' name='asdf'/> >> </channel> >> <channel type='unix'> >> - <source mode='connect' path='/tmp/channel/domain-oldname/fdsa'> >> + <source mode='connect' path='/tmp/channel/fdsa'> >> <reconnect enabled='no'/> >> </source> >> <target type='virtio' name='fdsa'/> >> > > This looks like it's fixing just a symptom not the cause. What if I have > a domain that has autogenerated path and I also set reconnect? If you try to define a domain with this configuration the post-parse callback removes the path, see qemuDomainChrDefDropDefaultPath(). When the guest is started there is another function, see qemuDomainPrepareChannel(), where we generate a path in one is missing and also changes the mode to "bind". So yes, this fixes the symptom but it's still a valid fix because the test XML is invalid, you should not use this generated path.

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

On 08/30/2017 02:54 PM, Pavel Hrdina wrote: > 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. Exactly. So what are we worried more about? Pushing this temporal workaround or having not nice looking, but still valid and sensible XML? Technically, mode='bind' and reconnect='no' is a valid combination.