Hi, On Tue, Aug 28, 2018 at 7:52 AM Victor Toso <victortoso@xxxxxxxxxx> 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@xxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel