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

30 Aug 2017

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@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.

Michal

    