Hi, On Mon, Dec 10, 2018 at 08:39:46AM -0500, Frediano Ziglio wrote: > > On Mon, Dec 10, 2018 at 07:42:35AM -0500, Frediano Ziglio wrote: > > > > > > > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > > > > > Spice client should listen to SpiceMainChannel::agent-connected > > > > notification and avoid calling any clipboard related functions such as > > > > spice_gtk_session_paste_from_guest() from client-gtk library. > > > > > > > > This patch removes the silent return of agent_clipboard_grab() in > > > > order to properly catch bugs with critical messages. > > > > > > > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx> > > > > > > Documentation for agent-connected: "Whether the agent is connected" > > > > > > Documentation for main-agent-update signal: > > > > > > /** > > > * SpiceMainChannel::main-agent-update: > > > * @main: the #SpiceMainChannel that emitted the signal > > > * > > > * Notify when the %SpiceMainChannel:agent-connected or > > > * %SpiceMainChannel:agent-caps-0 property change. > > > **/ > > > > > > Documentation for spice_main_channel_clipboard_selection_grab > > > > > > /** > > > * spice_main_channel_clipboard_selection_grab: > > > * @channel: a #SpiceMainChannel > > > * @selection: one of the clipboard #VD_AGENT_CLIPBOARD_SELECTION_* > > > * @types: an array of #VD_AGENT_CLIPBOARD types available in the clipboard > > > * @ntypes: the number of @types > > > * > > > * Grab the guest clipboard, with #VD_AGENT_CLIPBOARD @types. > > > * > > > * Since: 0.35 > > > **/ > > > > > > Documentation for spice_gtk_session_paste_from_guest > > > > > > /** > > > * spice_gtk_session_paste_from_guest: > > > * @self: #SpiceGtkSession > > > * > > > * Copy guest clipboard to client-side clipboard. > > > * > > > * Since 0.8 > > > **/ > > > > > > I don't see any requirement for the client to check agent status. > > > > You mean that we should improve documentation or something else? > > > > I was expecting a "is documented here". Documentation saying that this feature rely on agent seems to be lacking. e.g: grep for 'agent' on SpiceGtkSession > As spice-gtk is a public library with an API speaking of "Spice > client" is not a good thing, should be Spice clientS, meaning > all possible clients and being ALL would mean even > unknown/proprietary ones. Hm, I did not mention a specific client. My usage of 'Spice client' is meant to be generic. I have no problem in changing this to make you happy but I don't see why is wrong. > As all clients should respect API documentation you are allow to change > API behaviour where the behaviour was documented as not defined (for > instance free() behaviour was changed in respect to NULL from undefined > behaviour to ignored). Because I'm verbose on commit log, doesn't mean that I changed the behavior? If any function dealing with clipboard is handled while the agent is disconnected, that's bug and some warning/critical should be hit. I'm making it explicit in agent_clipboard_*() functions. > At least we should check that the "expected" clients (remote-viewer, > virt-viewer, virt-manager I would say) already have this behaviour > before changing API behaviour. SpiceGtkSession's clipboard handling is automatic if auto-clipboard property set or when it is false, by using spice_gtk_session_paste_from_guest() and spice_gtk_session_copy_to_guest() functions. "auto-clipboard" is preferred by gnome-boxes, virt-viewer, virt-manager. Spicy **testing tool** has menu to use both functions above but they are disabled when agent is disconnected. > Is only a warning change but does not help if the change is > producing hundred of warnings which would hide any possible > issue raised by warnings, not having the agent running is not > an impossible use case. The warning would happen anyway. It should happen. I agree with lack of documentation. Cheers, > > > As always, thanks for reviewing. > > > > > > > > > --- > > > > src/channel-main.c | 4 +--- > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > diff --git a/src/channel-main.c b/src/channel-main.c > > > > index 223043a..aa563d2 100644 > > > > --- a/src/channel-main.c > > > > +++ b/src/channel-main.c > > > > @@ -1352,9 +1352,7 @@ static bool agent_clipboard_grab(SpiceMainChannel > > > > *channel, guint selection, > > > > size_t size; > > > > int i; > > > > > > > > - if (!c->agent_connected) > > > > - return false; > > > > - > > > > + g_return_val_if_fail(c->agent_connected, false); > > > > g_return_val_if_fail(test_agent_cap(channel, > > > > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND), false); > > > > > > > > size = sizeof(VDAgentClipboardGrab) + sizeof(uint32_t) * ntypes; > > > > > > Frediano > >
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel