[PATCH 0/3] virdnsmasq: Lookup DNSMASQ in PATH
This is an alternative implementation to Olaf's original patch posted here: https://listman.redhat.com/archives/libvir-list/2021-August/msg00273.html I find my patches to be a bit cleaner, but either way we should merge his or mine. Michal Prívozník (3): virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty() virdnsmasq: Lookup DNSMASQ in PATH virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath() src/util/virdnsmasq.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) -- 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> --- 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
On Mon, Jan 10, 2022 at 04:44:53PM +0100, Michal Privoznik wrote:
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> --- src/util/virdnsmasq.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 it is returned (and virFindFileInPath() is a NOP). Signed-off-by: Michal Privoznik <mprivozn@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 d304929d51..b6ccb9d0a4 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -708,7 +708,7 @@ dnsmasqCapsNewEmpty(void) return NULL; if (!(caps = virObjectNew(dnsmasqCapsClass))) return NULL; - caps->binaryPath = g_strdup(DNSMASQ); + caps->binaryPath = virFindFileInPath(DNSMASQ); return caps; } -- 2.34.1
On Mon, Jan 10, 2022 at 04:44:55PM +0100, Michal Privoznik wrote:
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 it is returned (and virFindFileInPath() is a NOP).
Saying that virFindFileInPath() is a NOP is not quite correct: if you pass an absolute path, it will return a copy. So I'd rewrite the above as If DNSMASQ already contains an absolute path then virFindFileInPath() will simply return a copy. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On 1/10/22 18:41, Andrea Bolognani wrote:
On Mon, Jan 10, 2022 at 04:44:55PM +0100, Michal Privoznik wrote:
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 it is returned (and virFindFileInPath() is a NOP).
Saying that virFindFileInPath() is a NOP is not quite correct: if you pass an absolute path, it will return a copy. So I'd rewrite the above as
If DNSMASQ already contains an absolute path then virFindFileInPath() will simply return a copy.
Yes, this makes sense.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks, but as I was looking through virFindFileInPath() I've noticed that if the file exists but is not executable then NULL is returned, so we will need a fallback case. Effectively this needs to be squashed in: diff --git c/src/util/virdnsmasq.c w/src/util/virdnsmasq.c index b6ccb9d0a4..7792a65bc3 100644 --- c/src/util/virdnsmasq.c +++ w/src/util/virdnsmasq.c @@ -703,12 +703,17 @@ static dnsmasqCaps * dnsmasqCapsNewEmpty(void) { dnsmasqCaps *caps; + g_autofree char *binaryPath = NULL; if (dnsmasqCapsInitialize() < 0) return NULL; if (!(caps = virObjectNew(dnsmasqCapsClass))) return NULL; - caps->binaryPath = virFindFileInPath(DNSMASQ); + + if (!(binaryPath = virFindFileInPath(DNSMASQ))) + binaryPath = g_strdup(DNSMASQ); + + caps->binaryPath = g_steal_pointer(&binaryPath); return caps; } I'll post v2 of this patch shortly. Michal
On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
Thanks, but as I was looking through virFindFileInPath() I've noticed that if the file exists but is not executable then NULL is returned, so we will need a fallback case.
Right. But considering that dnsmasqCapsRefreshInternal() includes a check for whether or not the file is executable, and an error is returned if it's not, wouldn't it make more sense to remove that check and simply exit early if virFindFileInPath() returns NULL? diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index a869b398c7..e31e78224d 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); @@ -708,7 +698,6 @@ dnsmasqCapsNewEmpty(void) return NULL; if (!(caps = virObjectNew(dnsmasqCapsClass))) return NULL; - caps->binaryPath = virFindFileInPath(DNSMASQ); return caps; } @@ -720,6 +709,8 @@ dnsmasqCapsNewFromBuffer(const char *buf) if (!caps) return NULL; + caps->binaryPath = g_strdup(DNSMASQ); + if (dnsmasqCapsSetFromBuffer(caps, buf) < 0) { virObjectUnref(caps); return NULL; @@ -735,6 +726,11 @@ dnsmasqCapsNewFromBinary(void) if (!caps) return NULL; + if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) { + virObjectUnref(caps); + return NULL; + } + if (dnsmasqCapsRefreshInternal(caps, true) < 0) { virObjectUnref(caps); return NULL; -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
On 1/10/22 18:41, Andrea Bolognani wrote:
On Mon, Jan 10, 2022 at 04:44:55PM +0100, Michal Privoznik wrote:
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 it is returned (and virFindFileInPath() is a NOP).
Saying that virFindFileInPath() is a NOP is not quite correct: if you pass an absolute path, it will return a copy. So I'd rewrite the above as
If DNSMASQ already contains an absolute path then virFindFileInPath() will simply return a copy.
Yes, this makes sense.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks, but as I was looking through virFindFileInPath() I've noticed that if the file exists but is not executable then NULL is returned, so we will need a fallback case. Effectively this needs to be squashed in:
Why do we need a fallback ? If someone has put 'dnsmasq' in $PATH without the execute bit set, surely that's just a broken deployment and returning NULL is correct.
diff --git c/src/util/virdnsmasq.c w/src/util/virdnsmasq.c index b6ccb9d0a4..7792a65bc3 100644 --- c/src/util/virdnsmasq.c +++ w/src/util/virdnsmasq.c @@ -703,12 +703,17 @@ static dnsmasqCaps * dnsmasqCapsNewEmpty(void) { dnsmasqCaps *caps; + g_autofree char *binaryPath = NULL;
if (dnsmasqCapsInitialize() < 0) return NULL; if (!(caps = virObjectNew(dnsmasqCapsClass))) return NULL; - caps->binaryPath = virFindFileInPath(DNSMASQ); + + if (!(binaryPath = virFindFileInPath(DNSMASQ))) + binaryPath = g_strdup(DNSMASQ); + + caps->binaryPath = g_steal_pointer(&binaryPath); return caps; }
I'll post v2 of this patch shortly.
Michal
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Jan 11, 2022 at 09:59:06AM +0000, Daniel P. Berrangé wrote:
On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
Thanks, but as I was looking through virFindFileInPath() I've noticed that if the file exists but is not executable then NULL is returned, so we will need a fallback case. Effectively this needs to be squashed in:
Why do we need a fallback ? If someone has put 'dnsmasq' in $PATH without the execute bit set, surely that's just a broken deployment and returning NULL is correct.
Agreed in general, but we want the test suite to still pass even if dnsmasq is not installed on the build machine. The approach I suggested in my other message would achieve that. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Jan 11, 2022 at 10:18:23AM +0000, Andrea Bolognani wrote:
On Tue, Jan 11, 2022 at 09:59:06AM +0000, Daniel P. Berrangé wrote:
On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
Thanks, but as I was looking through virFindFileInPath() I've noticed that if the file exists but is not executable then NULL is returned, so we will need a fallback case. Effectively this needs to be squashed in:
Why do we need a fallback ? If someone has put 'dnsmasq' in $PATH without the execute bit set, surely that's just a broken deployment and returning NULL is correct.
Agreed in general, but we want the test suite to still pass even if dnsmasq is not installed on the build machine. The approach I suggested in my other message would achieve that.
Shouldn't we just mock the virFindFileInPath call in the test suite to return whatever fixed binary path we need, so we don't have to modify the driver code with special logic for tests. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Jan 11, 2022 at 10:24:00AM +0000, Daniel P. Berrangé wrote:
On Tue, Jan 11, 2022 at 10:18:23AM +0000, Andrea Bolognani wrote:
On Tue, Jan 11, 2022 at 09:59:06AM +0000, Daniel P. Berrangé wrote:
Why do we need a fallback ? If someone has put 'dnsmasq' in $PATH without the execute bit set, surely that's just a broken deployment and returning NULL is correct.
Agreed in general, but we want the test suite to still pass even if dnsmasq is not installed on the build machine. The approach I suggested in my other message would achieve that.
Shouldn't we just mock the virFindFileInPath call in the test suite to return whatever fixed binary path we need, so we don't have to modify the driver code with special logic for tests.
We'd have to also mock *running* the binary, as we get some information out of that. Probably a better long-term approach, but I don't think it should necessarily be a blocker for this series, which already represents an improvement over the status quo. -- 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> --- 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 b6ccb9d0a4..a869b398c7 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -745,7 +745,7 @@ dnsmasqCapsNewFromBinary(void) const char * dnsmasqCapsGetBinaryPath(dnsmasqCaps *caps) { - return caps ? caps->binaryPath : DNSMASQ; + return caps->binaryPath; } /** dnsmasqDhcpHostsToString: -- 2.34.1
On Mon, Jan 10, 2022 at 04:44:57PM +0100, Michal Privoznik wrote:
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> --- src/util/virdnsmasq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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. But, if we failed to find the binary in $PATH then do not report error. Either the dnsmasqCapsNewEmpty() function is called from dnsmasqCapsNewFromBinary() which will later validate the path and return appropriate error, or the function is called from a test suite (via dnsmasqCapsNewFromBuffer()) and the caps are parsed from a fixed string. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Diff to v1: - Per my experiments we need a fallback case when DNSMASQ exists but is not executable. At any rate, this whole code could use cleanup, but let's save that for after the release. src/util/virdnsmasq.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index d304929d51..8ddc2d0b78 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -703,12 +703,21 @@ static dnsmasqCaps * dnsmasqCapsNewEmpty(void) { dnsmasqCaps *caps; + g_autofree char *binaryPath = NULL; if (dnsmasqCapsInitialize() < 0) return NULL; if (!(caps = virObjectNew(dnsmasqCapsClass))) return NULL; - caps->binaryPath = g_strdup(DNSMASQ); + + if (!(binaryPath = virFindFileInPath(DNSMASQ))) { + /* Don't report error here because we might be running + * from a test suite an initializing capabilities from + * a buffer (dnsmasqCapsNewFromBuffer()). */ + binaryPath = g_strdup(DNSMASQ); + } + + caps->binaryPath = g_steal_pointer(&binaryPath); return caps; } -- 2.34.1
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník