[Qemu-devel] [PATCH 0/6] Simple qemu-ga fixes and cleanups

[Qemu-devel] [PATCH 0/6] Simple qemu-ga fixes and cleanups

[Markus Armbruster]

Markus Armbruster (6):
  qemu-ga: Document intentional fall through in channel_event_cb()
  qemu-ga: Drop pointless lseek() from ga_open_pidfile()
  qemu-ga: Plug file descriptor leak on ga_open_pidfile() error path
  qemu-ga: Plug fd leak on ga_channel_listen_accept() error path
  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          |  5 ++++-
 2 files changed, 13 insertions(+), 5 deletions(-)

-- 
1.7.11.7

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

[Markus Armbruster]

For clarity, and to hush up Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qga/main.c b/qga/main.c
index a9b968c..47a6bea 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -605,6 +605,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.11.7

Re: [Qemu-devel] [PATCH 1/6] qemu-ga: Document intentional fall through in channel_event_cb()

[mdroth]

On Fri, Jan 11, 2013 at 11:24:57AM +0100, Markus Armbruster wrote:
> For clarity, and to hush up Coverity.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-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 a9b968c..47a6bea 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -605,6 +605,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.11.7
> 

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

[Markus Armbruster]

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/main.c b/qga/main.c
index 47a6bea..1a22d8d 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -276,7 +276,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.11.7

Re: [Qemu-devel] [PATCH 2/6] qemu-ga: Drop pointless lseek() from ga_open_pidfile()

[mdroth]

On Fri, Jan 11, 2013 at 11:24:58AM +0100, Markus Armbruster wrote:
> 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: 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 47a6bea..1a22d8d 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -276,7 +276,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.11.7
> 

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

[Markus Armbruster]

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qga/main.c b/qga/main.c
index 1a22d8d..db47427 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -286,10 +286,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.11.7

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

[mdroth]

On Fri, Jan 11, 2013 at 11:24:59AM +0100, Markus Armbruster wrote:
> Spotted by Coverity.  Also document why we keep it open on success.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-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 1a22d8d..db47427 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -286,10 +286,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.11.7
> 

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

[Markus Armbruster]

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/channel-posix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index d4fd628..5579185 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -45,6 +45,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.11.7

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

[mdroth]

On Fri, Jan 11, 2013 at 11:25:00AM +0100, Markus Armbruster wrote:
> Spotted by Coverity.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-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 d4fd628..5579185 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -45,6 +45,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.11.7
> 

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

[Markus Armbruster]

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/channel-posix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 5579185..b530808 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -153,6 +153,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.11.7

Re: [Qemu-devel] [PATCH 5/6] qemu-ga: Plug fd leak on ga_channel_open() error paths

[mdroth]

On Fri, Jan 11, 2013 at 11:25:01AM +0100, Markus Armbruster wrote:
> Spotted by Coverity.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-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 5579185..b530808 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -153,6 +153,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.11.7
> 

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

[Markus Armbruster]

We detect errors in seven 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>
---
 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 b530808..96274f5 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -140,14 +140,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);
@@ -163,7 +164,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 */
@@ -183,7 +184,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.11.7

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

[mdroth]

On Fri, Jan 11, 2013 at 11:25:02AM +0100, Markus Armbruster wrote:
> We detect errors in seven places.  One reports with g_error(), which

Do you mean "in several places"? I can fix this in tree.

> 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: 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 b530808..96274f5 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -140,14 +140,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);
> @@ -163,7 +164,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 */
> @@ -183,7 +184,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.11.7
> 

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

[Markus Armbruster]

mdroth <mdroth@linux.vnet.ibm.com> writes:

> On Fri, Jan 11, 2013 at 11:25:02AM +0100, Markus Armbruster wrote:
>> We detect errors in seven places.  One reports with g_error(), which
>
> Do you mean "in several places"? I can fix this in tree.

I counted seven places, but it doesn't really matter, "several" would be
fine, too.

>> 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: Michael Roth <mdroth@linux.vnet.ibm.com>

Thanks!

Re: [Qemu-devel] [PATCH 0/6] Simple qemu-ga fixes and cleanups

[Eric Blake]

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

On 01/11/2013 03:24 AM, Markus Armbruster wrote:
> Markus Armbruster (6):
>   qemu-ga: Document intentional fall through in channel_event_cb()
>   qemu-ga: Drop pointless lseek() from ga_open_pidfile()
>   qemu-ga: Plug file descriptor leak on ga_open_pidfile() error path
>   qemu-ga: Plug fd leak on ga_channel_listen_accept() error path
>   qemu-ga: Plug fd leak on ga_channel_open() error paths
>   qemu-ga: Handle errors uniformely in ga_channel_open()

Series: 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: 619 bytes --]

Re: [Qemu-devel] [PATCH 0/6] Simple qemu-ga fixes and cleanups

[Luiz Capitulino]

On Fri, 11 Jan 2013 11:24:56 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Markus Armbruster (6):
>   qemu-ga: Document intentional fall through in channel_event_cb()
>   qemu-ga: Drop pointless lseek() from ga_open_pidfile()
>   qemu-ga: Plug file descriptor leak on ga_open_pidfile() error path
>   qemu-ga: Plug fd leak on ga_channel_listen_accept() error path
>   qemu-ga: Plug fd leak on ga_channel_open() error paths
>   qemu-ga: Handle errors uniformely in ga_channel_open()

series:

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> 
>  qga/channel-posix.c | 13 +++++++++----
>  qga/main.c          |  5 ++++-
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/6] Simple qemu-ga fixes and cleanups

[mdroth]

On Fri, Jan 11, 2013 at 11:24:56AM +0100, Markus Armbruster wrote:
> Markus Armbruster (6):
>   qemu-ga: Document intentional fall through in channel_event_cb()
>   qemu-ga: Drop pointless lseek() from ga_open_pidfile()
>   qemu-ga: Plug file descriptor leak on ga_open_pidfile() error path
>   qemu-ga: Plug fd leak on ga_channel_listen_accept() error path
>   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          |  5 ++++-
>  2 files changed, 13 insertions(+), 5 deletions(-)

Thanks, applied to qga branch with minor commit msg fix for patch 6.

> 
> -- 
> 1.7.11.7
>