4:26 a.m.
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(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().
Yep, my bad, sorry. Thanks.
Pavel