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

[Martin Kletzander]

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

Yep, my bad, sorry.  Thanks.

>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/20170515/31eedbd7/attachment-0001.sig>