[PATCH] qemu_slirp: Check if helper exists before fetching its capabilities
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();
participants (3)
-
Daniel P. Berrangé -
Laine Stump -
Michal Privoznik