Re: [PATCH vdagent 09/11] udscs: allow fd control outside udscs

[Frediano Ziglio <fziglio@xxxxxxxxxx>]

On Thu, Sep 28, 2017 at 3:13 PM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:

On Wed, Sep 27, 2017 at 12:44 PM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:

>
> From: Victor Toso <me@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>

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 

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel