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

[Jakub Janků]

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?

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

>
> > ---
> >  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/674e6427/attachment.html>