[Qemu-devel] [PULL 0/8] qemu-ga: cleanups and minor fixes

[Qemu-devel] [PULL 0/8] qemu-ga: cleanups and minor fixes

[Michael Roth]

The following changes since commit a507db9599599ce33007b524276a6ea88e521662:

  Merge remote-tracking branch 'kraxel/pixman.v6' into staging (2013-01-14 10:27:41 -0600)

are available in the git repository at:


  git://github.com/mdroth/qemu.git qga-pull-1-14-2013

for you to fetch changes up to 7868181f98ff1fbcd7f7034153eec5e03615d023:

  qemu-ga: Handle errors uniformely in ga_channel_open() (2013-01-14 12:08:05 -0600)

----------------------------------------------------------------
Luiz Capitulino (2):
      qemu-ga: ga_open_pidfile(): use qemu_open()
      qemu-ga: add ga_open_logfile()

Markus Armbruster (6):
      qemu-ga: Document intentional fall through in channel_event_cb()
      qemu-ga: Plug fd leak on ga_channel_open() error paths
      qemu-ga: Handle errors uniformely in ga_channel_open()

 qga/channel-posix.c |   13 +++++++++----
 qga/main.c          |   24 ++++++++++++++++++++----
 2 files changed, 29 insertions(+), 8 deletions(-)

[Qemu-devel] [PATCH 1/8] qemu-ga: ga_open_pidfile(): use qemu_open()

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

This ensures that O_CLOEXEC is passed to open(), this way the
pid file fd is not leaked to executed processes.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Acked-by: Amos Kong <akong@redhat.com>
Tested-by: Amos Kong <akong@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/main.c b/qga/main.c
index a9b968c..4239150 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -267,7 +267,7 @@ static bool ga_open_pidfile(const char *pidfile)
     int pidfd;
     char pidstr[32];
 
-    pidfd = open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
+    pidfd = qemu_open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
     if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) {
         g_critical("Cannot lock pid file, %s", strerror(errno));
         if (pidfd != -1) {
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/8] qemu-ga: add ga_open_logfile()

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

This function sets O_CLOEXEC on the log file fd so that it isn't
leaked to executed processes.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Acked-by: Amos Kong <akong@redhat.com>
Tested-by: Amos Kong <akong@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 4239150..bd70ead 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -261,6 +261,19 @@ void ga_set_response_delimited(GAState *s)
     s->delimit_response = true;
 }
 
+static FILE *ga_open_logfile(const char *logfile)
+{
+    FILE *f;
+
+    f = fopen(logfile, "a");
+    if (!f) {
+        return NULL;
+    }
+
+    qemu_set_cloexec(fileno(f));
+    return f;
+}
+
 #ifndef _WIN32
 static bool ga_open_pidfile(const char *pidfile)
 {
@@ -402,7 +415,7 @@ void ga_unset_frozen(GAState *s)
      * in a frozen state at start up, do it now
      */
     if (s->deferred_options.log_filepath) {
-        s->log_file = fopen(s->deferred_options.log_filepath, "a");
+        s->log_file = ga_open_logfile(s->deferred_options.log_filepath);
         if (!s->log_file) {
             s->log_file = stderr;
         }
@@ -884,7 +897,7 @@ int main(int argc, char **argv)
             become_daemon(pid_filepath);
         }
         if (log_filepath) {
-            FILE *log_file = fopen(log_filepath, "a");
+            FILE *log_file = ga_open_logfile(log_filepath);
             if (!log_file) {
                 g_critical("unable to open specified log file: %s",
                            strerror(errno));
-- 
1.7.9.5

[Qemu-devel] [PATCH 3/8] qemu-ga: Document intentional fall through in channel_event_cb()

[Michael Roth]

From: Markus Armbruster <armbru@redhat.com>

For clarity, and to hush up Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/qga/main.c b/qga/main.c
index bd70ead..e8a9a9e 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -618,6 +618,7 @@ static gboolean channel_event_cb(GIOCondition condition, gpointer data)
         if (!s->virtio) {
             return false;
         }
+        /* fall through */
     case G_IO_STATUS_AGAIN:
         /* virtio causes us to spin here when no process is attached to
          * host-side chardev. sleep a bit to mitigate this
-- 
1.7.9.5

[Qemu-devel] [PATCH 4/8] qemu-ga: Drop pointless lseek() from ga_open_pidfile()

[Michael Roth]

From: Markus Armbruster <armbru@redhat.com>

After open(), the file offset is already zero, and neither lockf() nor
ftruncate() change it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/main.c b/qga/main.c
index e8a9a9e..96d3cfa 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -289,7 +289,7 @@ static bool ga_open_pidfile(const char *pidfile)
         return false;
     }
 
-    if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
+    if (ftruncate(pidfd, 0)) {
         g_critical("Failed to truncate pid file");
         goto fail;
     }
-- 
1.7.9.5

[Qemu-devel] [PATCH 5/8] qemu-ga: Plug file descriptor leak on ga_open_pidfile() error path

[Michael Roth]

From: Markus Armbruster <armbru@redhat.com>

Spotted by Coverity.  Also document why we keep it open on success.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/qga/main.c b/qga/main.c
index 96d3cfa..db281a5 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -299,10 +299,12 @@ static bool ga_open_pidfile(const char *pidfile)
         goto fail;
     }
 
+    /* keep pidfile open & locked forever */
     return true;
 
 fail:
     unlink(pidfile);
+    close(pidfd);
     return false;
 }
 #else /* _WIN32 */
-- 
1.7.9.5

[Qemu-devel] [PATCH 6/8] qemu-ga: Plug fd leak on ga_channel_listen_accept() error path

[Michael Roth]

From: Markus Armbruster <armbru@redhat.com>

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/channel-posix.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index ca9e4aa..9a5c05d 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -46,6 +46,7 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
     ret = ga_channel_client_add(c, client_fd);
     if (ret) {
         g_warning("error setting up connection");
+        close(client_fd);
         goto out;
     }
     accepted = true;
-- 
1.7.9.5

[Qemu-devel] [PATCH 7/8] qemu-ga: Plug fd leak on ga_channel_open() error paths

[Michael Roth]

From: Markus Armbruster <armbru@redhat.com>

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/channel-posix.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 9a5c05d..05e8386 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -154,6 +154,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod
         ret = ga_channel_client_add(c, fd);
         if (ret) {
             g_critical("error adding channel to main loop");
+            close(fd);
             return false;
         }
         break;
-- 
1.7.9.5

[Qemu-devel] [PATCH 8/8] qemu-ga: Handle errors uniformely in ga_channel_open()

[Michael Roth]

From: Markus Armbruster <armbru@redhat.com>

We detect errors in several places.  One reports with g_error(), which
calls abort(), the others report with g_critical().  Three of them
exit(), three return false.

Always report with g_critical(), and return false.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

*minor fix-up of commit msg

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/channel-posix.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 05e8386..e65dda3 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -141,14 +141,15 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod
                            );
         if (fd == -1) {
             g_critical("error opening channel: %s", strerror(errno));
-            exit(EXIT_FAILURE);
+            return false;
         }
 #ifdef CONFIG_SOLARIS
         ret = ioctl(fd, I_SETSIG, S_OUTPUT | S_INPUT | S_HIPRI);
         if (ret == -1) {
             g_critical("error setting event mask for channel: %s",
                        strerror(errno));
-            exit(EXIT_FAILURE);
+            close(fd);
+            return false;
         }
 #endif
         ret = ga_channel_client_add(c, fd);
@@ -164,7 +165,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod
         int fd = qemu_open(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
         if (fd == -1) {
             g_critical("error opening channel: %s", strerror(errno));
-            exit(EXIT_FAILURE);
+            return false;
         }
         tcgetattr(fd, &tio);
         /* set up serial port for non-canonical, dumb byte streaming */
@@ -184,7 +185,9 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod
         tcsetattr(fd, TCSANOW, &tio);
         ret = ga_channel_client_add(c, fd);
         if (ret) {
-            g_error("error adding channel to main loop");
+            g_critical("error adding channel to main loop");
+            close(fd);
+            return false;
         }
         break;
     }
-- 
1.7.9.5

Re: [Qemu-devel] [PULL 0/8] qemu-ga: cleanups and minor fixes

[Anthony Liguori]

Pulled, thanks.

Regards,

Anthony Liguori