[libvirt] [PATCH 0/5] chardev source reconnect cleanup and fixes
Pavel Hrdina (5): tests: remove unused file conf: add reconnect to virDomainChrSourceDef(Copy|IsEqual) tests: don't use unix socket path that matches auto-generated path conf: make sure that chardev reconnect is formatted only for connect mode qemu: make sure that chardev reconnect is formatted only for connect mode src/conf/domain_conf.c | 24 ++++++++++++----- src/qemu/qemu_command.c | 8 +++--- .../qemuxml2argv-channel-reconnect.args | 31 ---------------------- .../qemuxml2argv-chardev-reconnect.args | 6 ++--- .../qemuxml2argv-chardev-reconnect.xml | 4 +-- 5 files changed, 26 insertions(+), 47 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args -- 2.13.5
Introduced by 95fd63b1700d. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- .../qemuxml2argv-channel-reconnect.args | 31 ---------------------- 1 file changed, 31 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args b/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args deleted file mode 100644 index 43a5d5bb3e..0000000000 --- a/tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args +++ /dev/null @@ -1,31 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/home/test \ -USER=test \ -LOGNAME=test \ -QEMU_AUDIO_DRV=none \ -/usr/bin/qemu-system-i686 \ --name QEMUGuest1 \ --S \ --M pc \ --m 214 \ --smp 1,sockets=1,cores=1,threads=1 \ --uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ --nographic \ --nodefconfig \ --nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ --mon chardev=charmonitor,id=monitor,mode=readline \ --no-acpi \ --boot c \ --device virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa \ --device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x3 \ --usb \ --chardev socket,id=charchannel0,host=localhost,port=1234,reconnect=10 \ --device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\ -id=channel0,name=asdf \ --chardev socket,id=charchannel1,path=/tmp/channel/domain--1-QEMUGuest1/fdsa,\ -server,nowait,reconnect=10 \ --device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,\ -id=channel1,name=fdsa -- 2.13.5
On 08/30/2017 01:40 PM, Pavel Hrdina wrote:
Introduced by 95fd63b1700d.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- .../qemuxml2argv-channel-reconnect.args | 31 ---------------------- 1 file changed, 31 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-reconnect.args
ACK and safe for the freeze. Michal
Missed by 9aa72a6dd5b3. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d97aab4834..f7574d77b6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2180,11 +2180,17 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, dest->data.tcp.haveTLS = src->data.tcp.haveTLS; dest->data.tcp.tlsFromConfig = src->data.tcp.tlsFromConfig; + + dest->data.tcp.reconnect.enabled = src->data.tcp.reconnect.enabled; + dest->data.tcp.reconnect.timeout = src->data.tcp.reconnect.timeout; break; case VIR_DOMAIN_CHR_TYPE_UNIX: if (VIR_STRDUP(dest->data.nix.path, src->data.nix.path) < 0) return -1; + + dest->data.nix.reconnect.enabled = src->data.nix.reconnect.enabled; + dest->data.nix.reconnect.timeout = src->data.nix.reconnect.timeout; break; case VIR_DOMAIN_CHR_TYPE_NMDM: @@ -2259,11 +2265,15 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, return src->data.tcp.listen == tgt->data.tcp.listen && src->data.tcp.protocol == tgt->data.tcp.protocol && STREQ_NULLABLE(src->data.tcp.host, tgt->data.tcp.host) && - STREQ_NULLABLE(src->data.tcp.service, tgt->data.tcp.service); + STREQ_NULLABLE(src->data.tcp.service, tgt->data.tcp.service) && + src->data.tcp.reconnect.enabled == tgt->data.tcp.reconnect.enabled && + src->data.tcp.reconnect.timeout == tgt->data.tcp.reconnect.timeout; break; case VIR_DOMAIN_CHR_TYPE_UNIX: return src->data.nix.listen == tgt->data.nix.listen && - STREQ_NULLABLE(src->data.nix.path, tgt->data.nix.path); + STREQ_NULLABLE(src->data.nix.path, tgt->data.nix.path) && + src->data.nix.reconnect.enabled == tgt->data.nix.reconnect.enabled && + src->data.nix.reconnect.timeout == tgt->data.nix.reconnect.timeout; break; case VIR_DOMAIN_CHR_TYPE_SPICEPORT: -- 2.13.5
On 08/30/2017 01:40 PM, Pavel Hrdina wrote:
Missed by 9aa72a6dd5b3.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
ACK and safe for the freeze. Michal
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@redhat.com> --- 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.args b/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.args index 133a2c6039..8c6586e483 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-chardev-reconnect.args @@ -23,14 +23,12 @@ server,nowait \ -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x3 \ -device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ --chardev socket,id=charsmartcard0,path=/tmp/channel/domain-oldname/asdf,\ -reconnect=20 \ +-chardev socket,id=charsmartcard0,path=/tmp/channel/asdf,reconnect=20 \ -device ccid-card-passthru,chardev=charsmartcard0,id=smartcard0,bus=ccid0.0 \ -chardev socket,id=charchannel0,host=localhost,port=1234,reconnect=10 \ -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\ id=channel0,name=asdf \ --chardev socket,id=charchannel1,path=/tmp/channel/domain--1-QEMUGuest1/fdsa,\ -server,nowait,reconnect=0 \ +-chardev socket,id=charchannel1,path=/tmp/channel/fdsa,reconnect=0 \ -device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,\ id=channel1,name=fdsa \ -chardev socket,id=charredir0,host=localhost,port=3456,reconnect=15 \ 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'/> -- 2.13.5
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@redhat.com> --- 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? Michal
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@redhat.com> --- 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. Pavel
On 08/30/2017 02:45 PM, Pavel Hrdina wrote:
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@redhat.com> --- 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.
Ah, right. You really want to fix just the test here. ACK then and safe for the freeze. Michal
On Wed, Aug 30, 2017 at 03:38:12PM +0200, Michal Privoznik wrote:
On 08/30/2017 02:45 PM, Pavel Hrdina wrote:
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@redhat.com> --- 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.
Ah, right. You really want to fix just the test here.
ACK then and safe for the freeze.
Thanks, I've pushed the first two patches, I'll push this one as well. For the last two patches I'll try to send a different approach that would be safe for freeze. Pavel
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; -- 2.13.5
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
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. Pavel
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
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@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
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9a27987d4a..2bc6bea888 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5159,8 +5159,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (dev->data.tcp.listen) virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); - - qemuBuildChrChardevReconnectStr(&buf, &dev->data.tcp.reconnect); + else + qemuBuildChrChardevReconnectStr(&buf, &dev->data.tcp.reconnect); if (dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) { qemuDomainChrSourcePrivatePtr chrSourcePriv = @@ -5195,8 +5195,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path); if (dev->data.nix.listen) virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); - - qemuBuildChrChardevReconnectStr(&buf, &dev->data.nix.reconnect); + else + qemuBuildChrChardevReconnectStr(&buf, &dev->data.nix.reconnect); break; case VIR_DOMAIN_CHR_TYPE_SPICEVMC: -- 2.13.5
On 08/30/2017 01:40 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK and safe for the freeze. Thank you for looking into this. Michal
participants (2)
-
Michal Privoznik -
Pavel Hrdina