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

[Andrea Bolognani]

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