On Wed, Aug 30, 2017 at 03:38:13PM +0200, Michal Privoznik wrote: > 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@xxxxxxxxxx> > >>> --- > >>> 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. The thing is that this also formats <reconnect enabled='yes' timeout='10'/> for "bind" mode which is wrong and needs to be fixed. BTW: this is same workaround as for the qemu command line format code. Pavel
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
