<html><body><div style="font-family: times new roman, new york, times, serif; font-size: 12pt; color: #000000"><div><br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Sep 28, 2017 at 3:13 PM Frediano Ziglio <<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><div><br></div><blockquote style="border-left:2px solid #1010ff;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div><br></div><div class="gmail_quote"><div dir="ltr">On Wed, Sep 27, 2017 at 12:44 PM Frediano Ziglio <<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">><br>
> From: Victor Toso <<a href="mailto:me@victortoso.com" target="_blank">me@victortoso.com</a>><br>
><br>
> This patch export two existing functions `udscs_do_read()` and<br>
> `udscs_do_write()` and also creates a new one `udscs_client_get_fd()`.<br>
><br>
> The intention of this functions is to allow vdagent to check if<br>
> connection's socket is ready to read or write. This will be done<br>
> together with GMainLoop integration in a followup patch.<br>
><br>
> Signed-off-by: Victor Toso <<a href="mailto:victortoso@redhat.com" target="_blank">victortoso@redhat.com</a>><br><div><br></div>
This patch starts exporting lot of information from udscs and<br>
the event handling is done half in vdagent and half in udscs<br>
code creating a circular dependency from some component to vdagent.<br>
Would not be better if udscs know GLib (we already are including<br>
glib.h in this patch) context/loop and deal directly with it?<br>
Just pass a GMainLoop building udscs and move the source in<br>
udscs. In this way in 11/11 you won't have to change all udscs_write<br>
calls and include vdagent.h.<br></blockquote><div><br></div><div>OK, so what you are proposing is:</div><div>- add optional GMainLoop argument to udscs_connect()</div><div>- move g_io_add_watch(G_IO_IN) to udscs_connect()</div><div>- move conn_channel and conn_out_source_id from VDAgent object to udscs_connection struct</div><div>- move conn_channel_io_callback() to udscs</div><div>- move g_io_add_watch(G_IO_OUT) from spice_vdagent_write_msg() to udscs_write()</div><div> and remove spice_vdagent_write_msg()</div><div><br></div><div>I think that could work, the only problem I see is that udscs_write() takes udscs_connection * as argument.</div><div>We have to call g_io_add_watch(G_IO_OUT) in udscs_write(), passing udscs_connection ** so that</div><div>when conn_channel_io_cb() is called, it can destroy the connection.</div><div>So we would have to add udscs_connection ** in the udscs_connection struct, or</div><div>develop a mechanism to determine whether the GMainLoop was quit by udscs.c or not.</div><div>Is that correct?</div></div></div></blockquote></div></div><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><div>Why not using the udscs_disconnect_callback callback, designed actually for that purpose?<br></div></div></div><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><div><br></div><blockquote style="border-left:2px solid #1010ff;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Potentially you can also avoit 6/11 entirely.<br></blockquote><div><br></div><div>How? The individual VDAGENTD_ messages should be handled in vdagent.c</div><div>Since we removed most of the global variables, we need to pass the VDAgent object</div><div>to daemon_read_complete().</div></div></div></blockquote></div></div><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><div>Why you had to add user_data instead of using udscs_set_user_data/udscs_get_user_data?<br></div><div>You are actually storing the same data (the agent pointer).<br></div><div><br></div><div>Looks like to me that udscs already has all support you need.<br></div></div></div></blockquote><div><br></div><div>I somehow didn't notice that, thanks. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><div><br></div><div>I would attempt to remove udscs_client_fill_fds and udscs_client_handle_fds, they are<br></div><div>there to support select call. After these patches they are used only by the daemon as the<br></div><div>daemon is not using Glib loop.<br></div></div></div></blockquote><div><br></div><div>They are used internally in udscs.c, so they don't need to be exported.</div></div></div></blockquote><div>Yes, sorry, the server ones, confused client with server.<br></div><div><br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><div><br></div><div>Looking at the code looks like the event handler is removed from udscs socket<br></div><div>if the write is accomplished and enabled again on next write but you are not<br></div><div>handling read in the meantime.<br></div></div></div><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><div><br></div></div></div></blockquote><div>There are actually two sources:</div><div>- for read, which is added in spice_vdagent_init_async_cb() and is not removed until</div><div> the connection is destroyed</div><div>- for write, added with each spice_vdagent_write_msg() call, removed after write is finished</div><div>These two sources just use the same callback - conn_channel_io_cb(),</div><div>so this should work just fine imho.</div><div>Is that what you meant?</div></div></div></blockquote><div>Yes, right, though you used the same watch, this way you are right, will continue to work.<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><blockquote style="border-left:2px solid #1010ff;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> ---<br>
> src/udscs.c | 17 +++++++++--------<br>
> src/udscs.h | 17 +++++++++++++++++<br>
> 2 files changed, 26 insertions(+), 8 deletions(-)<br>
><br>
> diff --git a/src/udscs.c b/src/udscs.c<br>
> index f67d0a0..2761cbb 100644<br>
> --- a/src/udscs.c<br>
> +++ b/src/udscs.c<br>
> @@ -31,6 +31,7 @@<br>
> #include <errno.h><br>
> #include <sys/socket.h><br>
> #include <sys/un.h><br>
> +#include <glib.h><br>
> #include "udscs.h"<br>
><br>
> struct udscs_buf {<br>
> @@ -246,8 +247,7 @@ static void udscs_read_complete(struct udscs_connection<br>
> **connp)<br>
> conn->header_read = 0;<br>
> }<br>
><br>
> -/* A helper for udscs_client_handle_fds() */<br>
> -static void udscs_do_read(struct udscs_connection **connp)<br>
> +void udscs_do_read(struct udscs_connection **connp)<br>
> {<br>
> ssize_t n;<br>
> size_t to_read;<br>
> @@ -298,19 +298,15 @@ static void udscs_do_read(struct udscs_connection<br>
> **connp)<br>
> }<br>
><br>
> /* A helper for udscs_client_handle_fds() */<br>
> -static void udscs_do_write(struct udscs_connection **connp)<br>
> +void udscs_do_write(struct udscs_connection **connp)<br>
> {<br>
> ssize_t n;<br>
> size_t to_write;<br>
> struct udscs_connection *conn = *connp;<br>
><br>
> struct udscs_buf* wbuf = conn->write_buf;<br>
> - if (!wbuf) {<br>
> - syslog(LOG_ERR,<br>
> - "%p do_write called on a connection without a write buf ?!",<br>
> - conn);<br>
> + if (!wbuf)<br>
> return;<br>
> - }<br>
><br>
> to_write = wbuf->size - wbuf->pos;<br>
> n = write(conn->fd, wbuf->buf + wbuf->pos, to_write);<br>
> @@ -357,6 +353,11 @@ int udscs_client_fill_fds(struct udscs_connection *conn,<br>
> fd_set *readfds,<br>
> return conn->fd + 1;<br>
> }<br>
><br>
> +int udscs_client_get_fd(struct udscs_connection *conn)<br>
> +{<br>
> + g_return_val_if_fail(conn != NULL, -1);<br>
> + return conn->fd;<br>
> +}<br>
><br>
> #ifndef UDSCS_NO_SERVER<br>
><br>
> diff --git a/src/udscs.h b/src/udscs.h<br>
> index 04377ba..08e71c8 100644<br>
> --- a/src/udscs.h<br>
> +++ b/src/udscs.h<br>
> @@ -109,6 +109,23 @@ void udscs_set_user_data(struct udscs_connection *conn,<br>
> void *data);<br>
> */<br>
> void *udscs_get_user_data(struct udscs_connection *conn);<br>
><br>
> +/* Get fd from connection or return -1 if NULL */<br>
> +int udscs_client_get_fd(struct udscs_connection *conn);<br>
> +<br>
> +/* Performs a read in socket's connection. Note that fd should be ready for<br>
> read<br>
> + * otherwise it will destroy the connection. Use udscs_client_fill_fds(),<br>
> + * select() and udscs_client_handle_fds() to delegate those checks to udscs.<br>
> + */<br>
> +void udscs_do_read(struct udscs_connection **connp);<br>
> +<br>
> +/* Performs a write in socket's connection. Note that fd should be ready for<br>
> + * write otherwise it will destroy the connection. Use<br>
> udscs_client_fill_fds(),<br>
> + * select() and udscs_client_handle_fds() to delegate those checks to udscs.<br>
> + * If no buffer is ready to be written from udscs side, this function simply<br>
> + * returns.<br>
> + */<br>
> +void udscs_do_write(struct udscs_connection **connp);<br>
> +<br>
><br>
> #ifndef UDSCS_NO_SERVER<br>
><br><div><br></div>
Frediano<br></blockquote><div><br></div><div>Cheers,</div><div> Jakub </div></div></div></blockquote><div><br></div></div></div></blockquote></div></div></blockquote><div><br></div></div></body></html>