[libvirt] [PATCH v2 3/3] qemu: improve detection of UNIX path generated by libvirt

[Martin Kletzander]

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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170512/27b54112/attachment-0001.sig>