Re: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
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(a)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