[Spice-devel] [RFC spice-vdagent 02/18] vport: add by_user param to vdagent_virtio_port_disconnect_callback
Victor Toso
victortoso at redhat.com
Tue Sep 4 05:13:15 UTC 2018
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 at 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 at 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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180904/7936bafb/attachment.sig>
More information about the Spice-devel
mailing list