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

[Daniel P. Berrangé]

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 at 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 :|