Re: [PATCH vdagent 08/11] vdagent: Introduce SpiceVDAgent object

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Wed, Sep 27, 2017 at 1:02 PM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
> Related to ongoing work to use GMainLoop and GTK integration
>
> Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> ---
>  Makefile.am           |   3 +
configure.ac          |   1 +
>  src/vdagent/vdagent.c | 149
>  +++++++++++++++++++++++++++++++-------------------
>  src/vdagent/vdagent.h |  54 ++++++++++++++++++
>  4 files changed, 150 insertions(+), 57 deletions(-)
>  create mode 100644 src/vdagent/vdagent.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 45f7177..efd058a 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -14,6 +14,7 @@ common_sources =                            \
>  src_spice_vdagent_CFLAGS =                   \
>       $(X_CFLAGS)                             \
>       $(SPICE_CFLAGS)                         \
> +     $(GOBJECT2_CFLAGS)                      \
>       $(GLIB2_CFLAGS)                         \
>       $(ALSA_CFLAGS)                          \
>       -I$(srcdir)/src                         \
> @@ -23,6 +24,7 @@ src_spice_vdagent_CFLAGS =                  \
>  src_spice_vdagent_LDADD =                    \
>       $(X_LIBS)                               \
>       $(SPICE_LIBS)                           \
> +     $(GOBJECT2_LIBS)                        \
>       $(GLIB2_LIBS)                           \
>       $(ALSA_LIBS)                            \
>       $(NULL)
> @@ -38,6 +40,7 @@ src_spice_vdagent_SOURCES =                 \
>       src/vdagent/x11.c                       \
>       src/vdagent/x11.h                       \
>       src/vdagent/vdagent.c                   \
> +     src/vdagent/vdagent.h                   \
>       $(NULL)
>
>  src_spice_vdagentd_CFLAGS =                  \
> diff --git a/configure.ac b/configure.ac
> index d92b527..12b7d23 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -86,6 +86,7 @@ AC_ARG_ENABLE([static-uinput],
>                [enable_static_uinput="no"])
>
>  PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= 2.34])
> +PKG_CHECK_MODULES(GOBJECT2, [gobject-2.0])
>  PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11])
>  PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.13])
>  PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22])
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index 0b6df3f..cdbda8b 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -37,16 +37,13 @@
>  #include <glib.h>
>  #include <poll.h>
>
> -#include "udscs.h"
>  #include "vdagentd-proto.h"
>  #include "vdagentd-proto-strings.h"
>  #include "audio.h"
> -#include "x11.h"
> -#include "file-xfers.h"
> +#include "vdagent.h"
> +
> +G_DEFINE_TYPE (SpiceVDAgent, spice_vdagent, G_TYPE_OBJECT);
>

Lot of functions use already the vdagent_ prefix.
Would not easier to call the class just VDAgent ?

Victor used "SessionAgent" in the earlier version, what about that?
I was suggesting something like

G_DEFINE_TYPE (VDAgent, vdagent, G_TYPE_OBJECT);

the "vdagent" (second argument) is the prefix used and there are already lot of functions
in this file with this prefix.
> -static struct vdagent_x11 *x11 = NULL;
> -static struct vdagent_file_xfers *vdagent_file_xfers = NULL;
> -static struct udscs_connection *client = NULL;
>  static int quit = 0;
>  static int version_mismatch = 0;
>
> @@ -90,12 +87,12 @@ static GOptionEntry entries[] = {
>   * Return path where transferred files should be stored.
>   * Returned path should not be freed or modified.
>   **/
> -static const gchar *xfer_get_download_directory(void)
> +static const gchar *xfer_get_download_directory(SpiceVDAgent *agent)
>  {
>      if (fx_dir != NULL)
>          return fx_dir;
>
> -    if (vdagent_x11_has_icons_on_desktop(x11))
> +    if (vdagent_x11_has_icons_on_desktop(agent->x11))
>          return g_get_user_special_dir(G_USER_DIRECTORY_DESKTOP);
>
>      return g_get_user_special_dir(G_USER_DIRECTORY_DOWNLOAD);
> @@ -105,61 +102,63 @@ static const gchar *xfer_get_download_directory(void)
>   * vdagent_init_file_xfer
>   *
>   * Initialize handler for file xfer,
> - * return TRUE on success (vdagent_file_xfers is not NULL).
> + * return TRUE on success (agent->xfers is not NULL).
>   **/
> -static gboolean vdagent_init_file_xfer(void)
> +static gboolean vdagent_init_file_xfer(SpiceVDAgent *agent)
>  {
>      const gchar *xfer_dir;
>
> -    if (vdagent_file_xfers != NULL) {
> +    if (agent->xfers != NULL) {
>          syslog(LOG_DEBUG, "File-xfer already initialized");
>          return TRUE;
>      }
>
> -    xfer_dir = xfer_get_download_directory();
> +    xfer_dir = xfer_get_download_directory(agent);
>      if (xfer_dir == NULL) {
>          syslog(LOG_WARNING,
>                 "warning could not get file xfer save dir, "
>                 "file transfers will be disabled");
> -        vdagent_file_xfers = NULL;
>          return FALSE;
>      }
>
> -    vdagent_file_xfers = vdagent_file_xfers_create(client, xfer_dir,
> -                                                   fx_open_dir, debug);
> -    return (vdagent_file_xfers != NULL);
> +    agent->xfers = vdagent_file_xfers_create(agent->conn, xfer_dir,
> +                                             fx_open_dir, debug);
> +    return (agent->xfers != NULL);
>  }
>
> -static gboolean vdagent_finalize_file_xfer(void)
> +static gboolean vdagent_finalize_file_xfer(SpiceVDAgent *agent)
>  {
> -    if (vdagent_file_xfers == NULL)
> +    if (agent->xfers == NULL)
>          return FALSE;
>
> -    g_clear_pointer(&vdagent_file_xfers, vdagent_file_xfers_destroy);
> +    g_clear_pointer(&agent->xfers, vdagent_file_xfers_destroy);
>      return TRUE;
>  }
>
>  static void daemon_read_complete(struct udscs_connection **connp,
>      struct udscs_message_header *header, uint8_t *data,
> -    G_GNUC_UNUSED void *user_data)
> +    void *user_data)
>  {
> +    g_return_if_fail(SPICE_IS_VDAGENT(user_data));
> +    SpiceVDAgent *agent = user_data;
> +
>      switch (header->type) {
>      case VDAGENTD_MONITORS_CONFIG:
> -        vdagent_x11_set_monitor_config(x11, (VDAgentMonitorsConfig *)data,
> 0);
> +        vdagent_x11_set_monitor_config(agent->x11, (VDAgentMonitorsConfig
> *)data, 0);
>          break;
>      case VDAGENTD_CLIPBOARD_REQUEST:
> -        vdagent_x11_clipboard_request(x11, header->arg1, header->arg2);
> +        vdagent_x11_clipboard_request(agent->x11, header->arg1,
> header->arg2);
>          break;
>      case VDAGENTD_CLIPBOARD_GRAB:
> -        vdagent_x11_clipboard_grab(x11, header->arg1, (uint32_t *)data,
> +        vdagent_x11_clipboard_grab(agent->x11, header->arg1, (uint32_t
> *)data,
>                                     header->size / sizeof(uint32_t));
>          break;
>      case VDAGENTD_CLIPBOARD_DATA:
> -        vdagent_x11_clipboard_data(x11, header->arg1, header->arg2,
> +        vdagent_x11_clipboard_data(agent->x11, header->arg1, header->arg2,
>                                     data, header->size);
>          break;
>      case VDAGENTD_CLIPBOARD_RELEASE:
> -        vdagent_x11_clipboard_release(x11, header->arg1);
> +        vdagent_x11_clipboard_release(agent->x11, header->arg1);
>          break;
>      case VDAGENTD_VERSION:
>          if (strcmp((char *)data, VERSION) != 0) {
> @@ -170,8 +169,8 @@ static void daemon_read_complete(struct udscs_connection
> **connp,
>          }
>          break;
>      case VDAGENTD_FILE_XFER_START:
> -        if (vdagent_file_xfers != NULL) {
> -            vdagent_file_xfers_start(vdagent_file_xfers,
> +        if (agent->xfers != NULL) {
> +            vdagent_file_xfers_start(agent->xfers,
>                                       (VDAgentFileXferStartMessage *)data);
>          } else {
>              vdagent_file_xfers_error_disabled(*connp,
> @@ -179,8 +178,8 @@ static void daemon_read_complete(struct udscs_connection
> **connp,
>          }
>          break;
>      case VDAGENTD_FILE_XFER_STATUS:
> -        if (vdagent_file_xfers != NULL) {
> -            vdagent_file_xfers_status(vdagent_file_xfers,
> +        if (agent->xfers != NULL) {
> +            vdagent_file_xfers_status(agent->xfers,
>                                        (VDAgentFileXferStatusMessage *)data);
>          } else {
>              vdagent_file_xfers_error_disabled(*connp,
> @@ -191,7 +190,7 @@ static void daemon_read_complete(struct udscs_connection
> **connp,
>          if (debug)
>              syslog(LOG_DEBUG, "Disabling file-xfers");
>
> -        vdagent_finalize_file_xfer();
> +        vdagent_finalize_file_xfer(agent);
>          break;
>      case VDAGENTD_AUDIO_VOLUME_SYNC: {
>          VDAgentAudioVolumeSync *avs = (VDAgentAudioVolumeSync *)data;
> @@ -203,8 +202,8 @@ static void daemon_read_complete(struct udscs_connection
> **connp,
>          break;
>      }
>      case VDAGENTD_FILE_XFER_DATA:
> -        if (vdagent_file_xfers != NULL) {
> -            vdagent_file_xfers_data(vdagent_file_xfers,
> +        if (agent->xfers != NULL) {
> +            vdagent_file_xfers_data(agent->xfers,
>                                      (VDAgentFileXferDataMessage *)data);
>          } else {
>              vdagent_file_xfers_error_disabled(*connp,
> @@ -212,9 +211,9 @@ static void daemon_read_complete(struct udscs_connection
> **connp,
>          }
>          break;
>      case VDAGENTD_CLIENT_DISCONNECTED:
> -        vdagent_x11_client_disconnected(x11);
> -        if (vdagent_finalize_file_xfer()) {
> -            vdagent_init_file_xfer();
> +        vdagent_x11_client_disconnected(agent->x11);
> +        if (vdagent_finalize_file_xfer(agent)) {
> +            vdagent_init_file_xfer(agent);
>          }
>          break;
>      default:
> @@ -223,12 +222,12 @@ static void daemon_read_complete(struct
> udscs_connection **connp,
>      }
>  }
>
> -static struct udscs_connection *client_setup_sync(void)
> +static struct udscs_connection *client_setup_sync(SpiceVDAgent *agent)
>  {
>      struct udscs_connection *conn = NULL;
>
>      while (!quit) {
> -        conn = udscs_connect(vdagentd_socket, daemon_read_complete, NULL,
> +        conn = udscs_connect(vdagentd_socket, daemon_read_complete, agent,
>                               NULL, vdagentd_messages, VDAGENTD_NO_MESSAGES,
>                               debug);
>          if (conn || !do_daemonize || quit) {
> @@ -299,6 +298,49 @@ static int file_test(const char *path)
>      return stat(path, &buffer);
>  }
>
> +static void spice_vdagent_init(SpiceVDAgent *self)
> +{
> +}
> +
> +static void spice_vdagent_finalize(GObject *gobject)
> +{
> +    SpiceVDAgent *self = SPICE_VDAGENT(gobject);
> +    vdagent_finalize_file_xfer(self);
> +    vdagent_x11_destroy(self->x11, self->conn == NULL);
> +
> +    if (self->conn != NULL) {
> +        udscs_destroy_connection(&self->conn);
> +        self->conn = NULL;
> +    }
> +

g_clear_pointer here?

Sure. 

> +    if (G_OBJECT_CLASS(spice_vdagent_parent_class)->finalize)

this if is not necessary, see https://developer.gnome.org/gobject/stable/howto-gobject-destruction.html

OK.
> +        G_OBJECT_CLASS(spice_vdagent_parent_class)->finalize(gobject);
> +}
> +
> +static void spice_vdagent_class_init(SpiceVDAgentClass  *klass)
> +{
> +    GObjectClass *gobject_class = G_OBJECT_CLASS(klass);
> +    gobject_class->finalize = spice_vdagent_finalize;
> +}
> +
> +static gboolean spice_vdagent_init_sync(SpiceVDAgent *agent)
> +{
> +    g_return_val_if_fail(SPICE_IS_VDAGENT(agent), FALSE);
> +

I don't think this check is necessary.

> +    agent->conn = client_setup_sync(agent);
> +    if (agent->conn == NULL)
> +        return FALSE;
> +
> +    agent->x11 = vdagent_x11_create(agent->conn, debug, x11_sync);
> +    if (agent->x11 == NULL)
> +        return FALSE;
> +
> +    if (!vdagent_init_file_xfer(agent))
> +        syslog(LOG_WARNING, "File transfer is disabled");
> +
> +    return TRUE;
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      fd_set readfds, writefds;
> @@ -307,6 +349,7 @@ int main(int argc, char *argv[])
>      struct sigaction act;
>      GOptionContext *context;
>      GError *error = NULL;
> +    SpiceVDAgent *agent;
>
>      context = g_option_context_new(NULL);
>      g_option_context_add_main_entries(context, entries, NULL);
> @@ -357,20 +400,13 @@ reconnect:
>          execvp(argv[0], argv);
>      }
>
> -    client = client_setup_sync();
> -    if (client == NULL) {
> -        return 1;
> +    agent = g_object_new(SPICE_TYPE_VDAGENT, NULL);
> +    agent->parent_socket = parent_socket;
> +    if (!spice_vdagent_init_sync(agent)) {
> +        quit = 1;
> +        goto end_main;
>      }
>
> -    x11 = vdagent_x11_create(client, debug, x11_sync);
> -    if (!x11) {
> -        udscs_destroy_connection(&client);
> -        return 1;
> -    }
> -
> -    if (!vdagent_init_file_xfer())
> -        syslog(LOG_WARNING, "File transfer is disabled");
> -
>      if (parent_socket) {
>          if (write(parent_socket, "OK", 2) != 2)
>              syslog(LOG_WARNING, "Parent already gone.");
> @@ -378,12 +414,12 @@ reconnect:
>          parent_socket = 0;
>      }
>
> -    while (client && !quit) {
> +    while (agent->conn && !quit) {
>          FD_ZERO(&readfds);
>          FD_ZERO(&writefds);
>
> -        nfds = udscs_client_fill_fds(client, &readfds, &writefds);
> -        x11_fd = vdagent_x11_get_fd(x11);
> +        nfds = udscs_client_fill_fds(agent->conn, &readfds, &writefds);
> +        x11_fd = vdagent_x11_get_fd(agent->x11);
>          FD_SET(x11_fd, &readfds);
>          if (x11_fd >= nfds)
>              nfds = x11_fd + 1;
> @@ -397,13 +433,12 @@ reconnect:
>          }
>
>          if (FD_ISSET(x11_fd, &readfds))
> -            vdagent_x11_do_read(x11);
> -        udscs_client_handle_fds(&client, &readfds, &writefds);
> +            vdagent_x11_do_read(agent->x11);
> +        udscs_client_handle_fds(&agent->conn, &readfds, &writefds);
>      }
>
> -    vdagent_finalize_file_xfer();
> -    vdagent_x11_destroy(x11, client == NULL);
> -    udscs_destroy_connection(&client);
> +end_main:
> +    g_clear_object(&agent);
>      if (!quit && do_daemonize)
>          goto reconnect;
>
> diff --git a/src/vdagent/vdagent.h b/src/vdagent/vdagent.h
> new file mode 100644
> index 0000000..f98c9b2
> --- /dev/null
> +++ b/src/vdagent/vdagent.h
> @@ -0,0 +1,54 @@
> +/*  vdagent.h
> +
> +    Copyright 2017 Red Hat, Inc.
> +
> +    This program is free software: you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation, either version 3 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#ifndef __VDAGENT_H_
> +#define __VDAGENT_H_
> +
> +#include <glib-object.h>
> +#include "udscs.h"
> +#include "x11.h"
> +#include "file-xfers.h"
> +
> +G_BEGIN_DECLS
> +
> +#define SPICE_TYPE_VDAGENT            (spice_vdagent_get_type ())
> +#define SPICE_VDAGENT(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj),
> SPICE_TYPE_VDAGENT, SpiceVDAgent))
> +#define SPICE_VDAGENT_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass),
> SPICE_TYPE_VDAGENT, SpiceVDAgentClass))
> +#define SPICE_IS_VDAGENT(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj),
> SPICE_TYPE_VDAGENT))
> +#define SPICE_IS_VDAGENT_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass),
> SPICE_TYPE_VDAGENT))
> +#define SPICE_VDAGENT_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj),
> SPICE_TYPE_VDAGENT, SpiceVDAgentClass))
> +
> +typedef struct _SpiceVDAgent {

Ids that starts with underscore and capital are not C99 complaint.

I'm not sure I understand that, why do we use these in spice-gtk then?
Historical reasons, C89 had not this. But after 18 years maybe we should consider to use C99 style.

> +    GObject parent;
> +
> +    struct vdagent_x11             *x11;
> +    struct vdagent_file_xfers      *xfers;
> +    struct udscs_connection        *conn;
> +
> +    int                             parent_socket;
> +} SpiceVDAgent;
> +
> +typedef struct _SpiceVDAgentClass {

ditto

> +    GObjectClass parent;
> +} SpiceVDAgentClass;
> +
> +GType spice_vdagent_get_type(void);
> +
> +G_END_DECLS
> +
> +#endif
> \ No newline at end of file

Frediano
Frediano

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

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]