10:49 a.m.
Pavel Hrdina (2):
util: introduce virStringMatch
qemu: improve detection of UNIX path generated by libvirt
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 78 +++++++++++++++++++---
src/util/virstring.c | 34 ++++++++++
src/util/virstring.h | 3 +
.../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 ++
tests/virstringtest.c | 47 +++++++++++++
14 files changed, 356 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
--
2.12.2
10:49 a.m.
New subject: [libvirt] [PATCH 1/2] util: introduce virStringMatch
Simply tries to match the provided regex on a string and returns
the result. Useful if caller don't care about the matched substring
and want to just test if some pattern patches a string.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virstring.c | 34 ++++++++++++++++++++++++++++++++++
src/util/virstring.h | 3 +++
tests/virstringtest.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 85 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index afb9100c50..d32c6e7549 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2621,6 +2621,7 @@ virStringListHasString;
virStringListJoin;
virStringListLength;
virStringListRemove;
+virStringMatch;
virStringReplace;
virStringSearch;
virStringSortCompare;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 335e773d78..b95a8926bd 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -979,6 +979,40 @@ virStringSearch(const char *str,
}
/**
+ * virStringMatch:
+ * @str: string to match
+ * @regexp: POSIX Extended regular expression pattern used for matching
+ *
+ * Performs a POSIX extended regex search against a string.
+ * Returns 0 on match, -1 on error, 1 on no match.
+ */
+int
+virStringMatch(const char *str,
+ const char *regexp)
+{
+ regex_t re;
+ int ret = -1;
+ int rv;
+
+ VIR_DEBUG("match '%s' for '%s'", str, regexp);
+
+ if ((rv = regcomp(&re, regexp, REG_EXTENDED | REG_NOSUB)) != 0) {
+ char error[100];
+ regerror(rv, &re, error, sizeof(error));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("error while compiling regular expression '%s':
%s"),
+ regexp, error);
+ return -1;
+ }
+
+ if ((ret = regexec(&re, str, 0, NULL, 0)) != 0)
+ ret = 1;
+
+ regfree(&re);
+ return ret;
+}
+
+/**
* virStringReplace:
* @haystack: the source string to process
* @oldneedle: the substring to locate
diff --git a/src/util/virstring.h b/src/util/virstring.h
index c545ca3f0d..3221d99752 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -274,6 +274,9 @@ ssize_t virStringSearch(const char *str,
char ***matches)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);
+int virStringMatch(const char *str,
+ const char *regexp);
+
char *virStringReplace(const char *haystack,
const char *oldneedle,
const char *newneedle)
diff --git a/tests/virstringtest.c b/tests/virstringtest.c
index 96bc79f832..f8402cd163 100644
--- a/tests/virstringtest.c
+++ b/tests/virstringtest.c
@@ -480,6 +480,38 @@ testStringSearch(const void *opaque)
}
+struct stringMatchData {
+ const char *str;
+ const char *regexp;
+ bool expectMatch;
+};
+
+static int
+testStringMatch(const void *opaque)
+{
+ const struct stringMatchData *data = opaque;
+ int rv;
+
+ rv = virStringMatch(data->str, data->regexp);
+
+ if (data->expectMatch) {
+ if (rv != 0) {
+ fprintf(stderr, "expected match for '%s' on '%s' but got
no match\n",
+ data->regexp, data->str);
+ return -1;
+ }
+ } else {
+ if (rv != 1) {
+ fprintf(stderr, "expected no match for '%s' on '%s' but
got match\n",
+ data->regexp, data->str);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
struct stringReplaceData {
const char *haystack;
const char *oldneedle;
@@ -803,6 +835,21 @@ mymain(void)
const char *matches3[] = { "foo", "bar" };
TEST_SEARCH("1foo2bar3eek", "([a-z]+)", 2, 2, matches3, false);
+#define TEST_MATCH(s, r, m) \
+ do { \
+ struct stringMatchData data = { \
+ .str = s, \
+ .regexp = r, \
+ .expectMatch = m, \
+ }; \
+ if (virTestRun("virStringMatch " s, testStringMatch, &data) < 0)
\
+ ret = -1; \
+ } while (0)
+
+ TEST_MATCH("foo", "foo", true);
+ TEST_MATCH("foobar", "f[o]+", true);
+ TEST_MATCH("foobar", "^f[o]+$", false);
+
#define TEST_REPLACE(h, o, n, r) \
do { \
struct stringReplaceData data = { \
--
2.12.2
4:03 a.m.
New subject: [libvirt] [PATCH 1/2] util: introduce virStringMatch
On Thu, May 11, 2017 at 05:49:53PM +0200, Pavel Hrdina wrote:
Simply tries to match the provided regex on a string and returns
the result. Useful if caller don't care about the matched substring
and want to just test if some pattern patches a string.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virstring.c | 34 ++++++++++++++++++++++++++++++++++
src/util/virstring.h | 3 +++
tests/virstringtest.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 85 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index afb9100c50..d32c6e7549 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2621,6 +2621,7 @@ virStringListHasString;
virStringListJoin;
virStringListLength;
virStringListRemove;
+virStringMatch;
virStringReplace;
virStringSearch;
virStringSortCompare;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 335e773d78..b95a8926bd 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -979,6 +979,40 @@ virStringSearch(const char *str,
}
/**
+ * virStringMatch:
+ * @str: string to match
+ * @regexp: POSIX Extended regular expression pattern used for matching
+ *
+ * Performs a POSIX extended regex search against a string.
+ * Returns 0 on match, -1 on error, 1 on no match.
+ */
+int
+virStringMatch(const char *str,
+ const char *regexp)
+{
+ regex_t re;
+ int ret = -1;
+ int rv;
+
+ VIR_DEBUG("match '%s' for '%s'", str, regexp);
+
+ if ((rv = regcomp(&re, regexp, REG_EXTENDED | REG_NOSUB)) != 0) {
+ char error[100];
+ regerror(rv, &re, error, sizeof(error));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("error while compiling regular expression '%s':
%s"),
+ regexp, error);
+ return -1;
+ }
+
+ if ((ret = regexec(&re, str, 0, NULL, 0)) != 0)
+ ret = 1;
This could be made easier if you just retuned bool true on match and
false on no match/error. Even though that would ignore one (very
unlikely) error, it works much more nicely with any usage this might get
in libvirt.
ACK with that changed.
5:44 a.m.
New subject: [libvirt] [PATCH 1/2] util: introduce virStringMatch
On 05/11/2017 11:49 AM, Pavel Hrdina wrote:
Simply tries to match the provided regex on a string and returns
the result. Useful if caller don't care about the matched substring
and want to just test if some pattern patches a string.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virstring.c | 34 ++++++++++++++++++++++++++++++++++
src/util/virstring.h | 3 +++
tests/virstringtest.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 85 insertions(+)
Something to consider seeing as this function could be "reused" if made
a bit more generic...
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index afb9100c50..d32c6e7549 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2621,6 +2621,7 @@ virStringListHasString;
virStringListJoin;
virStringListLength;
virStringListRemove;
+virStringMatch;
virStringReplace;
virStringSearch;
virStringSortCompare;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 335e773d78..b95a8926bd 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -979,6 +979,40 @@ virStringSearch(const char *str,
}
/**
+ * virStringMatch:
+ * @str: string to match
+ * @regexp: POSIX Extended regular expression pattern used for matching
+ *
+ * Performs a POSIX extended regex search against a string.
+ * Returns 0 on match, -1 on error, 1 on no match.
+ */
+int
+virStringMatch(const char *str,
+ const char *regexp)
+{
+ regex_t re;
+ int ret = -1;
+ int rv;
+
+ VIR_DEBUG("match '%s' for '%s'", str, regexp);
+
+ if ((rv = regcomp(&re, regexp, REG_EXTENDED | REG_NOSUB)) != 0) {
Why not pass the last arg as @flags
+ char error[100];
+ regerror(rv, &re, error, sizeof(error));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("error while compiling regular expression '%s':
%s"),
+ regexp, error);
+ return -1;
+ }
+
+ if ((ret = regexec(&re, str, 0, NULL, 0)) != 0)
and perhaps the 3rd/4th args as params...
Could lead to some reuse of this particular function by others that also
call regcomp and regexec (virStorageBackendLogicalParseVolExtents has an
example of the args to regexec)
John
+ ret = 1;
+
+ regfree(&re);
+ return ret;
+}
+
+/**
* virStringReplace:
* @haystack: the source string to process
* @oldneedle: the substring to locate
diff --git a/src/util/virstring.h b/src/util/virstring.h
index c545ca3f0d..3221d99752 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -274,6 +274,9 @@ ssize_t virStringSearch(const char *str,
char ***matches)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);
+int virStringMatch(const char *str,
+ const char *regexp);
+
char *virStringReplace(const char *haystack,
const char *oldneedle,
const char *newneedle)
diff --git a/tests/virstringtest.c b/tests/virstringtest.c
index 96bc79f832..f8402cd163 100644
--- a/tests/virstringtest.c
+++ b/tests/virstringtest.c
@@ -480,6 +480,38 @@ testStringSearch(const void *opaque)
}
+struct stringMatchData {
+ const char *str;
+ const char *regexp;
+ bool expectMatch;
+};
+
+static int
+testStringMatch(const void *opaque)
+{
+ const struct stringMatchData *data = opaque;
+ int rv;
+
+ rv = virStringMatch(data->str, data->regexp);
+
+ if (data->expectMatch) {
+ if (rv != 0) {
+ fprintf(stderr, "expected match for '%s' on '%s' but
got no match\n",
+ data->regexp, data->str);
+ return -1;
+ }
+ } else {
+ if (rv != 1) {
+ fprintf(stderr, "expected no match for '%s' on '%s' but
got match\n",
+ data->regexp, data->str);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
struct stringReplaceData {
const char *haystack;
const char *oldneedle;
@@ -803,6 +835,21 @@ mymain(void)
const char *matches3[] = { "foo", "bar" };
TEST_SEARCH("1foo2bar3eek", "([a-z]+)", 2, 2, matches3, false);
+#define TEST_MATCH(s, r, m) \
+ do { \
+ struct stringMatchData data = { \
+ .str = s, \
+ .regexp = r, \
+ .expectMatch = m, \
+ }; \
+ if (virTestRun("virStringMatch " s, testStringMatch, &data) <
0) \
+ ret = -1; \
+ } while (0)
+
+ TEST_MATCH("foo", "foo", true);
+ TEST_MATCH("foobar", "f[o]+", true);
+ TEST_MATCH("foobar", "^f[o]+$", false);
+
#define TEST_REPLACE(h, o, n, r) \
do { \
struct stringReplaceData data = { \
6:10 a.m.
New subject: [libvirt] [PATCH 1/2] util: introduce virStringMatch
On Fri, May 12, 2017 at 06:44:16AM -0400, John Ferlan wrote:
On 05/11/2017 11:49 AM, Pavel Hrdina wrote:
> Simply tries to match the provided regex on a string and returns
> the result. Useful if caller don't care about the matched substring
> and want to just test if some pattern patches a string.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virstring.c | 34 ++++++++++++++++++++++++++++++++++
> src/util/virstring.h | 3 +++
> tests/virstringtest.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 85 insertions(+)
>
Something to consider seeing as this function could be "reused" if made
a bit more generic...
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index afb9100c50..d32c6e7549 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2621,6 +2621,7 @@ virStringListHasString;
> virStringListJoin;
> virStringListLength;
> virStringListRemove;
> +virStringMatch;
> virStringReplace;
> virStringSearch;
> virStringSortCompare;
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index 335e773d78..b95a8926bd 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -979,6 +979,40 @@ virStringSearch(const char *str,
> }
>
> /**
> + * virStringMatch:
> + * @str: string to match
> + * @regexp: POSIX Extended regular expression pattern used for matching
> + *
> + * Performs a POSIX extended regex search against a string.
> + * Returns 0 on match, -1 on error, 1 on no match.
> + */
> +int
> +virStringMatch(const char *str,
> + const char *regexp)
> +{
> + regex_t re;
> + int ret = -1;
> + int rv;
> +
> + VIR_DEBUG("match '%s' for '%s'", str, regexp);
> +
> + if ((rv = regcomp(&re, regexp, REG_EXTENDED | REG_NOSUB)) != 0) {
Why not pass the last arg as @flags
In most cases there is no need to change those @flags, other possible
are REG_ICASE and REG_NEWLINE.
> + char error[100];
> + regerror(rv, &re, error, sizeof(error));
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("error while compiling regular expression
'%s': %s"),
> + regexp, error);
> + return -1;
> + }
> +
> + if ((ret = regexec(&re, str, 0, NULL, 0)) != 0)
and perhaps the 3rd/4th args as params...
Could lead to some reuse of this particular function by others that also
call regcomp and regexec (virStorageBackendLogicalParseVolExtents has an
example of the args to regexec)
There is virStringSearch() that does exactly what you are referring to.
However, virStringSearch() doesn't allow more than one match group,
which virStorageBackendLogicalParseVolExtents uses.
The purpose of virStringMatch is to match the regex in cases where
you don't need the matched groups. We might extend/improve
virStringSearch().
Pavel
10:49 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: improve detection of UNIX path generated by libvirt
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.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446980
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_domain.c | 78 +++++++++++++++++++---
.../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, 271 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
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cc02c801e1..99bfd8f320 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3154,24 +3154,84 @@ 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
qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
virQEMUDriverPtr driver)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ char *path;
+ char *prefix = NULL;
+ int prefixLen;
+ int ret = -1;
+ int rv;
- 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)) {
- VIR_FREE(chr->source->data.nix.path);
+ 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) {
+ ret = 0;
+ goto cleanup;
}
+ if (!STRPREFIX(chr->source->data.nix.path, cfg->channelTargetDir)) {
+ ret = 0;
+ goto cleanup;
+ }
+
+ path = chr->source->data.nix.path + strlen(cfg->channelTargetDir);
+ prefixLen = strlen(path) - strlen(chr->target.name);
+
+ if (STRNEQ(path + prefixLen, chr->target.name)) {
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (!VIR_STRNDUP(prefix, path, prefixLen))
+ goto cleanup;
+
+ /* Now we've isolated the middle part of the path by removing the
+ * cfg->channelTargetDir part from the beginning and chr->target.name
+ * from the end. The middle part is the one that changed in the past
+ * and the only part that we need to try to match with. */
+
+#define VIR_CLEAR_CHR_DEF_PATH(regex) \
+ if ((rv = virStringMatch(prefix, regex)) < 0) \
+ goto cleanup; \
+ \
+ if (rv == 0) { \
+ VIR_FREE(chr->source->data.nix.path); \
+ ret = 0; \
+ goto cleanup; \
+ }
+
+ VIR_CLEAR_CHR_DEF_PATH("^/[^/]+\\.$")
+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[^/]+/$")
+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[0-9]+-[^/]+/$")
+
+#undef VIR_CLEAR_CHR_DEF_PATH
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(prefix);
virObjectUnref(cfg);
+ return ret;
}
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml
b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml
new file mode 100644
index 0000000000..25c84e922b
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path1.xml
@@ -0,0 +1,17 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ </os>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <channel type='unix'>
+ <source mode='bind'
path='/tmp/channel/QEMUGuest1.org.qemu.guest_agent.0'/>
+ <target type='virtio' name='org.qemu.guest_agent.0'/>
+ </channel>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml
b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml
new file mode 100644
index 0000000000..2d7ca0ae77
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path2.xml
@@ -0,0 +1,17 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ </os>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <channel type='unix'>
+ <source mode='bind'
path='/tmp/channel/domain-QEMUGuest1/org.qemu.guest_agent.0'/>
+ <target type='virtio' name='org.qemu.guest_agent.0'/>
+ </channel>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml
b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml
new file mode 100644
index 0000000000..20477016c9
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-gen-path3.xml
@@ -0,0 +1,17 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ </os>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <channel type='unix'>
+ <source mode='bind'
path='/tmp/channel/domain-1-QEMUGuest1/org.qemu.guest_agent.0'/>
+ <target type='virtio' name='org.qemu.guest_agent.0'/>
+ </channel>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml
b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml
new file mode 100644
index 0000000000..45fdf08a66
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-unix-user-path.xml
@@ -0,0 +1,17 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ </os>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <channel type='unix'>
+ <source mode='bind'
path='/tmp/channel/QEMUGuest1/org.qemu.guest_agent.0'/>
+ <target type='virtio' name='org.qemu.guest_agent.0'/>
+ </channel>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml
new file mode 100644
index 0000000000..aa2a3099d7
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path1.xml
@@ -0,0 +1,32 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <controller type='virtio-serial' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </controller>
+ <channel type='unix'>
+ <target type='virtio' name='org.qemu.guest_agent.0'/>
+ <address type='virtio-serial' controller='0' bus='0'
port='1'/>
+ </channel>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml
new file mode 100644
index 0000000000..aa2a3099d7
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path2.xml
@@ -0,0 +1,32 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <controller type='virtio-serial' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </controller>
+ <channel type='unix'>
+ <target type='virtio' name='org.qemu.guest_agent.0'/>
+ <address type='virtio-serial' controller='0' bus='0'
port='1'/>
+ </channel>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml
new file mode 100644
index 0000000000..aa2a3099d7
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-gen-path3.xml
@@ -0,0 +1,32 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <controller type='virtio-serial' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </controller>
+ <channel type='unix'>
+ <target type='virtio' name='org.qemu.guest_agent.0'/>
+ <address type='virtio-serial' controller='0' bus='0'
port='1'/>
+ </channel>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml
new file mode 100644
index 0000000000..488212d761
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-unix-user-path.xml
@@ -0,0 +1,33 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <controller type='virtio-serial' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </controller>
+ <channel type='unix'>
+ <source mode='bind'
path='/tmp/channel/QEMUGuest1/org.qemu.guest_agent.0'/>
+ <target type='virtio' name='org.qemu.guest_agent.0'/>
+ <address type='virtio-serial' controller='0' bus='0'
port='1'/>
+ </channel>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 2dccde746e..044faf2a5d 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -551,6 +551,11 @@ mymain(void)
DO_TEST("channel-virtio", NONE);
DO_TEST("channel-virtio-state", NONE);
+ DO_TEST_FULL("channel-unix-gen-path1", WHEN_INACTIVE, GIC_NONE, NONE);
+ DO_TEST_FULL("channel-unix-gen-path2", WHEN_INACTIVE, GIC_NONE, NONE);
+ DO_TEST_FULL("channel-unix-gen-path3", WHEN_INACTIVE, GIC_NONE, NONE);
+ DO_TEST_FULL("channel-unix-user-path", WHEN_INACTIVE, GIC_NONE, NONE);
+
DO_TEST("hostdev-usb-address", NONE);
DO_TEST("hostdev-pci-address", NONE);
DO_TEST("hostdev-vfio", NONE);
--
2.12.2
4:27 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: improve detection of UNIX path generated by libvirt
On Thu, May 11, 2017 at 05:49:54PM +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
<rant tldr="sigh">
That assumption is pretty OK from my POV, any name that's not generated
by libvirt under that path can collide with anything. Only libvirt paths
can be guaranteed not to collide. That's where generated paths go and
users or mgmt apps should query that information from the running XML.
That's also the only way we can guarantee access works.
But hey, why not make our codebase bigger and more complex, right...
</rant>
better by actually checking whether the whole path matches one of the
paths that we generate or generated in the past.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446980
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_domain.c | 78 +++++++++++++++++++---
.../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, 271 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
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cc02c801e1..99bfd8f320 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3154,24 +3154,84 @@ 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}
+ *
Looks like we could just clean the last one. People with previous
versions might rely on the generated paths already...
We are clearing the path since 1.3.1. Might be worth putting it in the
commit message.
+ * 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
qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
virQEMUDriverPtr driver)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ char *path;
+ char *prefix = NULL;
+ int prefixLen;
+ int ret = -1;
+ int rv;
- 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)) {
- VIR_FREE(chr->source->data.nix.path);
+ 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) {
+ ret = 0;
+ goto cleanup;
}
+ if (!STRPREFIX(chr->source->data.nix.path, cfg->channelTargetDir)) {
+ ret = 0;
+ goto cleanup;
+ }
+
+ path = chr->source->data.nix.path + strlen(cfg->channelTargetDir);
+ prefixLen = strlen(path) - strlen(chr->target.name);
+
+ if (STRNEQ(path + prefixLen, chr->target.name)) {
If this can happen, that means it can eventually (in very rare
circumstances) segfault if the target name is very long.
I hope target names cannot have international characters in them,
otherwise it's even worse.
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (!VIR_STRNDUP(prefix, path, prefixLen))
+ goto cleanup;
+
+ /* Now we've isolated the middle part of the path by removing the
+ * cfg->channelTargetDir part from the beginning and chr->target.name
+ * from the end. The middle part is the one that changed in the past
+ * and the only part that we need to try to match with. */
+
+#define VIR_CLEAR_CHR_DEF_PATH(regex) \
+ if ((rv = virStringMatch(prefix, regex)) < 0) \
+ goto cleanup; \
+ \
+ if (rv == 0) { \
+ VIR_FREE(chr->source->data.nix.path); \
+ ret = 0; \
+ goto cleanup; \
+ }
+
+ VIR_CLEAR_CHR_DEF_PATH("^/[^/]+\\.$")
+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[^/]+/$")
+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[0-9]+-[^/]+/$")
+
Just check for ^{cfg->channelTargetDir}/[^/]+[./]qemu/{chr->targetName}
And make sure you escape stuff in squiggly brackets. That should work
and be easier. Also don't forget adapting it to the boolean return
value of virStringMatch
5:02 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: improve detection of UNIX path generated by libvirt
On Fri, May 12, 2017 at 11:27:36AM +0200, Martin Kletzander wrote:
On Thu, May 11, 2017 at 05:49:54PM +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
<rant tldr="sigh">
That assumption is pretty OK from my POV, any name that's not generated
by libvirt under that path can collide with anything. Only libvirt paths
can be guaranteed not to collide. That's where generated paths go and
users or mgmt apps should query that information from the running XML.
That's also the only way we can guarantee access works.
But hey, why not make our codebase bigger and more complex, right...
Yes, I agree, but we should've documented it and possible check the path
provided by user and error out if it matches the prefix of our internal
paths and we shouldn't have put the path into config XML.
A lot of code in libvirt is there just to make sure that every bad
design or decision still works :).
</rant>
>better by actually checking whether the whole path matches one of the
>paths that we generate or generated in the past.
>
>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446980
>
>Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>---
> src/qemu/qemu_domain.c | 78 +++++++++++++++++++---
> .../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, 271 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
>
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index cc02c801e1..99bfd8f320 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -3154,24 +3154,84 @@ 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}
>+ *
Looks like we could just clean the last one. People with previous
versions might rely on the generated paths already...
I don't think so. Currently we clear all of them because we match only
the prefix, this relaxes the logic only to the paths that we've actually
generated.
This code is also used to clear the path for migration so removing only
the latest format would break migration of guests that would use the old
format. You can have a running guest, you update libvirt and restart
the daemon and after that you try to migrate and it suddenly fails, that
would be a regression. It would also break incoming migration from
libvirt that doesn't clear the path before sending an XML.
We need to live with the fact, that the generated-path wasn't somehow
distinguished from path provided by user and because of that we need
to have an extra code to handle it.
We are clearing the path since 1.3.1. Might be worth putting it in
the
commit message.
>+ * 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
> qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
> virQEMUDriverPtr driver)
> {
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>+ char *path;
>+ char *prefix = NULL;
>+ int prefixLen;
>+ int ret = -1;
>+ int rv;
>
>- 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)) {
>- VIR_FREE(chr->source->data.nix.path);
>+ 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) {
>+ ret = 0;
>+ goto cleanup;
> }
>
>+ if (!STRPREFIX(chr->source->data.nix.path, cfg->channelTargetDir)) {
>+ ret = 0;
>+ goto cleanup;
>+ }
>+
>+ path = chr->source->data.nix.path + strlen(cfg->channelTargetDir);
>+ prefixLen = strlen(path) - strlen(chr->target.name);
>+
>+ if (STRNEQ(path + prefixLen, chr->target.name)) {
If this can happen, that means it can eventually (in very rare
circumstances) segfault if the target name is very long.
I hope target names cannot have international characters in them,
otherwise it's even worse.
Nice catch, I'll fix it.
>+ ret = 0;
>+ goto cleanup;
>+ }
>+
>+ if (!VIR_STRNDUP(prefix, path, prefixLen))
>+ goto cleanup;
>+
>+ /* Now we've isolated the middle part of the path by removing the
>+ * cfg->channelTargetDir part from the beginning and chr->target.name
>+ * from the end. The middle part is the one that changed in the past
>+ * and the only part that we need to try to match with. */
>+
>+#define VIR_CLEAR_CHR_DEF_PATH(regex) \
>+ if ((rv = virStringMatch(prefix, regex)) < 0) \
>+ goto cleanup; \
>+ \
>+ if (rv == 0) { \
>+ VIR_FREE(chr->source->data.nix.path); \
>+ ret = 0; \
>+ goto cleanup; \
>+ }
>+
>+ VIR_CLEAR_CHR_DEF_PATH("^/[^/]+\\.$")
>+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[^/]+/$")
>+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[0-9]+-[^/]+/$")
>+
Just check for ^{cfg->channelTargetDir}/[^/]+[./]qemu/{chr->targetName}
And make sure you escape stuff in squiggly brackets. That should work
and be easier. Also don't forget adapting it to the boolean return
value of virStringMatch
That's exactly what I wanted to avoid, escaping the path where we can
simply use STRPREFIX and STRNEQ.
Pavel
6:28 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: improve detection of UNIX path generated by libvirt
On Fri, May 12, 2017 at 12:02:24PM +0200, Pavel Hrdina wrote:
On Fri, May 12, 2017 at 11:27:36AM +0200, Martin Kletzander wrote:
> On Thu, May 11, 2017 at 05:49:54PM +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
>
> <rant tldr="sigh">
> That assumption is pretty OK from my POV, any name that's not generated
> by libvirt under that path can collide with anything. Only libvirt paths
> can be guaranteed not to collide. That's where generated paths go and
> users or mgmt apps should query that information from the running XML.
> That's also the only way we can guarantee access works.
>
> But hey, why not make our codebase bigger and more complex, right...
Yes, I agree, but we should've documented it and possible check the path
provided by user and error out if it matches the prefix of our internal
paths and we shouldn't have put the path into config XML.
A lot of code in libvirt is there just to make sure that every bad
design or decision still works :).
And even more code that is left out so that people can shoot themselves
in their feet if they want to. And precisely for the same reason.
> </rant>
>
> >better by actually checking whether the whole path matches one of the
> >paths that we generate or generated in the past.
> >
> >Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446980
> >
> >Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> >---
> > src/qemu/qemu_domain.c | 78 +++++++++++++++++++---
> > .../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, 271 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
> >
> >diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> >index cc02c801e1..99bfd8f320 100644
> >--- a/src/qemu/qemu_domain.c
> >+++ b/src/qemu/qemu_domain.c
> >@@ -3154,24 +3154,84 @@ 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}
> >+ *
>
> Looks like we could just clean the last one. People with previous
> versions might rely on the generated paths already...
I don't think so. Currently we clear all of them because we match only
the prefix, this relaxes the logic only to the paths that we've actually
generated.
This code is also used to clear the path for migration so removing only
the latest format would break migration of guests that would use the old
format. You can have a running guest, you update libvirt and restart
the daemon and after that you try to migrate and it suddenly fails, that
would be a regression. It would also break incoming migration from
libvirt that doesn't clear the path before sending an XML.
We need to live with the fact, that the generated-path wasn't somehow
distinguished from path provided by user and because of that we need
to have an extra code to handle it.
I know, I wrote the code that cleans it up. I just meant to say not
many people will upgrade from version older than 1.3.1. Also it's not a
requirement, just another 'sigh'.
> We are clearing the path since 1.3.1. Might be worth putting it
in the
> commit message.
>
> >+ * 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
> > qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
> > virQEMUDriverPtr driver)
> > {
> > virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> >+ char *path;
> >+ char *prefix = NULL;
> >+ int prefixLen;
> >+ int ret = -1;
> >+ int rv;
> >
> >- 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))
{
> >- VIR_FREE(chr->source->data.nix.path);
> >+ 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) {
> >+ ret = 0;
> >+ goto cleanup;
> > }
> >
> >+ if (!STRPREFIX(chr->source->data.nix.path, cfg->channelTargetDir))
{
> >+ ret = 0;
> >+ goto cleanup;
> >+ }
> >+
> >+ path = chr->source->data.nix.path + strlen(cfg->channelTargetDir);
> >+ prefixLen = strlen(path) - strlen(chr->target.name);
> >+
> >+ if (STRNEQ(path + prefixLen, chr->target.name)) {
>
> If this can happen, that means it can eventually (in very rare
> circumstances) segfault if the target name is very long.
>
> I hope target names cannot have international characters in them,
> otherwise it's even worse.
Nice catch, I'll fix it.
> >+ ret = 0;
> >+ goto cleanup;
> >+ }
> >+
> >+ if (!VIR_STRNDUP(prefix, path, prefixLen))
> >+ goto cleanup;
> >+
> >+ /* Now we've isolated the middle part of the path by removing the
> >+ * cfg->channelTargetDir part from the beginning and chr->target.name
> >+ * from the end. The middle part is the one that changed in the past
> >+ * and the only part that we need to try to match with. */
> >+
> >+#define VIR_CLEAR_CHR_DEF_PATH(regex) \
> >+ if ((rv = virStringMatch(prefix, regex)) < 0)
\
> >+ goto cleanup; \
> >+ \
> >+ if (rv == 0) { \
> >+ VIR_FREE(chr->source->data.nix.path);
\
> >+ ret = 0; \
> >+ goto cleanup; \
> >+ }
> >+
> >+ VIR_CLEAR_CHR_DEF_PATH("^/[^/]+\\.$")
> >+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[^/]+/$")
> >+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[0-9]+-[^/]+/$")
> >+
>
> Just check for ^{cfg->channelTargetDir}/[^/]+[./]qemu/{chr->targetName}
>
> And make sure you escape stuff in squiggly brackets. That should work
> and be easier. Also don't forget adapting it to the boolean return
> value of virStringMatch
That's exactly what I wanted to avoid, escaping the path where we can
simply use STRPREFIX and STRNEQ.
Which you can't. And you are constructing and doing the regexp match
three times. Moreover you don't need to escape much. Just
"^$.|?*+()[]{}\" and all those with just a backslash. You can add few
more checks there, but just doing one escape and one match is way more
readable.
Pavel
7:15 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: improve detection of UNIX path generated by libvirt
On Fri, May 12, 2017 at 01:28:22PM +0200, Martin Kletzander wrote:
On Fri, May 12, 2017 at 12:02:24PM +0200, Pavel Hrdina wrote:
>On Fri, May 12, 2017 at 11:27:36AM +0200, Martin Kletzander wrote:
>> On Thu, May 11, 2017 at 05:49:54PM +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
>>
>> <rant tldr="sigh">
>> That assumption is pretty OK from my POV, any name that's not generated
>> by libvirt under that path can collide with anything. Only libvirt paths
>> can be guaranteed not to collide. That's where generated paths go and
>> users or mgmt apps should query that information from the running XML.
>> That's also the only way we can guarantee access works.
>>
>> But hey, why not make our codebase bigger and more complex, right...
>
>Yes, I agree, but we should've documented it and possible check the path
>provided by user and error out if it matches the prefix of our internal
>paths and we shouldn't have put the path into config XML.
>
>A lot of code in libvirt is there just to make sure that every bad
>design or decision still works :).
>
And even more code that is left out so that people can shoot themselves
in their feet if they want to. And precisely for the same reason.
>> </rant>
>>
>> >better by actually checking whether the whole path matches one of the
>> >paths that we generate or generated in the past.
>> >
>> >Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1446980
>> >
>> >Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>> >---
>> > src/qemu/qemu_domain.c | 78
+++++++++++++++++++---
>> > .../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, 271 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
>> >
>> >diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> >index cc02c801e1..99bfd8f320 100644
>> >--- a/src/qemu/qemu_domain.c
>> >+++ b/src/qemu/qemu_domain.c
>> >@@ -3154,24 +3154,84 @@ 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}
>> >+ *
>>
>> Looks like we could just clean the last one. People with previous
>> versions might rely on the generated paths already...
>
>I don't think so. Currently we clear all of them because we match only
>the prefix, this relaxes the logic only to the paths that we've actually
>generated.
>
>This code is also used to clear the path for migration so removing only
>the latest format would break migration of guests that would use the old
>format. You can have a running guest, you update libvirt and restart
>the daemon and after that you try to migrate and it suddenly fails, that
>would be a regression. It would also break incoming migration from
>libvirt that doesn't clear the path before sending an XML.
>
>We need to live with the fact, that the generated-path wasn't somehow
>distinguished from path provided by user and because of that we need
>to have an extra code to handle it.
>
I know, I wrote the code that cleans it up. I just meant to say not
many people will upgrade from version older than 1.3.1. Also it's not a
requirement, just another 'sigh'.
>> We are clearing the path since 1.3.1. Might be worth putting it in the
>> commit message.
>>
>> >+ * 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
>> > qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
>> > virQEMUDriverPtr driver)
>> > {
>> > virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> >+ char *path;
>> >+ char *prefix = NULL;
>> >+ int prefixLen;
>> >+ int ret = -1;
>> >+ int rv;
>> >
>> >- 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)) {
>> >- VIR_FREE(chr->source->data.nix.path);
>> >+ 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) {
>> >+ ret = 0;
>> >+ goto cleanup;
>> > }
>> >
>> >+ if (!STRPREFIX(chr->source->data.nix.path,
cfg->channelTargetDir)) {
>> >+ ret = 0;
>> >+ goto cleanup;
>> >+ }
>> >+
>> >+ path = chr->source->data.nix.path +
strlen(cfg->channelTargetDir);
>> >+ prefixLen = strlen(path) - strlen(chr->target.name);
>> >+
>> >+ if (STRNEQ(path + prefixLen, chr->target.name)) {
>>
>> If this can happen, that means it can eventually (in very rare
>> circumstances) segfault if the target name is very long.
>>
>> I hope target names cannot have international characters in them,
>> otherwise it's even worse.
>
>Nice catch, I'll fix it.
>
>> >+ ret = 0;
>> >+ goto cleanup;
>> >+ }
>> >+
>> >+ if (!VIR_STRNDUP(prefix, path, prefixLen))
>> >+ goto cleanup;
>> >+
>> >+ /* Now we've isolated the middle part of the path by removing the
>> >+ * cfg->channelTargetDir part from the beginning and
chr->target.name
>> >+ * from the end. The middle part is the one that changed in the past
>> >+ * and the only part that we need to try to match with. */
>> >+
>> >+#define VIR_CLEAR_CHR_DEF_PATH(regex)
\
>> >+ if ((rv = virStringMatch(prefix, regex)) < 0)
\
>> >+ goto cleanup;
\
>> >+
\
>> >+ if (rv == 0) {
\
>> >+ VIR_FREE(chr->source->data.nix.path);
\
>> >+ ret = 0;
\
>> >+ goto cleanup;
\
>> >+ }
>> >+
>> >+ VIR_CLEAR_CHR_DEF_PATH("^/[^/]+\\.$")
>> >+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[^/]+/$")
>> >+ VIR_CLEAR_CHR_DEF_PATH("^/domain-[0-9]+-[^/]+/$")
>> >+
>>
>> Just check for ^{cfg->channelTargetDir}/[^/]+[./]qemu/{chr->targetName}
>>
>> And make sure you escape stuff in squiggly brackets. That should work
>> and be easier. Also don't forget adapting it to the boolean return
>> value of virStringMatch
>
>That's exactly what I wanted to avoid, escaping the path where we can
>simply use STRPREFIX and STRNEQ.
>
Which you can't. And you are constructing and doing the regexp match
three times. Moreover you don't need to escape much. Just
"^$.|?*+()[]{}\" and all those with just a backslash. You can add few
more checks there, but just doing one escape and one match is way more
readable.
Well, I can :) but constructing only one regexp to match all three
different path patterns is better. I'll introduce virBufferEscapeRegex
to do the escape and the regexp would be slightly different than the one
you've proposed:
^{cfg->channelTargetDir}/([^/]+\\.)|(domain-[^/]+/){chr->targetName}$
There is no "qemu" in the path.
I'll send v2 shortly, thanks.
Pavel
2787
days inactive
2788
days old
9 comments
3 participants
participants (3)
-
John Ferlan -
Martin Kletzander -
Pavel Hrdina