[Spice-devel] [RFC spice-vdagent 03/18] vdagentd: use GMainLoop
Victor Toso
victortoso at redhat.com
Tue Sep 4 05:44:22 UTC 2018
Hi,
On Mon, Sep 03, 2018 at 06:38:58PM +0200, Jakub Janku wrote:
> Hi Victor,
>
> On Tue, Aug 28, 2018 at 9:38 AM Victor Toso <victortoso at redhat.com> wrote:
> >
> > Hi,
> >
> > On Tue, Aug 14, 2018 at 08:53:37PM +0200, Jakub Janků wrote:
> > > This is purely a preparatory patch as it renders
> > > the vdagentd non-functional.
> >
> > I would rather not break it unless it really helps a lot. It was
> > possible to do it for vdagent code at 3fcf2e944ae3bf7, not sure
> > why we can't here.
>
> We surely can :) but the situation in vdagent.c was simpler, IMHO.
> In vdagent.c, we started using GMainLoop and g_io_add_watch() both in
> one commit which was relatively small.
>
> If we wanted to do the same here, we'd have to squash these 3 commits:
> * "vdagentd: use GMainLoop"
> * "udscs: use VDAgentConnection"
> * "vport: use VDAgentConnection"
> Apart from that, g_io_add_watch() would need to be used with the
> session_info_get_fd().
>
> The resulting patch would be huge, so I opted for this variant.
> I don't know what's the best way to go, I'll leave it up to you ;)
Right, I trust your judgment and I'll review with that in mind in
the next iteration but for sure, if something is not working we
should add it in commit log.
eg:
- This commit does not build and $reason_to_have_it
- This commit breaks $feature and $reason_to_have_it
- This commit introduces $bug and $reason_to_have_it
And future commit in the series that makes code stable should
have that info too.
Besides helping review and another set of eyes looking for an
alternative to that, etc. this helps for sure when try to find a
problematic commit with git bisect.
Thanks again for your work on this.
Victor
> Cheers,
> Jakub
>
> >
> > If it is really the case that it is better to have a commit with
> > non functional vdagentd, the commit log must state the rationale
> > behind it and which commit it is expected to have it working
> > again (the immediate follow-up is better, I guess)
> >
> > Reviewed-by: Victor Toso <victortoso at redhat.com>
> >
> > Victor
> >
> > > Remove main while loop with FD polling.
> > >
> > > udscs, virtio-port and session-info will be integrated
> > > into the GMainLoop in the following commits.
> > >
> > > Use g_unix_signal_add() to handle SIGINT, SIGHUP, SIGTERM.
> > > SIGQUIT handling is not supported by GLib.
> > > ---
> > > src/vdagentd/vdagentd.c | 129 ++++++++++++++--------------------------
> > > 1 file changed, 44 insertions(+), 85 deletions(-)
> > >
> > > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > > index d88bbc7..8abc63c 100644
> > > --- a/src/vdagentd/vdagentd.c
> > > +++ b/src/vdagentd/vdagentd.c
> > > @@ -31,10 +31,9 @@
> > > #include <errno.h>
> > > #include <signal.h>
> > > #include <syslog.h>
> > > -#include <sys/select.h>
> > > #include <sys/stat.h>
> > > #include <spice/vd_agent.h>
> > > -#include <glib.h>
> > > +#include <glib-unix.h>
> > >
> > > #ifdef WITH_SYSTEMD_SOCKET_ACTIVATION
> > > #include <systemd/sd-daemon.h>
> > > @@ -82,11 +81,12 @@ static const char *active_session = NULL;
> > > static unsigned int session_count = 0;
> > > static struct udscs_connection *active_session_conn = NULL;
> > > static int agent_owns_clipboard[256] = { 0, };
> > > -static int quit = 0;
> > > static int retval = 0;
> > > static int client_connected = 0;
> > > static int max_clipboard = -1;
> > >
> > > +static GMainLoop *loop;
> > > +
> > > /* utility functions */
> > > static void virtio_msg_uint32_to_le(uint8_t *_msg, uint32_t size, uint32_t offset)
> > > {
> > > @@ -175,7 +175,7 @@ void do_client_mouse(struct vdagentd_uinput **uinputp, VDAgentMouseState *mouse)
> > > if (!*uinputp) {
> > > syslog(LOG_CRIT, "Fatal uinput error");
> > > retval = 1;
> > > - quit = 1;
> > > + g_main_loop_quit(loop);
> > > }
> > > }
> > > }
> > > @@ -588,6 +588,33 @@ static int virtio_port_read_complete(
> > > return 0;
> > > }
> > >
> > > +static void virtio_port_disconnect_cb(struct vdagent_virtio_port *vport,
> > > + gboolean by_user)
> > > +{
> > > + virtio_port = NULL;
> > > +
> > > + if (!g_main_loop_is_running(loop))
> > > + return;
> > > +
> > > + if (by_user == FALSE) {
> > > + /* virtio_port was destroyed because of an internal error */
> > > + gboolean old_client_connected = client_connected;
> > > + syslog(LOG_CRIT, "AIIEEE lost spice client connection, reconnecting");
> > > + virtio_port = vdagent_virtio_port_create(portdev,
> > > + virtio_port_read_complete,
> > > + virtio_port_disconnect_cb);
> > > + if (virtio_port == NULL) {
> > > + syslog(LOG_CRIT, "Fatal error opening vdagent virtio channel");
> > > + retval = 1;
> > > + g_main_loop_quit(loop);
> > > + return;
> > > + }
> > > + do_client_disconnect();
> > > + client_connected = old_client_connected;
> > > + } else if (only_once)
> > > + g_main_loop_quit(loop);
> > > +}
> > > +
> > > static void virtio_write_clipboard(uint8_t selection, uint32_t msg_type,
> > > uint32_t data_type, uint8_t *data, uint32_t data_size)
> > > {
> > > @@ -727,7 +754,7 @@ static void check_xorg_resolution(void)
> > > if (!uinput) {
> > > syslog(LOG_CRIT, "Fatal uinput error");
> > > retval = 1;
> > > - quit = 1;
> > > + g_main_loop_quit(loop);
> > > return;
> > > }
> > >
> > > @@ -735,11 +762,11 @@ static void check_xorg_resolution(void)
> > > syslog(LOG_INFO, "opening vdagent virtio channel");
> > > virtio_port = vdagent_virtio_port_create(portdev,
> > > virtio_port_read_complete,
> > > - NULL);
> > > + virtio_port_disconnect_cb);
> > > if (!virtio_port) {
> > > syslog(LOG_CRIT, "Fatal error opening vdagent virtio channel");
> > > retval = 1;
> > > - quit = 1;
> > > + g_main_loop_quit(loop);
> > > return;
> > > }
> > > send_capabilities(virtio_port, 1);
> > > @@ -992,76 +1019,10 @@ static void daemonize(void)
> > > }
> > > }
> > >
> > > -static void main_loop(void)
> > > +static gboolean signal_handler(gpointer user_data)
> > > {
> > > - fd_set readfds, writefds;
> > > - int n, nfds;
> > > - int ck_fd = 0;
> > > - int once = 0;
> > > -
> > > - while (!quit) {
> > > - FD_ZERO(&readfds);
> > > - FD_ZERO(&writefds);
> > > -
> > > - nfds = udscs_server_fill_fds(server, &readfds, &writefds);
> > > - n = vdagent_virtio_port_fill_fds(virtio_port, &readfds, &writefds);
> > > - if (n >= nfds)
> > > - nfds = n + 1;
> > > -
> > > - if (session_info) {
> > > - ck_fd = session_info_get_fd(session_info);
> > > - FD_SET(ck_fd, &readfds);
> > > - if (ck_fd >= nfds)
> > > - nfds = ck_fd + 1;
> > > - }
> > > -
> > > - n = select(nfds, &readfds, &writefds, NULL, NULL);
> > > - if (n == -1) {
> > > - if (errno == EINTR)
> > > - continue;
> > > - syslog(LOG_CRIT, "Fatal error select: %m");
> > > - retval = 1;
> > > - break;
> > > - }
> > > -
> > > - udscs_server_handle_fds(server, &readfds, &writefds);
> > > -
> > > - if (virtio_port) {
> > > - once = 1;
> > > - vdagent_virtio_port_handle_fds(&virtio_port, &readfds, &writefds);
> > > - if (!virtio_port) {
> > > - int old_client_connected = client_connected;
> > > - syslog(LOG_CRIT,
> > > - "AIIEEE lost spice client connection, reconnecting");
> > > - virtio_port = vdagent_virtio_port_create(portdev,
> > > - virtio_port_read_complete,
> > > - NULL);
> > > - if (!virtio_port) {
> > > - syslog(LOG_CRIT,
> > > - "Fatal error opening vdagent virtio channel");
> > > - retval = 1;
> > > - break;
> > > - }
> > > - do_client_disconnect();
> > > - client_connected = old_client_connected;
> > > - }
> > > - }
> > > - else if (only_once && once)
> > > - {
> > > - syslog(LOG_INFO, "Exiting after one client session.");
> > > - break;
> > > - }
> > > -
> > > - if (session_info && FD_ISSET(ck_fd, &readfds)) {
> > > - active_session = session_info_get_active_session(session_info);
> > > - update_active_session_connection(NULL);
> > > - }
> > > - }
> > > -}
> > > -
> > > -static void quit_handler(int sig)
> > > -{
> > > - quit = 1;
> > > + g_main_loop_quit(loop);
> > > + return G_SOURCE_REMOVE;
> > > }
> > >
> > > static gboolean parse_debug_level_cb(const gchar *option_name,
> > > @@ -1115,7 +1076,6 @@ int main(int argc, char *argv[])
> > > {
> > > GOptionContext *context;
> > > GError *err = NULL;
> > > - struct sigaction act;
> > > gboolean own_socket = TRUE;
> > >
> > > context = g_option_context_new(NULL);
> > > @@ -1138,13 +1098,9 @@ int main(int argc, char *argv[])
> > > if (uinput_device == NULL)
> > > uinput_device = g_strdup(DEFAULT_UINPUT_DEVICE);
> > >
> > > - memset(&act, 0, sizeof(act));
> > > - act.sa_flags = SA_RESTART;
> > > - act.sa_handler = quit_handler;
> > > - sigaction(SIGINT, &act, NULL);
> > > - sigaction(SIGHUP, &act, NULL);
> > > - sigaction(SIGTERM, &act, NULL);
> > > - sigaction(SIGQUIT, &act, NULL);
> > > + g_unix_signal_add(SIGINT, signal_handler, NULL);
> > > + g_unix_signal_add(SIGHUP, signal_handler, NULL);
> > > + g_unix_signal_add(SIGTERM, signal_handler, NULL);
> > >
> > > openlog("spice-vdagentd", do_daemonize ? 0 : LOG_PERROR, LOG_USER);
> > >
> > > @@ -1214,7 +1170,9 @@ int main(int argc, char *argv[])
> > > syslog(LOG_WARNING, "no session info, max 1 session agent allowed");
> > >
> > > active_xfers = g_hash_table_new(g_direct_hash, g_direct_equal);
> > > - main_loop();
> > > +
> > > + loop = g_main_loop_new(NULL, FALSE);
> > > + g_main_loop_run(loop);
> > >
> > > release_clipboards();
> > >
> > > @@ -1223,6 +1181,7 @@ int main(int argc, char *argv[])
> > > vdagent_virtio_port_destroy(&virtio_port);
> > > session_info_destroy(session_info);
> > > udscs_destroy_server(server);
> > > + g_main_loop_unref(loop);
> > >
> > > /* leave the socket around if it was provided by systemd */
> > > if (own_socket) {
> > > --
> > > 2.17.1
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180904/f2f72dc2/attachment-0001.sig>
More information about the Spice-devel
mailing list