[PATCH v3 0/7] virdnsmasq: Lookup DNSMASQ in PATH
v3 of: https://listman.redhat.com/archives/libvir-list/2022-January/msg00386.html diff to v2: - Make dnsmasqCapsGetBinaryPath() fail if dnsmasq is not found - Change the way caps are constructed in networkxml2conftest - More code cleanup However, as I was going through the code I realized that what we really do is check dnsmasq's version against the minimal required one (2.67). No actual capabilities are parsed. This is due to cleanup merged earlier this release (v8.0.0-rc1~138 and commits around). While we could drop more code I figured it may be worth keeping it for future use. I mean, one day we might want to have an capability for give feature. Michal Prívozník (7): virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty() lib: Prefer g_autoptr(dnsmasqCaps) instead of explicit unref virdnsmasq: Lookup DNSMASQ in PATH virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath() networkxml2conftest: Use dnsmasqCapsNewFromBinary() to construct caps virdnsmasq: Drop dnsmasqCapsNewFromBuffer() virdnsmasq: Join dnsmasqCapsNewEmpty() and dnsmasqCapsNewFromBinary() src/libvirt_private.syms | 1 - src/util/virdnsmasq.c | 50 ++++++++----------------------------- src/util/virdnsmasq.h | 1 - tests/meson.build | 1 + tests/networkmock.c | 46 ++++++++++++++++++++++++++++++++++ tests/networkxml2conftest.c | 46 ++++++++++++++++++++++++++++++---- 6 files changed, 98 insertions(+), 47 deletions(-) create mode 100644 tests/networkmock.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> --- src/util/virdnsmasq.c | 18 ++++++++---------- tests/networkxml2conftest.c | 7 +++---- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index d304929d51..f4bdab116e 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -715,31 +715,29 @@ dnsmasqCapsNewEmpty(void) 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
On Wed, Jan 12, 2022 at 09:47:53AM +0100, Michal Privoznik wrote:
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> --- src/util/virdnsmasq.c | 18 ++++++++---------- tests/networkxml2conftest.c | 7 +++---- 2 files changed, 11 insertions(+), 14 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
While it's true that our virCommand subsystem is happy with non-absolute paths, the dnsmasq capability code is not. For instance, it does call stat() over the binary to learn its mtime (and thus decide whether capabilities need to be fetched again or not). 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 virFileIsExecutable() means we have to mock it in test suite because dnsmasqCaps are created in networkxml2conftest. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virdnsmasq.c | 22 +++++++++------------- tests/meson.build | 1 + tests/networkmock.c | 30 ++++++++++++++++++++++++++++++ tests/networkxml2conftest.c | 3 ++- 4 files changed, 42 insertions(+), 14 deletions(-) create mode 100644 tests/networkmock.c diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index f4bdab116e..79060e2021 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -670,16 +670,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) 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. - */ - 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); @@ -702,14 +692,20 @@ 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; + + if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) { + virReportSystemError(ENOENT, "%s", + _("Unable to find 'dnsmasq' binary in $PATH")); + return NULL; + } + + return g_steal_pointer(&caps); } dnsmasqCaps * diff --git a/tests/meson.build b/tests/meson.build index 4792220b48..e8f5d4c1f7 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -57,6 +57,7 @@ endif mock_libs = [ { 'name': 'domaincapsmock' }, + { 'name': 'networkmock' }, { 'name': 'shunload', 'sources': [ 'shunloadhelper.c' ] }, { 'name': 'vircgroupmock' }, { 'name': 'virfilecachemock' }, diff --git a/tests/networkmock.c b/tests/networkmock.c new file mode 100644 index 0000000000..a9c13311a6 --- /dev/null +++ b/tests/networkmock.c @@ -0,0 +1,30 @@ +/* + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "internal.h" +#include "virfile.h" + +char * +virFindFileInPath(const char *file) +{ + if (file && g_strrstr(file, "dnsmasq")) + return g_strdup(file); + + /* We should not need any other binaries so return NULL. */ + return NULL; +} diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index d79c2b4783..8a6657654a 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("network")) -- 2.34.1
On Wed, Jan 12, 2022 at 09:47:54AM +0100, Michal Privoznik wrote:
With this code in place, the virFileIsExecutable() check can be removed from dnsmasqCapsRefreshInternal() because virFindFileInPath() already made sure the binary is executable.
But introducing virFileIsExecutable() means we have to mock it in
I believe you meant virFindFileInPath() here.
+++ b/src/util/virdnsmasq.c @@ -702,14 +692,20 @@ 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; + + if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) { + virReportSystemError(ENOENT, "%s", + _("Unable to find 'dnsmasq' binary in $PATH")); + return NULL; + } + + return g_steal_pointer(&caps); }
Can you please squash the g_autoptr conversion into the previous patch?
+++ b/tests/networkmock.c @@ -0,0 +1,30 @@ +/* + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */
Missing copyright information. Also can we have an SPDX tag instead of the full license blurb? See tests/virhostdevmock.c for an example of what I have in mind.
+char * +virFindFileInPath(const char *file) +{ + if (file && g_strrstr(file, "dnsmasq")) + return g_strdup(file);
This mock is somewhat inaccurate because, if dnsmasq was not found at configure time, then DNSMASQ will be defined to "dnsmasq" and the mocked function will return that string instead of an absolute path. Clearly this doesn't break the test case, but it still feels off. I wonder if we should drop the configure time detection for dnsmasq altogether, same as we've done for other optional binaries, always define DNSMASQ to be "dnsmasq", and here do if (STREQ_NULLABLE(file, "dnsmasq")) return g_strdup("/usr/bin/dnsmasq"); What do you think? -- Andrea Bolognani / Red Hat / Virtualization
On 1/14/22 16:29, Andrea Bolognani wrote:
On Wed, Jan 12, 2022 at 09:47:54AM +0100, Michal Privoznik wrote:
With this code in place, the virFileIsExecutable() check can be removed from dnsmasqCapsRefreshInternal() because virFindFileInPath() already made sure the binary is executable.
But introducing virFileIsExecutable() means we have to mock it in
I believe you meant virFindFileInPath() here.
Oh, of course.
+++ b/src/util/virdnsmasq.c @@ -702,14 +692,20 @@ 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; + + if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) { + virReportSystemError(ENOENT, "%s", + _("Unable to find 'dnsmasq' binary in $PATH")); + return NULL; + } + + return g_steal_pointer(&caps); }
Can you please squash the g_autoptr conversion into the previous patch?
Fair enough. I thought this is okay, since it's only this commit that introduces an alternative return path from dnsmasqCapsNewEmpty(). But I don't mind either way. Consider it done.
+++ b/tests/networkmock.c @@ -0,0 +1,30 @@ +/* + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */
Missing copyright information. Also can we have an SPDX tag instead of the full license blurb? See tests/virhostdevmock.c for an example of what I have in mind.
Okay.
+char * +virFindFileInPath(const char *file) +{ + if (file && g_strrstr(file, "dnsmasq")) + return g_strdup(file);
This mock is somewhat inaccurate because, if dnsmasq was not found at configure time, then DNSMASQ will be defined to "dnsmasq" and the mocked function will return that string instead of an absolute path. Clearly this doesn't break the test case, but it still feels off.
I wonder if we should drop the configure time detection for dnsmasq altogether, same as we've done for other optional binaries, always define DNSMASQ to be "dnsmasq", and here do
if (STREQ_NULLABLE(file, "dnsmasq")) return g_strdup("/usr/bin/dnsmasq");
What do you think?
Yup, will do. Except s/bin/sbin/ because that's where dnsmasq usually is. Michal
On Wed, Jan 12, 2022 at 09:47:54AM +0100, Michal Privoznik wrote:
+VIR_TEST_MAIN_PRELOAD(mymain, + VIR_TEST_MOCK("network"))
Also I think "virdnsmasqmock" would be a better name for this mock library, as it mocks stuff that influences the "virdnsmasq" module. -- Andrea Bolognani / Red Hat / Virtualization
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 79060e2021..6d63e9e3c5 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -739,7 +739,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/networkmock.c | 16 ++++++++++++++++ tests/networkxml2conftest.c | 38 ++++++++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/tests/networkmock.c b/tests/networkmock.c index a9c13311a6..25014de8b8 100644 --- a/tests/networkmock.c +++ b/tests/networkmock.c @@ -28,3 +28,19 @@ virFindFileInPath(const char *file) /* We should not need any other binaries so return NULL. */ return NULL; } + +static int +virMockStatRedirect(const char *path G_GNUC_UNUSED, + char **newpath G_GNUC_UNUSED) +{ + /* We don't need to redirect stat. Do nothing. */ + return 0; +} + +#define VIR_MOCK_STAT_HOOK \ + if (strstr(path, "dnsmasq")) { \ + memset(sb, 0, sizeof(*sb)); \ + return 0; \ + } + +#include "virmockstathelpers.c" diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 8a6657654a..68dd3023e1 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,47 @@ 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[1], "--version")) { + *output = g_strdup("Dnsmasq version 2.67\n"); + *status = EXIT_SUCCESS; + } else if (STREQ(args[1], "--help")) { + *output = g_strdup("--bind-dynamic\n--ra-param"); + *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\n--bind-dynamic\n--ra-param"); + full = buildCaps(); #define DO_TEST(xname, xcaps) \ do { \ -- 2.34.1
On Wed, Jan 12, 2022 at 09:47:56AM +0100, Michal Privoznik wrote:
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/networkmock.c | 16 ++++++++++++++++ tests/networkxml2conftest.c | 38 ++++++++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-)
This all works, but I wonder if we couldn't just create a trivial shell script that behaves minimally the way we expect dnsmasq to, and change our virFindFileInPath() mock so that it returns the absolute path to it? That way we wouldn't need to implement any additional mocking and the code would end up being much simpler. Diff below. Also note that there is a pre-existing issue with the test, in that we don't check that the value returned by dnsmasqCapsNewFrom*() is non-NULL, and as a result if you change the version number in the test string to something like 0.1 the test will still pass where it definitely shouldn't. diff --git a/tests/networkmock.c b/tests/networkmock.c index a9c13311a6..dc1209a367 100644 --- a/tests/networkmock.c +++ b/tests/networkmock.c @@ -23,7 +23,7 @@ char * virFindFileInPath(const char *file) { if (file && g_strrstr(file, "dnsmasq")) - return g_strdup(file); + return g_strdup_printf("%s/virdnsmasqmock.sh", abs_srcdir); /* We should not need any other binaries so return NULL. */ return NULL; diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 8a6657654a..fd2116756e 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 = dnsmasqCapsNewFromBinary(); #define DO_TEST(xname, xcaps) \ do { \ diff --git a/tests/virdnsmasqmock.sh b/tests/virdnsmasqmock.sh new file mode 100755 index 0000000000..faaa94268c --- /dev/null +++ b/tests/virdnsmasqmock.sh @@ -0,0 +1,16 @@ +#!/bin/sh + +case "$1" in + "--version") + echo "Dnsmasq version 2.67" + ;; + "--help") + echo "--bind-dynamic" + echo "--ra-param" + ;; + *) + exit 1 + ;; +esac + +exit 0 -- Andrea Bolognani / Red Hat / Virtualization
On 1/14/22 17:49, Andrea Bolognani wrote:
On Wed, Jan 12, 2022 at 09:47:56AM +0100, Michal Privoznik wrote:
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/networkmock.c | 16 ++++++++++++++++ tests/networkxml2conftest.c | 38 ++++++++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-)
This all works, but I wonder if we couldn't just create a trivial shell script that behaves minimally the way we expect dnsmasq to, and change our virFindFileInPath() mock so that it returns the absolute path to it? That way we wouldn't need to implement any additional mocking and the code would end up being much simpler. Diff below.
I thought that we should avoid shell for new contributions: https://libvirt.org/programming-languages.html
Also note that there is a pre-existing issue with the test, in that we don't check that the value returned by dnsmasqCapsNewFrom*() is non-NULL, and as a result if you change the version number in the test string to something like 0.1 the test will still pass where it definitely shouldn't.
Okay, let me address that in v3. Michal
On Mon, Jan 17, 2022 at 01:11:29PM +0100, Michal Prívozník wrote:
On 1/14/22 17:49, Andrea Bolognani wrote:
This all works, but I wonder if we couldn't just create a trivial shell script that behaves minimally the way we expect dnsmasq to, and change our virFindFileInPath() mock so that it returns the absolute path to it? That way we wouldn't need to implement any additional mocking and the code would end up being much simpler. Diff below.
I thought that we should avoid shell for new contributions:
Fair enough. Python version below. #!/usr/bin/env python3 import sys output = { "--version": "Dnsmasq version 2.67", "--help": "--bind-dynamic\n--ra-param", } if len(sys.argv) != 2 or sys.argv[1] not in output: print("invalid usage") sys.exit(1) print(output[sys.argv[1]]) -- Andrea Bolognani / Red Hat / Virtualization
On 1/17/22 13:37, Andrea Bolognani wrote:
On Mon, Jan 17, 2022 at 01:11:29PM +0100, Michal Prívozník wrote:
On 1/14/22 17:49, Andrea Bolognani wrote:
This all works, but I wonder if we couldn't just create a trivial shell script that behaves minimally the way we expect dnsmasq to, and change our virFindFileInPath() mock so that it returns the absolute path to it? That way we wouldn't need to implement any additional mocking and the code would end up being much simpler. Diff below.
I thought that we should avoid shell for new contributions:
Fair enough. Python version below.
#!/usr/bin/env python3
import sys
output = { "--version": "Dnsmasq version 2.67", "--help": "--bind-dynamic\n--ra-param", }
if len(sys.argv) != 2 or sys.argv[1] not in output: print("invalid usage") sys.exit(1)
print(output[sys.argv[1]])
And what exactly is the point? I'm failing to see why this would be any better than mocking virCommand. Can you elaborate please? Michal
On Mon, Jan 17, 2022 at 01:39:31PM +0100, Michal Prívozník wrote:
On 1/17/22 13:37, Andrea Bolognani wrote:
On Mon, Jan 17, 2022 at 01:11:29PM +0100, Michal Prívozník wrote:
On 1/14/22 17:49, Andrea Bolognani wrote:
This all works, but I wonder if we couldn't just create a trivial shell script that behaves minimally the way we expect dnsmasq to, and change our virFindFileInPath() mock so that it returns the absolute path to it? That way we wouldn't need to implement any additional mocking and the code would end up being much simpler. Diff below.
I thought that we should avoid shell for new contributions:
Fair enough. Python version below.
#!/usr/bin/env python3
import sys
output = { "--version": "Dnsmasq version 2.67", "--help": "--bind-dynamic\n--ra-param", }
if len(sys.argv) != 2 or sys.argv[1] not in output: print("invalid usage") sys.exit(1)
print(output[sys.argv[1]])
And what exactly is the point? I'm failing to see why this would be any better than mocking virCommand. Can you elaborate please?
I believe the diffstats speak for themselves :) $ git diff --stat df09e45310..64325fa9ef tests/networkmock.c | 16 ++++++++++++++++ tests/networkxml2conftest.c | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) $ git diff --stat df09e45310..2b64fb492b tests/networkmock.c | 2 +- tests/networkxml2conftest.c | 2 +- tests/virdnsmasqmock.py | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 2 deletions(-) -- Andrea Bolognani / Red Hat / Virtualization
On 1/17/22 14:09, Andrea Bolognani wrote:
On Mon, Jan 17, 2022 at 01:39:31PM +0100, Michal Prívozník wrote:
On 1/17/22 13:37, Andrea Bolognani wrote:
On Mon, Jan 17, 2022 at 01:11:29PM +0100, Michal Prívozník wrote:
On 1/14/22 17:49, Andrea Bolognani wrote:
This all works, but I wonder if we couldn't just create a trivial shell script that behaves minimally the way we expect dnsmasq to, and change our virFindFileInPath() mock so that it returns the absolute path to it? That way we wouldn't need to implement any additional mocking and the code would end up being much simpler. Diff below.
I thought that we should avoid shell for new contributions:
Fair enough. Python version below.
#!/usr/bin/env python3
import sys
output = { "--version": "Dnsmasq version 2.67", "--help": "--bind-dynamic\n--ra-param", }
if len(sys.argv) != 2 or sys.argv[1] not in output: print("invalid usage") sys.exit(1)
print(output[sys.argv[1]])
And what exactly is the point? I'm failing to see why this would be any better than mocking virCommand. Can you elaborate please?
I believe the diffstats speak for themselves :)
$ git diff --stat df09e45310..64325fa9ef tests/networkmock.c | 16 ++++++++++++++++ tests/networkxml2conftest.c | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-)
$ git diff --stat df09e45310..2b64fb492b tests/networkmock.c | 2 +- tests/networkxml2conftest.c | 2 +- tests/virdnsmasqmock.py | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 2 deletions(-)
Alright, let's switch to python. I'm starting to not care about these patches fate so I won't object. Michal
On Mon, Jan 17, 2022 at 02:17:10PM +0100, Michal Prívozník wrote:
On 1/17/22 14:09, Andrea Bolognani wrote:
On Mon, Jan 17, 2022 at 01:39:31PM +0100, Michal Prívozník wrote:
And what exactly is the point? I'm failing to see why this would be any better than mocking virCommand. Can you elaborate please?
I believe the diffstats speak for themselves :)
$ git diff --stat df09e45310..64325fa9ef tests/networkmock.c | 16 ++++++++++++++++ tests/networkxml2conftest.c | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-)
$ git diff --stat df09e45310..2b64fb492b tests/networkmock.c | 2 +- tests/networkxml2conftest.c | 2 +- tests/virdnsmasqmock.py | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 2 deletions(-)
Alright, let's switch to python. I'm starting to not care about these patches fate so I won't object.
If you don't feel like doing that, you can consider your original patch Reviewed-by: Andrea Bolognani <abologna@redhat.com> and I'll try the script approach myself as a follow-up. -- Andrea Bolognani / Red Hat / Virtualization
The function is no longer used. Remove it. Signed-off-by: Michal Privoznik <mprivozn@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 ee14b99d88..aa23e8c0ec 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2114,7 +2114,6 @@ dnsmasqAddDhcpHost; dnsmasqAddHost; dnsmasqCapsGetBinaryPath; dnsmasqCapsNewFromBinary; -dnsmasqCapsNewFromBuffer; dnsmasqContextFree; dnsmasqContextNew; dnsmasqDelete; diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 6d63e9e3c5..ca52bf7871 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -708,20 +708,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
On Wed, Jan 12, 2022 at 09:47:57AM +0100, Michal Privoznik wrote:
The function is no longer used. Remove it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virdnsmasq.c | 14 -------------- src/util/virdnsmasq.h | 1 - 3 files changed, 16 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> Further cleanup opportunities: * drop the 'force' argument of dnsmasqCapsRefreshInternal(), as the only caller always passes true; * drop the 'noRefresh' field of the dnsmasqCaps struct, which AFAICT doesn't really do anything even before your changes; * rework dnsmasqCapsSetFromBuffer() so that it takes the output of the two calls to dnsmasq as separate arguments, instead of doing the silly dance of joining the two in the caller only to have the function split them again immediately. -- Andrea Bolognani / Red Hat / Virtualization
On 1/14/22 18:01, Andrea Bolognani wrote:
On Wed, Jan 12, 2022 at 09:47:57AM +0100, Michal Privoznik wrote:
The function is no longer used. Remove it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virdnsmasq.c | 14 -------------- src/util/virdnsmasq.h | 1 - 3 files changed, 16 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Further cleanup opportunities:
* drop the 'force' argument of dnsmasqCapsRefreshInternal(), as the only caller always passes true;
* drop the 'noRefresh' field of the dnsmasqCaps struct, which AFAICT doesn't really do anything even before your changes;
* rework dnsmasqCapsSetFromBuffer() so that it takes the output of the two calls to dnsmasq as separate arguments, instead of doing the silly dance of joining the two in the caller only to have the function split them again immediately.
My reasoning to not do all of this was that one day we might want to query capabilities. In which case we might need force/noRefresh. But since we removed the caps enum I guess your suggestion makes more sense. Will fix in v3. Michal
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> --- 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 ca52bf7871..4108fc262b 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -689,13 +689,14 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) return dnsmasqCapsSetFromBuffer(caps, complete); } -static dnsmasqCaps * -dnsmasqCapsNewEmpty(void) +dnsmasqCaps * +dnsmasqCapsNewFromBinary(void) { g_autoptr(dnsmasqCaps) caps = NULL; if (dnsmasqCapsInitialize() < 0) return NULL; + if (!(caps = virObjectNew(dnsmasqCapsClass))) return NULL; @@ -705,17 +706,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, true) < 0) return NULL; -- 2.34.1
On Wed, Jan 12, 2022 at 09:47:58AM +0100, Michal Privoznik wrote:
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> --- src/util/virdnsmasq.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Michal Privoznik -
Michal Prívozník