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

[Frediano Ziglio]

> On Thu, Sep 28, 2017 at 3:13 PM Frediano Ziglio < fziglio at redhat.com > wrote:

> > > 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 somehow didn't notice that, thanks.

> > 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.
> 

> They are used internally in udscs.c, so they don't need to be exported.

Yes, sorry, the server ones, confused client with server. 

> > 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.
> 

> There are actually two sources:
> - for read, which is added in spice_vdagent_init_async_cb() and is not
> removed until
> the connection is destroyed
> - for write, added with each spice_vdagent_write_msg() call, removed after
> write is finished
> These two sources just use the same callback - conn_channel_io_cb(),
> so this should work just fine imho.
> Is that what you meant?

Yes, right, though you used the same watch, this way you are right, will continue to work. 

> > > > > ---
> > > 
> > 
> 
> > > > > 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/f59fcd75/attachment-0001.html>