virdnsmasq: fix runtime search for executable

[PATCH v1] virdnsmasq: fix runtime search for executable

Posted by Olaf Hering 2 years, 8 months ago

dnsmasq is an optional binary which does not neccessary exist during build.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 src/util/virdnsmasq.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index f2f606913f..06d192c99d 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -729,8 +729,26 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
     return ret;
 }
 
+static char *
+dnsmasqGetBinaryPath(void)
+{
+    static const char binary[] = DNSMASQ;
+    char *binary_path;
+
+    if (g_path_is_absolute(binary))
+        return g_strdup(binary);
+
+    binary_path = virFindFileInPath(binary);
+    if (!binary_path) {
+        virReportSystemError(ENOENT, _("Cannot find '%s' in path"), binary);
+        binary_path = g_strdup(binary);
+    }
+
+    return binary_path;
+}
+
 static dnsmasqCaps *
-dnsmasqCapsNewEmpty(const char *binaryPath)
+dnsmasqCapsNewEmpty(void)
 {
     dnsmasqCaps *caps;
 
@@ -739,14 +757,14 @@ dnsmasqCapsNewEmpty(const char *binaryPath)
     if (!(caps = virObjectNew(dnsmasqCapsClass)))
         return NULL;
     caps->flags = virBitmapNew(DNSMASQ_CAPS_LAST);
-    caps->binaryPath = g_strdup(binaryPath ? binaryPath : DNSMASQ);
+    caps->binaryPath = dnsmasqGetBinaryPath();
     return caps;
 }
 
 dnsmasqCaps *
 dnsmasqCapsNewFromBuffer(const char *buf)
 {
-    dnsmasqCaps *caps = dnsmasqCapsNewEmpty(DNSMASQ);
+    dnsmasqCaps *caps = dnsmasqCapsNewEmpty();
 
     if (!caps)
         return NULL;
@@ -761,7 +779,7 @@ dnsmasqCapsNewFromBuffer(const char *buf)
 dnsmasqCaps *
 dnsmasqCapsNewFromBinary(void)
 {
-    dnsmasqCaps *caps = dnsmasqCapsNewEmpty(DNSMASQ);
+    dnsmasqCaps *caps = dnsmasqCapsNewEmpty();
 
     if (!caps)
         return NULL;
@@ -776,7 +794,7 @@ dnsmasqCapsNewFromBinary(void)
 const char *
 dnsmasqCapsGetBinaryPath(dnsmasqCaps *caps)
 {
-    return caps ? caps->binaryPath : DNSMASQ;
+    return caps ? caps->binaryPath : dnsmasqGetBinaryPath();
 }
 
 unsigned long

Re: [PATCH v1] virdnsmasq: fix runtime search for executable

Posted by Pavel Hrdina 2 years, 8 months ago

On Wed, Aug 11, 2021 at 11:47:13AM +0200, Olaf Hering wrote:
> dnsmasq is an optional binary which does not neccessary exist during build.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  src/util/virdnsmasq.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)

Is there any error or incorrect behavior that you are trying to fix?
I did audit the code and within the virdnsmasq.c file we use the
binaryPath with virCommandRun() which will do this same check before
acutally execing, see virExec().

Pavel

> diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
> index f2f606913f..06d192c99d 100644
> --- a/src/util/virdnsmasq.c
> +++ b/src/util/virdnsmasq.c
> @@ -729,8 +729,26 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
>      return ret;
>  }
>  
> +static char *
> +dnsmasqGetBinaryPath(void)
> +{
> +    static const char binary[] = DNSMASQ;
> +    char *binary_path;
> +
> +    if (g_path_is_absolute(binary))
> +        return g_strdup(binary);
> +

No need for this check, virFindFileInPath calls g_find_program_in_path
which will do the correct thing if the path is already absolute.

> +    binary_path = virFindFileInPath(binary);
> +    if (!binary_path) {
> +        virReportSystemError(ENOENT, _("Cannot find '%s' in path"), binary);
> +        binary_path = g_strdup(binary);
> +    }
> +
> +    return binary_path;
> +}
> +
>  static dnsmasqCaps *
> -dnsmasqCapsNewEmpty(const char *binaryPath)
> +dnsmasqCapsNewEmpty(void)
>  {
>      dnsmasqCaps *caps;
>  
> @@ -739,14 +757,14 @@ dnsmasqCapsNewEmpty(const char *binaryPath)
>      if (!(caps = virObjectNew(dnsmasqCapsClass)))
>          return NULL;
>      caps->flags = virBitmapNew(DNSMASQ_CAPS_LAST);
> -    caps->binaryPath = g_strdup(binaryPath ? binaryPath : DNSMASQ);
> +    caps->binaryPath = dnsmasqGetBinaryPath();
>      return caps;
>  }
>  
>  dnsmasqCaps *
>  dnsmasqCapsNewFromBuffer(const char *buf)
>  {
> -    dnsmasqCaps *caps = dnsmasqCapsNewEmpty(DNSMASQ);
> +    dnsmasqCaps *caps = dnsmasqCapsNewEmpty();
>  
>      if (!caps)
>          return NULL;
> @@ -761,7 +779,7 @@ dnsmasqCapsNewFromBuffer(const char *buf)
>  dnsmasqCaps *
>  dnsmasqCapsNewFromBinary(void)
>  {
> -    dnsmasqCaps *caps = dnsmasqCapsNewEmpty(DNSMASQ);
> +    dnsmasqCaps *caps = dnsmasqCapsNewEmpty();
>  
>      if (!caps)
>          return NULL;
> @@ -776,7 +794,7 @@ dnsmasqCapsNewFromBinary(void)
>  const char *
>  dnsmasqCapsGetBinaryPath(dnsmasqCaps *caps)
>  {
> -    return caps ? caps->binaryPath : DNSMASQ;
> +    return caps ? caps->binaryPath : dnsmasqGetBinaryPath();
>  }
>  
>  unsigned long
>

Re: [PATCH v1] virdnsmasq: fix runtime search for executable

Posted by Olaf Hering 2 years, 8 months ago

Am Tue, 17 Aug 2021 15:13:05 +0200
schrieb Pavel Hrdina <phrdina@redhat.com>:

> Is there any error or incorrect behavior that you are trying to fix?

Some code paths call stat(DNSMASQ), which this patch tries to address.

Olaf

Re: [PATCH v1] virdnsmasq: fix runtime search for executable

Posted by Olaf Hering 2 years, 3 months ago

Tue, 17 Aug 2021 15:13:05 +0200 Pavel Hrdina <phrdina@redhat.com>:

> On Wed, Aug 11, 2021 at 11:47:13AM +0200, Olaf Hering wrote:
> > dnsmasq is an optional binary which does not neccessary exist during build.
> >
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > ---
> >  src/util/virdnsmasq.c | 28 +++++++++++++++++++++++-----
> >  1 file changed, 23 insertions(+), 5 deletions(-)  
> 
> Is there any error or incorrect behavior that you are trying to fix?
> I did audit the code and within the virdnsmasq.c file we use the
> binaryPath with virCommandRun() which will do this same check before
> acutally execing, see virExec().

The error was, and still is today, like this:

Jan 10 12:53:36 rdma03 libvirtd[1725]: Cannot check dnsmasq binary dnsmasq: No such file or directory

How do you intent to fix this?

Olaf

Re: [PATCH v1] virdnsmasq: fix runtime search for executable

Posted by Michal Prívozník 2 years, 3 months ago

On 1/10/22 14:29, Olaf Hering wrote:
> Tue, 17 Aug 2021 15:13:05 +0200 Pavel Hrdina <phrdina@redhat.com>:
> 
>> On Wed, Aug 11, 2021 at 11:47:13AM +0200, Olaf Hering wrote:
>>> dnsmasq is an optional binary which does not neccessary exist during build.
>>>
>>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>>> ---
>>>  src/util/virdnsmasq.c | 28 +++++++++++++++++++++++-----
>>>  1 file changed, 23 insertions(+), 5 deletions(-)  
>>
>> Is there any error or incorrect behavior that you are trying to fix?
>> I did audit the code and within the virdnsmasq.c file we use the
>> binaryPath with virCommandRun() which will do this same check before
>> acutally execing, see virExec().
> 
> The error was, and still is today, like this:
> 
> Jan 10 12:53:36 rdma03 libvirtd[1725]: Cannot check dnsmasq binary dnsmasq: No such file or directory
> 
> How do you intent to fix this?
> 
> Olaf

Yeah, this is a real problem. I think I have a small, non-intrusive
patch. Let me post it.

Michal

Re: [PATCH v1] virdnsmasq: fix runtime search for executable

Posted by Michal Prívozník 2 years, 3 months ago

On 1/10/22 16:21, Michal Prívozník wrote:

> Yeah, this is a real problem. I think I have a small, non-intrusive
> patch. Let me post it.
> 

Huh, so my patches turned out to be similar to yours.

https://listman.redhat.com/archives/libvir-list/2022-January/msg00386.html

Let me know if you want me to add your SoB line.

Michal

Re: [PATCH v1] virdnsmasq: fix runtime search for executable

Posted by Olaf Hering 2 years, 3 months ago

Mon, 10 Jan 2022 16:46:49 +0100 Michal Prívozník <mprivozn@redhat.com>:

> Huh, so my patches turned out to be similar to yours.

They appear to work for me, no more error reported.
I do not use dnsmasq, no idea if it actually works with or without this series.

Olaf