[PATCH v5 00/13] virdnsmasq: Lookup DNSMASQ in PATH
v5 of: https://listman.redhat.com/archives/libvir-list/2022-January/msg00715.html diff to v4: - Switched to virCommandSetDryRun() in commit 10/13, because using a separate script was conflicting with ASAN. Tried to debug, failed miserably. - Extended error message from patch 11/13 to include virGetLastErrorMessage() (because if a test fails one simply doesn't access libvirt log in the CI). 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/meson.build | 1 + tests/networkxml2conftest.c | 47 ++++++++++++++++++--- tests/virdnsmasqmock.c | 19 +++++++++ 7 files changed, 75 insertions(+), 79 deletions(-) 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> Reviewed-by: Andrea Bolognani <abologna@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> Reviewed-by: Andrea Bolognani <abologna@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> Reviewed-by: Andrea Bolognani <abologna@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> Reviewed-by: Andrea Bolognani <abologna@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> Reviewed-by: Andrea Bolognani <abologna@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> Reviewed-by: Andrea Bolognani <abologna@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 and --help and mock a stat() that's done in dnsmasqCapsRefreshInternal(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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
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 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 718a031879..0bc9e128e3 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -147,7 +147,11 @@ mymain(void) int ret = 0; g_autoptr(dnsmasqCaps) full = NULL; - full = buildCaps(); + if (!(full = buildCaps())) { + fprintf(stderr, "failed to create the fake capabilities: %s", + virGetLastErrorMessage()); + 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 Tue, Jan 18, 2022 at 03:37:07PM +0100, Michal Privoznik wrote:
v5 of:
https://listman.redhat.com/archives/libvir-list/2022-January/msg00715.html
diff to v4: - Switched to virCommandSetDryRun() in commit 10/13, because using a separate script was conflicting with ASAN. Tried to debug, failed miserably. - Extended error message from patch 11/13 to include virGetLastErrorMessage() (because if a test fails one simply doesn't access libvirt log in the CI).
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> -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Michal Privoznik