Currently we consider all UNIX paths with specific prefix as generated
by libvirt, but that's a wrong assumption.  Let's make the detection
better by actually checking whether the whole path matches one of the
paths that we generate or generated in the past.

The UNIX path isn't stored in config XML since libvirt-1.3.0.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446980

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---

changes in v2:
    - dropped the magic to split the path into 3 parts and use only one
      regexp to match the path

 src/qemu/qemu_domain.c                             | 51 ++++++++++++++++++----
 .../qemuxml2argv-channel-unix-gen-path1.xml        | 17 ++++++++
 .../qemuxml2argv-channel-unix-gen-path2.xml        | 17 ++++++++
 .../qemuxml2argv-channel-unix-gen-path3.xml        | 17 ++++++++
 .../qemuxml2argv-channel-unix-user-path.xml        | 17 ++++++++
 .../qemuxml2xmlout-channel-unix-gen-path1.xml      | 32 ++++++++++++++
 .../qemuxml2xmlout-channel-unix-gen-path2.xml      | 32 ++++++++++++++
 .../qemuxml2xmlout-channel-unix-gen-path3.xml      | 32 ++++++++++++++
 .../qemuxml2xmlout-channel-unix-user-path.xml      | 33 ++++++++++++++
 tests/qemuxml2xmltest.c                            |  5 +++
 10 files changed, 244 insertions(+), 9 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cc02c801e1..00e37d3428 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3154,24 +3154,57 @@ qemuDomainDefaultNetModel(const virDomainDef *def,
 
 
 /*
- * Clear auto generated unix socket path, i.e., the one which starts with our
- * channel directory.
+ * Clear auto generated unix socket paths:
+ *
+ * libvirt 1.2.18 and older:
+ *     {cfg->channelTargetDir}/{dom-name}.{target-name}
+ *
+ * libvirt 1.2.19 - 1.3.2:
+ *     {cfg->channelTargetDir}/domain-{dom-name}/{target-name}
+ *
+ * libvirt 1.3.3 and newer:
+ *     {cfg->channelTargetDir}/domain-{dom-id}-{short-dom-name}/{target-name}
+ *
+ * The unix socket path was stored in config XML until libvirt 1.3.0.
+ * If someone specifies the same path as we generate, they shouldn't do it.
+ *
+ * This function clears the path for migration as well, so we need to clear
+ * the path event if we are not storing it in the XML.
  */
-static void
+static int
 qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
                                 virQEMUDriverPtr driver)
 {
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    char *regexp = NULL;
+    int ret = -1;
 
-    if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
-        chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
-        chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
-        chr->source->data.nix.path &&
-        STRPREFIX(chr->source->data.nix.path, cfg->channelTargetDir)) {
+    if (chr->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL ||
+        chr->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO ||
+        chr->source->type != VIR_DOMAIN_CHR_TYPE_UNIX ||
+        !chr->source->data.nix.path) {
+        ret = 0;
+        goto cleanup;
+    }
+
+    virBufferEscapeRegex(&buf, "^%s", cfg->channelTargetDir);
+    virBufferAddLit(&buf, "/([^/]+\\.)|(domain-[^/]+/)");
+    virBufferEscapeRegex(&buf, "%s$", chr->target.name);
+
+    if (virBufferCheckError(&buf) < 0)
+        goto cleanup;
+
+    regexp = virBufferContentAndReset(&buf);
+
+    if (virStringMatch(chr->source->data.nix.path, regexp))
         VIR_FREE(chr->source->data.nix.path);
-    }
 
+    ret = 0;
+ cleanup:
+    VIR_FREE(regexp);
     virObjectUnref(cfg);
+    return ret;
 }
 
 
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml
new file mode 100644
index 0000000000..25c84e922b
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml
@@ -0,0 +1,17 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+  </os>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <channel type='unix'>
+      <source mode='bind' path='/tmp/channel/QEMUGuest1.org.qemu.guest_agent.0'/>
+      <target type='virtio' name='org.qemu.guest_agent.0'/>
+    </channel>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml
new file mode 100644
index 0000000000..2d7ca0ae77
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml
@@ -0,0 +1,17 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+  </os>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <channel type='unix'>
+      <source mode='bind' path='/tmp/channel/domain-QEMUGuest1/org.qemu.guest_agent.0'/>
+      <target type='virtio' name='org.qemu.guest_agent.0'/>
+    </channel>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml
new file mode 100644
index 0000000000..20477016c9
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml
@@ -0,0 +1,17 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+  </os>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <channel type='unix'>
+      <source mode='bind' path='/tmp/channel/domain-1-QEMUGuest1/org.qemu.guest_agent.0'/>
+      <target type='virtio' name='org.qemu.guest_agent.0'/>
+    </channel>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml
new file mode 100644
index 0000000000..45fdf08a66
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml
@@ -0,0 +1,17 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+  </os>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <channel type='unix'>
+      <source mode='bind' path='/tmp/channel/QEMUGuest1/org.qemu.guest_agent.0'/>
+      <target type='virtio' name='org.qemu.guest_agent.0'/>
+    </channel>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml
new file mode 100644
index 0000000000..aa2a3099d7
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml
@@ -0,0 +1,32 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <currentMemory unit='KiB'>219100</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'/>
+    <controller type='virtio-serial' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </controller>
+    <channel type='unix'>
+      <target type='virtio' name='org.qemu.guest_agent.0'/>
+      <address type='virtio-serial' controller='0' bus='0' port='1'/>
+    </channel>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml
new file mode 100644
index 0000000000..aa2a3099d7
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml
@@ -0,0 +1,32 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <currentMemory unit='KiB'>219100</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'/>
+    <controller type='virtio-serial' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </controller>
+    <channel type='unix'>
+      <target type='virtio' name='org.qemu.guest_agent.0'/>
+      <address type='virtio-serial' controller='0' bus='0' port='1'/>
+    </channel>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml
new file mode 100644
index 0000000000..aa2a3099d7
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml
@@ -0,0 +1,32 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <currentMemory unit='KiB'>219100</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'/>
+    <controller type='virtio-serial' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </controller>
+    <channel type='unix'>
+      <target type='virtio' name='org.qemu.guest_agent.0'/>
+      <address type='virtio-serial' controller='0' bus='0' port='1'/>
+    </channel>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml
new file mode 100644
index 0000000000..488212d761
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml
@@ -0,0 +1,33 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <currentMemory unit='KiB'>219100</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'/>
+    <controller type='virtio-serial' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </controller>
+    <channel type='unix'>
+      <source mode='bind' path='/tmp/channel/QEMUGuest1/org.qemu.guest_agent.0'/>
+      <target type='virtio' name='org.qemu.guest_agent.0'/>
+      <address type='virtio-serial' controller='0' bus='0' port='1'/>
+    </channel>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 2dccde746e..044faf2a5d 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -551,6 +551,11 @@ mymain(void)
     DO_TEST("channel-virtio", NONE);
     DO_TEST("channel-virtio-state", NONE);
 
+    DO_TEST_FULL("channel-unix-gen-path1", WHEN_INACTIVE, GIC_NONE, NONE);
+    DO_TEST_FULL("channel-unix-gen-path2", WHEN_INACTIVE, GIC_NONE, NONE);
+    DO_TEST_FULL("channel-unix-gen-path3", WHEN_INACTIVE, GIC_NONE, NONE);
+    DO_TEST_FULL("channel-unix-user-path", WHEN_INACTIVE, GIC_NONE, NONE);
+
     DO_TEST("hostdev-usb-address", NONE);
     DO_TEST("hostdev-pci-address", NONE);
     DO_TEST("hostdev-vfio", NONE);
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, May 12, 2017 at 02:57:56PM +0200, Pavel Hrdina wrote:
>Currently we consider all UNIX paths with specific prefix as generated
>by libvirt, but that's a wrong assumption.  Let's make the detection
>better by actually checking whether the whole path matches one of the
>paths that we generate or generated in the past.
>
>The UNIX path isn't stored in config XML since libvirt-1.3.0.
>

1.3.1, I believe.

>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446980
>
>Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>---
>
>changes in v2:
>    - dropped the magic to split the path into 3 parts and use only one
>      regexp to match the path
>
> src/qemu/qemu_domain.c                             | 51 ++++++++++++++++++----
> .../qemuxml2argv-channel-unix-gen-path1.xml        | 17 ++++++++
> .../qemuxml2argv-channel-unix-gen-path2.xml        | 17 ++++++++
> .../qemuxml2argv-channel-unix-gen-path3.xml        | 17 ++++++++
> .../qemuxml2argv-channel-unix-user-path.xml        | 17 ++++++++
> .../qemuxml2xmlout-channel-unix-gen-path1.xml      | 32 ++++++++++++++
> .../qemuxml2xmlout-channel-unix-gen-path2.xml      | 32 ++++++++++++++
> .../qemuxml2xmlout-channel-unix-gen-path3.xml      | 32 ++++++++++++++
> .../qemuxml2xmlout-channel-unix-user-path.xml      | 33 ++++++++++++++
> tests/qemuxml2xmltest.c                            |  5 +++
> 10 files changed, 244 insertions(+), 9 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml
>

Just have one file that tests it all.

>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index cc02c801e1..00e37d3428 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -3154,24 +3154,57 @@ qemuDomainDefaultNetModel(const virDomainDef *def,
>
>
> /*
>- * Clear auto generated unix socket path, i.e., the one which starts with our
>- * channel directory.
>+ * Clear auto generated unix socket paths:
>+ *
>+ * libvirt 1.2.18 and older:
>+ *     {cfg->channelTargetDir}/{dom-name}.{target-name}
>+ *
>+ * libvirt 1.2.19 - 1.3.2:
>+ *     {cfg->channelTargetDir}/domain-{dom-name}/{target-name}
>+ *
>+ * libvirt 1.3.3 and newer:
>+ *     {cfg->channelTargetDir}/domain-{dom-id}-{short-dom-name}/{target-name}
>+ *
>+ * The unix socket path was stored in config XML until libvirt 1.3.0.
>+ * If someone specifies the same path as we generate, they shouldn't do it.
>+ *
>+ * This function clears the path for migration as well, so we need to clear
>+ * the path event if we are not storing it in the XML.
>  */
>-static void
>+static int

This ^^ is not reflected anywhere.  It's a pity that such function (that
just conditionally frees something) can fail.

> qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
>                                 virQEMUDriverPtr driver)
> {
>     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>+    virBuffer buf = VIR_BUFFER_INITIALIZER;
>+    char *regexp = NULL;
>+    int ret = -1;
>
>-    if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
>-        chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
>-        chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
>-        chr->source->data.nix.path &&
>-        STRPREFIX(chr->source->data.nix.path, cfg->channelTargetDir)) {
>+    if (chr->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL ||
>+        chr->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO ||
>+        chr->source->type != VIR_DOMAIN_CHR_TYPE_UNIX ||
>+        !chr->source->data.nix.path) {

This would be more readable if you postponed the initialization of @cfg
and just return 0 from this.  Optionally break this into multiple
conditions.

>+        ret = 0;
>+        goto cleanup;
>+    }
>+
>+    virBufferEscapeRegex(&buf, "^%s", cfg->channelTargetDir);
>+    virBufferAddLit(&buf, "/([^/]+\\.)|(domain-[^/]+/)");
>+    virBufferEscapeRegex(&buf, "%s$", chr->target.name);
>+
>+    if (virBufferCheckError(&buf) < 0)
>+        goto cleanup;
>+

No need to do this ^^, [1]

>+    regexp = virBufferContentAndReset(&buf);
>+

[1] Just do this:

if ((regexp = virBufferContentAndReset(&buf)) < 0)
  goto cleanup;

or similar.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, May 12, 2017 at 04:26:35PM +0200, Martin Kletzander wrote:
> On Fri, May 12, 2017 at 02:57:56PM +0200, Pavel Hrdina wrote:
> >Currently we consider all UNIX paths with specific prefix as generated
> >by libvirt, but that's a wrong assumption.  Let's make the detection
> >better by actually checking whether the whole path matches one of the
> >paths that we generate or generated in the past.
> >
> >The UNIX path isn't stored in config XML since libvirt-1.3.0.
> >
> 
> 1.3.1, I believe.
> 
> >Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446980
> >
> >Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >---
> >
> >changes in v2:
> >    - dropped the magic to split the path into 3 parts and use only one
> >      regexp to match the path
> >
> > src/qemu/qemu_domain.c                             | 51 ++++++++++++++++++----
> > .../qemuxml2argv-channel-unix-gen-path1.xml        | 17 ++++++++
> > .../qemuxml2argv-channel-unix-gen-path2.xml        | 17 ++++++++
> > .../qemuxml2argv-channel-unix-gen-path3.xml        | 17 ++++++++
> > .../qemuxml2argv-channel-unix-user-path.xml        | 17 ++++++++
> > .../qemuxml2xmlout-channel-unix-gen-path1.xml      | 32 ++++++++++++++
> > .../qemuxml2xmlout-channel-unix-gen-path2.xml      | 32 ++++++++++++++
> > .../qemuxml2xmlout-channel-unix-gen-path3.xml      | 32 ++++++++++++++
> > .../qemuxml2xmlout-channel-unix-user-path.xml      | 33 ++++++++++++++
> > tests/qemuxml2xmltest.c                            |  5 +++
> > 10 files changed, 244 insertions(+), 9 deletions(-)
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml
> > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml
> > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml
> > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml
> > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml
> >
> 
> Just have one file that tests it all.
> 
> >diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> >index cc02c801e1..00e37d3428 100644
> >--- a/src/qemu/qemu_domain.c
> >+++ b/src/qemu/qemu_domain.c
> >@@ -3154,24 +3154,57 @@ qemuDomainDefaultNetModel(const virDomainDef *def,
> >
> >
> > /*
> >- * Clear auto generated unix socket path, i.e., the one which starts with our
> >- * channel directory.
> >+ * Clear auto generated unix socket paths:
> >+ *
> >+ * libvirt 1.2.18 and older:
> >+ *     {cfg->channelTargetDir}/{dom-name}.{target-name}
> >+ *
> >+ * libvirt 1.2.19 - 1.3.2:
> >+ *     {cfg->channelTargetDir}/domain-{dom-name}/{target-name}
> >+ *
> >+ * libvirt 1.3.3 and newer:
> >+ *     {cfg->channelTargetDir}/domain-{dom-id}-{short-dom-name}/{target-name}
> >+ *
> >+ * The unix socket path was stored in config XML until libvirt 1.3.0.
> >+ * If someone specifies the same path as we generate, they shouldn't do it.
> >+ *
> >+ * This function clears the path for migration as well, so we need to clear
> >+ * the path event if we are not storing it in the XML.
> >  */
> >-static void
> >+static int
> 
> This ^^ is not reflected anywhere.  It's a pity that such function (that
> just conditionally frees something) can fail.

I've somehow lost the change to the callers to handle the failure, sigh.

> 
> > qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
> >                                 virQEMUDriverPtr driver)
> > {
> >     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> >+    virBuffer buf = VIR_BUFFER_INITIALIZER;
> >+    char *regexp = NULL;
> >+    int ret = -1;
> >
> >-    if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
> >-        chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
> >-        chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
> >-        chr->source->data.nix.path &&
> >-        STRPREFIX(chr->source->data.nix.path, cfg->channelTargetDir)) {
> >+    if (chr->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL ||
> >+        chr->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO ||
> >+        chr->source->type != VIR_DOMAIN_CHR_TYPE_UNIX ||
> >+        !chr->source->data.nix.path) {
> 
> This would be more readable if you postponed the initialization of @cfg
> and just return 0 from this.  Optionally break this into multiple
> conditions.
> 
> >+        ret = 0;
> >+        goto cleanup;
> >+    }
> >+
> >+    virBufferEscapeRegex(&buf, "^%s", cfg->channelTargetDir);
> >+    virBufferAddLit(&buf, "/([^/]+\\.)|(domain-[^/]+/)");
> >+    virBufferEscapeRegex(&buf, "%s$", chr->target.name);
> >+
> >+    if (virBufferCheckError(&buf) < 0)
> >+        goto cleanup;
> >+
> 
> No need to do this ^^, [1]
> 
> >+    regexp = virBufferContentAndReset(&buf);
> >+
> 
> [1] Just do this:
> 
> if ((regexp = virBufferContentAndReset(&buf)) < 0)
>   goto cleanup;
> 
> or similar.

It's not equivalent, the virBufferCheckError() also reports an error
which I want to do.  I'll fix the callers to check the return value
of qemuDomainChrDefDropDefaultPath().

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, May 12, 2017 at 04:45:11PM +0200, Pavel Hrdina wrote:
>On Fri, May 12, 2017 at 04:26:35PM +0200, Martin Kletzander wrote:
>> On Fri, May 12, 2017 at 02:57:56PM +0200, Pavel Hrdina wrote:
>> >Currently we consider all UNIX paths with specific prefix as generated
>> >by libvirt, but that's a wrong assumption.  Let's make the detection
>> >better by actually checking whether the whole path matches one of the
>> >paths that we generate or generated in the past.
>> >
>> >The UNIX path isn't stored in config XML since libvirt-1.3.0.
>> >
>>
>> 1.3.1, I believe.
>>
>> >Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446980
>> >
>> >Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> >---
>> >
>> >changes in v2:
>> >    - dropped the magic to split the path into 3 parts and use only one
>> >      regexp to match the path
>> >
>> > src/qemu/qemu_domain.c                             | 51 ++++++++++++++++++----
>> > .../qemuxml2argv-channel-unix-gen-path1.xml        | 17 ++++++++
>> > .../qemuxml2argv-channel-unix-gen-path2.xml        | 17 ++++++++
>> > .../qemuxml2argv-channel-unix-gen-path3.xml        | 17 ++++++++
>> > .../qemuxml2argv-channel-unix-user-path.xml        | 17 ++++++++
>> > .../qemuxml2xmlout-channel-unix-gen-path1.xml      | 32 ++++++++++++++
>> > .../qemuxml2xmlout-channel-unix-gen-path2.xml      | 32 ++++++++++++++
>> > .../qemuxml2xmlout-channel-unix-gen-path3.xml      | 32 ++++++++++++++
>> > .../qemuxml2xmlout-channel-unix-user-path.xml      | 33 ++++++++++++++
>> > tests/qemuxml2xmltest.c                            |  5 +++
>> > 10 files changed, 244 insertions(+), 9 deletions(-)
>> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml
>> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml
>> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml
>> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml
>> > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml
>> > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml
>> > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml
>> > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml
>> >
>>
>> Just have one file that tests it all.
>>
>> >diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> >index cc02c801e1..00e37d3428 100644
>> >--- a/src/qemu/qemu_domain.c
>> >+++ b/src/qemu/qemu_domain.c
>> >@@ -3154,24 +3154,57 @@ qemuDomainDefaultNetModel(const virDomainDef *def,
>> >
>> >
>> > /*
>> >- * Clear auto generated unix socket path, i.e., the one which starts with our
>> >- * channel directory.
>> >+ * Clear auto generated unix socket paths:
>> >+ *
>> >+ * libvirt 1.2.18 and older:
>> >+ *     {cfg->channelTargetDir}/{dom-name}.{target-name}
>> >+ *
>> >+ * libvirt 1.2.19 - 1.3.2:
>> >+ *     {cfg->channelTargetDir}/domain-{dom-name}/{target-name}
>> >+ *
>> >+ * libvirt 1.3.3 and newer:
>> >+ *     {cfg->channelTargetDir}/domain-{dom-id}-{short-dom-name}/{target-name}
>> >+ *
>> >+ * The unix socket path was stored in config XML until libvirt 1.3.0.
>> >+ * If someone specifies the same path as we generate, they shouldn't do it.
>> >+ *
>> >+ * This function clears the path for migration as well, so we need to clear
>> >+ * the path event if we are not storing it in the XML.
>> >  */
>> >-static void
>> >+static int
>>
>> This ^^ is not reflected anywhere.  It's a pity that such function (that
>> just conditionally frees something) can fail.
>
>I've somehow lost the change to the callers to handle the failure, sigh.
>
>>
>> > qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
>> >                                 virQEMUDriverPtr driver)
>> > {
>> >     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> >+    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> >+    char *regexp = NULL;
>> >+    int ret = -1;
>> >
>> >-    if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
>> >-        chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
>> >-        chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX &&
>> >-        chr->source->data.nix.path &&
>> >-        STRPREFIX(chr->source->data.nix.path, cfg->channelTargetDir)) {
>> >+    if (chr->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL ||
>> >+        chr->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO ||
>> >+        chr->source->type != VIR_DOMAIN_CHR_TYPE_UNIX ||
>> >+        !chr->source->data.nix.path) {
>>
>> This would be more readable if you postponed the initialization of @cfg
>> and just return 0 from this.  Optionally break this into multiple
>> conditions.
>>
>> >+        ret = 0;
>> >+        goto cleanup;
>> >+    }
>> >+
>> >+    virBufferEscapeRegex(&buf, "^%s", cfg->channelTargetDir);
>> >+    virBufferAddLit(&buf, "/([^/]+\\.)|(domain-[^/]+/)");
>> >+    virBufferEscapeRegex(&buf, "%s$", chr->target.name);
>> >+
>> >+    if (virBufferCheckError(&buf) < 0)
>> >+        goto cleanup;
>> >+
>>
>> No need to do this ^^, [1]
>>
>> >+    regexp = virBufferContentAndReset(&buf);
>> >+
>>
>> [1] Just do this:
>>
>> if ((regexp = virBufferContentAndReset(&buf)) < 0)
>>   goto cleanup;
>>
>> or similar.
>
>It's not equivalent, the virBufferCheckError() also reports an error
>which I want to do.  I'll fix the callers to check the return value
>of qemuDomainChrDefDropDefaultPath().
>

Yep, my bad, sorry.  Thanks.

>Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list