[Spice-devel] [PATCH vdagent 09/11] udscs: allow fd control outside udscs
Jakub Janků
janku.jakub.jj at gmail.com
Thu Sep 28 14:01:17 UTC 2017
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.
>
> 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?
>
>> > ---
>> > 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/0118faa1/attachment.html>
More information about the Spice-devel
mailing list