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

[Pavel Hrdina]

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

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
-------------- 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/d41143de/attachment-0001.sig>