Hi,
On Sun, Feb 18, 2018 at 11:44:38PM +0100, Jakub Janku wrote:
> >> +typedef struct {
> >> + GMainLoop *loop;
> >> + GtkSelectionData *sel_data;
> >> +} DataRetrieval;
> >> +
> >> +struct VDAgentClipboards {
> >> + struct udscs_connection *conn;
> >> +
> >> + GtkClipboard *clipboard[SELECTION_COUNT];
> >> + guint owner[SELECTION_COUNT];
> >> +
> >> + GList *data_retrievals[SELECTION_COUNT];
> >> + GList *data_requests[SELECTION_COUNT];
> >
> > I think we could use something more clear instead of
> > 'data_retrievals' and 'data_requests' but I'm terrible with
> > names myself so I suggest 'pending_set_request' and
> > 'pending_get_requests' or just 'set_requests' and
> > 'get_requests'.
>
> I agree, the names are far from ideal and rather confusing, but
> 'set_requests' and 'get_requests' aren't making it more clear
> either imho - "get request" could stand for a request from the
> client as well as a request from an app on the vdagent side.
> What about 'request_from_client' and 'request_from_app'?
Works for me :)
> > A comment would help too, like /* Client -> Guest */ and vice
> > versa.
>
> Definitely, I should have added some comments.
> >
> >> + gpointer *last_targets_req[SELECTION_COUNT];
> >> +
> >> + GdkAtom targets[SELECTION_COUNT][TYPE_COUNT];
> >
> > Maybe wrap them in inner struct { ... } selection[SELECTION_TYPE] ?
> >
> > e.g.
> > - c->data_requests[sel] = ...
> > + c->selection[id].data_request = ...
> >
> Makes sense probably. It might also be handy to typedef something like
> DataSelection and use
> DataSelection *sel = &c->selection[sel_id];
> sel->data_requests = ...
> where the struct members are accessed frequently.
It was more like a possible suggestion then a request but I like
it as we would access the `selection[]` array only once to get
the right struct instead of accessing the array for each field.
> >> +};
> >> +
> >> +static const struct {
> >> + guint type;
> >> + const gchar *atom_name;
> >> +} atom2agent[] = {
> >> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, "UTF8_STRING"},
> >> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, "text/plain;charset=utf-8"},
> >> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, "STRING"},
> >> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, "TEXT"},
> >> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, "text/plain"},
> >> + {VD_AGENT_CLIPBOARD_IMAGE_PNG, "image/png"},
> >> + {VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/bmp"},
> >> + {VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-bmp"},
> >> + {VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-MS-bmp"},
> >> + {VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-win-bitmap"},
> >> + {VD_AGENT_CLIPBOARD_IMAGE_TIFF,"image/tiff"},
> >> + {VD_AGENT_CLIPBOARD_IMAGE_JPG, "image/jpeg"},
> >> +};
> >> +
> >> +static guint get_type_from_atom(GdkAtom atom)
> >> +{
> >> + int i;
> >> + gchar *name = gdk_atom_name(atom);
> >> + for (i = 0; i < G_N_ELEMENTS(atom2agent); i++)
> >> + if (!g_ascii_strcasecmp(name, atom2agent[i].atom_name))
> >> + break;
> >> + g_free(name);
> >> + return i < G_N_ELEMENTS(atom2agent) ? atom2agent[i].type
> >> + : VD_AGENT_CLIPBOARD_NONE;
> >> +}
> >> +
> >> +/* gtk_clipboard_request_(, callback, user_data) cannot be cancelled. Instead,
> >> + gpointer *ref = get_weak_ref(VDAgentClipboards *) is passed to the callback.
> >> + The callback returns if free_weak_ref(ref) == NULL, which can be done
> >> + by setting *ref = NULL. This substitutes cancellation of the request.
> >> + */
> >> +static gpointer *get_weak_ref(gpointer data)
> >> +{
> >> + gpointer *ref = g_new(gpointer, 1);
> >> + *ref = data;
> >> + return ref;
> >> +}
> >
> > So, this is similar to GWeakRef but we are not using a GObject at
> > all.
> >
> > I might be missing something but why not an actual structure for
> > the pending request? I think it would be easier to understand and
> > track what is going.
> >
> > At least we should have some functions with the clear meaning
> > around this, like weak_ref_cancel(gpointer ref) { /* check and
> > set to NULL */ } ... but I do think something around GCancellable
> > would be nicer or even bool is_cancelled; in a custom struct.
> >
> The get_weak_ref() is only used with callbacks since we need to pass a
> reference to VDAgentClipboards. So I'm not sure GCancellable would
> help us here.
> The struct would need to contain pointer to VDAgentClipboards as well
> as the bool is_cancelled, so using something like GWeakRef seemed to
> be the easiest solution, although creating a struct might be nicer...
Right, maybe some helper functions would be enough. For instance:
| /* if there's a pending request for clipboard targets, cancel it */
| if (c->last_targets_req[sel])
| *(c->last_targets_req[sel]) = NULL;
Should use some weak_ref_cancel()
> >> +static void clipboard_get_cb(GtkClipboard *clipboard,
> >> + GtkSelectionData *sel_data,
> >> + guint info,
> >> + gpointer user_data)
> >> +{
> >> + DataRetrieval retrv;
> >> + VDAgentClipboards *c = user_data;
> >> + guint sel, type;
> >> +
> >> + sel = sel_from_clip(clipboard);
> >> + g_return_if_fail(c->owner[sel] == OWNER_CLIENT);
> >> +
> >> + type = get_type_from_atom(gtk_selection_data_get_target(sel_data));
> >> + g_return_if_fail(type != VD_AGENT_CLIPBOARD_NONE);
> >> +
> >> + retrv.sel_data = sel_data;
> >> + retrv.loop = g_main_loop_new(NULL, FALSE);
> >> + c->data_retrievals[sel] = g_list_prepend(c->data_retrievals[sel], &retrv);
> >> +
> >> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_REQUEST, sel, type, NULL, 0);
> >> +
> >> +G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> >> + gdk_threads_leave();
> >> + g_main_loop_run(retrv.loop);
> >
> > Wouldn't this block the agent while we don't receive input from
> > the client? I'm mostly worried in case of errors (client
> > disconnect, data not sent, etc).
> >
> >
> I'm not sure what you mean by that. "input from the client" =
> clipboard data?
Yes
> The g_main_loop_run() call runs a loop inside the
> clipboard_get_cb() = it runs a loop inside one iteration of the
> main loop in vdagent.c This doesn't block the agent in any way.
Yes, I kind of ignored the gdk_threads_enter/leave() when I
commented this.
> Apart from that, all loops started here are quit with owner
> change (clipboard_new_owner()), so there's no loop hanging
> "forever". But we could still add some kind of timeout using
> g_timeout_add_seconds().
I don't think a timeout is needed (anymore) ;)
>
> >> + gdk_threads_enter();
> >> +G_GNUC_END_IGNORE_DEPRECATIONS
> >> +
> >> + g_main_loop_unref(retrv.loop);
> >> +}
> >> +
> >> +static void clipboard_clear_cb(GtkClipboard *clipboard, gpointer user_data)
> >> +{
> >> + VDAgentClipboards *c = user_data;
> >> + clipboard_new_owner(c, sel_from_clip(clipboard), OWNER_NONE);
> >> +}
> >> +
> >> +void vdagent_clipboard_grab(VDAgentClipboards *c, guint sel,
> >> + guint32 *types, guint n_types)
> >> +{
> >> + GtkTargetEntry targets[G_N_ELEMENTS(atom2agent)];
> >> + guint n_targets = 0, i, t;
> >> +
> >> + g_return_if_fail(sel < SELECTION_COUNT);
> >> +
> >> + for (i = 0; i < G_N_ELEMENTS(atom2agent); i++)
> >> + for (t = 0; t < n_types; t++)
> >> + if (atom2agent[i].type == types[t]) {
> >> + targets[n_targets].target = (gchar *)atom2agent[i].atom_name;
> >> + n_targets++;
> >> + break;
> >> + }
> >> +
> >> + if (n_targets == 0) {
> >> + syslog(LOG_WARNING, "%s: sel=%u: no type supported", __func__, sel);
> >> + return;
> >> + }
> >> +
> >> + if (gtk_clipboard_set_with_data(c->clipboard[sel], targets, n_targets,
> >> + clipboard_get_cb, clipboard_clear_cb, c))
> >> + clipboard_new_owner(c, sel, OWNER_CLIENT);
> >> + else {
> >> + syslog(LOG_ERR, "%s: sel=%u: clipboard grab failed", __func__, sel);
> >> + clipboard_new_owner(c, sel, OWNER_NONE);
> >> + }
> >> +}
> >> +
> >> +void vdagent_clipboard_data(VDAgentClipboards *c, guint sel,
> >> + guint type, const guchar *data, guint size)
> >> +{
> >> + g_return_if_fail(sel < SELECTION_COUNT);
> >> +
> >> + DataRetrieval *retrv;
> >> + GList *l;
> >> + for (l = c->data_retrievals[sel]; l != NULL; l = l->next) {
> >> + retrv = l->data;
> >> + if (get_type_from_atom(gtk_selection_data_get_target(retrv->sel_data)) == type)
> >> + break;
> >> + }
> >> + if (l == NULL) {
> >> + syslog(LOG_WARNING, "%s: sel=%u: no corresponding request found for "
> >> + "type=%u, skipping", __func__, sel, type);
> >> + return;
> >
> > g_main_loop_quit() here?
>
> We didn't find the matching request, so which loop would you
> want to quit here?
My bad, doubled checked and you are right.
> Unlike the previous X11 implementation, multiple requests can
> be pending (I probably should have mentioned that in the commit
> message, sorry).
No worries.
I'll try to test and review faster the next version ;)
Cheers,
toso
> >
> >> + }
> >> + c->data_retrievals[sel] = g_list_delete_link(c->data_retrievals[sel], l);
> >> +
> >> + gtk_selection_data_set(retrv->sel_data,
> >> + gtk_selection_data_get_target(retrv->sel_data),
> >> + 8, data, size);
> >> +
> >> + g_main_loop_quit(retrv->loop);
> >> +}
> >> +
> >> +void vdagent_clipboard_release(VDAgentClipboards *c, guint sel)
> >> +{
> >> + g_return_if_fail(sel < SELECTION_COUNT);
> >> + if (c->owner[sel] != OWNER_CLIENT)
> >> + return;
> >> +
> >> + clipboard_new_owner(c, sel, OWNER_NONE);
> >> + gtk_clipboard_clear(c->clipboard[sel]);
> >> +}
> >> +
> >> +void vdagent_clipboard_release_all(VDAgentClipboards *c)
> >> +{
> >> + guint sel, owner;
> >> +
> >> + for (sel = 0; sel < SELECTION_COUNT; sel++) {
> >> + owner = c->owner[sel];
> >> + clipboard_new_owner(c, sel, OWNER_NONE);
> >> + if (owner == OWNER_CLIENT)
> >> + gtk_clipboard_clear(c->clipboard[sel]);
> >> + else if (owner == OWNER_GUEST && c->conn)
> >> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel, 0, NULL, 0);
> >> + }
> >> +}
> >> +
> >> +void vdagent_clipboard_request(VDAgentClipboards *c, guint sel, guint type)
> >> +{
> >> + if (sel >= SELECTION_COUNT || c->owner[sel] != OWNER_GUEST)
> >
> > I'm all for logging debug or warnings in such situations.
> >
> > Reviewed-by: Victor Toso <victortoso@xxxxxxxxxx>
> >
> > toso
> >
> >> + goto err;
> >> + if (type >= TYPE_COUNT || c->targets[sel][type] == GDK_NONE) {
> >> + syslog(LOG_WARNING, "%s: sel=%d: unadvertised data type requested",
> >> + __func__, sel);
> >> + goto err;
> >> + }
> >> +
> >> + gpointer *ref = get_weak_ref(c);
> >> + c->data_requests[sel] = g_list_prepend(c->data_requests[sel], ref);
> >> + gtk_clipboard_request_contents(c->clipboard[sel], c->targets[sel][type],
> >> + clipboard_contents_received_cb, ref);
> >> + return;
> >> +err:
> >> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_DATA, sel,
> >> + VD_AGENT_CLIPBOARD_NONE, NULL, 0);
> >> +}
> >> +
> >> +VDAgentClipboards *vdagent_clipboard_init(struct udscs_connection *conn)
> >> +{
> >> + const GdkAtom sel_atom[SELECTION_COUNT] = {
> >> + GDK_SELECTION_CLIPBOARD, /* VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD */
> >> + GDK_SELECTION_PRIMARY, /* VD_AGENT_CLIPBOARD_SELECTION_PRIMARY */
> >> + };
> >> +
> >> + VDAgentClipboards *c;
> >> + c = g_new0(VDAgentClipboards, 1);
> >> + c->conn = conn;
> >> +
> >> + for (guint sel = 0; sel < SELECTION_COUNT; sel++) {
> >> + c->clipboard[sel] = gtk_clipboard_get(sel_atom[sel]);
> >> + /* enables the use of sel_from_clipboard(clipboard) macro */
> >> + g_object_set_data(G_OBJECT(c->clipboard[sel]), "vdagent-selection",
> >> + GUINT_TO_POINTER(sel));
> >> + g_signal_connect(G_OBJECT(c->clipboard[sel]), "owner-change",
> >> + G_CALLBACK(clipboard_owner_change_cb), c);
> >> + }
> >> +
> >> + return c;
> >> +}
> >> +
> >> +void vdagent_clipboard_finalize(VDAgentClipboards *c,
> >> + struct udscs_connection *conn)
> >> +{
> >> + for (guint sel = 0; sel < SELECTION_COUNT; sel++)
> >> + g_signal_handlers_disconnect_by_func(c->clipboard[sel],
> >> + G_CALLBACK(clipboard_owner_change_cb), c);
> >> +
> >> + c->conn = conn;
> >> + vdagent_clipboard_release_all(c);
> >> +
> >> + g_free(c);
> >> +}
> >> diff --git a/src/vdagent/clipboard.h b/src/vdagent/clipboard.h
> >> new file mode 100644
> >> index 0000000..f02d8a4
> >> --- /dev/null
> >> +++ b/src/vdagent/clipboard.h
> >> @@ -0,0 +1,42 @@
> >> +/* clipboard.h - vdagent clipboard handling header
> >> +
> >> + 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_CLIPBOARD_H
> >> +#define __VDAGENT_CLIPBOARD_H
> >> +
> >> +#include "udscs.h"
> >> +
> >> +typedef struct VDAgentClipboards VDAgentClipboards;
> >> +
> >> +VDAgentClipboards *vdagent_clipboard_init(struct udscs_connection *conn);
> >> +void vdagent_clipboard_finalize(VDAgentClipboards *c,
> >> + struct udscs_connection *conn);
> >> +
> >> +void vdagent_clipboard_request(VDAgentClipboards *c, guint sel, guint type);
> >> +
> >> +void vdagent_clipboard_release(VDAgentClipboards *c, guint sel);
> >> +
> >> +void vdagent_clipboard_release_all(VDAgentClipboards *c);
> >> +
> >> +void vdagent_clipboard_data(VDAgentClipboards *c, guint sel,
> >> + guint type, const guchar *data, guint size);
> >> +
> >> +void vdagent_clipboard_grab(VDAgentClipboards *c, guint sel,
> >> + guint32 *types, guint n_types);
> >> +
> >> +#endif
> >> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> >> index 92ffcf3..81b7a5a 100644
> >> --- a/src/vdagent/vdagent.c
> >> +++ b/src/vdagent/vdagent.c
> >> @@ -44,8 +44,10 @@
> >> #include "audio.h"
> >> #include "x11.h"
> >> #include "file-xfers.h"
> >> +#include "clipboard.h"
> >>
> >> typedef struct VDAgent {
> >> + VDAgentClipboards *clipboards;
> >> struct vdagent_x11 *x11;
> >> struct vdagent_file_xfers *xfers;
> >> struct udscs_connection *conn;
> >> @@ -154,6 +156,17 @@ static gboolean vdagent_finalize_file_xfer(VDAgent *agent)
> >> return TRUE;
> >> }
> >>
> >> +static void vdagent_quit_loop(VDAgent *agent)
> >> +{
> >> + /* other GMainLoop(s) might be running, quit them before agent->loop */
> >> + if (agent->clipboards) {
> >> + vdagent_clipboard_finalize(agent->clipboards, agent->conn);
> >> + agent->clipboards = NULL;
> >> + }
> >> + if (agent->loop)
> >> + g_main_loop_quit(agent->loop);
> >> +}
> >> +
> >> static void daemon_read_complete(struct udscs_connection **connp,
> >> struct udscs_message_header *header, uint8_t *data)
> >> {
> >> @@ -164,18 +177,24 @@ static void daemon_read_complete(struct udscs_connection **connp,
> >> vdagent_x11_set_monitor_config(agent->x11, (VDAgentMonitorsConfig *)data, 0);
> >> break;
> >> case VDAGENTD_CLIPBOARD_REQUEST:
> >> + vdagent_clipboard_request(agent->clipboards, header->arg1, header->arg2);
> >> break;
> >> case VDAGENTD_CLIPBOARD_GRAB:
> >> + vdagent_clipboard_grab(agent->clipboards, header->arg1,
> >> + (guint32 *)data, header->size / sizeof(guint32));
> >> break;
> >> case VDAGENTD_CLIPBOARD_DATA:
> >> + vdagent_clipboard_data(agent->clipboards, header->arg1, header->arg2,
> >> + data, header->size);
> >> break;
> >> case VDAGENTD_CLIPBOARD_RELEASE:
> >> + vdagent_clipboard_release(agent->clipboards, header->arg1);
> >> break;
> >> case VDAGENTD_VERSION:
> >> if (strcmp((char *)data, VERSION) != 0) {
> >> syslog(LOG_INFO, "vdagentd version mismatch: got %s expected %s",
> >> data, VERSION);
> >> - g_main_loop_quit(agent->loop);
> >> + vdagent_quit_loop(agent);
> >> version_mismatch = 1;
> >> }
> >> break;
> >> @@ -222,6 +241,7 @@ static void daemon_read_complete(struct udscs_connection **connp,
> >> }
> >> break;
> >> case VDAGENTD_CLIENT_DISCONNECTED:
> >> + vdagent_clipboard_release_all(agent->clipboards);
> >> if (vdagent_finalize_file_xfer(agent)) {
> >> vdagent_init_file_xfer(agent);
> >> }
> >> @@ -236,8 +256,7 @@ static void daemon_disconnect_cb(struct udscs_connection *conn)
> >> {
> >> VDAgent *agent = udscs_get_user_data(conn);
> >> agent->conn = NULL;
> >> - if (agent->loop)
> >> - g_main_loop_quit(agent->loop);
> >> + vdagent_quit_loop(agent);
> >> }
> >>
> >> /* When we daemonize, it is useful to have the main process
> >> @@ -309,7 +328,7 @@ gboolean vdagent_signal_handler(gpointer user_data)
> >> {
> >> VDAgent *agent = user_data;
> >> quit = TRUE;
> >> - g_main_loop_quit(agent->loop);
> >> + vdagent_quit_loop(agent);
> >> return G_SOURCE_REMOVE;
> >> }
> >>
> >> @@ -368,6 +387,8 @@ static gboolean vdagent_init_async_cb(gpointer user_data)
> >> if (!vdagent_init_file_xfer(agent))
> >> syslog(LOG_WARNING, "File transfer is disabled");
> >>
> >> + agent->clipboards = vdagent_clipboard_init(agent->conn);
> >> +
> >> if (parent_socket != -1) {
> >> if (write(parent_socket, "OK", 2) != 2)
> >> syslog(LOG_WARNING, "Parent already gone.");
> >> @@ -378,7 +399,7 @@ static gboolean vdagent_init_async_cb(gpointer user_data)
> >> return G_SOURCE_REMOVE;
> >>
> >> err_init:
> >> - g_main_loop_quit(agent->loop);
> >> + vdagent_quit_loop(agent);
> >> quit = TRUE;
> >> return G_SOURCE_REMOVE;
> >> }
> >> --
> >> 2.14.3
> >>
> >> _______________________________________________
> >> Spice-devel mailing list
> >> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> >> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> Concerning the naming, I also considered using "Selection" instead of
> "Clipboard" (e.g. "vdagent_selection_request" instead of
> "vdagent_clipboard_request"). I think this would make more sense when
> later intoducing the drag&drop support. What do you think?
>
> Thanks for the review.
> Cheers,
> Jakub
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel