[Qemu-devel] [PATCH 0/2]: qemu-ga: Fix fd handling in daemon mode

[Qemu-devel] [PATCH 0/2]: qemu-ga: Fix fd handling in daemon mode

[Luiz Capitulino]

Hi Michael,

There's a problem on how we handle fds in daemon mode that may cause
suspend to fail. This series fixes it. Please, check patch 2/2 for full
details.

 qemu-ga.c              |   23 ++++++++++++++++++++---
 qga/commands-posix.c   |   19 -------------------
 qga/guest-agent-core.h |    4 ++++
 3 files changed, 24 insertions(+), 22 deletions(-)

[Qemu-devel] [PATCH 1/2] qemu-ga: make reopen_fd_to_null() public

[Luiz Capitulino]

The next commit wants to use it.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-ga.c              |   17 +++++++++++++++++
 qga/commands-posix.c   |   19 -------------------
 qga/guest-agent-core.h |    4 ++++
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 216be39..7896565 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -140,6 +140,23 @@ static gboolean register_signal_handlers(void)
 
     return true;
 }
+
+/* TODO: use this in place of all post-fork() fclose(std*) callers */
+void reopen_fd_to_null(int fd)
+{
+    int nullfd;
+
+    nullfd = open("/dev/null", O_RDWR);
+    if (nullfd < 0) {
+        return;
+    }
+
+    dup2(nullfd, fd);
+
+    if (nullfd != fd) {
+        close(nullfd);
+    }
+}
 #endif
 
 static void usage(const char *cmd)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index d58730a..2ce9b82 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -34,25 +34,6 @@
 #endif
 #endif
 
-#if defined(__linux__)
-/* TODO: use this in place of all post-fork() fclose(std*) callers */
-static void reopen_fd_to_null(int fd)
-{
-    int nullfd;
-
-    nullfd = open("/dev/null", O_RDWR);
-    if (nullfd < 0) {
-        return;
-    }
-
-    dup2(nullfd, fd);
-
-    if (nullfd != fd) {
-        close(nullfd);
-    }
-}
-#endif /* defined(__linux__) */
-
 void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
 {
     int ret;
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index bbb8b9b..6dba104 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -35,3 +35,7 @@ void ga_set_response_delimited(GAState *s);
 bool ga_is_frozen(GAState *s);
 void ga_set_frozen(GAState *s);
 void ga_unset_frozen(GAState *s);
+
+#ifndef _WIN32
+void reopen_fd_to_null(int fd);
+#endif
-- 
1.7.9.2.384.g4a92a

Re: [Qemu-devel] [PATCH 1/2] qemu-ga: make reopen_fd_to_null() public

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

On 05/10/2012 01:50 PM, Luiz Capitulino wrote:
> The next commit wants to use it.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-ga.c              |   17 +++++++++++++++++
>  qga/commands-posix.c   |   19 -------------------
>  qga/guest-agent-core.h |    4 ++++
>  3 files changed, 21 insertions(+), 19 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[Qemu-devel] [PATCH 2/2] qemu-ga: become_daemon(): reopen standard fds to /dev/null

[Luiz Capitulino]

This fixes a bug where qemu-ga doesn't suspend the guest because it
fails to detect suspend support even when the guest does support
suspend. This happens because of the way qemu-ga fds are managed in
daemon mode.

When starting qemu-ga with --daemon, become_daemon() will close all
standard fds. This will cause qemu-ga to end up with the following
fds (if started with 'qemu-ga --daemon'):

    0 -> /dev/vport0p1
    3 -> /run/qemu-ga.pid

Then a guest-suspend-* function is issued. They call bios_supports_mode(),
which will call pipe(), and qemu-ga's fd will be:

    0 -> /dev/vport0p1
    1 -> pipe:[16247]
    2 -> pipe:[16247]
    3 -> /run/qemu-ga.pid

bios_supports_mode() forks off a child and blocks waiting for the child
to write something to the pipe. The child, however, closes its reading
end of the pipe _and_ reopen all standard fds to /dev/null. This will
cause the child's fds to be:

    0 -> /dev/null
    1 -> /dev/null
    2 -> /dev/null
    3 -> /run/qemu-ga.pid

In other words, the child's writing end of the pipe is now /dev/null.
It writes there and exits. The parent process (blocked on read()) will
get an EOF and interpret this as "something unexpected happened in
the child, let's assume the guest doesn't support suspend". And suspend
will fail.

To solve this problem we have to reopen standard fds to /dev/null
in become_daemon(), instead of closing them.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-ga.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 7896565..c64bc71 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -445,9 +445,9 @@ static void become_daemon(const char *pidfile)
         goto fail;
     }
 
-    close(STDIN_FILENO);
-    close(STDOUT_FILENO);
-    close(STDERR_FILENO);
+    reopen_fd_to_null(STDIN_FILENO);
+    reopen_fd_to_null(STDOUT_FILENO);
+    reopen_fd_to_null(STDERR_FILENO);
     return;
 
 fail:
-- 
1.7.9.2.384.g4a92a

Re: [Qemu-devel] [PATCH 2/2] qemu-ga: become_daemon(): reopen standard fds to /dev/null

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1827 bytes --]

On 05/10/2012 01:50 PM, Luiz Capitulino wrote:
> This fixes a bug where qemu-ga doesn't suspend the guest because it
> fails to detect suspend support even when the guest does support
> suspend. This happens because of the way qemu-ga fds are managed in
> daemon mode.
> 
> When starting qemu-ga with --daemon, become_daemon() will close all
> standard fds. This will cause qemu-ga to end up with the following
> fds (if started with 'qemu-ga --daemon'):
> 
>     0 -> /dev/vport0p1
>     3 -> /run/qemu-ga.pid
> 
> Then a guest-suspend-* function is issued. They call bios_supports_mode(),
> which will call pipe(), and qemu-ga's fd will be:
> 
>     0 -> /dev/vport0p1
>     1 -> pipe:[16247]
>     2 -> pipe:[16247]
>     3 -> /run/qemu-ga.pid

Very nasty.


> To solve this problem we have to reopen standard fds to /dev/null
> in become_daemon(), instead of closing them.

Yes, POSIX warns that applications should never call exec() with fd 0,
1, or 2 closed, at least not if the application wants the child to
behave in a conforming environment.

> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-ga.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 7896565..c64bc71 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -445,9 +445,9 @@ static void become_daemon(const char *pidfile)
>          goto fail;
>      }
>  
> -    close(STDIN_FILENO);
> -    close(STDOUT_FILENO);
> -    close(STDERR_FILENO);
> +    reopen_fd_to_null(STDIN_FILENO);
> +    reopen_fd_to_null(STDOUT_FILENO);
> +    reopen_fd_to_null(STDERR_FILENO);

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 0/2]: qemu-ga: Fix fd handling in daemon mode

[Michael Roth]

On Thu, May 10, 2012 at 04:50:40PM -0300, Luiz Capitulino wrote:
> Hi Michael,
> 
> There's a problem on how we handle fds in daemon mode that may cause
> suspend to fail. This series fixes it. Please, check patch 2/2 for full
> details.

Thanks, applied to qemu-ga branch.

> 
>  qemu-ga.c              |   23 ++++++++++++++++++++---
>  qga/commands-posix.c   |   19 -------------------
>  qga/guest-agent-core.h |    4 ++++
>  3 files changed, 24 insertions(+), 22 deletions(-)
>