[vdagent-linux,2/4] vport: add @err to disconnect_callback

Submitted by Jakub Janku on Sept. 30, 2018, 6:05 p.m.

Details

Message ID 20180930180523.14162-3-jjanku@redhat.com
State New
Headers show
Series "GLib & GIO integration: VDAgentConnection" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Jakub Janku Sept. 30, 2018, 6:05 p.m.
This is necessary for the following GMainLoop integration:

vdagentd currently uses a while-loop in which
vdagent_virtio_port_handle_fds() is repeatedly called.
If an IO error occurs, the function sets virtio_port to NULL.
This way, we can detect when the virtio_port has been destroyed and
try to reconnect if desired. (see main_loop() in vdagentd.c)
The vdagent_virtio_port_disconnect_callback is not used.

In the following commit, the main_loop() is going to be removed and replaced
by a GMainLoop. In order to be able to detect when the virtio_port has been
destroyed, vdagent_virtio_port_disconnect_callback will have to be used.
The virtio_port should be reconnected only when an IO error happened.
However, the vdagent_virtio_port_disconnect_callback is always called,
even when the diconnect was initiated by vdagent_virtio_port_destroy().

To be able to distinguish these two cases, add @err parameter
to the vdagent_virtio_port_disconnect_callback:

1) disconnect was initiated by vdagent_virtio_port_destroy()
   --> @err is NULL
2) disconnect was caused by an IO error
   --> @err is set accordingly

Signed-off-by: Jakub Janků <jjanku@redhat.com>
---
 src/vdagentd/virtio-port.c | 44 +++++++++++++++++++++++++-------------
 src/vdagentd/virtio-port.h |  8 +++----
 2 files changed, 32 insertions(+), 20 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
index e48d107..497811e 100644
--- a/src/vdagentd/virtio-port.c
+++ b/src/vdagentd/virtio-port.c
@@ -29,6 +29,7 @@ 
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <glib.h>
+#include <gio/gio.h>
 
 #include "virtio-port.h"
 
@@ -120,7 +121,8 @@  error:
     return NULL;
 }
 
-void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
+static void virtio_port_destroy(struct vdagent_virtio_port **vportp,
+                                GError *err)
 {
     struct vdagent_virtio_port_buf *wbuf, *next_wbuf;
     struct vdagent_virtio_port *vport = *vportp;
@@ -130,7 +132,9 @@  void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
         return;
 
     if (vport->disconnect_callback)
-        vport->disconnect_callback(vport);
+        vport->disconnect_callback(vport, err);
+
+    g_clear_error(&err);
 
     wbuf = vport->write_buf;
     while (wbuf) {
@@ -148,6 +152,11 @@  void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
     g_clear_pointer(vportp, g_free);
 }
 
+void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
+{
+    virtio_port_destroy(vportp, NULL);
+}
+
 int vdagent_virtio_port_fill_fds(struct vdagent_virtio_port *vport,
         fd_set *readfds, fd_set *writefds)
 {
@@ -314,8 +323,9 @@  static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
         avail = vport->chunk_header.size - pos;
 
         if (avail > read) {
-            syslog(LOG_ERR, "chunk larger than message, lost sync?");
-            vdagent_virtio_port_destroy(vportp);
+            GError *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
+                                      "chunk larger than message, lost sync?");
+            virtio_port_destroy(vportp, err);
             return;
         }
 
@@ -333,7 +343,7 @@  static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
                 int r = vport->read_callback(vport, vport->chunk_header.port,
                                  &port->message_header, port->message_data);
                 if (r == -1) {
-                    vdagent_virtio_port_destroy(vportp);
+                    virtio_port_destroy(vportp, NULL);
                     return;
                 }
             }
@@ -372,7 +382,6 @@  static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
     if (n < 0) {
         if (errno == EINTR)
             return;
-        syslog(LOG_ERR, "reading from vdagent virtio port: %m");
     }
     if (n == 0 && vport->opening) {
         /* When we open the virtio serial port, the following happens:
@@ -399,7 +408,9 @@  static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
         return;
     }
     if (n <= 0) {
-        vdagent_virtio_port_destroy(vportp);
+        GError *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
+                                  "reading from vdagent virtio port: %m");
+        virtio_port_destroy(vportp, err);
         return;
     }
     vport->opening = 0;
@@ -410,15 +421,17 @@  static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
             vport->chunk_header.size = GUINT32_FROM_LE(vport->chunk_header.size);
             vport->chunk_header.port = GUINT32_FROM_LE(vport->chunk_header.port);
             if (vport->chunk_header.size > VD_AGENT_MAX_DATA_SIZE) {
-                syslog(LOG_ERR, "chunk size %u too large",
-                       vport->chunk_header.size);
-                vdagent_virtio_port_destroy(vportp);
+                GError *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
+                                          "chunk size %u too large",
+                                          vport->chunk_header.size);
+                virtio_port_destroy(vportp, err);
                 return;
             }
             if (vport->chunk_header.port >= VDP_END_PORT) {
-                syslog(LOG_ERR, "chunk port %u out of range",
-                       vport->chunk_header.port);
-                vdagent_virtio_port_destroy(vportp);
+                GError *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
+                                          "chunk port %u out of range",
+                                          vport->chunk_header.port);
+                virtio_port_destroy(vportp, err);
                 return;
             }
         }
@@ -465,8 +478,9 @@  static void vdagent_virtio_port_do_write(struct vdagent_virtio_port **vportp)
     if (n < 0) {
         if (errno == EINTR)
             return;
-        syslog(LOG_ERR, "writing to vdagent virtio port: %m");
-        vdagent_virtio_port_destroy(vportp);
+        GError *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
+                                  "writing to vdagent virtio port: %m");
+        virtio_port_destroy(vportp, err);
         return;
     }
     if (n > 0)
diff --git a/src/vdagentd/virtio-port.h b/src/vdagentd/virtio-port.h
index dffb410..dfbe27b 100644
--- a/src/vdagentd/virtio-port.h
+++ b/src/vdagentd/virtio-port.h
@@ -41,14 +41,12 @@  typedef int (*vdagent_virtio_port_read_callback)(
     uint8_t *data);
 
 /* Callbacks with this type will be called when the port is disconnected.
+   If the virtio port was disconnected due to an IO error, @err is set.
    Note:
    1) vdagent_virtio_port will destroy the port in question itself after
-      this callback has completed!
-   2) This callback is always called, even if the disconnect is initiated
-      by the vdagent_virtio_port user through returning -1 from a read
-      callback, or by explicitly calling vdagent_virtio_port_destroy */
+      this callback has completed! */
 typedef void (*vdagent_virtio_port_disconnect_callback)(
-    struct vdagent_virtio_port *conn);
+    struct vdagent_virtio_port *conn, GError *err);
 
 
 /* Create a vdagent virtio port object for port portname */

Comments

Hi,

On Sun, Sep 30, 2018 at 08:05:21PM +0200, Jakub Janků wrote:
> This is necessary for the following GMainLoop integration:
> 
> vdagentd currently uses a while-loop in which
> vdagent_virtio_port_handle_fds() is repeatedly called.
> If an IO error occurs, the function sets virtio_port to NULL.
> This way, we can detect when the virtio_port has been destroyed and
> try to reconnect if desired. (see main_loop() in vdagentd.c)
> The vdagent_virtio_port_disconnect_callback is not used.
>
> In the following commit, the main_loop() is going to be removed and replaced
> by a GMainLoop. In order to be able to detect when the virtio_port has been
> destroyed, vdagent_virtio_port_disconnect_callback will have to be used.
> The virtio_port should be reconnected only when an IO error happened.
> However, the vdagent_virtio_port_disconnect_callback is always called,
> even when the diconnect was initiated by vdagent_virtio_port_destroy().
> 
> To be able to distinguish these two cases, add @err parameter
> to the vdagent_virtio_port_disconnect_callback:
> 
> 1) disconnect was initiated by vdagent_virtio_port_destroy()
>    --> @err is NULL
> 2) disconnect was caused by an IO error
>    --> @err is set accordingly
> 
> Signed-off-by: Jakub Janků <jjanku@redhat.com>

Thanks for the commit log. So, the public
vdagent_virtio_port_destroy() will be called to do port_destroy
while internally in virtio-port you use virtio_port_destroy()
with error set; The error is propagated to disconnect_callback()
which is not used yet but it will be used with glib mainloop in
follow up patch.

> ---
>  src/vdagentd/virtio-port.c | 44 +++++++++++++++++++++++++-------------
>  src/vdagentd/virtio-port.h |  8 +++----
>  2 files changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
> index e48d107..497811e 100644
> --- a/src/vdagentd/virtio-port.c
> +++ b/src/vdagentd/virtio-port.c
> @@ -29,6 +29,7 @@
>  #include <sys/socket.h>
>  #include <sys/un.h>
>  #include <glib.h>
> +#include <gio/gio.h>
>  
>  #include "virtio-port.h"
>  
> @@ -120,7 +121,8 @@ error:
>      return NULL;
>  }
>  
> -void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> +static void virtio_port_destroy(struct vdagent_virtio_port **vportp,
> +                                GError *err)
>  {
>      struct vdagent_virtio_port_buf *wbuf, *next_wbuf;
>      struct vdagent_virtio_port *vport = *vportp;
> @@ -130,7 +132,9 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
>          return;
>  
>      if (vport->disconnect_callback)
> -        vport->disconnect_callback(vport);
> +        vport->disconnect_callback(vport, err);
> +
> +    g_clear_error(&err);

I think the callback above should be the one doing error cleanup
or we should define a const GError * in disconnect_callback
typedef.

Code would need to change to something like below.

	if (vport->disconnect_callback) {
		vport->disconnect_callback(vport, err);
	} else if (err != NULL) {
		syslog(LOG_ERR, "Error with virtio channel: %s",
		       err->message);
		g_clear_error(&err);
	}

>      wbuf = vport->write_buf;
>      while (wbuf) {
> @@ -148,6 +152,11 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
>      g_clear_pointer(vportp, g_free);
>  }
>  
> +void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> +{
> +    virtio_port_destroy(vportp, NULL);
> +}
> +
>  int vdagent_virtio_port_fill_fds(struct vdagent_virtio_port *vport,
>          fd_set *readfds, fd_set *writefds)
>  {
> @@ -314,8 +323,9 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
>          avail = vport->chunk_header.size - pos;
>  
>          if (avail > read) {
> -            syslog(LOG_ERR, "chunk larger than message, lost sync?");
> -            vdagent_virtio_port_destroy(vportp);
> +            GError *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
> +                                      "chunk larger than message, lost sync?");
> +            virtio_port_destroy(vportp, err);
>              return;
>          }
>  
> @@ -333,7 +343,7 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
>                  int r = vport->read_callback(vport, vport->chunk_header.port,
>                                   &port->message_header, port->message_data);
>                  if (r == -1) {
> -                    vdagent_virtio_port_destroy(vportp);
> +                    virtio_port_destroy(vportp, NULL);
>                      return;
>                  }
>              }
> @@ -372,7 +382,6 @@ static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
>      if (n < 0) {
>          if (errno == EINTR)
>              return;
> -        syslog(LOG_ERR, "reading from vdagent virtio port: %m");
>      }
>      if (n == 0 && vport->opening) {
>          /* When we open the virtio serial port, the following happens:
> @@ -399,7 +408,9 @@ static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
>          return;
>      }
>      if (n <= 0) {
> -        vdagent_virtio_port_destroy(vportp);
> +        GError *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
> +                                  "reading from vdagent virtio port: %m");
> +        virtio_port_destroy(vportp, err);
>          return;
>      }
>      vport->opening = 0;
> @@ -410,15 +421,17 @@ static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
>              vport->chunk_header.size = GUINT32_FROM_LE(vport->chunk_header.size);
>              vport->chunk_header.port = GUINT32_FROM_LE(vport->chunk_header.port);
>              if (vport->chunk_header.size > VD_AGENT_MAX_DATA_SIZE) {
> -                syslog(LOG_ERR, "chunk size %u too large",
> -                       vport->chunk_header.size);
> -                vdagent_virtio_port_destroy(vportp);
> +                GError *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
> +                                          "chunk size %u too large",
> +                                          vport->chunk_header.size);
> +                virtio_port_destroy(vportp, err);
>                  return;
>              }
>              if (vport->chunk_header.port >= VDP_END_PORT) {
> -                syslog(LOG_ERR, "chunk port %u out of range",
> -                       vport->chunk_header.port);
> -                vdagent_virtio_port_destroy(vportp);
> +                GError *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
> +                                          "chunk port %u out of range",
> +                                          vport->chunk_header.port);
> +                virtio_port_destroy(vportp, err);

In general, I would have to say that in both if blocks below,
GError *err should be created in top of block but I can't see
that stated in our code style document and I see this being used
more and more in our codebase. I don't really mind.

	https://www.spice-space.org/spice-project-coding-style-and-coding-conventions.html

>                  return;
>              }
>          }
> @@ -465,8 +478,9 @@ static void vdagent_virtio_port_do_write(struct vdagent_virtio_port **vportp)
>      if (n < 0) {
>          if (errno == EINTR)
>              return;
> -        syslog(LOG_ERR, "writing to vdagent virtio port: %m");
> -        vdagent_virtio_port_destroy(vportp);
> +        GError *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
> +                                  "writing to vdagent virtio port: %m");
> +        virtio_port_destroy(vportp, err);
>          return;
>      }
>      if (n > 0)
> diff --git a/src/vdagentd/virtio-port.h b/src/vdagentd/virtio-port.h
> index dffb410..dfbe27b 100644
> --- a/src/vdagentd/virtio-port.h
> +++ b/src/vdagentd/virtio-port.h
> @@ -41,14 +41,12 @@ typedef int (*vdagent_virtio_port_read_callback)(
>      uint8_t *data);
>  
>  /* Callbacks with this type will be called when the port is disconnected.
> +   If the virtio port was disconnected due to an IO error, @err is set.
>     Note:
>     1) vdagent_virtio_port will destroy the port in question itself after
> -      this callback has completed!
> -   2) This callback is always called, even if the disconnect is initiated
> -      by the vdagent_virtio_port user through returning -1 from a read
> -      callback, or by explicitly calling vdagent_virtio_port_destroy */
> +      this callback has completed! */
>  typedef void (*vdagent_virtio_port_disconnect_callback)(
> -    struct vdagent_virtio_port *conn);
> +    struct vdagent_virtio_port *conn, GError *err);

So, either what I suggested above which I think is preferable or
use const here.

I think those are minor things, patch looks good.

>  /* Create a vdagent virtio port object for port portname */
> -- 
> 2.17.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel