9:45 a.m.
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(a)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