[PATCH] qemu_slirp: Check if helper exists before fetching its capabilities

Michal Privoznik posted 1 patch 1 week ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/7fc62bd552b7838a8cdf111fafae3011c585a1ca.1602595948.git.mprivozn@redhat.com
src/qemu/qemu_slirp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

[PATCH] qemu_slirp: Check if helper exists before fetching its capabilities

Posted by Michal Privoznik 1 week ago
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

Re: [PATCH] qemu_slirp: Check if helper exists before fetching its capabilities

Posted by Laine Stump 1 week ago
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();


Re: [PATCH] qemu_slirp: Check if helper exists before fetching its capabilities

Posted by Daniel P. Berrangé 1 week ago
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 :|