When GLZ code attempts to send a 64 bit integer the 8 top bit of
the lower (32 bits) part of the number are stripped due to a bug.
This was discovered by Zhongqiang Huang <useprxf@gmail.com>
Reported-by: Zhongqiang Huang <useprxf@gmail.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Although not necessary for a single monitor DisplayChannel implementation
this make the DisplayChannels more coherent from the client
point of view.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
SpiceWhiteness/SpiceBlackness/SpiceInvers are 3 typedef for the same
type, no need to have 3 identical red_put_xxx/red_get_xxx methods.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
SpiceBlend is a typedef to SpiceCopy, and red_put_blend() and
red_put_copy() are identical, so we can add a #define red_put_blend
red_put_copy similar to the one we already have for red_get_blend.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In order to use the new TCP_CORK feature, disable auto flush.
Depending on channel implementation and purpose of the channel enabling
blindly for all channels could cause performance issues, specifically if
flush is not done at the right time.
CommonGraphicsChannel channels were tested to make sure is not that case.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Cork is a system interface implemented by Linux and some *BSD systems to
tell the system that other data are expected to be written to a socket.
This allows the system to reduce network fragmentation waiting for network
packets to be complete.
Using some replay capture and some instrumentation resulted in a
bandwith reduction of 11% and a packet reduction of 56%.
The tests was done using replay utility so results could be a bit different
from real cases as:
- replay goes as fast as it can, for instance packets could
be merged by the kernel decreasing packet numbers and a bit
byte spent (this actually make the following improves worse);
- there are fewer channels (no much cursor, sound, etc).
The following tests shows count packet and total bytes from server to
client using a real network. I used a direct cable connection using 1gb
connection and 2 laptops.
cork: 537 1582240
cork: 681 1823754
cork: 524 1583287
cork: 538 1582350
no cork: 1329 1834630
no cork: 1290 1829094
no cork: 1289 1830164
no cork: 1317 1833589
no cork: 1320 1835705
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The writing to network was always immediate.
Every write in the stream causes a write to the OS.
This can have some penalty if you don't write large data as network
packets can be more fragmented or you encrypt data in smaller chunks
(when data are encrypted some padding is added then data is split in
multiple of encryption block which is usually the size of encryption
key and this is done for every write).
Define an interface to allow higher levels code to tell low level when
data should be sent to remote or when can wait more data.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Due to a bug in current packaged Valgrind in the CI (1:3.13.0-13.fc27)
check-valgrind is failing with:
==17986== Source and destination overlap in memcpy_chk(0x72c060, 0x72c068, 33)
==17986== at 0x4C344F0: __memcpy_chk (vg_replace_strmem.c:1581)
==17986== by 0x40E7E9: check_vmc_error_message (test-stream-device.c:166)
==17986== by 0x40EFD4: test_stream_device_format_after_data (test-stream-device.c:349)
==17986== by 0x7A012E9: test_case_run (gtestutils.c:2157)
==17986== by 0x7A012E9: g_test_run_suite_internal (gtestutils.c:2241)
==17986== by 0x7A0121A: g_test_run_suite_internal (gtestutils.c:2253)
==17986== by 0x7A014C1: g_test_run_suite (gtestutils.c:2329)
==17986== by 0x7A014E0: g_test_run (gtestutils.c:1594)
==17986== by 0x40951A: main (test-stream-device.c:410)
==17986==
By default during CI build _FORTIFY_SOURCE is enabled, which turns memmove
into __memmove_chk, which is wrongly turned into __memcpy_chk when running
under Valgrind.
Setting _FORTIFY_SOURCE to 0 prevents the use of __memmove_chk, and avoids
triggering the Valgrind bug.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The name is more consistent with red_marshall_cursor_init.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
There's an implicit API/ABI contract between QEMU and SPICE that SPICE
will keep the guest QXL resources alive as long as QEMU can hold a
pointer to them. This implicit contract was broken in 1c6e7cf7 "Release
cursor as soon as possible", causing crashes at migration time.
While the proper fix would be in QEMU so that spice-server does not need
to have that kind of knowledge regarding QEMU internal implementation,
this commit reverts to the pre-1c6e7cf7 behaviour to avoid a regression
while QEMU is being fixed.
This version of the fix is based on a suggestion from Frediano Ziglio.
https://bugzilla.redhat.com/show_bug.cgi?id=1540919
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Code can have problems reading empty messages, check we can
handle it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe de Dinechin <dinechin@redhat.com>
Handle capabilities from guest device.
Send capability to the guest when device is opened.
Currently there's no capabilities set on the message sent.
On the tests we need to discard the capability message before
reading the error.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe de Dinechin <dinechin@redhat.com>
This function will be reused to initialise different message
headers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe de Dinechin <dinechin@redhat.com>
Currently, red-parse-qxl.c uses g_malloc+memcpy to duplicate the cursor
data when it could use g_memdup() instead. red-stream-device.c does the
same thing but uses spice_memdup(). This commit makes use of g_memdup()
in both cases so that this memory is consistently allocated through
glib.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
The name 'com.redhat.stream.0' is too generic and in no way denotes it
belongs to SPICE. It is preferred to have the project's domain in the
name and Red Hat doesn't own the project. Rename it to
org.spice-space.stream.0.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Reuse option from common code.
Also reuse spice_extra_checks constant instead of using the preprocessor
macro directly.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
The timeout is too short when the test run under Valgrind
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This is required by the new test-listen test to connect using
TLS.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
In order to avoid confusion with file named stream-device.h, from
spice-protocol.
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
These factor a bit of common code, and more importantly, help with
freeing all event loop related data at the end of each test.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
This test case will be testing the external spice-server API to
configure the address/port it's listening on. For now it sets up a
listening server, spawns a thread which is going to connect to that
port, and check it gets the REDQ magic upon connection. It will be
extended to test for Unix sockets, TLS sockets, ...
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
There is currently a debug printf which is always shown when a mainloop
event is triggered. This is unlikely to be useful unless one is
debugging the event loop code.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
From spice-gtk b312ca08 commit:
" At the moment:
- Fedora 26 has 2.52
- Fedora 25 has 2.50
- Fedora 24 has 2.48
- CentOS 7 has 2.46
- Debian 9 has 2.50"
RHEL6 only have 2.28, but glib 2.32 is only used in a test case at the
moment.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Currently if we fail to set up the watch waiting for accept() to be
called on the socket, we still keep the network socket(s) open even if we
are not going to be able to use it. This commit makes sure it's closed a
set to -1 when such a failure occurs rather than having a half
initialized spice-server instance.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Fix clang warning:
red-record-qxl.c:893:13: error: variable 'fd_in' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
if (ret)
This is technically impossible but is not on a hot path.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Address sanitizer use a larger stack than normal compiled programs
making the build fails. This causes this error on GitLab CI system:
stream-device.c: In function 'stream_device_partial_read':
stream-device.c:182:1: error: the frame size of 32992 bytes is larger than 20460 bytes [-Werror=frame-larger-than=]
Allow larger stack on "makecheck" to avoid this issue.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Due to ticket expiration, it is possible that the streaming channels for
the client are created after the ticket expires. Currently, streaming
channels are created dynamically when the guest starts streaming to the
server, which can happen at any time (for instance if you decide to start
the graphic server manually).
If the ticket has expired before the streaming channel is created,
authentication will fail and the client will not be able to connect.
To avoid this, create the channels when the first main channel connection
is made. This ensures that client will connect to all streaming channels.
This could be considered a temporary solution. There may be other
situations where it would be useful to connect new channels after the
ticket has expired, but enabling this behavior would require protocol
changes and a careful analysis of security implications.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Move public declaration (stream_device_connect) from char-device.h
to a new stream-device.h.
Add type declaration for StreamDevice.
This allows to use the type outside the implementation file and makes it
easier to extend the interface without changing char-device.h header.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This is a preparation for meson build, which has built-in support for
generating enums, but requires the template files to be renamed. It uses
the basename of template files to generate the output, and in this case
it would be the same file for both '.c' and '.h'. Ideally meson would
let us specify the name of the output files, but this is not the case.
Without renaming, the following error happens:
Meson encountered an error in file server/meson.build, line 30, column 0:
Tried to create target "spice-server-enums.tmpl", but a target of that
name already exists.
Reference: http://mesonbuild.com/Gnome-module.html#gnomemkenums
Note that by the time of this commit, the documentation is not accurate
and does not mention the fact that output files will get the base name
of the template files if they are specified, I submitted a pull request
to meson fixing this detail in docs:
https://github.com/mesonbuild/meson/pull/3191
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Test case for the issue fixed by previous commit.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If data message is followed by another message, it's theoretically
possible that device loses the sync with the guest.
The actual Qemu and streaming agent implementation avoids it, but better to
make sure this can't happen in the server code too.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Previous patch causes a bug in Qemu if the patch
46764fe09ca2e0f15c0981a672c166ed8cf57e72 ("virtio-serial: fix segfault
on disconnect") is not included in that version of Qemu (patch present in
version 2.10.0).
This crash happens when device is closed during a write operation.
For SPICE character device, spice_server_char_device_wakeup is called
to write data which handles both read and write pending operations.
As we want to close the device but we can't do it inside the handler
without causing a crash, this commit schedules a timer that will close the
guest device outside this callback.
The proper solution would be to patch Qemu but making sure of this is not
so easy, hence this workaround in spice-server.
Code is marked with some comments to remember to remove this
hack in a safe future.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Once the device is an error state, we don't want the guest to keep
reading/writing to it, especially as this could put the device in an
inconsistent state. This commit disables the device when an error occurs to
prevent further unintended use of the device by the guest.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Due to the way Qemu handle the device, when an error occurs we must consume
all pending data inside the callback which reads data from the device.
If we don't flush this data, the next time spice-server tries to read from
the device (after the guest closes/reopens it), we'll be getting stale
data. This can happen because we cannot prevent the guest from writing to
the device even after it got in an error state.
This needs to be done within this callback, as QEMU returns 0 if you call
SpiceCharDeviceInterface::read() outside of it. QEMU invokes this callback
through a call to spice_server_char_device_wakeup.
On the test now we must test that we receive an error from the device.
Previously we checked that last part of the data was not read. Now
potentially all data are read, so we need another way to check the device
detected the error.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This includes some rendering fixes.
Frediano Ziglio (19):
proto: Add some documentation to stream_report message
protocol: Allow to specify a surface will be streamed
canvas-base: Fix width computation for palette images
canvas: Simplify code using spice_memdup
canvas: Remove possible leak on LZ decompression failure
lz: Avoid temporary variable
lz: Simplify code
lz: Remove unused encode_level and only assigned io_start
canvas: Move PixmanData to C file
canvas: Remove mutex field from PixmanData
canvas: Remove Windows bitmap allocation
canvas: Remove unused dc parameter from surface_create
canvas: Remove dc parameter from SwCanvas::put_image
canvas: Remove dc fields from CanvasBase and LzDecodeUsrData
canvas: Unify __surface_create_stride and surface_create_stride
canvas: Remove unused include header
canvas: Prevent some error compiling spice-gtk
canvas: Fix some semi transparent drawing
canvas: Use SPICE_UNALIGNED_CAST to avoid -Wcast-align warnings
Pavel Grunt (1):
Remove GDI canvas
Paweł Pękala (1):
Fix build with LibreSSL
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
Test all batched (send together) messages are handled correctly
and device is not stuck.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Qemu does not trigger a new data read if we don't read all data in
the buffer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
