[RFC,spice-vdagent,02/18] vport: add by_user param to vdagent_virtio_port_disconnect_callback

Submitted by Jakub Janku on Aug. 14, 2018, 6:53 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Jakub Janku Aug. 14, 2018, 6:53 p.m.
If the virtio port is destroyed explicitly
by calling vdagent_virtio_port_destroy(),
by_user is set to TRUE, otherwise to FALSE.

This will be used later with GMainLoop.
---
 src/vdagentd/virtio-port.c | 24 +++++++++++++++---------
 src/vdagentd/virtio-port.h | 10 +++++-----
 2 files changed, 20 insertions(+), 14 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
index 3dc6f44..642c848 100644
--- a/src/vdagentd/virtio-port.c
+++ b/src/vdagentd/virtio-port.c
@@ -122,7 +122,8 @@  error:
     return NULL;
 }
 
-void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
+static void virtio_port_destroy(struct vdagent_virtio_port **vportp,
+                                gboolean by_user)
 {
     struct vdagent_virtio_port_buf *wbuf, *next_wbuf;
     struct vdagent_virtio_port *vport = *vportp;
@@ -132,7 +133,7 @@  void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
         return;
 
     if (vport->disconnect_callback)
-        vport->disconnect_callback(vport);
+        vport->disconnect_callback(vport, by_user);
 
     wbuf = vport->write_buf;
     while (wbuf) {
@@ -151,6 +152,11 @@  void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
     *vportp = NULL;
 }
 
+void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
+{
+    virtio_port_destroy(vportp, TRUE);
+}
+
 int vdagent_virtio_port_fill_fds(struct vdagent_virtio_port *vport,
         fd_set *readfds, fd_set *writefds)
 {
@@ -321,7 +327,7 @@  static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
                 port->message_data = malloc(port->message_header.size);
                 if (!port->message_data) {
                     syslog(LOG_ERR, "out of memory, disconnecting virtio");
-                    vdagent_virtio_port_destroy(vportp);
+                    virtio_port_destroy(vportp, FALSE);
                     return;
                 }
             }
@@ -335,7 +341,7 @@  static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
 
         if (avail > read) {
             syslog(LOG_ERR, "chunk larger than message, lost sync?");
-            vdagent_virtio_port_destroy(vportp);
+            virtio_port_destroy(vportp, FALSE);
             return;
         }
 
@@ -353,7 +359,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, TRUE);
                     return;
                 }
             }
@@ -420,7 +426,7 @@  static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
         return;
     }
     if (n <= 0) {
-        vdagent_virtio_port_destroy(vportp);
+        virtio_port_destroy(vportp, FALSE);
         return;
     }
     vport->opening = 0;
@@ -433,13 +439,13 @@  static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
             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);
+                virtio_port_destroy(vportp, FALSE);
                 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);
+                virtio_port_destroy(vportp, FALSE);
                 return;
             }
         }
@@ -487,7 +493,7 @@  static void vdagent_virtio_port_do_write(struct vdagent_virtio_port **vportp)
         if (errno == EINTR)
             return;
         syslog(LOG_ERR, "writing to vdagent virtio port: %m");
-        vdagent_virtio_port_destroy(vportp);
+        virtio_port_destroy(vportp, FALSE);
         return;
     }
     if (n > 0)
diff --git a/src/vdagentd/virtio-port.h b/src/vdagentd/virtio-port.h
index f899e30..3c701d6 100644
--- a/src/vdagentd/virtio-port.h
+++ b/src/vdagentd/virtio-port.h
@@ -41,14 +41,14 @@  typedef int (*vdagent_virtio_port_read_callback)(
     uint8_t *data);
 
 /* Callbacks with this type will be called when the port is disconnected.
+   If the disconnect is initiated by calling vdagent_virtio_port_destroy()
+   or by returning -1 from the vdagent_virtio_port_read_callback,
+   @by_user is set to TRUE, otherwise FALSE.
    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, gboolean by_user);
 
 
 /* Create a vdagent virtio port object for port portname */

Comments

Hi,

Took me a while to get the rationale behind this so some more
info in the commit log might help.

On Tue, Aug 14, 2018 at 08:53:36PM +0200, Jakub Janků wrote:
> If the virtio port is destroyed explicitly by calling
> vdagent_virtio_port_destroy(), by_user is set to TRUE,
> otherwise to FALSE.

One issue I had was around the 'by_user' name and its meaning.
Basically, if we want to virtio_port_destroy() without any
runtime failure, by_user would be True.

I think a GError* err would fit better, as if err != NULL we
could show err->message and retry if possible. It is also true
that, under a runtime failure, most of the times before
virtio_port_destroy() is called there is a syslog(LOG_ERR, ...)
so I think GError or even a const gchar *err_msg; would be better
fit but I might be missing something.

> This will be used later with GMainLoop.

Another line about how this is related to GMainLoop might help
out while reading the patch ;)
> ---
>  src/vdagentd/virtio-port.c | 24 +++++++++++++++---------
>  src/vdagentd/virtio-port.h | 10 +++++-----
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
> index 3dc6f44..642c848 100644
> --- a/src/vdagentd/virtio-port.c
> +++ b/src/vdagentd/virtio-port.c
> @@ -122,7 +122,8 @@ error:
>      return NULL;
>  }
>  
> -void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> +static void virtio_port_destroy(struct vdagent_virtio_port **vportp,
> +                                gboolean by_user)
>  {
>      struct vdagent_virtio_port_buf *wbuf, *next_wbuf;
>      struct vdagent_virtio_port *vport = *vportp;
> @@ -132,7 +133,7 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
>          return;
>  
>      if (vport->disconnect_callback)
> -        vport->disconnect_callback(vport);
> +        vport->disconnect_callback(vport, by_user);
>  
>      wbuf = vport->write_buf;
>      while (wbuf) {
> @@ -151,6 +152,11 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
>      *vportp = NULL;
>  }
>  
> +void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> +{
> +    virtio_port_destroy(vportp, TRUE);
> +}
> +
>  int vdagent_virtio_port_fill_fds(struct vdagent_virtio_port *vport,
>          fd_set *readfds, fd_set *writefds)
>  {
> @@ -321,7 +327,7 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
>                  port->message_data = malloc(port->message_header.size);
>                  if (!port->message_data) {
>                      syslog(LOG_ERR, "out of memory, disconnecting virtio");
> -                    vdagent_virtio_port_destroy(vportp);
> +                    virtio_port_destroy(vportp, FALSE);
>                      return;
>                  }
>              }
> @@ -335,7 +341,7 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
>  
>          if (avail > read) {
>              syslog(LOG_ERR, "chunk larger than message, lost sync?");
> -            vdagent_virtio_port_destroy(vportp);
> +            virtio_port_destroy(vportp, FALSE);
>              return;
>          }
>  
> @@ -353,7 +359,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, TRUE);

I wonder if this can't be an actual failure too? (i.e by_user =
FALSE)

Reviewed-by: Victor Toso <victortoso@redhat.com>

Cheers,
Victor
>                      return;
>                  }
>              }
> @@ -420,7 +426,7 @@ static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
>          return;
>      }
>      if (n <= 0) {
> -        vdagent_virtio_port_destroy(vportp);
> +        virtio_port_destroy(vportp, FALSE);
>          return;
>      }
>      vport->opening = 0;
> @@ -433,13 +439,13 @@ static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
>              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);
> +                virtio_port_destroy(vportp, FALSE);
>                  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);
> +                virtio_port_destroy(vportp, FALSE);
>                  return;
>              }
>          }
> @@ -487,7 +493,7 @@ static void vdagent_virtio_port_do_write(struct vdagent_virtio_port **vportp)
>          if (errno == EINTR)
>              return;
>          syslog(LOG_ERR, "writing to vdagent virtio port: %m");
> -        vdagent_virtio_port_destroy(vportp);
> +        virtio_port_destroy(vportp, FALSE);
>          return;
>      }
>      if (n > 0)
> diff --git a/src/vdagentd/virtio-port.h b/src/vdagentd/virtio-port.h
> index f899e30..3c701d6 100644
> --- a/src/vdagentd/virtio-port.h
> +++ b/src/vdagentd/virtio-port.h
> @@ -41,14 +41,14 @@ typedef int (*vdagent_virtio_port_read_callback)(
>      uint8_t *data);
>  
>  /* Callbacks with this type will be called when the port is disconnected.
> +   If the disconnect is initiated by calling vdagent_virtio_port_destroy()
> +   or by returning -1 from the vdagent_virtio_port_read_callback,
> +   @by_user is set to TRUE, otherwise FALSE.
>     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, gboolean by_user);
>  
>  
>  /* 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
Hi,

On Tue, Aug 28, 2018 at 7:52 AM Victor Toso <victortoso@redhat.com> wrote:
>
> Hi,
>
> Took me a while to get the rationale behind this so some more
> info in the commit log might help.

OK, will do, sorry.
>
> On Tue, Aug 14, 2018 at 08:53:36PM +0200, Jakub Janků wrote:
> > If the virtio port is destroyed explicitly by calling
> > vdagent_virtio_port_destroy(), by_user is set to TRUE,
> > otherwise to FALSE.
>
> One issue I had was around the 'by_user' name and its meaning.
> Basically, if we want to virtio_port_destroy() without any
> runtime failure, by_user would be True.

That's true. Without any runtime failure in virtio-port.c.
>
> I think a GError* err would fit better, as if err != NULL we
> could show err->message and retry if possible. It is also true
> that, under a runtime failure, most of the times before
> virtio_port_destroy() is called there is a syslog(LOG_ERR, ...)
> so I think GError or even a const gchar *err_msg; would be better
> fit but I might be missing something.

I agree, this is a better solution.
>
> > This will be used later with GMainLoop.
>
> Another line about how this is related to GMainLoop might help
> out while reading the patch ;)
> > ---
> >  src/vdagentd/virtio-port.c | 24 +++++++++++++++---------
> >  src/vdagentd/virtio-port.h | 10 +++++-----
> >  2 files changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
> > index 3dc6f44..642c848 100644
> > --- a/src/vdagentd/virtio-port.c
> > +++ b/src/vdagentd/virtio-port.c
> > @@ -122,7 +122,8 @@ error:
> >      return NULL;
> >  }
> >
> > -void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> > +static void virtio_port_destroy(struct vdagent_virtio_port **vportp,
> > +                                gboolean by_user)
> >  {
> >      struct vdagent_virtio_port_buf *wbuf, *next_wbuf;
> >      struct vdagent_virtio_port *vport = *vportp;
> > @@ -132,7 +133,7 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> >          return;
> >
> >      if (vport->disconnect_callback)
> > -        vport->disconnect_callback(vport);
> > +        vport->disconnect_callback(vport, by_user);
> >
> >      wbuf = vport->write_buf;
> >      while (wbuf) {
> > @@ -151,6 +152,11 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> >      *vportp = NULL;
> >  }
> >
> > +void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> > +{
> > +    virtio_port_destroy(vportp, TRUE);
> > +}
> > +
> >  int vdagent_virtio_port_fill_fds(struct vdagent_virtio_port *vport,
> >          fd_set *readfds, fd_set *writefds)
> >  {
> > @@ -321,7 +327,7 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
> >                  port->message_data = malloc(port->message_header.size);
> >                  if (!port->message_data) {
> >                      syslog(LOG_ERR, "out of memory, disconnecting virtio");
> > -                    vdagent_virtio_port_destroy(vportp);
> > +                    virtio_port_destroy(vportp, FALSE);
> >                      return;
> >                  }
> >              }
> > @@ -335,7 +341,7 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
> >
> >          if (avail > read) {
> >              syslog(LOG_ERR, "chunk larger than message, lost sync?");
> > -            vdagent_virtio_port_destroy(vportp);
> > +            virtio_port_destroy(vportp, FALSE);
> >              return;
> >          }
> >
> > @@ -353,7 +359,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, TRUE);
>
> I wonder if this can't be an actual failure too? (i.e by_user =
> FALSE)

I don't think by_user should be set to FALSE in this case.
There's no error in the virtio-port. The *user* of virtio-port
voluntarily decided to destroy the connection.
Apart from that, the current implementation never returns -1 in the
virtio_port_read_complete(), so it doesn't matter that much, I
guess...
>
> Reviewed-by: Victor Toso <victortoso@redhat.com>
>
> Cheers,
> Victor
> >                      return;
> >                  }
> >              }
> > @@ -420,7 +426,7 @@ static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
> >          return;
> >      }
> >      if (n <= 0) {
> > -        vdagent_virtio_port_destroy(vportp);
> > +        virtio_port_destroy(vportp, FALSE);
> >          return;
> >      }
> >      vport->opening = 0;
> > @@ -433,13 +439,13 @@ static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
> >              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);
> > +                virtio_port_destroy(vportp, FALSE);
> >                  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);
> > +                virtio_port_destroy(vportp, FALSE);
> >                  return;
> >              }
> >          }
> > @@ -487,7 +493,7 @@ static void vdagent_virtio_port_do_write(struct vdagent_virtio_port **vportp)
> >          if (errno == EINTR)
> >              return;
> >          syslog(LOG_ERR, "writing to vdagent virtio port: %m");
> > -        vdagent_virtio_port_destroy(vportp);
> > +        virtio_port_destroy(vportp, FALSE);
> >          return;
> >      }
> >      if (n > 0)
> > diff --git a/src/vdagentd/virtio-port.h b/src/vdagentd/virtio-port.h
> > index f899e30..3c701d6 100644
> > --- a/src/vdagentd/virtio-port.h
> > +++ b/src/vdagentd/virtio-port.h
> > @@ -41,14 +41,14 @@ typedef int (*vdagent_virtio_port_read_callback)(
> >      uint8_t *data);
> >
> >  /* Callbacks with this type will be called when the port is disconnected.
> > +   If the disconnect is initiated by calling vdagent_virtio_port_destroy()
> > +   or by returning -1 from the vdagent_virtio_port_read_callback,
> > +   @by_user is set to TRUE, otherwise FALSE.
> >     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, gboolean by_user);
> >
> >
> >  /* 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
Hi,

On Mon, Sep 03, 2018 at 06:20:44PM +0200, Jakub Janku wrote:
> Hi,
> 
> On Tue, Aug 28, 2018 at 7:52 AM Victor Toso <victortoso@redhat.com> wrote:
> >
> > Hi,
> >
> > Took me a while to get the rationale behind this so some more
> > info in the commit log might help.
> 
> OK, will do, sorry.

No worries

> >
> > On Tue, Aug 14, 2018 at 08:53:36PM +0200, Jakub Janků wrote:
> > > If the virtio port is destroyed explicitly by calling
> > > vdagent_virtio_port_destroy(), by_user is set to TRUE,
> > > otherwise to FALSE.
> >
> > One issue I had was around the 'by_user' name and its meaning.
> > Basically, if we want to virtio_port_destroy() without any
> > runtime failure, by_user would be True.
> 
> That's true. Without any runtime failure in virtio-port.c.
> >
> > I think a GError* err would fit better, as if err != NULL we
> > could show err->message and retry if possible. It is also true
> > that, under a runtime failure, most of the times before
> > virtio_port_destroy() is called there is a syslog(LOG_ERR, ...)
> > so I think GError or even a const gchar *err_msg; would be better
> > fit but I might be missing something.
> 
> I agree, this is a better solution.
> >
> > > This will be used later with GMainLoop.
> >
> > Another line about how this is related to GMainLoop might help
> > out while reading the patch ;)
> > > ---
> > >  src/vdagentd/virtio-port.c | 24 +++++++++++++++---------
> > >  src/vdagentd/virtio-port.h | 10 +++++-----
> > >  2 files changed, 20 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
> > > index 3dc6f44..642c848 100644
> > > --- a/src/vdagentd/virtio-port.c
> > > +++ b/src/vdagentd/virtio-port.c
> > > @@ -122,7 +122,8 @@ error:
> > >      return NULL;
> > >  }
> > >
> > > -void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> > > +static void virtio_port_destroy(struct vdagent_virtio_port **vportp,
> > > +                                gboolean by_user)
> > >  {
> > >      struct vdagent_virtio_port_buf *wbuf, *next_wbuf;
> > >      struct vdagent_virtio_port *vport = *vportp;
> > > @@ -132,7 +133,7 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> > >          return;
> > >
> > >      if (vport->disconnect_callback)
> > > -        vport->disconnect_callback(vport);
> > > +        vport->disconnect_callback(vport, by_user);
> > >
> > >      wbuf = vport->write_buf;
> > >      while (wbuf) {
> > > @@ -151,6 +152,11 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> > >      *vportp = NULL;
> > >  }
> > >
> > > +void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
> > > +{
> > > +    virtio_port_destroy(vportp, TRUE);
> > > +}
> > > +
> > >  int vdagent_virtio_port_fill_fds(struct vdagent_virtio_port *vport,
> > >          fd_set *readfds, fd_set *writefds)
> > >  {
> > > @@ -321,7 +327,7 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
> > >                  port->message_data = malloc(port->message_header.size);
> > >                  if (!port->message_data) {
> > >                      syslog(LOG_ERR, "out of memory, disconnecting virtio");
> > > -                    vdagent_virtio_port_destroy(vportp);
> > > +                    virtio_port_destroy(vportp, FALSE);
> > >                      return;
> > >                  }
> > >              }
> > > @@ -335,7 +341,7 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
> > >
> > >          if (avail > read) {
> > >              syslog(LOG_ERR, "chunk larger than message, lost sync?");
> > > -            vdagent_virtio_port_destroy(vportp);
> > > +            virtio_port_destroy(vportp, FALSE);
> > >              return;
> > >          }
> > >
> > > @@ -353,7 +359,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, TRUE);
> >
> > I wonder if this can't be an actual failure too? (i.e by_user =
> > FALSE)
> 
> I don't think by_user should be set to FALSE in this case.
> There's no error in the virtio-port. The *user* of virtio-port
> voluntarily decided to destroy the connection.

Right

> Apart from that, the current implementation never returns -1 in the
> virtio_port_read_complete(), so it doesn't matter that much, I
> guess...

Ouch. Seems that it is like this for long time :(

> > Reviewed-by: Victor Toso <victortoso@redhat.com>
> >
> > Cheers,
> > Victor
> > >                      return;
> > >                  }
> > >              }
> > > @@ -420,7 +426,7 @@ static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
> > >          return;
> > >      }
> > >      if (n <= 0) {
> > > -        vdagent_virtio_port_destroy(vportp);
> > > +        virtio_port_destroy(vportp, FALSE);
> > >          return;
> > >      }
> > >      vport->opening = 0;
> > > @@ -433,13 +439,13 @@ static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
> > >              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);
> > > +                virtio_port_destroy(vportp, FALSE);
> > >                  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);
> > > +                virtio_port_destroy(vportp, FALSE);
> > >                  return;
> > >              }
> > >          }
> > > @@ -487,7 +493,7 @@ static void vdagent_virtio_port_do_write(struct vdagent_virtio_port **vportp)
> > >          if (errno == EINTR)
> > >              return;
> > >          syslog(LOG_ERR, "writing to vdagent virtio port: %m");
> > > -        vdagent_virtio_port_destroy(vportp);
> > > +        virtio_port_destroy(vportp, FALSE);
> > >          return;
> > >      }
> > >      if (n > 0)
> > > diff --git a/src/vdagentd/virtio-port.h b/src/vdagentd/virtio-port.h
> > > index f899e30..3c701d6 100644
> > > --- a/src/vdagentd/virtio-port.h
> > > +++ b/src/vdagentd/virtio-port.h
> > > @@ -41,14 +41,14 @@ typedef int (*vdagent_virtio_port_read_callback)(
> > >      uint8_t *data);
> > >
> > >  /* Callbacks with this type will be called when the port is disconnected.
> > > +   If the disconnect is initiated by calling vdagent_virtio_port_destroy()
> > > +   or by returning -1 from the vdagent_virtio_port_read_callback,
> > > +   @by_user is set to TRUE, otherwise FALSE.
> > >     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, gboolean by_user);
> > >
> > >
> > >  /* 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