[Spice-devel] [PATCH vdagent 09/11] udscs: allow fd control outside udscs

[Frediano Ziglio]

> On Wed, Sep 27, 2017 at 12:44 PM Frediano Ziglio < fziglio at redhat.com >
> wrote:

> > >
> 
> > > From: Victor Toso < me at victortoso.com >
> 
> > >
> 
> > > This patch export two existing functions `udscs_do_read()` and
> 
> > > `udscs_do_write()` and also creates a new one `udscs_client_get_fd()`.
> 
> > >
> 
> > > The intention of this functions is to allow vdagent to check if
> 
> > > connection's socket is ready to read or write. This will be done
> 
> > > together with GMainLoop integration in a followup patch.
> 
> > >
> 
> > > Signed-off-by: Victor Toso < victortoso at redhat.com >
> 

> > This patch starts exporting lot of information from udscs and
> 
> > the event handling is done half in vdagent and half in udscs
> 
> > code creating a circular dependency from some component to vdagent.
> 
> > Would not be better if udscs know GLib (we already are including
> 
> > glib.h in this patch) context/loop and deal directly with it?
> 
> > Just pass a GMainLoop building udscs and move the source in
> 
> > udscs. In this way in 11/11 you won't have to change all udscs_write
> 
> > calls and include vdagent.h.
> 

> OK, so what you are proposing is:
> - add optional GMainLoop argument to udscs_connect()
> - move g_io_add_watch(G_IO_IN) to udscs_connect()
> - move conn_channel and conn_out_source_id from VDAgent object to
> udscs_connection struct
> - move conn_channel_io_callback() to udscs
> - move g_io_add_watch(G_IO_OUT) from spice_vdagent_write_msg() to
> udscs_write()
> and remove spice_vdagent_write_msg()

> I think that could work, the only problem I see is that udscs_write() takes
> udscs_connection * as argument.
> We have to call g_io_add_watch(G_IO_OUT) in udscs_write(), passing
> udscs_connection ** so that
> when conn_channel_io_cb() is called, it can destroy the connection.
> So we would have to add udscs_connection ** in the udscs_connection struct,
> or
> develop a mechanism to determine whether the GMainLoop was quit by udscs.c or
> not.
> Is that correct?

Why not using the udscs_disconnect_callback callback, designed actually for that purpose? 

> > Potentially you can also avoit 6/11 entirely.
> 

> How? The individual VDAGENTD_ messages should be handled in vdagent.c
> Since we removed most of the global variables, we need to pass the VDAgent
> object
> to daemon_read_complete().

Why you had to add user_data instead of using udscs_set_user_data/udscs_get_user_data? 
You are actually storing the same data (the agent pointer). 

Looks like to me that udscs already has all support you need. 

I would attempt to remove udscs_client_fill_fds and udscs_client_handle_fds, they are 
there to support select call. After these patches they are used only by the daemon as the 
daemon is not using Glib loop. 

Looking at the code looks like the event handler is removed from udscs socket 
if the write is accomplished and enabled again on next write but you are not 
handling read in the meantime. 

> > > ---
> 
> > > src/udscs.c | 17 +++++++++--------
> 
> > > src/udscs.h | 17 +++++++++++++++++
> 
> > > 2 files changed, 26 insertions(+), 8 deletions(-)
> 
> > >
> 
> > > diff --git a/src/udscs.c b/src/udscs.c
> 
> > > index f67d0a0..2761cbb 100644
> 
> > > --- a/src/udscs.c
> 
> > > +++ b/src/udscs.c
> 
> > > @@ -31,6 +31,7 @@
> 
> > > #include <errno.h>
> 
> > > #include <sys/socket.h>
> 
> > > #include <sys/un.h>
> 
> > > +#include <glib.h>
> 
> > > #include "udscs.h"
> 
> > >
> 
> > > struct udscs_buf {
> 
> > > @@ -246,8 +247,7 @@ static void udscs_read_complete(struct
> > > udscs_connection
> 
> > > **connp)
> 
> > > conn->header_read = 0;
> 
> > > }
> 
> > >
> 
> > > -/* A helper for udscs_client_handle_fds() */
> 
> > > -static void udscs_do_read(struct udscs_connection **connp)
> 
> > > +void udscs_do_read(struct udscs_connection **connp)
> 
> > > {
> 
> > > ssize_t n;
> 
> > > size_t to_read;
> 
> > > @@ -298,19 +298,15 @@ static void udscs_do_read(struct udscs_connection
> 
> > > **connp)
> 
> > > }
> 
> > >
> 
> > > /* A helper for udscs_client_handle_fds() */
> 
> > > -static void udscs_do_write(struct udscs_connection **connp)
> 
> > > +void udscs_do_write(struct udscs_connection **connp)
> 
> > > {
> 
> > > ssize_t n;
> 
> > > size_t to_write;
> 
> > > struct udscs_connection *conn = *connp;
> 
> > >
> 
> > > struct udscs_buf* wbuf = conn->write_buf;
> 
> > > - if (!wbuf) {
> 
> > > - syslog(LOG_ERR,
> 
> > > - "%p do_write called on a connection without a write buf ?!",
> 
> > > - conn);
> 
> > > + if (!wbuf)
> 
> > > return;
> 
> > > - }
> 
> > >
> 
> > > to_write = wbuf->size - wbuf->pos;
> 
> > > n = write(conn->fd, wbuf->buf + wbuf->pos, to_write);
> 
> > > @@ -357,6 +353,11 @@ int udscs_client_fill_fds(struct udscs_connection
> > > *conn,
> 
> > > fd_set *readfds,
> 
> > > return conn->fd + 1;
> 
> > > }
> 
> > >
> 
> > > +int udscs_client_get_fd(struct udscs_connection *conn)
> 
> > > +{
> 
> > > + g_return_val_if_fail(conn != NULL, -1);
> 
> > > + return conn->fd;
> 
> > > +}
> 
> > >
> 
> > > #ifndef UDSCS_NO_SERVER
> 
> > >
> 
> > > diff --git a/src/udscs.h b/src/udscs.h
> 
> > > index 04377ba..08e71c8 100644
> 
> > > --- a/src/udscs.h
> 
> > > +++ b/src/udscs.h
> 
> > > @@ -109,6 +109,23 @@ void udscs_set_user_data(struct udscs_connection
> > > *conn,
> 
> > > void *data);
> 
> > > */
> 
> > > void *udscs_get_user_data(struct udscs_connection *conn);
> 
> > >
> 
> > > +/* Get fd from connection or return -1 if NULL */
> 
> > > +int udscs_client_get_fd(struct udscs_connection *conn);
> 
> > > +
> 
> > > +/* Performs a read in socket's connection. Note that fd should be ready
> > > for
> 
> > > read
> 
> > > + * otherwise it will destroy the connection. Use
> > > udscs_client_fill_fds(),
> 
> > > + * select() and udscs_client_handle_fds() to delegate those checks to
> > > udscs.
> 
> > > + */
> 
> > > +void udscs_do_read(struct udscs_connection **connp);
> 
> > > +
> 
> > > +/* Performs a write in socket's connection. Note that fd should be ready
> > > for
> 
> > > + * write otherwise it will destroy the connection. Use
> 
> > > udscs_client_fill_fds(),
> 
> > > + * select() and udscs_client_handle_fds() to delegate those checks to
> > > udscs.
> 
> > > + * If no buffer is ready to be written from udscs side, this function
> > > simply
> 
> > > + * returns.
> 
> > > + */
> 
> > > +void udscs_do_write(struct udscs_connection **connp);
> 
> > > +
> 
> > >
> 
> > > #ifndef UDSCS_NO_SERVER
> 
> > >
> 

> > Frediano
> 

> Cheers,
> Jakub
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170928/224f38c7/attachment.html>