[PATCH v4 00/13] virdnsmasq: Lookup DNSMASQ in PATH
v4 of: https://listman.redhat.com/archives/libvir-list/2022-January/msg00498.html diff to v3: - Removed even more code - Switched from virCommandDryRun to a python script, per Andrea's request - Reordered patches so that the mock doesn't need to redirect stat() - Renamed mock - Removed exec() of 'dnsmasq --help' Michal Prívozník (13): virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty() lib: Prefer g_autoptr(dnsmasqCaps) instead of explicit unref virdnsmasq: Drop @force argument of dnsmasqCapsRefreshInternal() virdnsmasq: Drop mtime member from struct _dnsmasqCaps virdnsmasq: Drop noRefresh member from from struct _dnsmasqCaps virdnsmasq: Drop !caps check from dnsmasqCapsRefreshInternal() virdnsmasq: Don't run 'dnsmasq --help' virdnsmasq: Lookup DNSMASQ in PATH virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath() networkxml2conftest: Use dnsmasqCapsNewFromBinary() to construct caps networkxml2conftest: Check if capabilities were created successfully virdnsmasq: Drop dnsmasqCapsNewFromBuffer() virdnsmasq: Join dnsmasqCapsNewEmpty() and dnsmasqCapsNewFromBinary() meson.build | 1 - src/libvirt_private.syms | 1 - src/util/virdnsmasq.c | 84 ++++++------------------------------- src/util/virdnsmasq.h | 1 - tests/dnsmasqmock.py | 13 ++++++ tests/meson.build | 1 + tests/networkxml2conftest.c | 13 +++--- tests/virdnsmasqmock.c | 19 +++++++++ 8 files changed, 54 insertions(+), 79 deletions(-) create mode 100755 tests/dnsmasqmock.py create mode 100644 tests/virdnsmasqmock.c -- 2.34.1
Both callers of dnsmasqCapsNewEmpty() pass DNSMASQ as an argument which is then fed to a ternary operator which looks like this (after substitution). DNSMASQ ? DNSMASQ : DNSMASQ While I like tautologies, the code can be simplified by dropping the argument. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- src/util/virdnsmasq.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index e5edec2b64..d304929d51 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -700,7 +700,7 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) } static dnsmasqCaps * -dnsmasqCapsNewEmpty(const char *binaryPath) +dnsmasqCapsNewEmpty(void) { dnsmasqCaps *caps; @@ -708,14 +708,14 @@ dnsmasqCapsNewEmpty(const char *binaryPath) return NULL; if (!(caps = virObjectNew(dnsmasqCapsClass))) return NULL; - caps->binaryPath = g_strdup(binaryPath ? binaryPath : DNSMASQ); + caps->binaryPath = g_strdup(DNSMASQ); return caps; } dnsmasqCaps * dnsmasqCapsNewFromBuffer(const char *buf) { - dnsmasqCaps *caps = dnsmasqCapsNewEmpty(DNSMASQ); + dnsmasqCaps *caps = dnsmasqCapsNewEmpty(); if (!caps) return NULL; @@ -730,7 +730,7 @@ dnsmasqCapsNewFromBuffer(const char *buf) dnsmasqCaps * dnsmasqCapsNewFromBinary(void) { - dnsmasqCaps *caps = dnsmasqCapsNewEmpty(DNSMASQ); + dnsmasqCaps *caps = dnsmasqCapsNewEmpty(); if (!caps) return NULL; -- 2.34.1
The dnsmasqCaps type has its own cleanup function defined and ready to use via g_autoptr(). Use automatic cleanup instead of an explicit one. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- src/util/virdnsmasq.c | 22 ++++++++++------------ tests/networkxml2conftest.c | 7 +++---- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index d304929d51..9f3da1d5e6 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -702,44 +702,42 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) static dnsmasqCaps * dnsmasqCapsNewEmpty(void) { - dnsmasqCaps *caps; + g_autoptr(dnsmasqCaps) caps = NULL; if (dnsmasqCapsInitialize() < 0) return NULL; if (!(caps = virObjectNew(dnsmasqCapsClass))) return NULL; caps->binaryPath = g_strdup(DNSMASQ); - return caps; + return g_steal_pointer(&caps); } dnsmasqCaps * dnsmasqCapsNewFromBuffer(const char *buf) { - dnsmasqCaps *caps = dnsmasqCapsNewEmpty(); + g_autoptr(dnsmasqCaps) caps = dnsmasqCapsNewEmpty(); if (!caps) return NULL; - if (dnsmasqCapsSetFromBuffer(caps, buf) < 0) { - virObjectUnref(caps); + if (dnsmasqCapsSetFromBuffer(caps, buf) < 0) return NULL; - } - return caps; + + return g_steal_pointer(&caps); } dnsmasqCaps * dnsmasqCapsNewFromBinary(void) { - dnsmasqCaps *caps = dnsmasqCapsNewEmpty(); + g_autoptr(dnsmasqCaps) caps = dnsmasqCapsNewEmpty(); if (!caps) return NULL; - if (dnsmasqCapsRefreshInternal(caps, true) < 0) { - virObjectUnref(caps); + if (dnsmasqCapsRefreshInternal(caps, true) < 0) return NULL; - } - return caps; + + return g_steal_pointer(&caps); } const char * diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 178c74d9af..d79c2b4783 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -112,8 +112,9 @@ static int mymain(void) { int ret = 0; - dnsmasqCaps *full - = dnsmasqCapsNewFromBuffer("Dnsmasq version 2.67\n--bind-dynamic\n--ra-param"); + g_autoptr(dnsmasqCaps) full = NULL; + + full = dnsmasqCapsNewFromBuffer("Dnsmasq version 2.67\n--bind-dynamic\n--ra-param"); #define DO_TEST(xname, xcaps) \ do { \ @@ -154,8 +155,6 @@ mymain(void) DO_TEST("leasetime-hours", full); DO_TEST("leasetime-infinite", full); - virObjectUnref(full); - return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.34.1
This argument is not used really as the only caller passes true and dnsmasqCapsRefreshInternal() only checks for false value. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virdnsmasq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 9f3da1d5e6..7fcaadda3f 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -649,7 +649,7 @@ dnsmasqCapsSetFromBuffer(dnsmasqCaps *caps, const char *buf) } static int -dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) +dnsmasqCapsRefreshInternal(dnsmasqCaps *caps) { struct stat sb; g_autoptr(virCommand) vercmd = NULL; @@ -666,7 +666,7 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) caps->binaryPath); return -1; } - if (!force && caps->mtime == sb.st_mtime) + if (caps->mtime == sb.st_mtime) return 0; caps->mtime = sb.st_mtime; @@ -734,7 +734,7 @@ dnsmasqCapsNewFromBinary(void) if (!caps) return NULL; - if (dnsmasqCapsRefreshInternal(caps, true) < 0) + if (dnsmasqCapsRefreshInternal(caps) < 0) return NULL; return g_steal_pointer(&caps); -- 2.34.1
The _dnsmasqCaps struct has @mtime member which holds the mtime of the dnsmasq binary. The idea was that capabilities don't need to be queried if mtime hasn't changed since the last time. However, the code that would try to query capabilities again was removed and now we are left with code that stores mtime but has no use for it. Remove the member and code that uses it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virdnsmasq.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 7fcaadda3f..c3801d622f 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -577,7 +577,6 @@ struct _dnsmasqCaps { virObject parent; char *binaryPath; bool noRefresh; - time_t mtime; unsigned long version; }; @@ -651,7 +650,6 @@ dnsmasqCapsSetFromBuffer(dnsmasqCaps *caps, const char *buf) static int dnsmasqCapsRefreshInternal(dnsmasqCaps *caps) { - struct stat sb; g_autoptr(virCommand) vercmd = NULL; g_autoptr(virCommand) helpcmd = NULL; g_autofree char *help = NULL; @@ -661,15 +659,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps) if (!caps || caps->noRefresh) return 0; - if (stat(caps->binaryPath, &sb) < 0) { - virReportSystemError(errno, _("Cannot check dnsmasq binary %s"), - caps->binaryPath); - return -1; - } - if (caps->mtime == sb.st_mtime) - return 0; - caps->mtime = sb.st_mtime; - /* Make sure the binary we are about to try exec'ing exists. * Technically we could catch the exec() failure, but that's * in a sub-process so it's hard to feed back a useful error. -- 2.34.1
The noRefresh member of _dnsmasqCaps struct is set only after it was checked for and is never checked again. This is needless and the member can be removed. There is no way that dnsmasqCapsRefreshInternal() can be called after dnsmasqCapsSetFromBuffer(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virdnsmasq.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index c3801d622f..a8fb91ed2b 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -576,7 +576,6 @@ dnsmasqReload(pid_t pid G_GNUC_UNUSED) struct _dnsmasqCaps { virObject parent; char *binaryPath; - bool noRefresh; unsigned long version; }; @@ -609,8 +608,6 @@ dnsmasqCapsSetFromBuffer(dnsmasqCaps *caps, const char *buf) int len; const char *p; - caps->noRefresh = true; - p = STRSKIP(buf, DNSMASQ_VERSION_STR); if (!p) goto error; @@ -656,7 +653,7 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps) g_autofree char *version = NULL; g_autofree char *complete = NULL; - if (!caps || caps->noRefresh) + if (!caps) return 0; /* Make sure the binary we are about to try exec'ing exists. -- 2.34.1
There is no way that the dnsmasqCapsRefreshInternal() function can be called with @caps == NULL. Therefore, drop the if() that checks for that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virdnsmasq.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index a8fb91ed2b..9f50ce7755 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -653,9 +653,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps) g_autofree char *version = NULL; g_autofree char *complete = NULL; - if (!caps) - return 0; - /* Make sure the binary we are about to try exec'ing exists. * Technically we could catch the exec() failure, but that's * in a sub-process so it's hard to feed back a useful error. -- 2.34.1
We don't query any capabilities of dnsmasq. We are only interested in dnsmasq's version (obtained via 'dnsmasq --version'). Therefore, there's no point in running 'dnsmasq --help'. Its output is not processed even. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virdnsmasq.c | 14 +------------- tests/networkxml2conftest.c | 2 +- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 9f50ce7755..5bed8817e5 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -648,10 +648,7 @@ static int dnsmasqCapsRefreshInternal(dnsmasqCaps *caps) { g_autoptr(virCommand) vercmd = NULL; - g_autoptr(virCommand) helpcmd = NULL; - g_autofree char *help = NULL; g_autofree char *version = NULL; - g_autofree char *complete = NULL; /* Make sure the binary we are about to try exec'ing exists. * Technically we could catch the exec() failure, but that's @@ -670,16 +667,7 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps) if (virCommandRun(vercmd, NULL) < 0) return -1; - helpcmd = virCommandNewArgList(caps->binaryPath, "--help", NULL); - virCommandSetOutputBuffer(helpcmd, &help); - virCommandAddEnvPassCommon(helpcmd); - virCommandClearCaps(helpcmd); - if (virCommandRun(helpcmd, NULL) < 0) - return -1; - - complete = g_strdup_printf("%s\n%s", version, help); - - return dnsmasqCapsSetFromBuffer(caps, complete); + return dnsmasqCapsSetFromBuffer(caps, version); } static dnsmasqCaps * diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index d79c2b4783..6a2c70ead1 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -114,7 +114,7 @@ mymain(void) int ret = 0; g_autoptr(dnsmasqCaps) full = NULL; - full = dnsmasqCapsNewFromBuffer("Dnsmasq version 2.67\n--bind-dynamic\n--ra-param"); + full = dnsmasqCapsNewFromBuffer("Dnsmasq version 2.67"); #define DO_TEST(xname, xcaps) \ do { \ -- 2.34.1
While it's true that our virCommand subsystem is happy with non-absolute paths, the dnsmasq capability code is not. It stores the path to dnsmasq within and makes it accessible via dnsmasqCapsGetBinaryPath(). While strictly speaking no caller necessarily needs canonicalized path, let's find dnsmasq once and cache the result. Therefore, when constructing the capabilities structure look up the binary path. If DNSMASQ already contains an absolute path then virFindFileInPath() will simply return a copy. With this code in place, the virFileIsExecutable() check can be removed from dnsmasqCapsRefreshInternal() because virFindFileInPath() already made sure the binary is executable. But introducing virFindFileInPath() means we have to mock it in test suite because dnsmasqCaps are created in networkxml2conftest. Moreover, we don't need to check for dnsmasq in configure. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- meson.build | 1 - src/util/virdnsmasq.c | 19 ++++++++----------- tests/meson.build | 1 + tests/networkxml2conftest.c | 3 ++- tests/virdnsmasqmock.c | 19 +++++++++++++++++++ 5 files changed, 30 insertions(+), 13 deletions(-) create mode 100644 tests/virdnsmasqmock.c diff --git a/meson.build b/meson.build index c2e9619f1f..70843afcd5 100644 --- a/meson.build +++ b/meson.build @@ -813,7 +813,6 @@ endforeach optional_programs = [ 'augparse', 'dmidecode', - 'dnsmasq', 'ebtables', 'flake8', 'ip', diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 5bed8817e5..579c67d86a 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -46,6 +46,7 @@ VIR_LOG_INIT("util.dnsmasq"); +#define DNSMASQ "dnsmasq" #define DNSMASQ_HOSTSFILE_SUFFIX "hostsfile" #define DNSMASQ_ADDNHOSTSFILE_SUFFIX "addnhosts" @@ -650,16 +651,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps) g_autoptr(virCommand) vercmd = NULL; g_autofree char *version = NULL; - /* Make sure the binary we are about to try exec'ing exists. - * Technically we could catch the exec() failure, but that's - * in a sub-process so it's hard to feed back a useful error. - */ - if (!virFileIsExecutable(caps->binaryPath)) { - virReportSystemError(errno, _("dnsmasq binary %s is not executable"), - caps->binaryPath); - return -1; - } - vercmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); virCommandSetOutputBuffer(vercmd, &version); virCommandAddEnvPassCommon(vercmd); @@ -679,7 +670,13 @@ dnsmasqCapsNewEmpty(void) return NULL; if (!(caps = virObjectNew(dnsmasqCapsClass))) return NULL; - caps->binaryPath = g_strdup(DNSMASQ); + + if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) { + virReportSystemError(ENOENT, "%s", + _("Unable to find 'dnsmasq' binary in $PATH")); + return NULL; + } + return g_steal_pointer(&caps); } diff --git a/tests/meson.build b/tests/meson.build index 4792220b48..8f92f2033f 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -59,6 +59,7 @@ mock_libs = [ { 'name': 'domaincapsmock' }, { 'name': 'shunload', 'sources': [ 'shunloadhelper.c' ] }, { 'name': 'vircgroupmock' }, + { 'name': 'virdnsmasqmock' }, { 'name': 'virfilecachemock' }, { 'name': 'virfirewallmock' }, { 'name': 'virgdbusmock' }, diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 6a2c70ead1..13257c749b 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -158,4 +158,5 @@ mymain(void) return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIR_TEST_MAIN(mymain) +VIR_TEST_MAIN_PRELOAD(mymain, + VIR_TEST_MOCK("virdnsmasq")) diff --git a/tests/virdnsmasqmock.c b/tests/virdnsmasqmock.c new file mode 100644 index 0000000000..448332ec40 --- /dev/null +++ b/tests/virdnsmasqmock.c @@ -0,0 +1,19 @@ +/* + * Copyright (C) 2022 Red Hat, Inc. + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#include <config.h> + +#include "internal.h" +#include "virfile.h" + +char * +virFindFileInPath(const char *file) +{ + if (STREQ_NULLABLE(file, "dnsmasq")) + return g_strdup("/usr/sbin/dnsmasq"); + + /* We should not need any other binaries so return NULL. */ + return NULL; +} -- 2.34.1
First observation: There is no way that caps->binaryPath can be NULL. Second observation: There is no caller that passes NULL. Let's drop the ternary operator and access @caps directly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- src/util/virdnsmasq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 579c67d86a..841689b782 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -711,7 +711,7 @@ dnsmasqCapsNewFromBinary(void) const char * dnsmasqCapsGetBinaryPath(dnsmasqCaps *caps) { - return caps ? caps->binaryPath : DNSMASQ; + return caps->binaryPath; } /** dnsmasqDhcpHostsToString: -- 2.34.1
DISCLAIMER: dnsmasq capabilities are empty as of v8.0.0-rc1~145. In a real environment the dnsmasq capabilities are constructed using dnsmasqCapsNewFromBinary(). We also have dnsmasqCapsNewFromBuffer() to bypass checks that real code is doing and just get capabilities object. The latter is used from test suite. However, with a little bit of mocking we can test the real life code. All that's needed is to simulate dnsmasq's output for --version. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/dnsmasqmock.py | 13 +++++++++++++ tests/networkxml2conftest.c | 2 +- tests/virdnsmasqmock.c | 2 +- 3 files changed, 15 insertions(+), 2 deletions(-) create mode 100755 tests/dnsmasqmock.py diff --git a/tests/dnsmasqmock.py b/tests/dnsmasqmock.py new file mode 100755 index 0000000000..0cfc65a854 --- /dev/null +++ b/tests/dnsmasqmock.py @@ -0,0 +1,13 @@ +#!/usr/bin/env python3 + +import sys + +output = { + "--version": "Dnsmasq version 2.67", +} + +if len(sys.argv) != 2 or sys.argv[1] not in output: + print("invalid usage") + sys.exit(1) + +print(output[sys.argv[1]]) diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 13257c749b..f96a92b9f0 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -114,7 +114,7 @@ mymain(void) int ret = 0; g_autoptr(dnsmasqCaps) full = NULL; - full = dnsmasqCapsNewFromBuffer("Dnsmasq version 2.67"); + full = dnsmasqCapsNewFromBinary(); #define DO_TEST(xname, xcaps) \ do { \ diff --git a/tests/virdnsmasqmock.c b/tests/virdnsmasqmock.c index 448332ec40..b043c5ceca 100644 --- a/tests/virdnsmasqmock.c +++ b/tests/virdnsmasqmock.c @@ -12,7 +12,7 @@ char * virFindFileInPath(const char *file) { if (STREQ_NULLABLE(file, "dnsmasq")) - return g_strdup("/usr/sbin/dnsmasq"); + return g_strdup(abs_srcdir "/dnsmasqmock.py"); /* We should not need any other binaries so return NULL. */ return NULL; -- 2.34.1
Now that looking up dnsmasq is handled/mocked we can start checking whether dnsmasq capabilities were built successfully and error out if that wasn't the case. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/networkxml2conftest.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index f96a92b9f0..6bc0c1465e 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -114,7 +114,10 @@ mymain(void) int ret = 0; g_autoptr(dnsmasqCaps) full = NULL; - full = dnsmasqCapsNewFromBinary(); + if (!(full = dnsmasqCapsNewFromBinary())) { + fprintf(stderr, "failed to create the fake capabilities"); + return EXIT_FAILURE; + } #define DO_TEST(xname, xcaps) \ do { \ -- 2.34.1
The function is no longer used. Remove it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virdnsmasq.c | 14 -------------- src/util/virdnsmasq.h | 1 - 3 files changed, 16 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f75dea36c4..2b4a44d696 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2117,7 +2117,6 @@ dnsmasqAddDhcpHost; dnsmasqAddHost; dnsmasqCapsGetBinaryPath; dnsmasqCapsNewFromBinary; -dnsmasqCapsNewFromBuffer; dnsmasqContextFree; dnsmasqContextNew; dnsmasqDelete; diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 841689b782..f5029d2fdc 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -680,20 +680,6 @@ dnsmasqCapsNewEmpty(void) return g_steal_pointer(&caps); } -dnsmasqCaps * -dnsmasqCapsNewFromBuffer(const char *buf) -{ - g_autoptr(dnsmasqCaps) caps = dnsmasqCapsNewEmpty(); - - if (!caps) - return NULL; - - if (dnsmasqCapsSetFromBuffer(caps, buf) < 0) - return NULL; - - return g_steal_pointer(&caps); -} - dnsmasqCaps * dnsmasqCapsNewFromBinary(void) { diff --git a/src/util/virdnsmasq.h b/src/util/virdnsmasq.h index c74cc887f8..cc1c337d41 100644 --- a/src/util/virdnsmasq.h +++ b/src/util/virdnsmasq.h @@ -91,7 +91,6 @@ int dnsmasqSave(const dnsmasqContext *ctx); int dnsmasqDelete(const dnsmasqContext *ctx); int dnsmasqReload(pid_t pid); -dnsmasqCaps *dnsmasqCapsNewFromBuffer(const char *buf); dnsmasqCaps *dnsmasqCapsNewFromBinary(void); const char *dnsmasqCapsGetBinaryPath(dnsmasqCaps *caps); char *dnsmasqDhcpHostsToString(dnsmasqDhcpHost *hosts, -- 2.34.1
After previous cleanups, there's just one caller of dnsmasqCapsNewEmpty() and it is dnsmasqCapsNewFromBinary(). And the former is pretty short. Therefore, it is not necessary for the code to live in two separate functions. Dissolve the former in the latter. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- src/util/virdnsmasq.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index f5029d2fdc..57b24c9f6a 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -661,13 +661,14 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps) return dnsmasqCapsSetFromBuffer(caps, version); } -static dnsmasqCaps * -dnsmasqCapsNewEmpty(void) +dnsmasqCaps * +dnsmasqCapsNewFromBinary(void) { g_autoptr(dnsmasqCaps) caps = NULL; if (dnsmasqCapsInitialize() < 0) return NULL; + if (!(caps = virObjectNew(dnsmasqCapsClass))) return NULL; @@ -677,17 +678,6 @@ dnsmasqCapsNewEmpty(void) return NULL; } - return g_steal_pointer(&caps); -} - -dnsmasqCaps * -dnsmasqCapsNewFromBinary(void) -{ - g_autoptr(dnsmasqCaps) caps = dnsmasqCapsNewEmpty(); - - if (!caps) - return NULL; - if (dnsmasqCapsRefreshInternal(caps) < 0) return NULL; -- 2.34.1
On Mon, Jan 17, 2022 at 04:19:14PM +0100, Michal Privoznik wrote:
v4 of:
https://listman.redhat.com/archives/libvir-list/2022-January/msg00498.html
diff to v3: - Removed even more code - Switched from virCommandDryRun to a python script, per Andrea's request - Reordered patches so that the mock doesn't need to redirect stat() - Renamed mock - Removed exec() of 'dnsmasq --help'
Michal Prívozník (13): virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty() lib: Prefer g_autoptr(dnsmasqCaps) instead of explicit unref virdnsmasq: Drop @force argument of dnsmasqCapsRefreshInternal() virdnsmasq: Drop mtime member from struct _dnsmasqCaps virdnsmasq: Drop noRefresh member from from struct _dnsmasqCaps virdnsmasq: Drop !caps check from dnsmasqCapsRefreshInternal() virdnsmasq: Don't run 'dnsmasq --help' virdnsmasq: Lookup DNSMASQ in PATH virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath() networkxml2conftest: Use dnsmasqCapsNewFromBinary() to construct caps networkxml2conftest: Check if capabilities were created successfully virdnsmasq: Drop dnsmasqCapsNewFromBuffer() virdnsmasq: Join dnsmasqCapsNewEmpty() and dnsmasqCapsNewFromBinary()
Reviewed-by: Andrea Bolognani <abologna@redhat.com> Thanks for bearing with me! -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Jan 17, 2022 at 08:24:36AM -0800, Andrea Bolognani wrote:
On Mon, Jan 17, 2022 at 04:19:14PM +0100, Michal Privoznik wrote:
Michal Prívozník (13): virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty() lib: Prefer g_autoptr(dnsmasqCaps) instead of explicit unref virdnsmasq: Drop @force argument of dnsmasqCapsRefreshInternal() virdnsmasq: Drop mtime member from struct _dnsmasqCaps virdnsmasq: Drop noRefresh member from from struct _dnsmasqCaps virdnsmasq: Drop !caps check from dnsmasqCapsRefreshInternal() virdnsmasq: Don't run 'dnsmasq --help' virdnsmasq: Lookup DNSMASQ in PATH virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath() networkxml2conftest: Use dnsmasqCapsNewFromBinary() to construct caps networkxml2conftest: Check if capabilities were created successfully virdnsmasq: Drop dnsmasqCapsNewFromBuffer() virdnsmasq: Join dnsmasqCapsNewEmpty() and dnsmasqCapsNewFromBinary()
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
These changes seem to have made ASAN very unhappy, see https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244739 https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244740 Tim, do you have any idea why that would be the case? My uneducated guess is that the environment needed by ASAN is somehow lost when the dnsmasqmock.py script is called, but I'm unfamiliar with how these tools actually work. -- Andrea Bolognani / Red Hat / Virtualization
On 1/18/22 11:14, Andrea Bolognani wrote:
On Mon, Jan 17, 2022 at 08:24:36AM -0800, Andrea Bolognani wrote:
On Mon, Jan 17, 2022 at 04:19:14PM +0100, Michal Privoznik wrote:
Michal Prívozník (13): virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty() lib: Prefer g_autoptr(dnsmasqCaps) instead of explicit unref virdnsmasq: Drop @force argument of dnsmasqCapsRefreshInternal() virdnsmasq: Drop mtime member from struct _dnsmasqCaps virdnsmasq: Drop noRefresh member from from struct _dnsmasqCaps virdnsmasq: Drop !caps check from dnsmasqCapsRefreshInternal() virdnsmasq: Don't run 'dnsmasq --help' virdnsmasq: Lookup DNSMASQ in PATH virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath() networkxml2conftest: Use dnsmasqCapsNewFromBinary() to construct caps networkxml2conftest: Check if capabilities were created successfully virdnsmasq: Drop dnsmasqCapsNewFromBuffer() virdnsmasq: Join dnsmasqCapsNewEmpty() and dnsmasqCapsNewFromBinary()
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
These changes seem to have made ASAN very unhappy, see
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244739 https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244740
Tim, do you have any idea why that would be the case? My uneducated guess is that the environment needed by ASAN is somehow lost when the dnsmasqmock.py script is called, but I'm unfamiliar with how these tools actually work.
I might know what the problem is. When networkxml2conf script is called the linker links asan and in its initializer it finds no problem. But then main() of the test is called which modifies LD_PRELOAD (by putting all mock libs of ours at the beginning) and re-exec()-s itself. After that, mymain() is called which in turn calls dnsmasqCapsNewFromBinary() to construct capabilities. Transitionally, the following piece of code is executed: vercmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); virCommandSetOutputBuffer(vercmd, &version); virCommandAddEnvPassCommon(vercmd); virCommandClearCaps(vercmd); if (virCommandRun(vercmd, NULL) < 0) Here, virCommandAddEnvPassCommon() makes caps->binaryPath (= tests/dnsmasqmock.py) run with LD_PRELOAD preserved. But at this point the asan is not the first library on the list, our mocks are. Hence, asan is unhappy. The way this is resolved in other tests (e.g. qemuxml2argvtest) is by using virfilewrapper.c. At the beginning of mymain() in qemuxml2argvtest.c there are couple of calls to virFileWrapperAddPrefix(), for instance: virFileWrapperAddPrefix("/usr/libexec/qemu/vhost-user", abs_srcdir "/qemuvhostuserdata/usr/libexec/qemu/vhost-user"); Which means that later when our code wants to execute /usr/libexec/qemu/vhost-user/$binary it actually executes $abs_srcdir/qemuvhostuserdata/user/libexec/qemu/$binary. Sweet. Except that won't fly for networkxml2conf test, will it. In case of qemu we have control over binaries and paths we are executing, but for dnsmasq we want to look the binary up in PATH. Having said that, I could mock virFindFileInPath() just like I am now, except let it return a predictable path (say /usr/sbin/dnsmasq) and then use virFileWrapper...() to redirect /usr/sbin/ to abs_srcdir. Alternative to all of this is to keed virCommandSetDryRun() just like I had in one of previous patches. Remind me please, what was the issue with that? Michal
On Tue, Jan 18, 2022 at 12:30:30PM +0100, Michal Prívozník wrote:
On 1/18/22 11:14, Andrea Bolognani wrote:
These changes seem to have made ASAN very unhappy, see
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244739 https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244740
Tim, do you have any idea why that would be the case? My uneducated guess is that the environment needed by ASAN is somehow lost when the dnsmasqmock.py script is called, but I'm unfamiliar with how these tools actually work.
[...] I could mock virFindFileInPath() just like I am now, except let it return a predictable path (say /usr/sbin/dnsmasq) and then use virFileWrapper...() to redirect /usr/sbin/ to abs_srcdir.
Alternative to all of this is to keed virCommandSetDryRun() just like I had in one of previous patches. Remind me please, what was the issue with that?
The Python script approach seemed simpler, but in light of this issue I guess that argument has gone completely out of the window :) Can you please try a version of this series with your original dnsmasq mocking approach in CI and see whether ASAN is happy with it? If so, we can just go ahead with that one. -- Andrea Bolognani / Red Hat / Virtualization
On 1/18/22 13:57, Andrea Bolognani wrote:
On Tue, Jan 18, 2022 at 12:30:30PM +0100, Michal Prívozník wrote:
On 1/18/22 11:14, Andrea Bolognani wrote:
These changes seem to have made ASAN very unhappy, see
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244739 https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244740
Tim, do you have any idea why that would be the case? My uneducated guess is that the environment needed by ASAN is somehow lost when the dnsmasqmock.py script is called, but I'm unfamiliar with how these tools actually work.
[...] I could mock virFindFileInPath() just like I am now, except let it return a predictable path (say /usr/sbin/dnsmasq) and then use virFileWrapper...() to redirect /usr/sbin/ to abs_srcdir.
Alternative to all of this is to keed virCommandSetDryRun() just like I had in one of previous patches. Remind me please, what was the issue with that?
The Python script approach seemed simpler, but in light of this issue I guess that argument has gone completely out of the window :)
Can you please try a version of this series with your original dnsmasq mocking approach in CI and see whether ASAN is happy with it? If so, we can just go ahead with that one.
Will do. Although, since virCommandRun() wouldn't actually execute anything I don't expect ASAN to raise any issues. Meanwhile, I'm testing the approach I've outlined: https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/450382098 Let's see how it runs. Michal
On 1/18/22 14:05, Michal Prívozník wrote:
On 1/18/22 13:57, Andrea Bolognani wrote:
On Tue, Jan 18, 2022 at 12:30:30PM +0100, Michal Prívozník wrote:
On 1/18/22 11:14, Andrea Bolognani wrote:
These changes seem to have made ASAN very unhappy, see
https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244739 https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244740
Tim, do you have any idea why that would be the case? My uneducated guess is that the environment needed by ASAN is somehow lost when the dnsmasqmock.py script is called, but I'm unfamiliar with how these tools actually work.
[...] I could mock virFindFileInPath() just like I am now, except let it return a predictable path (say /usr/sbin/dnsmasq) and then use virFileWrapper...() to redirect /usr/sbin/ to abs_srcdir.
Alternative to all of this is to keed virCommandSetDryRun() just like I had in one of previous patches. Remind me please, what was the issue with that?
The Python script approach seemed simpler, but in light of this issue I guess that argument has gone completely out of the window :)
Can you please try a version of this series with your original dnsmasq mocking approach in CI and see whether ASAN is happy with it? If so, we can just go ahead with that one.
Will do. Although, since virCommandRun() wouldn't actually execute anything I don't expect ASAN to raise any issues. Meanwhile, I'm testing the approach I've outlined:
https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/450382098
Let's see how it runs.
Aaand I have the results: failed to create the fake capabilities: internal error: Child process (LC_ALL=C LD_PRELOAD=/builds/MichalPrivoznik/libvirt/build/tests/libvirdnsmasqmock.so PATH=/builds/MichalPrivoznik/libvirt/build/tests:/usr/libexec/ccache-wrappers:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin HOME=/bad-test-used-env-home /usr/local/sbin/dnsmasqmock.py --version) unexpected exit status 127: /usr/bin/env: symbol lookup error: /builds/MichalPrivoznik/libvirt/build/tests/../src/libvirt.so.0: undefined symbol: __asan_option_detect_stack_use_after_return Why does dnsmasqmock.py try to link with libvirt.so.0 or why there's a missing symbol is beyond me. So let me just stick with what I suggested initially. Michal
On Tue, Jan 18, 2022 at 02:13:23PM +0100, Michal Prívozník wrote:
On 1/18/22 14:05, Michal Prívozník wrote:
On 1/18/22 13:57, Andrea Bolognani wrote:
Can you please try a version of this series with your original dnsmasq mocking approach in CI and see whether ASAN is happy with it? If so, we can just go ahead with that one.
Will do. Although, since virCommandRun() wouldn't actually execute anything I don't expect ASAN to raise any issues. Meanwhile, I'm testing the approach I've outlined:
https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/450382098
Let's see how it runs.
Aaand I have the results:
failed to create the fake capabilities: internal error: Child process (LC_ALL=C LD_PRELOAD=/builds/MichalPrivoznik/libvirt/build/tests/libvirdnsmasqmock.so PATH=/builds/MichalPrivoznik/libvirt/build/tests:/usr/libexec/ccache-wrappers:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin HOME=/bad-test-used-env-home /usr/local/sbin/dnsmasqmock.py --version) unexpected exit status 127: /usr/bin/env: symbol lookup error: /builds/MichalPrivoznik/libvirt/build/tests/../src/libvirt.so.0: undefined symbol: __asan_option_detect_stack_use_after_return
Why does dnsmasqmock.py try to link with libvirt.so.0 or why there's a missing symbol is beyond me. So let me just stick with what I suggested initially.
Yeah, that sounds good. Can you please make sure the result passes CI and post it to the list? IIRC you've shuffled patches around in the meantime, so we should do one last quick sanity check before pushing. -- Andrea Bolognani / Red Hat / Virtualization
On 1/18/22 15:08, Andrea Bolognani wrote:
On Tue, Jan 18, 2022 at 02:13:23PM +0100, Michal Prívozník wrote:
On 1/18/22 14:05, Michal Prívozník wrote:
On 1/18/22 13:57, Andrea Bolognani wrote:
Can you please try a version of this series with your original dnsmasq mocking approach in CI and see whether ASAN is happy with it? If so, we can just go ahead with that one.
Will do. Although, since virCommandRun() wouldn't actually execute anything I don't expect ASAN to raise any issues. Meanwhile, I'm testing the approach I've outlined:
https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/450382098
Let's see how it runs.
Aaand I have the results:
failed to create the fake capabilities: internal error: Child process (LC_ALL=C LD_PRELOAD=/builds/MichalPrivoznik/libvirt/build/tests/libvirdnsmasqmock.so PATH=/builds/MichalPrivoznik/libvirt/build/tests:/usr/libexec/ccache-wrappers:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin HOME=/bad-test-used-env-home /usr/local/sbin/dnsmasqmock.py --version) unexpected exit status 127: /usr/bin/env: symbol lookup error: /builds/MichalPrivoznik/libvirt/build/tests/../src/libvirt.so.0: undefined symbol: __asan_option_detect_stack_use_after_return
Why does dnsmasqmock.py try to link with libvirt.so.0 or why there's a missing symbol is beyond me. So let me just stick with what I suggested initially.
Yeah, that sounds good. Can you please make sure the result passes CI and post it to the list? IIRC you've shuffled patches around in the meantime, so we should do one last quick sanity check before pushing.
Alright, although the only change I did really was in the patch 10/13 which I replaced with the corresponding patch from my earlier version. But anyway, let's respin another version (hopefully the last). The pipeline's green: https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/450418184 Michal
participants (3)
-
Andrea Bolognani -
Michal Privoznik -
Michal Prívozník