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@xxxxxxxxxx> > >--- > > > >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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
