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@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().
Yep, my bad, sorry. Thanks.
Pavel
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
