Re: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

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@xxxxxxxxxx>
> 

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