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(a)redhat.com&gt; Reviewed-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; Reviewed-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; Reviewed-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; Reviewed-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- 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

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 and --help and mock a stat() that's done in dnsmasqCapsRefreshInternal(). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/networkxml2conftest.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 13257c749b..718a031879 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -12,6 +12,8 @@ #include "viralloc.h" #include "network/bridge_driver.h" #include "virstring.h" +#define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW +#include "vircommandpriv.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -108,13 +110,44 @@ testCompareXMLToConfHelper(const void *data) return result; } +static void +buildCapsCallback(const char *const*args, + const char *const*env G_GNUC_UNUSED, + const char *input G_GNUC_UNUSED, + char **output, + char **error G_GNUC_UNUSED, + int *status, + void *opaque G_GNUC_UNUSED) +{ + if (STREQ(args[0], "/usr/sbin/dnsmasq") && STREQ(args[1], "--version")) { + *output = g_strdup("Dnsmasq version 2.67\n"); + *status = EXIT_SUCCESS; + } else { + *status = EXIT_FAILURE; + } +} + +static dnsmasqCaps * +buildCaps(void) +{ + g_autoptr(dnsmasqCaps) caps = NULL; + g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); + + virCommandSetDryRun(dryRunToken, NULL, true, true, buildCapsCallback, NULL); + + caps = dnsmasqCapsNewFromBinary(); + + return g_steal_pointer(&caps); +} + + static int mymain(void) { int ret = 0; g_autoptr(dnsmasqCaps) full = NULL; - full = dnsmasqCapsNewFromBuffer("Dnsmasq version 2.67"); + full = buildCaps(); #define DO_TEST(xname, xcaps) \ do { \ -- 2.34.1

The function is no longer used. Remove it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Reviewed-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- 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