In v6.10.0-rc1~221 I wanted to make virNetDevOpenvswitchGetVhostuserIfname() lookup interface name even for vhostuser interfaces with mode='server'. For these, were are given a socket path which is then created by QEMU and to which OpenVSwitch connects to and creates an interface. Because of this, we don't know the name of the interface upfront (when starting QEMU) and have to use the path to query OpenVSwitch later (using ovs-vsctl). What I intended to use was: ovs-vsctl --no-headings --columns=name find Interface options:vhost-server-path=$path But what my code does is: ovs-vsctl --no-headings --columns=name find Interface options:vhost-server-path=path and it's all because the argument to the function is named "path" which I then enclosed in double quotes while it should have been used as a variable. Fixes: e4c29e2904197472919d050c67acfd59f0144bbc Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1767013 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virnetdevopenvswitch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index d380b0cf22..7eabaa763d 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -494,7 +494,7 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path, if (server) { virCommandAddArgList(cmd, "--no-headings", "--columns=name", "find", "Interface", NULL); - virCommandAddArgPair(cmd, "options:vhost-server-path", "path"); + virCommandAddArgPair(cmd, "options:vhost-server-path", path); } else { const char *tmpIfname = NULL; -- 2.26.2

During testing of my patch v6.10.0-rc1~221 it was found that 'ovs-vsctl get Interface $name name' or 'ovs-vsctl find Interface options:vhost-server-path=$path' may return a string in double quotes, e.g. "vhost-user1". Later investigation of openvswitch code showed, that early versions (like 1.3.0) have somewhat restrictive set of safe characters (isalpha() || '_' || '-' || '.'), which is then refined with increasing version. For instance, version 2.11.4 has: isalnum() || '_' || '-' || '.'. If the string that ovs-vsctl wants to output contains any other character it is escaped. You want to be looking at ovsdb_atom_to_string() which handles outputting of a single string and calls string_needs_quotes() and possibly json_serialize_string() in openvswitch code base. Since the interfaces are usually named "vhost-userN" we are facing a problem where with one version we get the name in double quotes and with another we get plain name without funny business. Because of json involved I thought, let's make ovs-vsctl output into JSON format and then use our JSON parser, but guess what - ovs-vsctl ignores --format=json. But with a little help of g_strdup_printf() it can be turned into JSON. Fixes: e4c29e2904197472919d050c67acfd59f0144bbc Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1767013 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virnetdevopenvswitch.c | 47 +++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 4 +++ tests/virnetdevopenvswitchtest.c | 52 ++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c7c37d9689..583fc5800e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2676,6 +2676,7 @@ virNetDevOpenvswitchGetVhostuserIfname; virNetDevOpenvswitchInterfaceGetMaster; virNetDevOpenvswitchInterfaceParseStats; virNetDevOpenvswitchInterfaceStats; +virNetDevOpenvswitchMaybeUnescapeReply; virNetDevOpenvswitchRemovePort; virNetDevOpenvswitchSetMigrateData; virNetDevOpenvswitchSetTimeout; diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 7eabaa763d..14fa294ae1 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -460,6 +460,48 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master) } +/** + * virNetDevOpenvswitchMaybeUnescapeReply: + * @reply: a string to unescape + * + * Depending on ovs-vsctl version a string might be escaped. For instance: + * -version 2.11.4 allows only is_alpha(), an underscore, a dash or a dot, + * -version 2.14.0 allows only is_alnum(), an underscore, a dash or a dot, + * any other character causes the string to be escaped. + * + * What this function does, is it checks whether @reply string consists solely + * from safe, not escaped characters (as defined by version 2.14.0) and if not + * an error is reported. If @reply is a string enclosed in double quotes, but + * otherwise safe those double quotes are removed. + * + * Returns: 0 on success, + * -1 otherwise (with error reported). + */ +int +virNetDevOpenvswitchMaybeUnescapeReply(char *reply) +{ + g_autoptr(virJSONValue) json = NULL; + g_autofree char *jsonStr = NULL; + const char *tmp = NULL; + size_t replyLen = strlen(reply); + + if (*reply != '"') + return 0; + + jsonStr = g_strdup_printf("{\"name\": %s}", reply); + if (!(json = virJSONValueFromString(jsonStr))) + return -1; + + if (!(tmp = virJSONValueObjectGetString(json, "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed ovs-vsctl output")); + return -1; + } + + return virStrcpy(reply, tmp, replyLen); +} + + /** * virNetDevOpenvswitchGetVhostuserIfname: * @path: the path of the unix socket @@ -522,6 +564,11 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path, return 0; } + if (virNetDevOpenvswitchMaybeUnescapeReply(*ifname) < 0) { + VIR_FREE(*ifname); + return -1; + } + return 1; } diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 8409aa92ac..3571708582 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -60,6 +60,10 @@ int virNetDevOpenvswitchInterfaceStats(const char *ifname, int virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master) ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; +int +virNetDevOpenvswitchMaybeUnescapeReply(char *reply) + ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; + int virNetDevOpenvswitchGetVhostuserIfname(const char *path, bool server, char **ifname) diff --git a/tests/virnetdevopenvswitchtest.c b/tests/virnetdevopenvswitchtest.c index fd47e927ea..46172dae90 100644 --- a/tests/virnetdevopenvswitchtest.c +++ b/tests/virnetdevopenvswitchtest.c @@ -75,6 +75,42 @@ testInterfaceParseStats(const void *opaque) } +typedef struct _escapeData escapeData; +struct _escapeData { + const char *input; + const char *expect; +}; + + +static int +testNameEscape(const void *opaque) +{ + const escapeData *data = opaque; + g_autofree char *reply = g_strdup(data->input); + int rv; + + rv = virNetDevOpenvswitchMaybeUnescapeReply(reply); + + if (data->expect) { + if (rv < 0 || STRNEQ(reply, data->expect)) { + fprintf(stderr, + "Unexpected failure, expected: %s for input %s got %s\n", + data->expect, data->input, reply); + return -1; + } + } else { + if (rv >= 0) { + fprintf(stderr, + "Unexpected success, input %s got %s\n", + data->input, reply); + return -1; + } + } + + return 0; +} + + static int mymain(void) { @@ -94,6 +130,22 @@ mymain(void) TEST_INTERFACE_STATS("stats1.json", 9, 12, 11, 10, 2, 8, 5, 4); TEST_INTERFACE_STATS("stats2.json", 12406, 173, 0, 0, 0, 0, 0, 0); +#define TEST_NAME_ESCAPE(str, fail) \ + do { \ + const escapeData data = {str, fail};\ + if (virTestRun("Name escape " str, testNameEscape, &data) < 0) \ + ret = -1; \ + } while (0) + + TEST_NAME_ESCAPE("", ""); + TEST_NAME_ESCAPE("\"\"", ""); + TEST_NAME_ESCAPE("vhost-user1", "vhost-user1"); + TEST_NAME_ESCAPE("\"vhost-user1\"", "vhost-user1"); + TEST_NAME_ESCAPE("\"vhost_user-name.to.escape1", NULL); + TEST_NAME_ESCAPE("\"vhost_user-name.to\\\"escape1\"", "vhost_user-name.to\"escape1"); + TEST_NAME_ESCAPE("\"vhost\"user1\"", NULL); + TEST_NAME_ESCAPE("\"\\\\", NULL); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.26.2

On 12/16/20 8:17 PM, Laine Stump wrote: Honestly, I did not bother exactly because of the reason you're mentioning: we have to support both. Thank you sir! Michal