This is an intermediary step for the following patches to make the
changes more clear and easy to follow.
In file_xfer_send_progress(), I've renamed the variable self to
xfer_task as this is not SpiceFileTransferTask function.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This patch changes:
* rename function: file_xfer_queue -> file_xfer_queue_msg_to_agent
As it makes more clear what this helper function does;
* Use buffer provided by spice_file_transfer_task_read_finish()
instead of accessing SpiceFileTransferTask's private structure
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The errors related to SpiceFileTransferTask can be present:
* before the flush happens, on spice_file_transfer_task_read_async()
* during the async flush:
- cancel from user which is handled by file_xfer_flush_async
- cancel/error from agent, handled on main_agent_handle_xfer_status()
file_xfer_flush_finish() should not check internal errors of
SpiceFileTransferTask and only be worried about errors regarding the
flush itself.
Note that with or without this patch, in case of errors from agent, we
don't stop the flushing; it is only cancelled from user cancellation.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
By separating SpiceFileTransferTask from channel-main, we could choose
where to put the handler for messages. With the approach based on
spice_file_transfer_task_read_async(), we cannot have it under
spice-file-transfer-task.c due the need of callback and userdata on
_read_async.
It is much easier to keep this in channel-main and do not move any
VDAgent.
This patch reverts 349a52ca2d changes
but it does rename:
- function file_xfer_handle_status -> main_agent_handle_xfer_status
- variables task to xfer_task
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
* on VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA, if the file-transfer is
on pending state, spice_file_transfer_task_read_async() will call the
callback with error set.
* on VD_AGENT_FILE_XFER_STATUS_SUCCESS, if the file-transfer is on
pending state, spice_file_transfer_task_completed() will set the
error for this task.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Introduced functions (private):
* void spice_file_transfer_task_read_async()
* gssize spice_file_transfer_task_read_finish()
For a better abstraction of how to read from SpiceFileTransferTask and
handle its data, following the design of other objects like GFile and
GInputStream.
Due to the logic changes involved, some functions were created or
renamed to better address or match its place and purpose:
* spice_file_transfer_task_read_stream_cb
Callback for the actual read from GInpustStream; This is handling
the SpiceFileTransferTask bits only;
* file_xfer_read_cb -> file_xfer_read_async_cb
Renamed to match _read_async() function; This is handling the data
from reading the file by flushing it to the agent.
As the _read_async() uses GTask, the error handling is done on the
channel-main's callback, after _read_finish() is called.
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Introduced functions (private):
* void spice_file_transfer_task_init_task_async()
* GFileInfo *spice_file_transfer_task_init_task_finish()
The init process of SpiceFileTransferTask does initialize its
GFileInputStream (for reading the file) and also its GFileInfo
(necessary to protocol in order to start the file-transfer with agent)
Due the logic changed involved, some functions were renamed to better
match its place and purpose:
* file_xfer_info_async_cb -> file_xfer_init_task_async_cb
It is channel-main's callback for each _init_task_async()
* file_xfer_read_async_cb -> spice_file_transfer_task_read_file_cb
* file_xfer_info_async_cb -> spice_file_transfer_task_query_info_cb
Both should be private to SpiceFileTransferTask now.
As the _init_task_async() uses GTask, some error handling was moved to
channel-main's after _init_task_finish() is called.
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
We can split from file_xfer_send_start_msg_async() the logic in
creating the SpiceFileTransferTasks; The rest of the function can be
handled at spice_main_file_copy_async().
The new function, spice_file_transfer_task_create_tasks() returns a
GHashTable to optimize the access to a SpiceFileTransferTask from its
task-id, which is what we receive from the agent.
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
In order for channel-main to interact with each SpiceFileTransferTask
for the file-transfer operation, the following functions are
introduced:
* spice_file_transfer_task_get_id
* spice_file_transfer_task_get_channel
* spice_file_transfer_task_get_cancellable
Note that "id" property is public and could be acquired by
g_object_get but having the helper function is more practical.
This change is related to split SpiceFileTransferTask from
channel-main.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This is a follow-up of the previous commit ('usb-channel: Really stop
listening for USB events on disconnection').
Since the USB event thread has to be stopped when we destroy the
associated SpiceUsbDeviceManager, spice_usb_device_manager_dispose()
should force event_thread_run to FALSE even if
spice_usb_device_manager_stop_event_listening() was not enough. When
this happens, this means that there is a bug in the internal users of
spice_usb_device_manager_start_event_listening(), but with this change,
we'll at least warn about it, and avoid a thread leak/potential future
crash.
When using USB redirection, it's fairly easy to leak the thread handling
USB events, which will eventually cause problems in long lived apps.
In particular, in virt-manager, one can:
- start a VM
- connect to it with SPICE
- open the USB redirection window
- redirect a device
- close the SPICE window
-> the SpiceUsbDeviceManager instance will be destroyed (including the
USB context it owns), but the associated event thread will keep running.
Since it's running a loop blocking on libusb_handle_events(priv->context),
the loop will eventually try to use the USB context we just destroyed
causing a crash.
We can get in this situation when redirecting a USB device because we
will call spice_usb_device_manager_start_event_listening() in
spice_usbredir_channel_open_device(). The matching
spice_usb_device_manager_stop_event_listening() call is supposed to
happen in spice_usbredir_channel_disconnect_device(), however by the
time it's called in the scenario described above, the session associated
with the channel will already have been set to NULL in
spice_session_channel_destroy().
The session is only needed in order to get the SpiceUsbDeviceManager
instance we need to call spice_usb_device_manager_stop_event_listening()
on. This patch stores it in SpiceChannelUsbredir instead, this way we
don't need SpiceChannel::session to be non-NULL during device
disconnection.
This should fix the issues described in
https://bugzilla.redhat.com/show_bug.cgi?id=1217202
(virt-manager) and most
likely https://bugzilla.redhat.com/show_bug.cgi?id=1337007 (gnome-boxes)
as well.
spice_usbredir_channel_disconnect_device_async() creates a GTask and
then calls g_task_run_in_thread(). This method will take the reference
it needs on the GTask, it does not take ownership of the passed-in
GTask. This means we need to unref the GTask we created after calling
g_task_run_in_thread(), otherwise we are going to leak the GTask, as
well as the channel it's associated with.
Since it's an USB redir channel, this also causes some USB device
manager/USB event thread leaks.
According to rfc2732:
"To use a literal IPv6 address in a URL, the literal address should be
enclosed in "[" and "]" characters."
Resolves: rhbz#1331777
Acked-by: Marc-André Lureau <mlureau@redhat.com>
This is a minor fix in the logic as in both situations (with or
without the patch) the reference count for the SpiceFileTransferTask
object is the same.
The change is interesting as SpiceFileTransferTask is created but on
g_file_read_async() it increases its reference count while
c->file_xfer_tasks keeps the original one.
It should be the other way around.
Acked-by: Marc-André Lureau <marcandre.lureau@gmail.com>
We want to trigger rebuild of libspice-client-gtk-3.0.la or
libspice-client-glib-2.0.la whenever the corresponding symbol file
changes.
However _DEPENDENCIES is not the right way of handling that as it will
disable automatic automake dependency generation.
This was not causing issues mainly because _DEPENDENCIES was mispelt as
_DEPEDENCIES.
Quoting automake manual:
https://www.gnu.org/software/automake/manual/automake.html#index-EXTRA_005fmaude_005fDEPENDENCIES-1
« The EXTRA_*_DEPENDENCIES variable may be useful for cases where you
merely want to augment the automake-generated _DEPENDENCIES variable
rather than replacing it. »
So this commit switches to use EXTRA_*_DEPENDENCIES rather than
*_DEPENDENCIES.
Commit 0fafbe3 broke the build on win32, because it accesses
d->egl.enabled. Add a helper function and fix the build.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reported-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Since the switch to a container widget (gtkstack then gtkeventbox), the
grab may be lost when clicking on the display. Since events are treated
at the top level container, set widget "above-child" to trap all of them
to solve this.
Fixes:
https://bugs.freedesktop.org/show_bug.cgi?id=96595
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reported-by: Frediano Ziglio <fziglio@redhat.com>
Tested-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Fabiano Fidêncio <fidencio@redhat.com>
Ignoring the display area offset doesn't work nicely with virgl. Imho,
this condition is wrong even in QXL case.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Without (array zero-terminated=1), one null terminated array
parameter is translated into a single element, so its binding
isn't usable.
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This prevent Windows to handle IME on the widget which cause the
application to not receive keyboard events.
To test this issue set keyboard layout to Japanese and Microsoft
IME (the default one). Set the input method to full katakana, assure
your remote-viewer is using this method and start pressing
alphabetical keys. On the guest (open a terminal, an editor or
a word processor to make easier) you won't see any character.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
When using remote-viewer --full-screen with a VM/client with multiple
monitors, a race can be observed during auto-configuration. First, the
client monitors config is sent to the guest:
(remote-viewer:19480): GSpice-DEBUG: channel-main.c:1166 main-1:0: sending new monitors config to guest
(remote-viewer:19480): GSpice-DEBUG: channel-main.c:1183 main-1:0: monitor #0: 1920x1080+0+0 @ 32 bpp
(remote-viewer:19480): GSpice-DEBUG: channel-main.c:1183 main-1:0: monitor #1: 1920x1080+1920+0 @ 32 bpp
Then we receive messages from the agent which trigger a call to
update_display_timer(0)
This should cause the current monitors state to be sent to the server
very soon after this call. However, in the racy case, this is delayed
for nearly a second, and timer_set_display() ends up being called at the
wrong time, when information about the first display channel have been
received from the server, but not the second one.
When this happens, we inform the server that only one monitor is active,
then the server sends a MonitorsConfig message with 2 monitors (first
request we sent), and then with just 1 monitor (second request we sent).
This causes remote-viewer to show one fullscreen black window indicating
"Connected to server" on one monitor and a working fullscreen window on
the second monitor.
update_display_timer(0) schedules a timeout to be run with
g_timeout_add_seconds(0). However, g_timeout_add_seconds() schedules
timers with a granularity of a second, so the timeout we scheduled with
g_timeout_add_seconds() may fire up to 1 second later than when we
called it, while we really wanted it to fire as soon as possible.
Special-casing update_display_timer(0) and using g_timeout_add() in that
case avoid this issue. In theory, the race could probably still happen
with a very very bad timing, but in practice I don't think it will be
possible to trigger it after this change.
https://bugzilla.redhat.com/show_bug.cgi?id=1323092
SASL GSSAPI module will try to negotiate authentication based on the
credentials in the default credentials cache. It does not matter if
SPICE knows username or not as SASL negotiation will pass through the
discovered name from the GSSAPI module.
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Acked-by: Fabiano Fidêncio <fidencio@redhat.com>
GetLastError returns a DWORD which is a "unsigned long" on Windows
so use "%lu" formatting instead of "%u".
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Marc-André Lureau <mlureau@redhat.com>
This prevents a possible crash on windows 32 bit.
The linkage of UnhookWindowsHookEx is WINAPI which is __stdcall while
callback for g_clear_pointer is C. This could cause stack pointer
corruption depending on compiler flags.
On __stdcall linkage function change the stack pointer while returning
from a function removing the parameters. On C linkage function leave
the stack pointer unchanged. So if the compiler call a __stdcall
function as a C function it expect the stack pointer to be unchanged
causing the pointer to be inconsistent by an offset.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
If Windows layout does not support a given key the resulting virtual code
is set to 0xFF. To avoid losing this raw key (causing the key not been
sent to remote machine) detect this condition and handle the key.
The check for raw scancode is there to understand if we can handle
correctly the key in following code.
This problem can happen for instance using a 106 key Japanese keyboard
while the layout is set in English; in this case keys like CONVERT not
forwarded correctly.
Some note on how to reproduce and test this problem.
Physical way:
1- get a 106 key Japanese keyboard for your Windows client machine;
2- setup your client to English keyboard layout;
3- connect to a Linux machine (no matter the distro or version or
keyboard configuration);
4- open "xinput test-xi2 <device>" command on Linux (device is
the "AT" device in this case);
5- press CONVERT or other keys not present on an English keyboard.
Virtual way (Windows machine on a VM):
- set machine remote to VNC;
- assure Qmeu has lock-key-sync=off option to vnc;
- connect to Windows machine with a VNC client (I suggest TigerVNC
as remote-viewer do some keyboard insertion);
- do steps 2, 3, 4 above
- press the CONVERT key. If you don't have you can simulate, either
- change VNC client code to insert scancode 0x70 instead of another
key and press this key;
- disconnect main VNC client and use some tool to inject keys
(I use a modifier version of vncdotool).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The dimensions sent by the remote end are redundant and should not be
trusted.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
The returned value from do_pointer_grab() is treated as a boolean - grab
was successful or not. Change the function to return a boolean value.
Reported-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
We check that there is a matching frame in the queue before popping the
old ones. So we know the inner loop will find a match and thus that
frame will not be NULL. But figuring that out is too hard for the
compiler.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com>
Fixes the following warning:
../../src/vmcstream.c:124: warning: Symbol name not found at the start of the comment block.
../../src/win-usb-driver-install.c:347: warning: Symbol name not found at the start of the comment block.
Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com>
