[libvirt] [PATCH 4/5] conf: make sure that chardev reconnect is formatted only for connect mode
Pavel Hrdina
phrdina at redhat.com
Wed Aug 30 13:45:48 UTC 2017
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 at redhat.com>
> >>> ---
> >>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170830/992cf418/attachment-0001.sig>
More information about the libvir-list
mailing list