I've noticed when running libvirtd in the session mode that
whenever I start a virtual machine the following error is printed
into logs:

  error : cannot execute binary /usr/bin/slirp-helper: No such file or directory

The error message is produced in qemuSlirpNewForHelper() which
does attempt to be a NO-OP if the helper doesn't exist by
checking if its path is NULL, but that's not usually the case
because in the default config (in virQEMUDriverConfigNew()) its
path is initialized to QEMU_SLIRP_HELPER. And while it is true
that during configure we try to get the actual path of the helper
we fallback to the path above if not found.

Fix the check so that the function checks whether the helper
exists and is executable.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_slirp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c
index d2e4ed79be..4e5ab12727 100644
--- a/src/qemu/qemu_slirp.c
+++ b/src/qemu/qemu_slirp.c
@@ -101,7 +101,8 @@ qemuSlirpNewForHelper(const char *helper)
     virJSONValuePtr featuresJSON;
     size_t i, nfeatures;
 
-    if (!helper)
+    if (!helper ||
+        !virFileIsExecutable(helper))
         return NULL;
 
     slirp = qemuSlirpNew();
-- 
2.26.2

On Tue, Oct 13, 2020 at 03:32:28PM +0200, Michal Privoznik wrote:
> I've noticed when running libvirtd in the session mode that
> whenever I start a virtual machine the following error is printed
> into logs:
> 
>   error : cannot execute binary /usr/bin/slirp-helper: No such file or directory
> 
> The error message is produced in qemuSlirpNewForHelper() which
> does attempt to be a NO-OP if the helper doesn't exist by
> checking if its path is NULL, but that's not usually the case
> because in the default config (in virQEMUDriverConfigNew()) its
> path is initialized to QEMU_SLIRP_HELPER. And while it is true
> that during configure we try to get the actual path of the helper
> we fallback to the path above if not found.
> 
> Fix the check so that the function checks whether the helper
> exists and is executable.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_slirp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c
> index d2e4ed79be..4e5ab12727 100644
> --- a/src/qemu/qemu_slirp.c
> +++ b/src/qemu/qemu_slirp.c
> @@ -101,7 +101,8 @@ qemuSlirpNewForHelper(const char *helper)
>      virJSONValuePtr featuresJSON;
>      size_t i, nfeatures;
>  
> -    if (!helper)
> +    if (!helper ||
> +        !virFileIsExecutable(helper))
>          return NULL;

This API has way bigger problems than this. It is reporting errors using
virReportError, but the callers ignore them and can't distinguish real
errors from expected errors.

I'm working on a more comprehensive fix and will send patch shortly


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

On 10/13/20 9:32 AM, Michal Privoznik wrote:
> I've noticed when running libvirtd in the session mode that
> whenever I start a virtual machine the following error is printed
> into logs:
>
>    error : cannot execute binary /usr/bin/slirp-helper: No such file or directory
>
> The error message is produced in qemuSlirpNewForHelper() which
> does attempt to be a NO-OP if the helper doesn't exist by
> checking if its path is NULL, but that's not usually the case
> because in the default config (in virQEMUDriverConfigNew()) its
> path is initialized to QEMU_SLIRP_HELPER. And while it is true
> that during configure we try to get the actual path of the helper
> we fallback to the path above if not found.
>
> Fix the check so that the function checks whether the helper
> exists and is executable.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   src/qemu/qemu_slirp.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c
> index d2e4ed79be..4e5ab12727 100644
> --- a/src/qemu/qemu_slirp.c
> +++ b/src/qemu/qemu_slirp.c
> @@ -101,7 +101,8 @@ qemuSlirpNewForHelper(const char *helper)
>       virJSONValuePtr featuresJSON;
>       size_t i, nfeatures;
>   
> -    if (!helper)
> +    if (!helper ||
> +        !virFileIsExecutable(helper))


I would prefer if either then entire expression was on a single line 
(since it's so short), or if you're going to put it on multiple lines, 
that you surroung the "return NULL;" with { .... } (I think the coding 
standards say to do this, but the syntax-check doesn't check it)

Reviewed-by: Laine Stump <laine@redhat.com>


>           return NULL;
>   
>       slirp = qemuSlirpNew();