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