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@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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel