1. Patchew
  2. Libvirt
  3. View series

[libvirt PATCH] rpc: don't try to spawn non-existant daemon

Daniel P. Berrangé posted 1 patch 1 year, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20230125142709.830297-1-berrange@redhat.com
There is a newer version of this series
src/rpc/virnetsocket.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
[libvirt PATCH] rpc: don't try to spawn non-existant daemon
Posted by Daniel P. Berrangé 1 year, 3 months ago 
If libvirt is built in client only mode, the libvirtd/virtqemud/etc
daemons won't exist. If the client is told to connect to a local
hypervisor, it'll see the socket doesn't exist, try to spawn the
daemon and then re-try connecting to the socket for a few seconds.
Ultimately this will fail because the daemon doesn't exist and the
user gets an error message

  error: Failed to connect socket to '/run/user/1000/libvirt/virtqemud-sock': No such file or directory

technically this is accurate, but it doesn't help identify the root
cause. With this change it will now report

  error: binary 'virtqemud' does not exist in $PATH: No such file or directory

and will skip all the socket connect retries

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/rpc/virnetsocket.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 8280bda007..bb2e0c5d3d 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -123,9 +123,19 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket);
 #ifndef WIN32
 static int virNetSocketForkDaemon(const char *binary)
 {
-    g_autoptr(virCommand) cmd = virCommandNewArgList(binary,
-                                                     "--timeout=120",
-                                                     NULL);
+    g_autofree char *binarypath = virFindFileInPath(binary);
+    g_autoptr(virCommand) cmd = NULL;
+
+    if (!binarypath) {
+        virReportSystemError(ENOENT,
+                             _("binary '%s' does not exist in $PATH"),
+                             binary);
+        return -1;
+    }
+
+    cmd = virCommandNewArgList(binarypath,
+                               "--timeout=120",
+                               NULL);
 
     virCommandAddEnvPassCommon(cmd);
     virCommandAddEnvPass(cmd, "XDG_CACHE_HOME");
-- 
2.39.0
Re: [libvirt PATCH] rpc: don't try to spawn non-existant daemon
Posted by Martin Kletzander 1 year, 3 months ago 
On Wed, Jan 25, 2023 at 02:27:09PM +0000, Daniel P. Berrangé wrote:
>If libvirt is built in client only mode, the libvirtd/virtqemud/etc
>daemons won't exist. If the client is told to connect to a local
>hypervisor, it'll see the socket doesn't exist, try to spawn the
>daemon and then re-try connecting to the socket for a few seconds.
>Ultimately this will fail because the daemon doesn't exist and the
>user gets an error message
>
>  error: Failed to connect socket to '/run/user/1000/libvirt/virtqemud-sock': No such file or directory
>
>technically this is accurate, but it doesn't help identify the root
>cause. With this change it will now report
>
>  error: binary 'virtqemud' does not exist in $PATH: No such file or directory
>
>and will skip all the socket connect retries
>
>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

>---
> src/rpc/virnetsocket.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
>diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
>index 8280bda007..bb2e0c5d3d 100644
>--- a/src/rpc/virnetsocket.c
>+++ b/src/rpc/virnetsocket.c
>@@ -123,9 +123,19 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket);
> #ifndef WIN32
> static int virNetSocketForkDaemon(const char *binary)
> {
>-    g_autoptr(virCommand) cmd = virCommandNewArgList(binary,
>-                                                     "--timeout=120",
>-                                                     NULL);
>+    g_autofree char *binarypath = virFindFileInPath(binary);
>+    g_autoptr(virCommand) cmd = NULL;
>+
>+    if (!binarypath) {
>+        virReportSystemError(ENOENT,
>+                             _("binary '%s' does not exist in $PATH"),
>+                             binary);
>+        return -1;
>+    }
>+
>+    cmd = virCommandNewArgList(binarypath,
>+                               "--timeout=120",
>+                               NULL);
>
>     virCommandAddEnvPassCommon(cmd);
>     virCommandAddEnvPass(cmd, "XDG_CACHE_HOME");
>-- 
>2.39.0
>
Re: [libvirt PATCH] rpc: don't try to spawn non-existant daemon
Posted by Martin Kletzander 1 year, 3 months ago 
On Wed, Jan 25, 2023 at 04:06:49PM +0100, Martin Kletzander wrote:
>On Wed, Jan 25, 2023 at 02:27:09PM +0000, Daniel P. Berrangé wrote:
>>If libvirt is built in client only mode, the libvirtd/virtqemud/etc
>>daemons won't exist. If the client is told to connect to a local
>>hypervisor, it'll see the socket doesn't exist, try to spawn the
>>daemon and then re-try connecting to the socket for a few seconds.
>>Ultimately this will fail because the daemon doesn't exist and the
>>user gets an error message
>>
>>  error: Failed to connect socket to '/run/user/1000/libvirt/virtqemud-sock': No such file or directory
>>
>>technically this is accurate, but it doesn't help identify the root
>>cause. With this change it will now report
>>
>>  error: binary 'virtqemud' does not exist in $PATH: No such file or directory
>>
>>and will skip all the socket connect retries
>>

Actually, Michal had a good question, this should fail in virCommandRun,
and looking at virExec it even does the virFindFileInPath if the first
argument is not absolute and errors out if the path cannot be found.
You might still get ENOENT when running the binary in case a linked
library is missing or something and even without this check.  I guess
our double-fork() in virExec() might be at fault?  When not using a
pidfile there is no wait for the second child to let us know that it
failed (or close the pipesync[1]).  Maybe that is the root cause?

>>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>
>Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
>
>>---
>> src/rpc/virnetsocket.c | 16 +++++++++++++---
>> 1 file changed, 13 insertions(+), 3 deletions(-)
>>
>>diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
>>index 8280bda007..bb2e0c5d3d 100644
>>--- a/src/rpc/virnetsocket.c
>>+++ b/src/rpc/virnetsocket.c
>>@@ -123,9 +123,19 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket);
>> #ifndef WIN32
>> static int virNetSocketForkDaemon(const char *binary)
>> {
>>-    g_autoptr(virCommand) cmd = virCommandNewArgList(binary,
>>-                                                     "--timeout=120",
>>-                                                     NULL);
>>+    g_autofree char *binarypath = virFindFileInPath(binary);
>>+    g_autoptr(virCommand) cmd = NULL;
>>+
>>+    if (!binarypath) {
>>+        virReportSystemError(ENOENT,
>>+                             _("binary '%s' does not exist in $PATH"),
>>+                             binary);
>>+        return -1;
>>+    }
>>+
>>+    cmd = virCommandNewArgList(binarypath,
>>+                               "--timeout=120",
>>+                               NULL);
>>
>>     virCommandAddEnvPassCommon(cmd);
>>     virCommandAddEnvPass(cmd, "XDG_CACHE_HOME");
>>--
>>2.39.0
>>
Re: [libvirt PATCH] rpc: don't try to spawn non-existant daemon
Posted by Daniel P. Berrangé 1 year, 3 months ago 
On Wed, Jan 25, 2023 at 04:39:31PM +0100, Martin Kletzander wrote:
> On Wed, Jan 25, 2023 at 04:06:49PM +0100, Martin Kletzander wrote:
> > On Wed, Jan 25, 2023 at 02:27:09PM +0000, Daniel P. Berrangé wrote:
> > > If libvirt is built in client only mode, the libvirtd/virtqemud/etc
> > > daemons won't exist. If the client is told to connect to a local
> > > hypervisor, it'll see the socket doesn't exist, try to spawn the
> > > daemon and then re-try connecting to the socket for a few seconds.
> > > Ultimately this will fail because the daemon doesn't exist and the
> > > user gets an error message
> > > 
> > >  error: Failed to connect socket to '/run/user/1000/libvirt/virtqemud-sock': No such file or directory
> > > 
> > > technically this is accurate, but it doesn't help identify the root
> > > cause. With this change it will now report
> > > 
> > >  error: binary 'virtqemud' does not exist in $PATH: No such file or directory
> > > 
> > > and will skip all the socket connect retries
> > > 
> 
> Actually, Michal had a good question, this should fail in virCommandRun,
> and looking at virExec it even does the virFindFileInPath if the first
> argument is not absolute and errors out if the path cannot be found.
> You might still get ENOENT when running the binary in case a linked
> library is missing or something and even without this check.  I guess
> our double-fork() in virExec() might be at fault?  When not using a
> pidfile there is no wait for the second child to let us know that it
> failed (or close the pipesync[1]).  Maybe that is the root cause?

This in most cases virCommand would do the right thing in
reporting the missing binary.

In this specific case though, we're daemonizing the binary
we're spawning and letting it start asynchronously, so we
don't have a way to feed the ENOENT back from the child.


With 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 :|
Re: [libvirt PATCH] rpc: don't try to spawn non-existant daemon
Posted by Martin Kletzander 1 year, 3 months ago 
On Wed, Jan 25, 2023 at 03:44:26PM +0000, Daniel P. Berrangé wrote:
>On Wed, Jan 25, 2023 at 04:39:31PM +0100, Martin Kletzander wrote:
>> On Wed, Jan 25, 2023 at 04:06:49PM +0100, Martin Kletzander wrote:
>> > On Wed, Jan 25, 2023 at 02:27:09PM +0000, Daniel P. Berrangé wrote:
>> > > If libvirt is built in client only mode, the libvirtd/virtqemud/etc
>> > > daemons won't exist. If the client is told to connect to a local
>> > > hypervisor, it'll see the socket doesn't exist, try to spawn the
>> > > daemon and then re-try connecting to the socket for a few seconds.
>> > > Ultimately this will fail because the daemon doesn't exist and the
>> > > user gets an error message
>> > >
>> > >  error: Failed to connect socket to '/run/user/1000/libvirt/virtqemud-sock': No such file or directory
>> > >
>> > > technically this is accurate, but it doesn't help identify the root
>> > > cause. With this change it will now report
>> > >
>> > >  error: binary 'virtqemud' does not exist in $PATH: No such file or directory
>> > >
>> > > and will skip all the socket connect retries
>> > >
>>
>> Actually, Michal had a good question, this should fail in virCommandRun,
>> and looking at virExec it even does the virFindFileInPath if the first
>> argument is not absolute and errors out if the path cannot be found.
>> You might still get ENOENT when running the binary in case a linked
>> library is missing or something and even without this check.  I guess
>> our double-fork() in virExec() might be at fault?  When not using a
>> pidfile there is no wait for the second child to let us know that it
>> failed (or close the pipesync[1]).  Maybe that is the root cause?
>
>This in most cases virCommand would do the right thing in
>reporting the missing binary.
>
>In this specific case though, we're daemonizing the binary
>we're spawning and letting it start asynchronously, so we
>don't have a way to feed the ENOENT back from the child.
>

Oh, I get it, the binary is probably already an absolute path, so the
check is skipped in virExec().  Maybe virExec() should always do the
check with virFindFileInPath() and not just when the first argument is
not absolute.  That would catch the same error more places.

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.