1. Patchew
  2. Libvirt
  3. [PATCH 0/3] virdnsmasq: Lookup DNSMASQ in PATH
  4. View patch

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

Michal Privoznik posted 3 patches 4 years ago
There is a newer version of this series
[PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
Posted by Michal Privoznik 4 years ago 
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).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virdnsmasq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index d304929d51..b6ccb9d0a4 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -708,7 +708,7 @@ dnsmasqCapsNewEmpty(void)
         return NULL;
     if (!(caps = virObjectNew(dnsmasqCapsClass)))
         return NULL;
-    caps->binaryPath = g_strdup(DNSMASQ);
+    caps->binaryPath = virFindFileInPath(DNSMASQ);
     return caps;
 }
 
-- 
2.34.1
Re: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
Posted by Andrea Bolognani 4 years ago 
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.

Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
Posted by Michal Prívozník 4 years ago 
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
Re: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
Posted by Andrea Bolognani 4 years ago 
On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
> 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.

Right. But considering that dnsmasqCapsRefreshInternal() includes a
check for whether or not the file is executable, and an error is
returned if it's not, wouldn't it make more sense to remove that
check and simply exit early if virFindFileInPath() returns NULL?

diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index a869b398c7..e31e78224d 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -670,16 +670,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
         return 0;
     caps->mtime = sb.st_mtime;

-    /* Make sure the binary we are about to try exec'ing exists.
-     * Technically we could catch the exec() failure, but that's
-     * in a sub-process so it's hard to feed back a useful error.
-     */
-    if (!virFileIsExecutable(caps->binaryPath)) {
-        virReportSystemError(errno, _("dnsmasq binary %s is not executable"),
-                             caps->binaryPath);
-        return -1;
-    }
-
     vercmd = virCommandNewArgList(caps->binaryPath, "--version", NULL);
     virCommandSetOutputBuffer(vercmd, &version);
     virCommandAddEnvPassCommon(vercmd);
@@ -708,7 +698,6 @@ dnsmasqCapsNewEmpty(void)
         return NULL;
     if (!(caps = virObjectNew(dnsmasqCapsClass)))
         return NULL;
-    caps->binaryPath = virFindFileInPath(DNSMASQ);
     return caps;
 }

@@ -720,6 +709,8 @@ dnsmasqCapsNewFromBuffer(const char *buf)
     if (!caps)
         return NULL;

+    caps->binaryPath = g_strdup(DNSMASQ);
+
     if (dnsmasqCapsSetFromBuffer(caps, buf) < 0) {
         virObjectUnref(caps);
         return NULL;
@@ -735,6 +726,11 @@ dnsmasqCapsNewFromBinary(void)
     if (!caps)
         return NULL;

+    if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) {
+        virObjectUnref(caps);
+        return NULL;
+    }
+
     if (dnsmasqCapsRefreshInternal(caps, true) < 0) {
         virObjectUnref(caps);
         return NULL;
-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
Posted by Daniel P. Berrangé 4 years ago 
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@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 :|
Re: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
Posted by Andrea Bolognani 4 years ago 
On Tue, Jan 11, 2022 at 09:59:06AM +0000, Daniel P. Berrangé wrote:
> On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
> > 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.

Agreed in general, but we want the test suite to still pass even if
dnsmasq is not installed on the build machine. The approach I
suggested in my other message would achieve that.

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
Posted by Daniel P. Berrangé 4 years ago 
On Tue, Jan 11, 2022 at 10:18:23AM +0000, Andrea Bolognani wrote:
> On Tue, Jan 11, 2022 at 09:59:06AM +0000, Daniel P. Berrangé wrote:
> > On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
> > > 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.
> 
> Agreed in general, but we want the test suite to still pass even if
> dnsmasq is not installed on the build machine. The approach I
> suggested in my other message would achieve that.

Shouldn't we just mock the virFindFileInPath call in the test suite
to return whatever fixed binary path we need, so we don't have to
modify the driver code with special logic for tests.

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 :|
Re: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
Posted by Andrea Bolognani 4 years ago 
On Tue, Jan 11, 2022 at 10:24:00AM +0000, Daniel P. Berrangé wrote:
> On Tue, Jan 11, 2022 at 10:18:23AM +0000, Andrea Bolognani wrote:
> > On Tue, Jan 11, 2022 at 09:59:06AM +0000, Daniel P. Berrangé wrote:
> > > 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.
> >
> > Agreed in general, but we want the test suite to still pass even if
> > dnsmasq is not installed on the build machine. The approach I
> > suggested in my other message would achieve that.
>
> Shouldn't we just mock the virFindFileInPath call in the test suite
> to return whatever fixed binary path we need, so we don't have to
> modify the driver code with special logic for tests.

We'd have to also mock *running* the binary, as we get some
information out of that. Probably a better long-term approach, but I
don't think it should necessarily be a blocker for this series, which
already represents an improvement over the status quo.

-- 
Andrea Bolognani / Red Hat / Virtualization

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.