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 virFindFileInPath() will simply return a copy.

With this code in place, the virFileIsExecutable() check can be
removed from dnsmasqCapsRefreshInternal() because
virFindFileInPath() already made sure the binary is executable.

But introducing virFileIsExecutable() means we have to mock it in
test suite because dnsmasqCaps are created in
networkxml2conftest.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virdnsmasq.c       | 22 +++++++++-------------
 tests/meson.build           |  1 +
 tests/networkmock.c         | 30 ++++++++++++++++++++++++++++++
 tests/networkxml2conftest.c |  3 ++-
 4 files changed, 42 insertions(+), 14 deletions(-)
 create mode 100644 tests/networkmock.c

diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index f4bdab116e..79060e2021 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);
@@ -702,14 +692,20 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
 static dnsmasqCaps *
 dnsmasqCapsNewEmpty(void)
 {
-    dnsmasqCaps *caps;
+    g_autoptr(dnsmasqCaps) caps = NULL;
 
     if (dnsmasqCapsInitialize() < 0)
         return NULL;
     if (!(caps = virObjectNew(dnsmasqCapsClass)))
         return NULL;
-    caps->binaryPath = g_strdup(DNSMASQ);
-    return caps;
+
+    if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) {
+        virReportSystemError(ENOENT, "%s",
+                             _("Unable to find 'dnsmasq' binary in $PATH"));
+        return NULL;
+    }
+
+    return g_steal_pointer(&caps);
 }
 
 dnsmasqCaps *
diff --git a/tests/meson.build b/tests/meson.build
index 4792220b48..e8f5d4c1f7 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -57,6 +57,7 @@ endif
 
 mock_libs = [
   { 'name': 'domaincapsmock' },
+  { 'name': 'networkmock' },
   { 'name': 'shunload', 'sources': [ 'shunloadhelper.c' ] },
   { 'name': 'vircgroupmock' },
   { 'name': 'virfilecachemock' },
diff --git a/tests/networkmock.c b/tests/networkmock.c
new file mode 100644
index 0000000000..a9c13311a6
--- /dev/null
+++ b/tests/networkmock.c
@@ -0,0 +1,30 @@
+/*
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#include <config.h>
+
+#include "internal.h"
+#include "virfile.h"
+
+char *
+virFindFileInPath(const char *file)
+{
+    if (file && g_strrstr(file, "dnsmasq"))
+        return g_strdup(file);
+
+    /* We should not need any other binaries so return NULL. */
+    return NULL;
+}
diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c
index d79c2b4783..8a6657654a 100644
--- a/tests/networkxml2conftest.c
+++ b/tests/networkxml2conftest.c
@@ -158,4 +158,5 @@ mymain(void)
     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
-VIR_TEST_MAIN(mymain)
+VIR_TEST_MAIN_PRELOAD(mymain,
+                      VIR_TEST_MOCK("network"))
-- 
2.34.1

On Wed, Jan 12, 2022 at 09:47:54AM +0100, Michal Privoznik wrote:
> +VIR_TEST_MAIN_PRELOAD(mymain,
> +                      VIR_TEST_MOCK("network"))

Also I think "virdnsmasqmock" would be a better name for this mock
library, as it mocks stuff that influences the "virdnsmasq" module.

-- 
Andrea Bolognani / Red Hat / Virtualization

On Wed, Jan 12, 2022 at 09:47:54AM +0100, Michal Privoznik wrote:
> With this code in place, the virFileIsExecutable() check can be
> removed from dnsmasqCapsRefreshInternal() because
> virFindFileInPath() already made sure the binary is executable.
>
> But introducing virFileIsExecutable() means we have to mock it in

I believe you meant virFindFileInPath() here.

> +++ b/src/util/virdnsmasq.c
> @@ -702,14 +692,20 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
>  static dnsmasqCaps *
>  dnsmasqCapsNewEmpty(void)
>  {
> -    dnsmasqCaps *caps;
> +    g_autoptr(dnsmasqCaps) caps = NULL;
>
>      if (dnsmasqCapsInitialize() < 0)
>          return NULL;
>      if (!(caps = virObjectNew(dnsmasqCapsClass)))
>          return NULL;
> -    caps->binaryPath = g_strdup(DNSMASQ);
> -    return caps;
> +
> +    if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) {
> +        virReportSystemError(ENOENT, "%s",
> +                             _("Unable to find 'dnsmasq' binary in $PATH"));
> +        return NULL;
> +    }
> +
> +    return g_steal_pointer(&caps);
>  }

Can you please squash the g_autoptr conversion into the previous
patch?

> +++ b/tests/networkmock.c
> @@ -0,0 +1,30 @@
> +/*
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + */

Missing copyright information. Also can we have an SPDX tag instead
of the full license blurb? See tests/virhostdevmock.c for an example
of what I have in mind.

> +char *
> +virFindFileInPath(const char *file)
> +{
> +    if (file && g_strrstr(file, "dnsmasq"))
> +        return g_strdup(file);

This mock is somewhat inaccurate because, if dnsmasq was not found at
configure time, then DNSMASQ will be defined to "dnsmasq" and the
mocked function will return that string instead of an absolute path.
Clearly this doesn't break the test case, but it still feels off.

I wonder if we should drop the configure time detection for dnsmasq
altogether, same as we've done for other optional binaries, always
define DNSMASQ to be "dnsmasq", and here do

  if (STREQ_NULLABLE(file, "dnsmasq"))
      return g_strdup("/usr/bin/dnsmasq");

What do you think?

-- 
Andrea Bolognani / Red Hat / Virtualization

On 1/14/22 16:29, Andrea Bolognani wrote:
> On Wed, Jan 12, 2022 at 09:47:54AM +0100, Michal Privoznik wrote:
>> With this code in place, the virFileIsExecutable() check can be
>> removed from dnsmasqCapsRefreshInternal() because
>> virFindFileInPath() already made sure the binary is executable.
>>
>> But introducing virFileIsExecutable() means we have to mock it in
> 
> I believe you meant virFindFileInPath() here.

Oh, of course.

> 
>> +++ b/src/util/virdnsmasq.c
>> @@ -702,14 +692,20 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
>>  static dnsmasqCaps *
>>  dnsmasqCapsNewEmpty(void)
>>  {
>> -    dnsmasqCaps *caps;
>> +    g_autoptr(dnsmasqCaps) caps = NULL;
>>
>>      if (dnsmasqCapsInitialize() < 0)
>>          return NULL;
>>      if (!(caps = virObjectNew(dnsmasqCapsClass)))
>>          return NULL;
>> -    caps->binaryPath = g_strdup(DNSMASQ);
>> -    return caps;
>> +
>> +    if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) {
>> +        virReportSystemError(ENOENT, "%s",
>> +                             _("Unable to find 'dnsmasq' binary in $PATH"));
>> +        return NULL;
>> +    }
>> +
>> +    return g_steal_pointer(&caps);
>>  }
> 
> Can you please squash the g_autoptr conversion into the previous
> patch?
> 

Fair enough. I thought this is okay, since it's only this commit that
introduces an alternative return path from dnsmasqCapsNewEmpty(). But I
don't mind either way. Consider it done.

>> +++ b/tests/networkmock.c
>> @@ -0,0 +1,30 @@
>> +/*
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library.  If not, see
>> + * <http://www.gnu.org/licenses/>.
>> + */
> 
> Missing copyright information. Also can we have an SPDX tag instead
> of the full license blurb? See tests/virhostdevmock.c for an example
> of what I have in mind.

Okay.

> 
>> +char *
>> +virFindFileInPath(const char *file)
>> +{
>> +    if (file && g_strrstr(file, "dnsmasq"))
>> +        return g_strdup(file);
> 
> This mock is somewhat inaccurate because, if dnsmasq was not found at
> configure time, then DNSMASQ will be defined to "dnsmasq" and the
> mocked function will return that string instead of an absolute path.
> Clearly this doesn't break the test case, but it still feels off.
> 
> I wonder if we should drop the configure time detection for dnsmasq
> altogether, same as we've done for other optional binaries, always
> define DNSMASQ to be "dnsmasq", and here do
> 
>   if (STREQ_NULLABLE(file, "dnsmasq"))
>       return g_strdup("/usr/bin/dnsmasq");
> 
> What do you think?
> 

Yup, will do. Except s/bin/sbin/ because that's where dnsmasq usually is.

Michal