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&gt; > 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