From Gitlab CI:
=17955== 16 bytes in 1 blocks are possibly lost in loss record 725 of 2,079
==17955== at 0x4C2DBAB: malloc (vg_replace_malloc.c:299)
==17955== by 0x4011D17: tls_get_addr_tail.isra.0 (in /usr/lib64/ld-2.27.so)
==17955== by 0x4017997: __tls_get_addr (in /usr/lib64/ld-2.27.so)
==17955== by 0xEE4534B: gnutls_rnd (in /usr/lib64/libgnutls.so.30.20.2)
==17955== by 0xEE1F254: ??? (in /usr/lib64/libgnutls.so.30.20.2)
==17955== by 0xEE1F947: ??? (in /usr/lib64/libgnutls.so.30.20.2)
==17955== by 0xEE231B5: ??? (in /usr/lib64/libgnutls.so.30.20.2)
==17955== by 0xEE24D67: gnutls_handshake (in /usr/lib64/libgnutls.so.30.20.2)
==17955== by 0xEBD4FEA: ??? (in /usr/lib64/gio/modules/libgiognutls.so)
==17955== by 0x7463936: g_task_thread_pool_thread (gtask.c:1331)
==17955== by 0x7A3E932: g_thread_pool_thread_proxy (gthreadpool.c:307)
==17955== by 0x7A3DF29: g_thread_proxy (gthread.c:784)
==17955== by 0x8284563: start_thread (in /usr/lib64/libpthread-2.27.so)
==17955== by 0x859631E: clone (in /usr/lib64/libc-2.27.so)
==17955==
==17955== 32 bytes in 1 blocks are possibly lost in loss record 1,234 of 2,079
==17955== at 0x4C2DBAB: malloc (vg_replace_malloc.c:299)
==17955== by 0x4011D17: tls_get_addr_tail.isra.0 (in /usr/lib64/ld-2.27.so)
==17955== by 0x4017997: __tls_get_addr (in /usr/lib64/ld-2.27.so)
==17955== by 0xCAA5173: __cxa_get_globals (in /usr/lib64/libstdc++.so.6.0.25)
==17955== by 0xCAA6186: __cxa_throw (in /usr/lib64/libstdc++.so.6.0.25)
==17955== by 0xC601457: ??? (in /usr/lib64/libproxy.so.1.0.0)
==17955== by 0xC5F6BB6: ??? (in /usr/lib64/libproxy.so.1.0.0)
==17955== by 0xC5F7089: ??? (in /usr/lib64/libproxy.so.1.0.0)
==17955== by 0xC5F7470: px_proxy_factory_get_proxies (in /usr/lib64/libproxy.so.1.0.0)
==17955== by 0xC3E64E3: ??? (in /usr/lib64/gio/modules/libgiolibproxy.so)
==17955== by 0x7463936: g_task_thread_pool_thread (gtask.c:1331)
==17955== by 0x7A3E932: g_thread_pool_thread_proxy (gthreadpool.c:307)
==17955== by 0x7A3DF29: g_thread_proxy (gthread.c:784)
==17955== by 0x8284563: start_thread (in /usr/lib64/libpthread-2.27.so)
==17955== by 0x859631E: clone (in /usr/lib64/libc-2.27.so)
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
encode_32 already deals with endian, don't swap twice.
Tested with a ppc64 server machine and a x64 client.
This looks the reverse of a previous patch (59c6c82) supposed to fix big
endian machine. encode_32 has been always:
static inline void encode_32(Encoder *encoder, unsigned int word)
{
encode(encoder, (uint8_t)(word >> 24));
encode(encoder, (uint8_t)(word >> 16) & 0x0000ff);
encode(encoder, (uint8_t)(word >> 8) & 0x0000ff);
encode(encoder, (uint8_t)(word & 0x0000ff));
}
while encode basically is similar to a putc on a FILE stream so is writing
numbers from host endian to big endian order.
The "main" endian (the one more tested since ever) is host/guest being
little endian. So if you call encode_32 with a 0x01020304 you get 4 bytes
in the order 1, 2, 3, 4.
Before and after 59c6c82 LZ_MAGIC was defined as:
#define LZ_MAGIC (*(uint32_t *)"LZ ")
so on little endian this was 0x4c, 0x5a, 0x20, 0x20 that is 0x20205a4c
which written through encode_32 become 0x20, 0x20, 0x5a, 0x4c so we can say
that at the end on the network we must have 0x20, 0x20, 0x5a, 0x4c.
On big endian however LZ_MAGIC got the value 0x4c5a2020 which written
through encode_32 get 0x4c, 0x5a, 0x20, 0x20 which is the opposite
expected. So patch 59c6c82 reverted the order having again 0x20, 0x20,
0x5a, 0x4c on the network.
However commit 5a7e587 (spice-common), in an attempt to avoid double
swapping on LZ, changed LZ_MAGIC to
#define LZ_MAGIC 0x20205a4c
breaking endianness again for GLZ code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
It's only called once, and when it's called, we will have dereferenced
worker->display_channel a few lines before in
display_channel_set_monitors_config_to_primary(), so this cannot be
NULL. The 'if (worker->display_channel)' check can thus be removed, so
display_is_connected() becomes just red_channel_is_connected().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
There's already a 'display' variable equal to worker->display_channel
which is not consistently used. This commit also adds a new 'channel'
local variable to limit the number of upcasts to RedChannel.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is reported by GCC 8.0.1 (Fedora 28).
Instead of doing a possible invalid cast destroy and create the
queue again.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Avoid casting function pointer with different argument providing
a proper utility instead.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
With GCC 8.0.1 (Fedora 28), cast to different function pointer
can lead to warnings, like:
event-loop.c: In function ‘watch_update_mask’:
event-loop.c:146:42: error: cast between incompatible function types from ‘gboolean (*)(GIOChannel *, GIOCondition, void *)’ {aka ‘int (*)(struct _GIOChannel *, enum <anonymous>, void *)’} to ‘gboolean (*)(void *)’ {aka ‘int (*)(void *)’} [-Werror=cast-function-type]
g_source_set_callback(watch->source, (GSourceFunc)watch_func, watch, NULL);
^
cc1: all warnings being treated as errors
As g_source_set_callback expect a function pointer which type
changes based on the type of source (so is expected) silent
the possible warning.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
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>
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>
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>
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>
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>
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>
If messages are sent together by the agent the device is reading
only part of the data. This cause Qemu to not poll for new data and
stream_device_read_msg_from_dev won't be called again.
This can cause a stall. To avoid this continue handling data
after a full message was processed.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
TLS 1.0 is considered now insecure.
TLS 1.1 was introduced in 2006.
Our SPICE clients uses OpenSSL to use TLS and the support for TLS 1.1
in OpenSSL was introduced in 2006 too so even in systems like
Windows XP which are not officially supporting TLS 1.0 will work
with SPICE and TLS 1.1.
This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1521053.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Code in rgb32_data_has_alpha possibly generate this warning using
clang:
utils.c:35:16: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Werror,-Wcast-align]
line = (uint32_t *)data;
^~~~~~~~~~~~~~~~
Although the images are expected to be all aligned in this respect
use byte access on the data instead. This, beside fixing the alignment
issue also avoid problem with big endian machines (images in SPICE are
expected to have the alpha channel as the forth byte).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
clang reports lot of warnings like:
spicevmc.c:47:1: error: unused function 'RED_CHAR_DEVICE_SPICEVMC_CLASS' [-Werror,-Wunused-function]
SPICE_DECLARE_TYPE(RedCharDeviceSpiceVmc, red_char_device_spicevmc, CHAR_DEVICE_SPICEVMC);
^
./red-common.h:110:43: note: expanded from macro 'SPICE_DECLARE_TYPE'
static inline ModuleObjName ## Class *G_PASTE(G_PASTE(RED_,OBJ_NAME),_CLASS)(void *klass) \
^
They are all static inline function and usually should not generate
warnings but for some reasons they do.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
clang is reporting:
sound.c:292:16: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Werror,-Wcast-align]
data = (uint32_t *)packet->data;
^~~~~~~~~~~~~~~~~~~~~~~~
however we are using memcpy to access "data" pointer so there's no
need to use uint32_t pointer. Also considering we don't do math with
that pointer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
clang reports may warnings like:
test-display-base.c:252:11: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Werror,-Wcast-align]
dst = (uint32_t *)bitmap;
^~~~~~~~~~~~~~~~~~
Use SPICE_ALIGNED_CAST/SPICE_UNALIGNED_CAST macros in common/mem.h to
mark the cast safe or possibly unsafe.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Not really possible but clang raise these warnings:
test-sasl.c:555:13: error: variable 'is_ok' is uninitialized when used here [-Werror,-Wuninitialized]
if (is_ok) {
^~~~~
test-sasl.c:553:22: note: initialize the variable 'is_ok' to silence this warning
uint8_t is_ok;
^
= '\0'
test-gst.c:792:18: error: variable 'height' is used uninitialized whenever '&&' condition is false [-Werror,-Wsometimes-uninitialized]
spice_assert(gst_structure_get_int(s, "width", &width) &&
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../spice-common/common/log.h:91:17: note: expanded from macro 'spice_assert'
if G_LIKELY(x) { } else { \
^
/usr/include/glib-2.0/glib/gmacros.h:376:60: note: expanded from macro 'G_LIKELY'
^~~~
/usr/include/glib-2.0/glib/gmacros.h:370:8: note: expanded from macro '_G_BOOLEAN_EXPR'
if (expr) \
^~~~
test-gst.c:799:17: note: uninitialized use occurs here
bitmap->y = height;
^~~~~~
test-gst.c:792:18: note: remove the '&&' if its condition is always true
spice_assert(gst_structure_get_int(s, "width", &width) &&
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../spice-common/common/log.h:91:17: note: expanded from macro 'spice_assert'
if G_LIKELY(x) { } else { \
^
/usr/include/glib-2.0/glib/gmacros.h:376:60: note: expanded from macro 'G_LIKELY'
^
/usr/include/glib-2.0/glib/gmacros.h:370:8: note: expanded from macro '_G_BOOLEAN_EXPR'
if (expr) \
^
test-gst.c:791:23: note: initialize the variable 'height' to silence this warning
gint width, height;
^
= 0
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Marked as obsolete with clang and some options is detected as
error.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This causes some warnings with clang:
char-device.c:898:29: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Werror,-Wcast-align]
write_to_dev_size_ptr = (uint32_t *)spice_marshaller_reserve_space(m, sizeof(uint32_t));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
char-device.c:899:31: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Werror,-Wcast-align]
write_to_dev_tokens_ptr = (uint32_t *)spice_marshaller_reserve_space(m, sizeof(uint32_t));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This also fixes some minor endianness issue (on big endian machine
integers were not properly encoded).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This give an hint to client which can optimise rendering.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe de Dinechin <dinechin@redhat.com>
Currently red_stream_async_read cannot handle read of 0 bytes.
This would cause a wrong assert in async_read_handler.
Fixing the assert would just make the code wrongly detect a
disconnection (usually a return of 0 from read is handled that
way but happens also if you try to read 0 bytes).
Current callers of these function does not pass 0 as size however
handling data protocols having data_length+data this can happen
and is handled manually in red_sasl_handle_auth_steplen.
Avoid needing manually to check for this condition.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe de Dinechin <dinechin@redhat.com>
After 497b8042dc
("lz4-encoder: Use GUINT32_TO_BE instead of htonl") patch this header
is not needed.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This causes some warnings with clang:
dcc-send.c:1799:28: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Werror,-Wcast-align]
num_surfaces_created = (uint32_t *)spice_marshaller_reserve_space(m2, sizeof(uint32_t));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This also fixes some endianness issue (on big endian machine integers
were not properly encoded).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Just a style change, almost of the code use similar macros for such
tasks.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
There's no reason to change data passed, the function just check
the alpha channel of the image.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This call sequence is included in test-display-base used in different
tests, no reason to have this test.
Also this test is not actually used for automated tests.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
AsyncRead is always included in RedStream and there are only
a possible operation pending on a RedStream.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The same function is used to reset writev field in SASL code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Some additional header are needed to avoid undefined types.
SOL_TCP and IPPROTO_TCP have the same value in Linux but SOL_TCP
is not defined in FreeBSD.
Provide pthread_setname_np using pthread_set_name_np (same parameters).
Patch is based on a patch from Oleg Ginzburg <olevole@olevole.ru>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This issue caused the glitches using the rectangular selection
tool in PaintShop 6.
The line was removed accidentally by "red_parse_qxl: fix throwing
away drawables that have masks" (812b65984d)
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pavelgrunt@gmail.com>
Marked as obsolete with clang and some options is detected as
error.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Instead of having half state in RedSASL and half in RedSASLAuth
move everything in RedSASLAuth. This also reduces memory usage
when we are using SASL but we finish the authentication step.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Most of these function are identical.
Only difference were basically debugging message but now
with a proper tests are less important.
The mechname field is used to differentiate between first step and
following ones.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
All SPICE protocol is little endian, there's no agreement on other
endian and currently we support only little endian so make sure
this will work even possibly running on a big endian machine.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Asynchronous code jumping from a file to another is tedious to read
also having code handling the same stuff in two files does not look
a good design.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The server on failure can just disconnect the client or report the
error. The error report can be done using new protocol 2 or just
a number (like protocol 1).
Detect the failure report to make possible to check it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Use some flags to specify which behaviour to change and different test
cases to test them.
Some cases specify when client stop sending data at different steps of
the process.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Try different connections with different tricky names.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Create a thread that emulates a client and starts SASL authentication
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Check some functions are called in a given sequence.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Not currently working, is defining SASL functions used by the code.
As the symbols defined in the objects have more priority than the ones
defined by the libraries these function take precedence compared to
system library.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These cast causes warnings if a 32 bit target is used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Network fields should be encoded as little endian.
This was discovered using an emulated MIPS machine.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Clang's warning on absolute-value.
> red-record-qxl.c:297:39: error: taking the absolute value of unsigned
> type 'uint32_t' (aka 'unsigned int') has no effect
> bitmap_size = qxl->bitmap.y * abs(qxl->bitmap.stride);
> ^
> red-record-qxl.c:297:39: note: remove the call to 'abs' since unsigned
> values cannot be negative
> bitmap_size = qxl->bitmap.y * abs(qxl->bitmap.stride);
> ^~~
> red-replay-qxl.c:471:39: error: taking the absolute value of unsigned type
> 'uint32_t' (aka 'unsigned int') has no effect
> bitmap_size = qxl->bitmap.y * abs(qxl->bitmap.stride);
> ^
> red-replay-qxl.c:471:39: note: remove the call to 'abs' since unsigned
> values cannot be negative
> bitmap_size = qxl->bitmap.y * abs(qxl->bitmap.stride);
> ^~~
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Qemu never used more than this number and today surfaces are not
much used so there's no reason to keep this limit so high.
This reduces quite a lot some internal structure
(DisplayChannelPrivate and DisplayChannelClientPrivate).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
These functions do not set errno so it is possible that errno has a
stale value which happens to be EAGAIN.
This would cause an infinite loop in functions like red_stream_write_all
(or potentially using the event loop).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Avoid over complicated matching using quoting and a simple strstr
operation.
The mech names are separated and quoted with the same chararacter (',')
making possible to search for ",MECHNAME," instead of manually check for
prefix and suffix after the search for "MECHNAME".
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
In case mcc->priv->initial_channels_list_sent is false we didn't
marshall any message so we should not call
red_channel_client_begin_send_message.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
There's no reason to copy mechname into mechlist to use mechlist
instead of mechname.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedLinkInfo stores reds in it no need to pass every time.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This avoids to expose some detail about the channel.
Like other APIs implement it move close to the part that handle
it instead of have everything in reds.c.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Coverity complaint that this field should be protected by
a mutex as other accesses are with the mutex locked.
Use atomic operation. Not in an hot path in any case.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
We need to free the connection if the mechanism name is wrong
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This structure is used to send a message related to streams.
There are already other items defined in video-stream.h so
move the declaration.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Store information directly in the RedStreamActivateReportItem
making easier to marshall the message.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Instead of just allocating in video_stream_clip_item_new and
than have to setup properly in dcc_video_stream_agent_clip
do all in video_stream_clip_item_new which is more consistent
with other part of the code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Make sure client_can_celt is passed before client_can_opus
The bug was present since introduction of opus in ce9b714137
Found by Coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Coverity complains the field is not initialized.
That's true but man recvmsg specifies that this
field is set by recvmsg.
To make coverity happy, initialize this field.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
SSL_OP_NO_COMPRESSION was introduced in OpenSSL_0_9_8k, which is no
longer supported. This commit raises the minimum OpenSSL version to
1.0.0, which is also out of support.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Nothing seems to be using openssl in red-worker.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
There are a few places which use $(top_srcdir) when $(srcdir) would be
equally valid.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
At the moment, changing spice-server.syms to add/remove a new symbol to
be exported does not regenerate spice-server.so. This commit added the
needed dependency for this to work.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Reuse already computed value and avoid to compute in every
iterations.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Originally this pool was used to avoid allocation/deallocations.
However the introduction of GList cause the code to do dynamic
allocations in order to update the list making this pooling
something useless.
The buffers limitation is now implemented with a simple counter.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
There's no reason to handle this message in a different
way in MainChannel and InputsChannel, the default handling
will return true in any case.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Maybe this check should just be removed?
RedVDIReadBuf::data is a static allocated buffer so checking for
NULL on it is useless. It would be NULL only if RedVDIReadBuf
pointer would be the opposite, in value, of the offset of
data field into it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
If there is a channel client there's surely a related channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Be a bit more defensive about handling incoming messages from the stream
device. This also makes these functions consistent with
handle_msg_format(). These assertions are only enabled if
ENABLE_EXTRA_CHECKS is defined.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Potentially a channel can run with a different core interface
than the global one attached to RedsState so instead of calling
reds_core_* functions use the code interface attached to the
channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
verify guarantee that the condition is always a compile
time constant.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This header is mainly exporting functions to handle public
interface for the QXL devices.
Avoid spreading its inclusion including this header in other
headers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The name is more consistent with the value of the flag and
the function red_channel_client_wait_pipe_item_sent where
the MarkerPipeItem structure is used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Avoid confusion with RedStream which is a totally unrelated object.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Just to avoid confusion between different uses of the word Stream (e.g.
RedStream) clarify that it's related to video streams
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
To prevent confusion between Stream (a video stream) and RedStream (a
generic data stream between client and server), change the name to
VideoStream to be more explicit about what it is.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Currently, red_channel_client_wait_pipe_item_sent() inserts a MarkerItem
which will sent after the item we want to wait for: the tail of the
queue is the first item to send, and the function uses
red_channel_client_pipe_add_after_pos(). Then, if the marker has been
successfully sent, the function calls
red_channel_client_wait_outgoing_item to wait for 'item' to be sent.
Instead of doing this, we can add the MarkerItem to the queue so that
it's sent after 'item' (ie, insert it _before_ 'item' in the queue).
This way, when the marker is marked as having been sent, we'll also know
that 'item' has been sent.
This avoids having to call red_channel_client_wait_outgoing_item and
possibly the case where the item was not queued and
red_channel_client_wait_outgoing_item returning TRUE even if the item
was not sent as required.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Avoid repeating the same code twice.
red_channel_client_send sends the pending item (or a part of it). If
there are no item pending, the function does nothing (so checking for
blocked channel is useless). Also red_channel_client_send is already
called from red_channel_client_push which has a check for blocked
channels, so having calls to both red_channel_client_send() and
red_channel_client_push() is redundant.
The function on its overall tries to wait for a given item to be sent.
The call for red_channel_client_receive is mainly needed to support the
cases were to send data messages from the client should be processed
(like if "handle-acks" is requested).
Moving the loop iteration check inside the for loop instead allows to
avoid some duplication.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Instead of having MarkerPipeItem pointing to an external variable with
the possibility to forget to reset it and have a dangling pointer, this
commit takes a reference on the item to keep it alive after it was sent.
This item is placed into the queue to understand when it was sent. The
current implementation detects the unqueue when the item is destroyed so
we currently store a pointer to an external variable in the item, this
way we can use a variable which will still be alive after the item is
released/destroyed.
This change updates the variable (stored in the item) when we try to
send the item, rather than at destruction time. The destruction happened
at the end of red_channel_client_send_item(), so we don't mark
item_in_pipe much earlier than before.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This is currently unlikely to happen since we communicate over a pipe
and the pipe buffer is sufficiently large to avoid splitting the
message. But for completeness, we should handle this scenario.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
put red_stream_disable_writev in an #ifdef HAVE_SASL block.
red_stream_disable_writev is only called from functions
that are already in an #ifdef HAVE_SASL block.
Currently when building with SASL disabled, I get:
CC red-stream.lo
red-stream.c:441:13: error: 'red_stream_disable_writev'
defined but not used [-Werror=unused-function]
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
All other inputs_channel_set_* functions do not have this
parameter and get it from the channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Current code does not free allocated tablet resources.
When a tablet is added some resources are allocated.
Resources should be released either removing the tablet or
freeing spice server object.
Added a test to check these conditions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Currently create device, open it and pass some messages checking
they are handled.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
common/mem.h contains mainly memory allocation functions.
As we decided to move to Glib calls directly avoid to include
function declaration we should not use anymore.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This prevent future problems supporting new channels.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This reduce the attack surface moving some data into read-only
sections.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Missing some names cause some debugging messages to be
generated and some of our tests to fail.
This patch was written by Christophe Fergeau.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The code for reading a StreamDevice message from the streaming agent has
code to handle a situation where you only read a part of the header. If
we've read only a part of the header, we will try to read the remaining
n bytes of the header within a loop until the full header is read.
However, when we try to read the last n bytes, we store it at beginning
of the header struct, which will overwrite the first part of the header.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Include protocol header file defining StreamMsgFormat which is used in a
function signature in this header.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: Frediano Ziglio <fziglio@redhat.com>
This commit adds red_channel_{debug,warning,printerr}() helpers which
will prepend the log message with "channel-name:id (%p)". It also
changes various locations which were doing this manually.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
spice_server_set_channel_security() is already mostly doing that. We can
make its code more generic, and introduce a red_channel_get_name()
method. This method will then be used to make debug messages more
readable by showing the actual channel name rather than its type as
an int.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The objects RedsStream and RedsSASL are currently using the namespace
"Reds" rather than the standard "Red" namespace used throughout the rest
of the project. Change these to be consistent. This also means changing
method names and some related enumeration types.
The files were also renamed to reflect the change:
reds-stream.[ch] -> red-stream.[ch]
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
There are already other debugging code showing channel closure.
Not closed file descriptors can easily be detected with other
tools (like netstat or /proc file system).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
In some cases mouse_mode is a bit field.
However for this structure is used always as a boolean
value.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This field was used just to store a value and retrieve again
while we can just pass it instead.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Setting the capability is not enough, each stream must be enabled
so do so if client support them.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Do not allow the guest to fill host memory.
Also having a huge queue mainly cause to have a higher video
latency.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This allows a better id allocation as devices are created after
fixed ones.
Also will allow to support more easily multiple monitor.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When guest close the device the host device has to be reset too.
This make easier to restart the guest device which can happen in case
of reboot, agent issues or if we want to update the agent.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Currently, red_char_device_reset() stops the device, clears all pending
messages, and clears its device instance. After this function is called,
the char device will not work again until it is assigned a new device
instance and restarted. This is fine for the vdagent char device, which
is currently the only user of this function. But for the stream device,
we want to be able to reset the char device to a working state (e.g.
clear all pending messages, etc) without stopping or disabling the char
device. So this function will now only reset the char device to a clean
working state, and the _stop() and _reset_dev_instance() calls will be
moved up to the caller.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Start showing something when we have a surface and stream
instead of showing a blank screen which is now not useful.
Was useful for debugging purposes to understand that the
new channel was sending messages correctly to client and
client could handle them.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When a new client is connected we must restart the stream so new
clients can receive correct data without having to wait for the
next full screen (which on idle screen could take ages).
On disconnection we should tell the guest to stop streaming
not wasting resources to stream not needed data.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The channel needs to communicate when it receive a new
stream request from the guest.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Remove the fixed size stream and support any display size.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Handle stream data from device sending to the channel.
The StreamChannel will forward the data to the clients using standard
DisplayChannel messages, and will create and destroy streams as
necessary.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
So can be used by the device to communicate with the clients.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Currently only compile, not used and not much sense
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Parse the data sent from the guest to the streaming device.
At the moment, the data is simply discarded after it is parsed.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Add a stub device in guest.
The aim of this device is to make it possible for the guest to send a
stream through a DisplayChannel (in the sense of protocol channel).
This stub allows the guest to send some data and you can see some debug
lines of data arrived on host logs.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This allows the server to add channels after the client is connected.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This function does not make much sense anymore.
Is called by RedVmcChannel which doesn't use RedChannelClient ACKs
so the variable changed are not used.
Moreover, at red_vmc_channel_constructed() time, there will be no
clients yet, so red_channel_init_outgoing_messages() will be a no-op.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Prevent possible buffer reading overflow.
Note that message pointer must be valid and data are checked
value by value so even on overflow you just get an error.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Since commit ef4b1bdb "red-channel-client: Prevent too tight loop
waiting for ACKs", after seamless migration, the display is no longer
updated on the client-side, even though the VM is functional (responds
to keyboard input, reconnecting the client restores the display
functionality, ...).
This is mainly caused because after migration,
red_channel_client_waiting_for_ack() will be true until
red_channel_client_ack_zero_messages_window() is called.
What happens is the following:
The dcc is created, and dcc_start() pushes a RED_PIPE_ITEM_TYPE_SET_ACK
message.
This calls prepare_pipe_add(), which will enable write event on the dcc
watch. red_channel_client_event() will be called, which will trigger a
red_channel_client_push(). Since red_channel_client_waiting_for_ack()
returns true, we won't get any item to push, and (because of commit
ef4b1bdb), we will disable write notifications on the watch.
At this point, rcc->priv->pipe is no longer empty, so prepare_pipe_add()
is not going to reenable the write notifications.
Then red_channel_client_ack_zero_messages_window() is finally called as
part of dcc_handle_migrate_data(), so from this point on,
red_channel_client_waiting_for_ack() is no longer true.
However, nothing ever reenables WRITE events, nor empties
rcc->priv->pipe, so nothing ever gets pushed, causing no display updates
at all after a migration, even if the VM is functional (input, ...)
apart from that.
This commit reenables WRITE events in
red_channel_client_ack_zero_messages_window() if we were waiting for
ack.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Since 2.8, QEMU no longer creates QXL primary surfaces when using GL.
This change broke client-side mouse mode, because Spice server relies on
having a primary surface.
When GL is enabled, use GL scanout informations.
Mouse mode is always client when GL surfaces are used.
This patch and most of the message are based on a patch from
Marc-André Lureau, just moving responsibility from reds to RedQxl.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe de Dinechin <dinechin@redhat.com>
Currently, the port used by most tests is hardcoded to 5912. However,
the test suite can be run in parallel, so if 2 tests run in parallel,
the 2nd one is not going to be able to bind to port 5912 and will fail.
After this commit, test_new() will try to find a free port between 5912
and 5922 and will abort if it can't find any.
The issue can be reproduced by adding a usleep(1000000) to the beginning
of test_destroy().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Start reducing the usage of spice_new*/spice_malloc allocations.
They were designed in a similar way to GLib ones.
Now that we use GLib make sense to remove them.
However the versions we support for GLib can use different memory
allocators so we have to match g_free with GLib allocations
and spice_* ones (which uses always malloc allocator) with free().
This patch remove some easy ones.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The macro will implement most of the boilerplate needed to declare an
object.
Its usage is similar to GLib G_DECLARE_*_TYPE macros.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedChannelClient has a "handle-acks" feature.
If this feature is enabled, after the configured number of messages it
waits for an ACK from the client.
If is waiting for an ACK it stops sending messages.
However the write notification was not disabled, causing the loop event
to always trigger, as the socket in this case is ready to accept data.
Specifically red_channel_client_event is continuously called.
This is noticeable using slow network environments and having
some additional loop instrumentation.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This structure was used to store the cookie for the async
reply and the message for the generic async callback.
Most async messages do not require extra action beside sending back the
cookie for the reply so instead of having a switch on the message type
in red_qxl_async_complete, this commit moves the message-specific
behaviour to the callers, which allows us to store the cookie directly
in RedWorkerMessageAsync rather than needing an intermediate
AsyncCommand structure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Now the push is done automatically when a PipeItem is added
(cfr commit 5c460de1a3
"worker: push data when clients can receive them"),
forcing a push cause only network fragmentation and is required only if
you are handling data in a polling loop (and thus, you are preventing
the default event loop from running).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
It was using g_list_remove_link() to remove an element from the
RedChannel::clients list while it really meant to be using
g_list_delete_link() which frees the memory associated with the link.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
All main test module have this test-XXXX.c naming, make
test-stat coherent.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Put non-trivial programs in separate sections, which makes it easier to
understand the relationship between macros.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
New automake test harness produce *.log and *.trs files for
each test.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This macro is exactly doing what RING_FOREACH just passing streams
ring.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe de Dinechin <cdupontd@redhat.com>
There is exactly one user in RedChannel, and this can be reimplemented
using already public RedChannelClient API. No need for an extra
function very specialized function with a not great name.
This commit thus removes one method from RedChannelClient public API,
and replaces it with an equivalent private helper in RedChannel.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
red_channel_disconnect_if_pending_send() and red_channel_wait_all_sent() are
always called together, we can remove one of the 2 methods.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In case GLib don't provide these functions we use replacements so
there's no need to have a warning if these functions are called.
This potentially capture other compatibility issues in the tests
that would be ignored having all deprecation warnings disabled.
Tested with GLib 2.28 and 2.52.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
When the worker is started it could take a while to start processing
commands.
The reason is that the dispatcher handler is called after the worker
so GLib will receive a FALSE answer to both prepare and check
callbacks of the RedWorkerSource causing GLib to wait till another
event is received.
This is a regression since the introduction of GLib event loop, before
the command processing was always attempted after any events.
Commands (from QXL interface for cursor and display) are processed
during the RedWorkerSource dispatch so if they are not processed just
when the VM is started they will be processed on next event which
could be from dispatcher (main thread requests), from existing
connections or from pending timers. However in the case there are no
clients connected and no other requests from main thread the worker
thread won't process them.
Setting the event_timeout to 0 cause the prepare callback to return
TRUE so GLib will dispatch the RedWorkerSource.
This was discovered attempting to use the tests in server/tests
directory to reproduce a leak in RedWorker.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Pipelines are never freed.
These are detected as leaks by leak detector tools like address sanitizer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Command line options are not freed at the end of the program.
These are detected as leaks by leak detector tools like address sanitizer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
encoder_name is never NULL as already initialized with "mjpeg" value.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If a client is unable to complete the TLS handshake phase
reds_init_client_ssl_connection leaked some memory as the stream is not
correctly freed.
This also causes the stream to send the SPICE_CHANNEL_EVENT_DISCONNECTED
event. Otherwise only SPICE_CHANNEL_EVENT_CONNECTED was sent.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Currently is possible to trigger a leak by passing an invalid
connection.
This can happen if the client opens a connection and then closes it
without writing or reading any data.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The code tests for the presence of RedChannelClient::stream while
initializing RedChannelClient.
However, the check was done too late, and a
RedChannelClient::config_socket implementation (for example
snd_channel_client_config_socket) could have tried to use it before the
check that it's not NULL.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This potentially can also save the copy if there is enough
space to resize the buffer in place.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This field was used only by RedChannelClient to mark when the socket
was shutdown. This condition can simply be tested by RedChannelClient
checking if there's a watch as is the only condition (beside object
destroying/disconnecting) where the watch is removed.
In any case the shutdown was used to understand if there were possible
data still to read.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedQxl and RedWorker are quite bound together running
CursorChannel and DisplayChannel in a separate thread
marshalling (RedQxl) and unmarshalling and executing
(RedWorker) requests.
Make the communication between them private trying
to facilitate maintaining these two files.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
For dispatcher_register_handler(), use 'false' instead of 0 since the
last argument is a bool type now.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
This callback was only executed for message types that were registered
with DISPATCHER_ASYNC ack type. However, the async_done handler was
called immediately after the message-specific handler and was called in
the same thread, so the async_done stuff can just as easily be done from
within the message-specific handler. This allows to simplify the
dispatcher_register_handler() method to simply require a boolean
argument for whether the message type requires an ACK or not.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Pass proper type to callback to avoid having to convert to
the right type for each call.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Is supposed to be used during initialization but is never
used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
AsyncCommand is used to handle asynchronous messages from the
dispatcher.
GL_DRAW_ASYNC is mainly using it to store the cookie.
The value of GL_DRAW_COOKIE_INVALID was choosen to allow implementing
cookies (which basically are handles) either using indexes (where 0 is
valid) or pointers (where 0 is invalid). Currently Qemu uses pointers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These 2 functions were doing the same stuff, calling
client_monitors_config callback in QXLInterface.
The only difference was that red_qxl_use_client_monitors_config
used a NULL value.
Added the check for proper version, QXLInstance before 3.3
did not have this callback.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
You could easily trigger this issue using multiple monitors and a
modified spice-gtk client with this patch:
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1699,6 +1699,7 @@ static gboolean _channel_new(channel_new_t *c)
{
g_return_val_if_fail(c != NULL, FALSE);
+ if (c->type == SPICE_CHANNEL_DISPLAY) c->id = 0;
spice_channel_new(c->session, c->type, c->id);
g_object_unref(c->session);
which cause a crash like
(process:28742): Spice-WARNING **: Failed to create channel client: Client 0x40246f5d0: duplicate channel type 2 id 0
2017-08-24 09:36:57.451+0000: shutting down, reason=crashed
RedChannelClient is an GInitable type, which means that the object is
constructed, and then the _init() function is called, which can fail.
If the _init() fails, the newly-created object will be destroyed. As
part of _init(), we add a new watch for the stream using the core
interface that is associated with the channel. After adding the watch,
our rcc creation fails (due to duplicate ID), and the rcc object is
unreffed. This results in a call to reds_stream_free() (since the rcc
now owns the stream). But in reds_stream_free, we were trying to remove
the watch from the core interface associated with the RedsState. For
most channels, these two core interfaces are equivalent. But for the
Display and Cursor channels, it is the core Glib-based interface
associated with the RedWorker.
The watch in RedsStream by default is bound to the Qemu provided
SpiceCoreInterface while RedChannelClient bound it to Glib one causing
the crash when the watch is deleted from RedsStream. Change the bound
interface.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When a stream is moved from the main thread to a
secondary one the events are potentially registered
using a different core interface. This cause memory
corruption accessing the watch registered in RedsStream.
This patch allows to always use the right interface.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This test runs 2 spice server in one program.
Use two different tcp port to be able to connect to both servers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Add some check that something happened during creation/destruction.
Set as running on "make check".
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Update tests names.
Remove tetris comments, never available and not planned.
Update some notes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
There's no need to not compile this feature, it just enable
a parameters which must be passed in order to change test
behaviour.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This allows to end the loop to end some tests.
Currently different tests enter the loop but never exit from
them.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
In C the sizeof(long) can be different than sizeof(void*),
use proper type.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Do not use calloc and malloc directly without checking
the result. Use instead spice functions to get a nice
error in case of allocation failures.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
For some reasons (documented in cursor_init) the function
uses 128 extra bytes of data causing a reading buffer overflow.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Timers in spice server are supposed to be used in a single thread
context. To avoid problems, protect the usage from multiple
thread with a mutex.
This sometimes caused a crash.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The wakeup timer is used by the worker thread and by the
main thread.
Destroying the object before destroying the worker thread
can lead to use after free.
Destroying the worker thread first makes sure we don't race.
This is detected easily when compiling the test with address sanitizer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
As the indexes are used to compute the index inside an array
using modulo operation when a signed value overflows, the
modulo becames negative, causing a buffer underflow.
Unlikely to happens (take lot of time) but is safer that way.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Cursor resources (basically the shape of it) was retained till
it was used however it was copied so there were no reason to not release
this resource.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
You could easily trigger this issue using multiple monitors and
a modified spice-gtk client with this patch:
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1699,6 +1699,7 @@ static gboolean _channel_new(channel_new_t *c)
{
g_return_val_if_fail(c != NULL, FALSE);
+ if (c->type == SPICE_CHANNEL_DISPLAY) c->id = 0;
spice_channel_new(c->session, c->type, c->id);
g_object_unref(c->session);
This as g_initable_new in this case returns NULL (dcc.c).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Although dispatcher_send_message() does not allow you to send a message
type that is invalid for a dispatcher, it still makes sense to verify
that the type is valid in the receiver. This should only be possible in
the case of some severe problem such as memory corruption, so if it is
invalid, we simply abort.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Is possible to have a leak processing update commands if
the update command is synchronous and the rectangle list
is empty. Note that Qemu always pass an empty list.
If the list is empty display_channel_update fill the list.
This is used to send back the list in case of asynchronous
requests. But in handle_dev_update_async (the callback that
handle the asynchronous case) the list is correctly freed.
This was discovered by accident looking at the code.
Reproduced with a Windows recording file using GCC address
sanitizer and this patch to spice-server-replay:
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -1280,7 +1280,13 @@ static void replay_handle_dev_input(QXLWorker *worker, SpiceReplay *replay,
replay->created_primary = FALSE;
worker->destroy_surfaces(worker);
break;
- case RED_WORKER_MESSAGE_UPDATE:
+ case RED_WORKER_MESSAGE_UPDATE: {
+ static uint8_t count = 0;
+ QXLRect dummy;
+ QXLRect update = { 0, 0, 100, 100 };
+ count ^= 1;
+ worker->update_area(worker, 0, &update, count ? &dummy : NULL, count ? 1 : 0, 0);
+ } break;
// XXX do anything? we record the correct bitmaps already.
case RED_WORKER_MESSAGE_DISPLAY_CONNECT:
// we want to ignore this one - it is sent on client connection, we
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedPipeItem already implements reference counting so
this avoid duplicating code to handle a object with reference
counting that points to another object with reference counting
that holds a RedCursorCmd.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Now the push is done automatically when a PipeItem is added
(cfr commit 5c460de1a3
"worker: push data when clients can receive them"),
forcing a push cause only network fragmentation and is required
only if you are handling data in a loop instead of using the
default loop.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Implements red_channel_pipes_add_type and
red_channel_pipes_add_empty_msg using red_channel_pipes_add.
This avoid duplicating items for each client.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Considering that now RedPipeItem have reference counting
and that lot of items are just used to store constant
data to send, using reference counting instead of creating
different items for each client is easier to do.
So this new red_channel_pipes_add allows to add a single item
to all clients.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Now the push is done automatically when a PipeItem is added
(cfr commit 5c460de1a3
"worker: push data when clients can receive them"),
forcing a push cause only network fragmentation and is required only if
you are handling data in a polling loop (and thus, you are preventing
the default event loop from running).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
display-channel.h contains lots of information used by different
DisplayChannel components.
In the past all RedWorker, CursorChannel and DisplayChannel code was in
a single file. Since lots of code to handle DisplayChannel is still in
RedWorker, display-channel.h contains a lot of declarations so that they
can be accessed from RedWorker.
Moving declarations that are not needed by RedWorker and other external
class components helps to reduce dependencies between RedWorker and
DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This patch allocates VMC IDs by finding the first ID not used
instead of using a global variable and incrementing the value
for each channel created.
This solves some potential issues:
- remove the global state potentially making possible
to use multiple SpiceServer on the same process;
- don't potentially overflow the variable. This can happen if
channels are allocated/deallocated multiple times
(currently not done by Qemu).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This can happen as the connection is asynchronous so (MT main thread,
CT channel thread):
- MT you get a new connection;
- MT a connection is sent to CT;
- MT you get a disconnection of main channel;
- MT red_client_destroy is called;
- CT you attempt to add the RCC to RedClient.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
A RedChannelClient is always attached to a valid RedChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Name will be visible in debugger and /proc filesystem
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The message is asynchronous so to avoid the object to potentially
been released before being processed keep a strong reference to
it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
red_disconnect_display() is duplicating what red_channel_disconnect()
already does, so red_disconnect_display() and red_disconnect_cursor()
are actually identical code-wise. We can directly call
red_channel_disconnect() from flush_commands() rather than passing a
'red_disconnect_t disconnect' argument to that function.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
cursor_channel_disconnect() calls
cursor_channel_client_reset_cursor_cache() on all CursorChannelClient
associated with the current CursorChannel before calling
red_channel_disconnect().
red_channel_disconnect() will iterate over all CursorChannelClient
calling red_channel_client_disconnect(), which will eventually call
CursorChannelClient::on_disconnect. This will in turn
cursor_channel_client_reset_cursor_cache(), so calling it in
cursor_channel_disconnect() before calling red_channel_disconnect() is
redundant.
cursor_channel_disconnect() can thus be replaced by a direct call to
red_channel_disconnect().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
SoundChannelClient has a stub implementation of
RedChannelClient::on_disconnect(), this commit removes the need for it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This vfunc only has a RedChannelClient * argument, and most of the time,
it operates on RedChannelClient, not on RedChannel. Moreover, the only
time it's used is from RedChannelClient. This commit moves the vfunc to
RedChannelClient, which seems like a better fit for it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
ORC library is used internally by GStreamer to generate code
dynamically.
If ORC cannot allocate executable memory, the failure causes
an abort(3) to be called.
This happens on some SELinux configurations that disable executable
memory allocation (execmem boolean).
Check that ORC could work before attempting to use GStreamer to
avoid crashes.
While this check is done, the ORC library outputs an error which will
be well visible in Qemu output.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If you are testing for NULL data this means that variable could be
NULL so avoid to access before the check to make sure the check is hit.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The multimedia time is defined as uint32_t.
Use the proper type instead of int.
Currently no arithmetic is done on this value but
just copies so considering that on the architectures
we support sizeof(int) == sizeof(uint32_t) there's
no change in the resulting machine code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
A RedClient can be freed from the main thread following a main channel
disconnection (reds_client_disconnect). This can happen while another
thread is allocating a new channel client for that client.
To prevent the usage of a pointer which can be invalid
take ownership of the pointer.
Note that we don't need this when disconnecting as disconnection is
done synchronously (the dispatch messages are registered with
DISPATCH_ACK).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Trace the number of loops done processing display commands
and the number of loops in which the queue was full.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
If a DisplayChannelClient cannot be instantiated capabilities
are not released correctly.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
display variable already contains the DCC_TO_DC(dcc) value so
reuse it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Some RedChannelClient data members were marked as int when they only
hold booleans.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
reds_get_n_clients is a single line and is used only by
spice_server_get_num_clients.
The 2 functions have very similar names so inlining
reds_get_n_clients does not make code less readable.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The leak detector we use currently is not enough to detect
some kind of leak in DisplayChannel so manually test.
These tests are enabled only when --enable-extra-checks is passed
to configure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Mostly of red_channel_destroy calls were preceded by
a call to unregister the channel.
The only exception was the main channel as this channel is
always present and its initialisation is a bit different.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
RedWorker should not handle directly to client but
defer the job to DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Make easier to understant the value to use in the code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
From spice_server_port_event API you can send port events to
any char device. Although currently this is used only for "port"
devices implemented in spicevmc.c this will allow to support
such events using different objects.
This will be used for instance for a streaming device which
will be a specific SpicePort implementation.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When a RedCursorCmd is passed to cursor_channel_process_cmd(), it
constructs a new CursorItem which takes ownership of that command. If
the cursor_cmd->type falls through to the default case of the switch
statement, we will print a warning and return without freeing the
CursorItem (and thus the RedCursorCmd).
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Now the push is done automatically when a PipeItem is added
(cfr commit 5c460de1a3
"worker: push data when clients can receive them"),
forcing a push cause only network fragmentation and is required
only if you are handling data in a loop instead of using the
default loop.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Under error: 'link' fields are being accessed, so it's
wrong to goto error with link == NULL.
Instead, return immediately.
Found by coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
If qxl->descriptor.type is QUIC, red_replay_data_chunks_free
frees qxl (data), so no need to free it again at the bottom
of the function.
Found by coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
It was also possible for a malicious client to set
VDAgentMonitorsConfig::num_of_monitors to a number larger
than the actual size of VDAgentMOnitorsConfig::monitors.
This would lead to buffer overflows, which could allow the guest to
read part of the host memory. This might cause write overflows in the
host as well, but controlling the content of such buffers seems
complicated.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Total message size received from the client was unlimited. There is
a 2kiB size check on individual agent messages, but the MonitorsConfig
message can be split in multiple chunks, and the size of the
non-chunked MonitorsConfig message was never checked. This could easily
lead to memory exhaustion on the host.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
To help avoid stream.c and dcc.c to access display-channel private
structure to get the nth Stream structure pointer.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
As we have a function for that, don't do the math elsewhere.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
When compiling, -Werror=missing-field-initializers is enabled.
However, some gcc versions (like Gentoo 4.9.4 one) fail to see
that all the members of the SpiceBaseInterface struct are
initialized:
test-display-base.c:844:5: error: missing initializer for field
'description' of 'SpiceBaseInterface'
[-Werror=missing-field-initializers] .base.description = "test
spice virtual channel char device",
The solution is to initialize .base member as a structure at once
instead of multiple times per each member.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
As discussed recently the usage of domain for logging has
different issues (they are not filtered and handled coherently)
and are not widely used in the code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This was not done until now, and it's only going to be needed if we receive
a partial ClientMonitorsConfig message.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The function name is always prepended by the spice_log macro, so we
don't need to explicitly add it in debug messages.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
RedsClientMonitorsConfig duplicates what SpiceBuffer does,
so using we can replace it with SpiceBuffer and make
reds_on_main_agent_monitors_config() simpler.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is causing issues with potential improvements to the logging
system, and I've always found this usage a bit odd anyway.
Using spice_debug(""); was not possible as this triggers
-Wformat-zero-length warnings from our use of -Wall.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In the past CursorItem structure was stored in
RedCursorCmd::device_data field so the check was there to check if the
structure fit into that field.
Since 2ba69f9f88
("libspice: add surface 0 support") the structure is no more stored in
the field so there's no reason for this check causing only confusion.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The maximum value for a 32 bit variable is UINT32_MAX and not
UINT_MAX. Currently all supported platforms have these two
constants having the same value so this patch don't change
nothing in the generated code but potentially this could
change in a future.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
File transfer and Copy & Paste can be disabled on the server even when
they're supported by the guest agent. Tell it the client by adjusting
the agent capabilities.
Related: rhbz#1373725
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When a guest is rebooted, the QXL driver gets unloaded at some point in
the reboot process. When the driver is unloaded, the spice server sets a
single flag to FALSE: RedWorker::driver_cap_monitors_config. This flag
indicates whether the driver is capable of sending its own monitors config
messages to the client.
The only place this flag is used is when a new primary surface is created. If
this flag is true, the server assumes that the driver will send its own
monitors config very soon after the surface is created. If it's false, the
server directly sends its own temporary monitors config message to the client
based on the size of the just-created primary surface. This temporary monitors
config message always has a maximum monitor count of 1.
This flag is set to false at startup so that in early boot (e.g. vga text
mode), the server will send out these 'temporary' monitor config messages to
the client whenever the primary surface is destroyed and re-created. This
causes the client to resize its window to match the size of the guest. When the
QXL driver is loaded and starts taking over the monitors config
responsibilities, we set this flag to true and the server stops sending
monitors config messages out on its own.
If we reboot and set this flag to false, it will result in the server sending a
monitors config message to the client indicating that the guest now supports a
maximum of 1 monitor. If the guest happens to have more than one display window
open, it will destroy those extra windows because they exceed the maximum
allowed number of monitors. This means that if you reboot a guest with 4
monitors, after reboot it will only have 1 monitor.
To avoid this, we assume that if we had the ability to support multiple
monitors at some point, that will return at some point. So when the server
constructs its own monitors config message to send to the client (when the
driver_cap_monitors_config flag is false), we send the previous maximum monitor
count instead of always sending a maximum of 1.
Resolves: rhbz#1274447
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The variable is used to store a boolean type.
Update style for this boolean variable.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Use a #define rather than a magic number to make the code a bit more
readable.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
rcc is already deferenced in red_channel_client_get_client so
checking for NULL after that is uselss.
Also this call is generated from red_channel_client_disconnect
which requires the rcc pointer to be valid.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The base RedChannel already keeps a list of channel clients, so there's
no need for the SndChannel to also keep track of the client itself.
Since the SndChannel only supports a single client (whereas other
channels may have some partial support for multiple clients), I've
provided a convenience function for getting the client and warning if
there is more than one.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The content of these functions almost exclusively deals with channel
client functionality except one line where the channel's active state is
set to TRUE.
These functions are called in two different places.
The first place is from the public API spice_server_record_start() and
spice_server_playback_start(). These functions should alter the
channel's active state, and then set the associated channel client to
active.
The second place is when a new channel client is created. In this
case, it is only called if the channel is already active, so it doesn't
make much sense to set the channel's active state inside of the
function.
To simplify things (and enable some future refactoring), this function
now only deals with the SndChannelClient. The functions have also been
renamed to reflect this fact. The SndChannel's active state is now only
modified from the public API functions.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Instead of putting a 'next' link within the channel structure itself,
just use a generic GList structure to keep a list of active sound
channels.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
When a new PlaybackChannelClient or RecordChannelClient is created,
there are several places where we make decisions based on whether the
client is active or not. But these checks are done before the 'active'
flag is ever set, so this code is effectively dead. This has been the
case since commit 6fdcb931 and d351bb35, so this code has not been
executed for approximately 7 years now.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
It is only called from the constructor, so move all of the code into
that function.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This function is only called from the constructor, so move all of that
code into the constructor.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Reverse return values of the various bool methods so that 'true' means
success, and 'false' failure rather than the opposite.
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The red_get_* methods in red-parse-qxl.c return a boolean, even though
their return type is an int, and they return 1/0. This commit changes
this to the more explicit bool/true/false.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This use to be an anonymous enum, used as an int in
RedCharDeviceWriteBufferPrivate::origin.
Introducing a typedef clarifies what kind of value it's holding.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
This is intended to hold the fields that only char-device.c has a use
for, but for now this only adds the boilerplate for it, the next commit
will move the relevant field there.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
red_channel_client_clear_sent_item() will clear send_data.blocked, so no
need to do it in red_channel_client_msg_sent() as well.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Now that red_channel_client_set_blocked() is no longer changing the
events we are watching for, we can call it from
red_channel_client_push().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Since 5c460d, we need to watch for WATCH_EVENT_WRITE as long as there are
items queued waiting to be sent, this does not need to be done only when
the network queue is full.
When red_channel_client_set_blocked() is called, as a message is being
sent, WATCH_EVENT_WRITE will already be set, so it does not need to set
it again.
red_channel_client_msg_sent() removes WATCH_EVENT_WRITE, but this will
be done later anyway by red_channel_client_push() if needed.
Since it's redundant, we can remove this.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
red_channel_client_msg_sent() calls red_channel_client_clear_sent_item()
which will clear the rcc->priv->send_data.blocked flag. Later on, it
contains a check for !red_channel_client_is_blocked(), which will always
be true as nothing in between could have set it again. This commit
removes this unneeded check.
This check was already redundant when it was introduced in
9a62a9a809
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Before this commit, red_channel_client_start_ping_timer() and
red_channel_client_cancel_ping_timer() had checks to be no-ops when
rcc->priv->latency_monitor.timer is NULL.
This commit adds a similar check to
red_channel_client_restart_ping_timer(), and removes explicit NULL
checks before calls to red_channel_client_{cancel,restart,start}_ping_timer()
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
rcc is just used on the next line so cannot be NULL.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Many function already used bool as boolean type.
Change last gboolean occurrencies and values.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Commit 590acf3c55 introduced a regression after migration:
PlaybackChannel::connection is never set even if a client is connected,
which means the playback channel is non-functional until a client
reconnects as all the snd_channel_set_xxx() method checks for a non-NULL
PlaybackChannel::connection before sending data to the client.
This commit slightly changes the code flow in
on_new_playback_channel_client()/playback_channel_client_constructed()
so that PlaybackChannel::connection is set regardless of what
red_client_during_migrate_at_target() returns. This is what was done
prior to 590acf3c55.
This resolves https://bugs.freedesktop.org/show_bug.cgi?id=100136
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This is a particularly opaque part of the code for managing pending
Drawable operations. This patch adds documentation atempting to explain
these functions.
It can be useful for debug to know what the codec of the playback and
record channels are, so add a debug-level print statement indicating
this.
Resolves: rhbz#1436251
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Main-channel-client.c has code to send pings when the statistics code is
enabled. This is done through a timer calling a ping_timer_cb callback.
However, this timer is created but never started, so this code is
actually dead-code.
Since this code does not seem to be doing much apart from sending the
pings (which can be achieved by simply setting monitor-latency to TRUE
in main_channel_client_create()), this commit removes it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Their uint32_t value is never used, all that matters is whether we
received data or not.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
in main_channel_push_agent_connected(), it used the convenience function
red_channel_test_remote_cap() to determine whether to send the
AGENT_CONNECTED_TOKENS message, or the AGENT_CONNECTED message. However,
this function returns false if *any* one client doesn't support the
capability. We should instead check each individual client separately to
see if they support the capability.
This was a convenience function around
red_channel_client_test_remote_common_cap() that simply iterated over
each client (currently we only really support a single client anyway)
and returned TRUE only if all clients supported the capability. But
nobody ever called this wrapper function.
Both _test_remote_cap() and _test_remote_common_cap() are used as
boolean values, so change the return type from int to bool to follow our
coding standard.
This is a setting for determining whether to compress the audio playback
channel or not. It is variously typed as int or uint32_t. Convert it to
a 'bool' to make it more clear that it is a true/false value rather than
an enumeration or something like that.
The DUMP_BITMAP compile option enable some debugging code to
output image bitmaps in a subdirectory of /tmp.
However if this directory does not exists the server will crash
as not able to create a file in it.
Try to create directory before creating the file.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
- glz_enc_dictionary_reset
- monitors_config_new
- red_channel_any_blocked
- red_channel_no_item_being_sent
- red_client_get_channel
are only used in the file where they are defined, so they can as well be
static.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is only available in newer FreeBSD releases (9.1 and later), and
will cause build errors or older versions
This fixes https://bugs.freedesktop.org/show_bug.cgi?id=99213
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This new function removes one place outside of RedsStream which needs to
access RedsStream::socket
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This allows to move some low-level code out of reds.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This allows to move some low-level code out of reds.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The code to enable/disable on a TCP socket is duplicated in multiple
places in the code base, this commit replaces this duplicated code with
a helper in RedsStream.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Doing a memset(0) on a SpiceStatNode does not create an empty/unattached
node, but instead creates a node '0'. This node could be non-existing,
or totally unrelated.
This patch creates a default '0' node as soon as the file is created.
This will make the behaviour of 0-filled nodes more in line with what
one would expect.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This changes the header guards in all .h files to follow this format:
/*
* Licensing block
*/
/* ... */
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This is the default value, and the property is marked as
_CONSTRUCT_ONLY, so we don't need to explicitly force the default when
instantiating a RedChannelClient.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
InputsChannelClient::new and SmartcardChannelClient::new both accept a
"monitor_latency" argument, which is always FALSE. It can be removed.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Before GLib 2.36 you should call g_type_init before attempting
any GLib type usage.
As constructor function are called before even main we need
to call g_type_init much before do_spice_init.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Stress a bit video encoders.
This check different combination of
- encoder type;
- image formats;
- image clipping (encoding partial frames);
- handling frames split into chunks.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Handle single chunk writev as normal write.
From some test more than 60% of the times writev is called with 1 as
counter. We can easily and very cheaply turn this call to a simpler
write avoiding the need to pass the array to the kernel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
As pointed out by danpb, tests/pki/server-cert.pem is only valid until
Fri Nov 29 15:51:44 UTC 2019
This changes tests/pki/server-cert.pem and tests/pki/ca-cert.pem to be
valid until 2048. These certificates were generated using the
instructions on https://www.spice-space.org/page/SSLConnection
The -subj args were omitted, and the defaults suggested by openssl used.
The -days parameter was changed to -days 10950.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This allows to remove a small hack in server/Makefile.am where we were
using make check-valgrind-memcheck rather than make check-valgrind to
make sure we get a non-0 exit code on failures.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This allows to run automatically our test-suite with valgrind to test
for memory leaks.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
test-qxl-parsing is really a series of several tests. Porting it to
GTest makes this more obvious. This also has the side-effect of making
it more friendly to 'make check-valgrind' (which would fail on SIGALRM,
and on unexpected g_warning()).
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
On my 64 bit Fedora 25, CMSG_SPACE() adds 4 bytes of padding after the
file descriptor in the control data. This causes warnings when ran under
valgrind as we set msg_controllen to CMSG_SPACE().
This commit fills the control data to 0 to avoid these warnings.
==30301== Syscall param sendmsg(msg.msg_control) points to uninitialised byte(s)
==30301== at 0x8127367: sendmsg (sendmsg.c:28)
==30301== by 0x41880B: reds_stream_send_msgfd (reds-stream.c:295)
==30301== by 0x40953F: main (test-stream.c:121)
==30301== Address 0xffefff1b4 is on thread 1's stack
==30301== in frame #1, created by reds_stream_send_msgfd (reds-stream.c:263)
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This reverts commit c3d237075b.
When you call gst_buffer_add_video_meta_full GStreamer assumes that
buffer is contiguous. Specifically GStreamer assumes that the first
chunk is big enough to hold a whole frame. This results usually in
some pixel shifts in the video. The pixel shifts you can see are
artifacts due to how the guest sends the frames.
Assuming you allocate the 2 chunks with 2 malloc:
p1 = malloc(size);
...
p2 = malloc(size);
Usually the memory allocator tend to allocate linearly if there are
space adding a prefix to describe next block leading to a memory
arrangement as:
+-------------------+
| p1 prefix |
+-------------------+
| p1 buffer |
+-------------------+
| p2 prefix |
+-------------------+
| p2 buffer |
+-------------------+
now if you take p1 pointer and you assume it points to a 2 * size
buffer you will get p2 prefix in the middle, this prefix is the pixel
shifts.
Problems happens specifically in gst_video_frame_map_id.
This bug is reported in https://bugzilla.gnome.org/show_bug.cgi?id=779524.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
A few dispose/finalize implementations were not chaining up to their
parent classes, causing memory leaks.
This fixes leaks like the following:
==16240== Memcheck, a memory error detector
==16240== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==16240== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==16240== Command: ./test-leaks
==16240==
==16240==
==16240== HEAP SUMMARY:
==16240== in use at exit: 245,490 bytes in 3,638 blocks
==16240== total heap usage: 5,418 allocs, 1,780 frees, 17,676,968 bytes allocated
==16240==
==16240== 16 bytes in 1 blocks are definitely lost in loss record 203 of 771
==16240== at 0x4C2DADE: malloc (vg_replace_malloc.c:298)
==16240== by 0x4C2FC91: realloc (vg_replace_malloc.c:785)
==16240== by 0x52864E: dispatcher_register_handler (dispatcher.c:374)
==16240== by 0x5293E0: main_dispatcher_constructed (main-dispatcher.c:315)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x4095B1: server_leaks (test-leaks.c:45)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== 16 bytes in 1 blocks are definitely lost in loss record 204 of 771
==16240== at 0x4C2DADE: malloc (vg_replace_malloc.c:298)
==16240== by 0x4C2FC91: realloc (vg_replace_malloc.c:785)
==16240== by 0x52864E: dispatcher_register_handler (dispatcher.c:374)
==16240== by 0x5293E0: main_dispatcher_constructed (main-dispatcher.c:315)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x40BFD4: test_new (test-display-base.c:902)
==16240== by 0x40979D: vmc_leaks (test-leaks.c:92)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== 96 bytes in 1 blocks are definitely lost in loss record 638 of 771
==16240== at 0x4C2FA50: calloc (vg_replace_malloc.c:711)
==16240== by 0x8427D3C: g_malloc0 (gmem.c:124)
==16240== by 0x842801F: g_malloc0_n (gmem.c:355)
==16240== by 0x527B44: dispatcher_constructed (dispatcher.c:141)
==16240== by 0x529321: main_dispatcher_constructed (main-dispatcher.c:307)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x4095B1: server_leaks (test-leaks.c:45)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== 96 bytes in 1 blocks are definitely lost in loss record 639 of 771
==16240== at 0x4C2FA50: calloc (vg_replace_malloc.c:711)
==16240== by 0x8427D3C: g_malloc0 (gmem.c:124)
==16240== by 0x842801F: g_malloc0_n (gmem.c:355)
==16240== by 0x527B44: dispatcher_constructed (dispatcher.c:141)
==16240== by 0x529321: main_dispatcher_constructed (main-dispatcher.c:307)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x40BFD4: test_new (test-display-base.c:902)
==16240== by 0x40979D: vmc_leaks (test-leaks.c:92)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== LEAK SUMMARY:
==16240== definitely lost: 224 bytes in 4 blocks
==16240== indirectly lost: 0 bytes in 0 blocks
==16240== possibly lost: 0 bytes in 0 blocks
==16240== still reachable: 207,718 bytes in 3,312 blocks
==16240== of which reachable via heuristic:
==16240== newarray : 1,536 bytes in 16 blocks
==16240== suppressed: 34,548 bytes in 302 blocks
==16240== Reachable blocks (those to which a pointer was found) are not shown.
==16240== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==16240==
==16240== For counts of detected and suppressed errors, rerun with: -v
==16240== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 20 from 20)
FAIL test-leaks (exit status: 1)
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The code that manages pending QXL Drawable operations is fairly complex
and difficult to understand. This is an attempt to start documenting
that code to save time when we have to work on this code in the future.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This commit changes all functions returning TRUE/FALSE from having an
'int' return value to 'bool'.
This way it's obvious that such a function is not going to return
anything else than TRUE or FALSE.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
config_socket is configuring the client stream socket.
As is responsibility of RedChannelClient to handle the stream
it make more sense to have the function in this object.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Verify stuff are freed correctly (like TLS context).
The different PKI file required are generated with
base values (localhost and rsa 1024).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Add and remove some vmc device to check for leaking.
These combination assure that currently implemented type
of devices (webdav, usb and generic) are checked.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The nested if statements could be confusing, no needs for them.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Send header and reply together.
This potentially save up to 160 bytes on the network which
is a considerable amount considering that the message is
about 50 bytes.
This as sending multiple chunks you can have different framing,
specifically:
- if you use TLS every chunk get encrypted separately
(reds-stream, currently usually 29 bytes for every chunks);
- tcp settings and no delay on socket. More likely with fast
connections or better network cards. The tcp framing is
usually about 80 bytes;
- additional tcp acknowledge (usually 64 bytes).
So 80 + 29 + 64 = 173 bytes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These vfuncs are more appropriate in RedChannelClient.
The buffer they allocated are related to the client stream
which is managed directly by RedChannelClient.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This prepare for the next patch.
The network recieve buffer should be per-client rather than per-channel.
The following patch will make this change, but this common base class
will allow the cursor client and the display client to share a common
implementation.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
As the counters are shared there is no reason why not
handling the byte count from RedChannelClient directly.
This remove a dependency and avoid some function calls.
The only visible difference at user level is that the
counters are created when a client connects.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Show messages sent to clients.
This is useful to understand the message number as an high
message number can affects performance and is not easy to
understand the message count from the byte count (which is
available).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Instead of disabling the code use the compatibility functions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
[0] SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE
This message provides a list of video codecs based on client's order
of preference.
We duplicate the video codecs array from reds.c and sort it using the
order of codecs as reference.
This message will not change an ongoing streaming but it could change
newly created streams depending the rank value of each video codec
that can be set by spice_server_set_video_codecs()
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Allows to use recording function for multiple purposes.
This will allow to register multiple screen VM or recording
additional stuff like sound.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The synchronization code is required to avoid mixing writing
from multiple threads.
Following patches will add this feature.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
For each channel there are two set of capabilities, one
for the common ones and one for the specific ones.
A single set were almost always passed using 2 arguments,
a number of elements and an array but then before using
these were converted to a GArray.
Use a single structure (already available) to pass all
channel capabilites using a single argument.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Add function to initialize and destroy this type.
Add GType type for boxing it.
These changes a in preparation for next patch.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These properties are not read and code is broken (the content of
the array would be uninitialized).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Watch was added but never removed.
The added basic_event_loop_destroy() addition allows to see that
this leak is gone).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This switches the test to using the GTest API, and add several tests
related to https://bugzilla.redhat.com/show_bug.cgi?id=1411194
This uses some API not available in glib 2.28, so this checks we have a
new enough glib before building this test, and disables warnings when
using too new glib API when building it.
The "multiple-vmc-devices" is based off code written by Frediano Ziglio.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Using spice_info() gets in the way of tests using
g_test_expect_message() as all the messages emitted using
a non-debug log level must be listed as expected, otherwise we get a
critical about an expected message not having been logged.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Currently, the network sockets opened by reds_init_net() are not closed
on destruction, in other words they are leaked.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This allows to chain several test cases by using
test_new()/test_destroy().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Do not assume the device passed as an argument to
spice_server_char_device_remove_interface() is the same as the current
Reds::vdagent instance. This commit adds a check that this is the case,
and returns early if not.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
spice_new is a bit more safe as return a properly typed pointer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The usage was removed with commit 7ea1f2c133
("sound: Use RedChannelClient to receive/send data").
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Using X == TRUE is fragile, since the input value is a uint8_t. It would be
surprising if the value was set to 2 or 0xFF and the test failed.
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Some constants/macros/function prototypes are defined in
red-channel-client.h while they are only used by red-channel-client.c.
This commit moves them to the .c file to make them private.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedChannelClient is responsible for talking to the client so it knows
if is connected or not.
These vfuncs where used by DummyChannel used by SoundChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Most channels don't need to do specific settings for the client socket
so avoid the need to do this setting making easier to setup the client
channnel.
Some improvements and commit subject suggested by Christophe Fergeau.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
TCP_NODELAY flag is set by default for all connection inside
reds.c so there's no need to set again for the single
client channel.
Note that there are still some calls to setsockopt to change this
option.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Since c3d237 "gstreamer: Avoid memory copy if strides are different" is
only needed when zero copy is disabled. This moves the function
definition to an already existing #ifdef block.
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This is a revert of b76e561d.
For a SpicePlaybackInstance, the base interface must be a
SpicePlaybackInterface instance, not a SpiceBaseInterface instance, or
spice-server code will end up reading out of bounds.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is a revert of 93b4f4050^ and 93b4f4050.
For a SpiceCharDeviceInstance, the base interface must be a
SpiceCharDeviceInterface instance, not a SpiceBaseInterface instance, or
spice-server code will end up reading out of bounds.
vmc_state/vmc_write/vmc_read implementations also have to be provided.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is a revert of 93b4f4050^ and 93b4f4050.
For a SpiceCharDeviceInstance, the base interface must be a
SpiceCharDeviceInterface instance, not a SpiceBaseInterface instance, or
spice-server code will end up reading out of bounds.
vmc_state/vmc_write/vmc_read implementations also have to be provided.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Use new structures and functions to implement the statistics code.
Use inline functions instead of macros for increased type-safety.
If statistics are disabled, the structures and functions become
empty. This confines the configuration-specific #defines to the
statistics implementation itself and avoids the need for #defines in
the calling functions. This greatly reduces the chance of accidentally
breaking the build for one configuration or the other. The reds option
was removed from stat_inc_counter() as it was not used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Non blocking flag is set for all connection inside reds.c so
there's no need to set again for the single client channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Do not make it assume vec contains IOV_MAX elements.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
They no longer contain any vfuncs, so calling them "handler" does not
make a lot of sense. This commit renames them to
OutgoingMessageBuffer/IncomingMessageBuffer.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Nothing outside of RedChannelClient needs access to data contained in
RedChannelClientPrivate, so we can move all the type definitions to the
.c file to make it fully opaque rather than relying on a private header.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The methods previously used by OutgoingHandlerInterface and
IncomingHandlerInterface are no longer used as generic callbacks,
but are directly called from RedChannelClient code. We can be explicit
about the type of their first argument (RedChannelClient *) rather than
using a generic void * pointer.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
There is only one implementation of OutgoingHandler which relies
OutgoingHandler::opaque being a RedChannelClient. This commit makes this
explicit in order to get rid of the OutgoingHandler::opaque data member.
This renames red_peer_handle_outgoing() to
red_channel_client_handle_outgoing() as the method is now very much tied
to RedChannelClient.
If we want to keep some genericity, we could return error codes from
red_channel_client_handle_outgoing() and handle RedChannelClient
disconnection/... from the caller rather than directly in the
_handle_outgoing() method. This would probably allow to move the
data reading logic to reds-stream.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
There is only one implementation of IncomingHandler which relies
IncomingHandler::opaque to be a RedChannlClient. This commit makes this
explicit. This allows to get rid of the IncomingHandler::opaque data
member.
This renames red_peer_handle_incoming() to
red_channel_client_handle_incoming() as the method is now very much tied
to RedChannelClient.
If we want to keep some genericity, we could return error codes from
red_channel_client_handle_incoming() and handle RedChannelClient
disconnection/... from the caller rather than directly in the
_handle_incoming() method. This would probably allow to move the
data reading logic to reds-stream.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This commit removes what remains of IncomingHandlerInterface. The
remaining function pointers were pointing to RedChannel vfuncs.
Moreover the IncomingHandlerInterface abstraction is unused, ie the
codebase only has a single implementation for it, so we can directly
call the relevant methods and make them static instead.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This commit removes IncomingHandlerInterface::on_error and
IncomingHandlerInterface::on_input. As with previous commits, these
vfuncs are always calling the method, and RedChannel::init sets them to
point to RedChannelClient methods, which RedChannelClient is then going
to call indirectly through the IncomingHandlerInterface instance.
This commit changes this to direct calls to the corresponding methods.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Similarly to the previous commits, this removes an indirection level,
IncomingHandlerInterface stores pointers to
alloc_recv_buf/release_recv_buf vfuncs, but these are always set to
RedChannel::alloc_recv_buf/RedChannel::release_recv_buf, which are also
vfuncs which can be overridden if needed. This commit removes the
indirection and directly calls the relevant methods.
These vfuncs really should be in RedChannelClient rather than
RedChannel.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
RedChannel uses OutgoingHandlerInterface to provide constant pointers to
RedChannelClient methods. This OutgoingHandlerInterface structure is
then used in RedChannelClient to indirectly call these methods.
The OutgoingHandlerInterface abstraction is unused, ie the codebase
only has a single implementation for it, so we can directly call the
relevant methods and make them static instead.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <figlio@redhat.com>
Have the RedChannelClient callback call into a RedChannel callback
rather than doing the opposite. This will be useful in some subsequent
refactoring of this code.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
red_channel_client_parse() currently does roughly:
if (klass->parser) {
parsed = klass->parser(msg, msg_size, &parsed_size);
klass->handle_parsed(rcc, parsed_size, msg_type, parsed);
} else {
klass->handle_message(rcc, msg_type, msg, msg_size);
}
The handle_parsed implementation expects a void * 'parsed' argument,
which will then be cast to the correct type. There is not really a need
to provide distinct handle_parsed/handle_message vfuncs, instead we can
say that if a RedChannel subclass provides a 'parser' vfunc, then it's
'handle_message' vfunc will be called with the parsed message, otherwise
it will be called with unparsed data (ie what handle_message currently
expects).
This makes the code slightly easier to follow as messages will always be
handled by the same vfunc.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
smartcard_channel_client_pipe_add_push was just calling
red_channel_client_pipe_add_push without any cast or other
changes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This makes the code slightly more readable as this means less local
variables to keep track of when taking a high level view of that code.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
read_binary() attaches 'mem' to the SpiceReplay::allocated list.
On failure, SpiceReplay::allocated and its content are freed by
spice_replay_free().
SpiceReplay::primary_mem is also freed, which causes a double free
as replay_handle_create_primary() added 'mem' both to
SpiceReplay::primary_mem and SpiceReplay::allocated.
This commit avoids this by ensuring SpiceReplay::primary_mem is not
kept in the SpiceReplay::allocated list.
Note that this double free can happen only on currupted or wrong
record images.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The Surface and Display channels each have a 'current_list' Ring, and
Surface also has a 'current' Ring. these names are confusing, so at
minimum, add a comment indicating the type of object they hold. The
DisplayChannel::current_list already had a comment, but it was
incorrect.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Caller is supposed the function return a buffer able to store
size bytes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The limits for capabilities are specified using 32 bit unsigned integers.
This could cause possible integer overflows causing buffer overflows.
For instance the sum of num_common_caps and num_caps can be 0 avoiding
additional checks.
As the link message is now capped to 4096 and the capabilities are
contained in the link message limit the capabilities to 1024
(capabilities are expressed in number of uint32_t items).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The limit for link message is specified using a 32 bit unsigned integer.
This could cause possible DoS due to excessive memory allocations and
some possible crashes.
For instance a value >= 2^31 causes a spice_assert to be triggered in
async_read_handler (reds-stream.c) due to an integer overflow at this
line:
int n = async->end - async->now;
This could be easily triggered with a program like
#!/usr/bin/env python
import socket
import time
from struct import pack
server = '127.0.0.1'
port = 5900
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect((server, port))
data = pack('<4sIII', 'REDQ', 2, 2, 0xaaaaaaaa)
s.send(data)
time.sleep(1)
without requiring any authentication (the same can be done
with TLS).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
red_channel_client_handle_message can handle base messages
so reuse it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Some gcc version reports this error:
stat-file.c: In function 'stat_file_add_node':
stat-file.c:180:15: error: 'node' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
g_strlcpy(node->name, name, sizeof(node->name));
^~~~
cc1: all warnings being treated as errors
This warning is a false positive as this loop:
for (ref = 0; ref <= stat_file->max_nodes; ref++) {
node = &stat_file->stat->nodes[ref];
...
}
will always iterate at least once.
This patch rewrite the loop in order to make more compilers
understand that node variable is always initialized.
There are two checks apparently removed in the patch:
- check for stat_file->stat not being NULL. This was worthless as the
field was already used in the function. Also this field is never
NULL (unless memory corruption happened);
- stat_file->stat->num_of_nodes >= stat_file->max_nodes. It's implicit
in the loop. If num_of_nodes >= max_nodes means that there are no
free nodes so all nodes should have SPICE_STAT_NODE_FLAG_ENABLED set,
loop will exit and function will return INVALID_STAT_REF.
Reported-by: Christophe Fergeau <cfergeau@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The stat file contains an array of max_nodes elements
so we must stay in [0, max_nodes) range, not [0, max_nodes].
There are no spice path that lead to these overflows but
it's better to have them fixed before creating one.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Adding conditional for having gstreamer_0_10 or gstreamer_1_0,
removing the previous conditionals and update relevant ifdefs
with the newly defined conditional
We support only a single client so don't waste code just
to check this.
The worst stuff can happen is that we'll migrate multiple
connections.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Move the freeing of SndChannel data members from snd_detach_common() to
the finalize function to encapsulate things a bit more cleanly. It
doesn't really change the behavior or order of destruction since
snd_detach_common() destroys the channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Sound implementation used internal RedChannelClient data while now
it just uses the public interface not thouching RedChannelClient
internal state so now is possible to make this field private.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Now that SndChannelClient has switched from using its own code for
sending data to using RedChannelClient, it's very close to being an
actual RedChannelClient.
This commit makes it directly inherit from RedChannelClient rather than
having a channel_client field. This allows to get rid of the whole
DummyChannel/DummyChannelClient code.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
snd_set_command()/snd_send() are higher level methods which take care of
scheduling calls to the corresponding snd_*_send_*() methods when
appropriate. This commit switches a few direct snd_*_send_*() calls to
snd_set_command()/snd_send().
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
You can see that SndChannelClient has much less field
as the code to read/write from/to client is reused from
RedChannelClient instead of creating a fake RedChannelClient
just to make the system happy.
One of the different between the old sound code and all other
RedChannelClient objects was that the sound channel don't use
a queue while RedChannelClient use RedPipeItem object. This was
the main reason why RedChannelClient was not used. To implement
the old behaviour a "persistent_pipe_item" is used. This RedPipeItem
will be queued to RedChannelClient (only one!) so signal code we
have data to send. The {playback,record}_channel_send_item will
then send the messages to the client using RedChannelClient functions.
For this reason snd_reset_send_data is replaced by a call to
red_channel_client_init_send_data and snd_begin_send_message is
replaced by red_channel_client_begin_send_message.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
The removed code was trying to read data when
spice_server_record_get_samples() is called. Since reading of data is
event-driven anyway (see snd_event), it's redundant to try
again to read more data.
This commit removes this code as this will some refactoring easier in
the next commits.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
We can get it from our DummyChannelClient rather than storing it in
SndChannelClient.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
We can use the marshaller provided by the dummy RedChannelClient
associated with SndChannelClient.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Filter out commands which should not happen. Should it be a
g_warn_if_fail() or such instead?
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is in preparation for switching SndChannelClient into a proper
RedChannelClient. The prototype of the new helper matches what is
expected from the RedChannel::config_socket vfunc.
To be able to achieve that, this commit associates the sound channel
RedsStream instance with the DummyChannelClient instance we have, and
then call snd_channel_config_socket() on that instance.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The main goal of this commit is to avoid to dereference 'client' before
it's checked for NULL. This meant splitting one error condition in 2
separate ones.
This also sets default values for the 'frame' and 'num-samples'
out parameters so that we just have to return on error conditions.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This structure is used to store format information for
both Gstreamer 0.10 and 1.0 however the two format uses
different fields from it.
Use a macro to filter only needed fields.
This currently also fixes a compile error using Gstreamer 0.10
(GST_VIDEO_FORMAT_RGB15 not defined as not available).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The structure is used only to allocate private data.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Currently, calling spice_server_remove_interface() twice in a row with
the same SPICE_INTERFACE_CHAR_DEVICE is going to cause a crash when
calling red_char_device_get_server(char_device->st); because
char_device->st will have been set to NULL by the first call.
This commit adds a few sanity checks before trying to use the various
'st' members of the interfaces.
This should avoid the crash described in
https://bugzilla.redhat.com/show_bug.cgi?id=1411194 even though it's not
clear how we got in that situation.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Avoid to use g_object_get if not necessary.
red_char_device_get_server is more type safe and we are
not bound to dynamic fields.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This field is used only for debugging.
Remove it reducing a bit all these "current" fields around.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If bitmap stride and stream stride are different copy was used.
Using GStreamer 1.0 you can avoid the copy setting correctly
image offset and stride.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Handling read returning 0 (usually end of connection/pipe)
is the same of handling an error (read result -1) with errno == 0
so merge the two paths to reuse code and simplify.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This allows the connection to early fail in case initial bytes
are not correct.
This allows for instance VNC client to graceful fail connecting
to a spice-server. This happens easily as the two protocols
share the same range of ports.
This resolves rhbz#1416692.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Tested-by: Daniel P. Berrange <berrange@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Code to read and process display commands were the same
so use a common function for better reuse.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This format is required to add metadata to the source buffer
using gst_buffer_add_video_meta_full.
This metadata can be used to pass strides/offsets, or
dmabuf-specific information.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This happened during VM resume.
RedSurfaceCmd were allocated but never freed.
We don't need to malloc the RedSurfaceCmd used in handle_dev_close()
as display_channel_process_surface_cmd() will not try to reference
it after it has returned.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Code to read and process cursor commands were the same
so use a common function for better reuse.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This allows to do some possible statistics or graph.
Currently the report contains encoded sizes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Currently MUTE and VOLUME commands use the same VOLUME mask. This commit
introduces a separate SND_MUTE_MASK for MUTE commands to make things
a bit more clear.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is no longer used since "sound: Convert SndChannel to GObject"
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It's already defined before in the same source file.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It became unused in 26027036c 'red_channel: remove unused migrate flag
from RedChannel' but was never removed from the function prototype.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This became obsolete when RedChannel became GObject-based.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
These formats were tested manually using test-gst utility
in server/tests.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Add an utility to make possible to check various features of
VideoEncoder.
2 GStreamer plugins are used in a chain like this:
(1) input pipeline -> (2) video encoder -> (3) output pipeline
While converting output from (1) is compared with output of (3)
making sure the streaming is working correctly.
You can set various options:
- part of the input pipeline description to allow specifying different
video from GStreamer test ones to a video file;
- the encoder to use;
- different image properties to use for (2) input:
- different bit depth;
- top/down or down/up;
- initial bitrate.
The idea is to use this helper in combination with a shell script
and some video sources to make able to test various settings.
Also can be used to extend the current encoder list.
As an example you can use a command like
$ ./test-gst -e gstreamer:vp8 -i \
'filesrc location=bbb_sunflower_1080p_30fps_normal.mp4 \
! decodebin ! videoconvert'
to check vp8 encoding.
Currently it does not emulate bandwidth changes as stream reports
from the client are not coded.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Small refactor. As reds_get_video_codecs() returns the video codecs as
GArray, we should match reds_set_video_codecs() to have a GArray as
parameter instead of string.
reds_set_video_codecs_from_string() seems more appropriate for the
previous function.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Previously, the object we now call SndChannel was named SndWorker, and
the object we now call SndChannelClient was called SndChannel. When
these names were changed, the functions
on_new_(record|playback)_channel() were not updated, so the function
names and the arguments are both a bit confusing now. Update them to
match the new names.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
When the initial image was sent to the client the reference
was not incremented leading to some user after free.
This regression was introduced in
3bde2e570c
("DCC: remove more init_send_data() arguments").
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This fixes a regression caused by
a43c21b6bc
("DCC: change how fill_bits() marshalls data by reference").
Before the mentioned patch there were a few references to Drawable
structure so an uint8_t was enough.
Now that every chunk of the image is accounted you can easily
get an overflow.
This fixes https://bugs.freedesktop.org/show_bug.cgi?id=99258.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Fix compilation error due to -Werror=maybe-uninitialized:
CC test-display-base.o
test-display-base.c: In function 'do_wakeup':
test-display-base.c:579:13: error: 'update' may be used uninitialized...
push_command(&update->ext);
Signed-off-by: Snir Sheriber <ssheribe@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This third argument (and the 'item' member of
RedChannelClient::priv::send_data) was a somewhat roundabout way to keep
the RedPipeItem alive until a message is sent, just in case some data
owned by that pipeitem was added to the marshaller by reference. This
was a rather confusing mechanism, however, since it did not have any
obvious connection to the _add_by_ref() call. It was never very clear
whether you needed to pass an item to this function or not. The previous
series of patches made this parameter unnecessary since the referencing
of the pipe item (or other related structure) is now more explicitly
connected to the calls to spice_marshaller_add_by_ref_full().
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The fill_bits() function marshalls some data by reference. This data is
owned by the RedDrawable that is owned by the Drawable that is owned by
the RedDrawablePipeItem. Instead of keeping the RedPipeItem alive by
passing it to red_channel_client_init_send_data(), simply reference the
Drawable and marshall it with _add_by_ref_full(). This means that we
can't use the _add_chunks_by_ref() convenience function since that
function doesn't allow us to pass a free function to clean up the data
after it is sent.
This change is not perfect since the fill_bits() function makes an
assumption that 'simage' is owned by the 'drawable'. On the other hand,
the previous code made a much bigger assumption: that the caller would
ensure that the data would be kept alive
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Use spice_marshaller_add_by_ref_full() instead of _add_by_ref() to
handle the referenced data properly rather than passing the pipe item to
red_channel_client_init_send_data() to keep the pipe item alive
indirectly.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Use spice_marshaller_add_by_ref_full() instead of
spice_marshaller_add_by_ref() to allow the marshaller to manage the
lifetime of the referenced data buffer rather than having to manage it
by passing a PipeItem to red_channel_client_init_send_data(). Since the
data is owned by CursorItem (which is not in fact a RedPipeItem, but is
owned by a pipe item and is itself refcounted), we take a reference on
the CursorItem when adding the data buf to the marshaller, and then
unreference it in the marshaller free func.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The only time that the pipe item needs to be passed as the third
argument to red_channel_client_init_send_data() is when the pipe item
holds a data buffer that has been added to the marshaller by reference
(spice_marshaller_add_by_ref()) and needs to be kept alive until the
data has been sent. In all other cases, the item does not need to be
kept alive, so we can safely pass NULL for this third parameter.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Any data that is added to the marshaller by reference (using e.g.
spice_marshaller_add_by_ref_full()) is freed during
spice_marshaller_reset(). But the marshaller is not currently reset
until we begin to send the next message (in
red_channel_client_send_item()). This means that the sent message data
lives longer than expected and can violate some assumptions in other
parts of the code.
To make sure that the data is cleaned up right after being sent, I've
added a reset call to clear_sent_item() and called that function from
_on_out_msg_done(). This means that _restore_main_sender() no longer
needs to reset the marshaller, and we no longer need to call
_reset_sent_item() within _on_out_msg_done() (since this function is
called from _clear_sent_item()).
This prepares the way for refactoring
red_channel_client_init_send_data() to change how we keep the
RedPipeItem alive while sending a message.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
We should replace the video_codecs GArray only after the parsing of
input is done, otherwise we might lose previous configuration.
Tests were updated to match this situation. Input that fails to
replace video_codecs are considered bad.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This make more obvious which directory they refer
and potentially avoid ignoring unwanted files.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The partial expression "MSEC_PER_SEC * size * 8" can overflow if
size is 536870 or more. This as the operation is done using
32 bit unsigned integers. Being the size potentially double of
a compressed frame size the limit can be easily reached.
As get_average_frame_size already return a 64 bit use 64 bit
even for the size to avoid the integer overflow.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Francois Gouget <fgouget@codeweavers.com>
This function is supposed to add an item to the queue to
be sent before all other queued items.
Was never used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
refs was used before GObject for reference counting.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
All RedWorker messages starts with RedWorker except
SpiceMsgDisplayGlDraw.
For coherence introduce a RedWorkerMessageGlDraw structure
holding just SpiceMsgDisplayGlDraw. This also allows possible
extensions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
The spice_marshaller_add_ref() family of functions is confusing since it
sounds like you're incrementing a reference on the marshaller. What it
is actually doing is adding a data buffer to the marshaller by reference
rather than by value. Changing the function names to _add_by_ref() makes
this clearer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
spice_debug was called for not-EINTR case, move
it to the right place.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
man 2 dup2 specifies:
The close-on-exec flag (FD_CLOEXEC; see fcntl(2)) for
the duplicate descriptor is off.
Since the purpose of the fcntl call is to turn off FD_CLOEXEC
flag, and it's already done, just remove this call.
Suggested-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Spice coding style suggests to use curly braces
for while blocks.
Some prefer the block to not be empty so continue
is untouched.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This patch prevents possible memory leak.
Found by coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In both image_encoders_restore_glz_dictionary() and
image_encoders_get_glz_dictionary() shared-dict may
be NULL if size is too large, and the server gets
size from the network.
Both functions end up calling glz_enc_dictionary_create()
that calls glz_dictionary_window_create() where size is
checked.
Found by coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It shadows the outer one.
Renamed also the outer 'container' variable.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Allow to catch minor issue with test code.
Although test usually are coded with less care than production
code the current changes required are not so extensive and
can catch different issues.
Most of the patch is making functions static to avoid warnings for
undeclared functions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This patch creates display_channel_get_video_codecs() helper to let
stream.c get the video-codecs without accessing the internal structure
of DisplayChannel.
As video-codecs is a property of DisplayChannel, this change means
making this property readable as well.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Allow to dynamically remove QXL interfaces. This could be used to
support hot swapping of QXL cards.
This code is actually not used in any way.
QXL interfaces are destroyed by spice_server_destroy automatically.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Make finalization of DisplayChannel consistent with other code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Reduce GObject changes coming in the next commit since we'll need to
change how we access the marshaller anyway. This will make the
following commits easier to review.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
As data is packae in a single piece of memory send it
altogether.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Including g_clear_pointer() in glib-compat.h and using it to avoid
warnings in odd situations.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
When qemu (for example) delivers audio samples to the spice server, it
does so by requesting a buffer from the spice server
(spice_server_playback_get_buffer()), filling them with audio data, and
then calling spice_server_playback_put_samples() to send them to the
client. Between the call to _get_buffer() and the call to
_put_samples(), we need to ensure that the buffer remains valid. Since
this buffer is allocated within PlaybackChannelClient, we did this by
incrementing a ref on the PlaybackChannelClient in _get_buffer(), and
decrementing the ref in _put_samples(). This has the effect of
potentially keeping the PlaybackChannelClient alive after the spice
client has disconnected.
This was not a problem when PlaybackChannelClient was a simple helper
class. But we intend to change PlaybackChannelClient so that it
inherits from RedChannelClient. In that case, the reference taken in
_get_buffer() would result in the RedChannelClient (and associated
RedChannel, etc) being kept alive longer than expected. To avoid this,
we add an additional ref-counted adapter class (AudioFrameContainer)
that owns the allocated audio frames and can outlive the
RedChannelClient if necessary. When the client is freed, the
AudioFrameContainer is just unreferenced.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Stops using the dummy channel.
Data handling still goes through DummyChannelClient which is why
empty implementation of some required vfuncs is working.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Try to arrange destruction in the opposite order of the creation
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The check to limit too low bit rates was setting encoder->bit_rate
instead of bit_rate. However after some lines bit_rate was used
to set encoder->bit_rate basically removing the lower threshold.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Francois Gouget <fgouget@codeweavers.com>
By removing the stream's video encoder we force the stream to send
future frames using the fallback code, that is as regular screen
updates.
However note that we keep the stream object: we have to. Otherwise
future frames would trigger the creation of a new stream object with a
new video encoder which would again try to stream the video and fail
again and again.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Reviewed-by: Victor Toso <victortoso@redhat.com>
Make easier to understand that they refer to client and not
all channel.
Specifically:
- RecordChannel -> RecordChannelClient
- PlaybackChannel -> PlaybackChannelClient
- playback_channel -> playback_client
- record_channel -> record_client
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
SndWorker has been historically based on RedChannel, initial git commit
has:
struct SndWorker {
Channel base;
...
};
SndChannel, contrary to what its name may imply is more focused on
marshalling/sending of sound data, which is the responsibility of
RedChannelClient for the other SPICE channels.
This commit and following ones make the naming of these 2 types more
consistent with how the rest of the code is structured.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
SpicePlaybackState and SpiceRecordState have same structures
changing only slightly the behaviour.
Using SndWorker instead allows some minor simplification and
more code reuse.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Most of the times the check is done externally
so moving inside the function reduce the code.
This is similar to the way free(3) works.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This function assume there is only one client.
Was used only by some obsolete functions.
Avoid to use such function in the future.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This function wrongly close the first client.
Wrongly as closing the file descriptor cause a dandling
file descriptor in the object potentially leading
to closing another file descriptor open later.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This will close all clients and release the channel properly
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Now that RedStatFile is private there is no need
to include some headers in stat-file.h.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
These functions are not used since years and are not supporting
multiple clients.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Use same constants for common commands.
This will allow code reuse between Record and Playback.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Allows to close worker thread.
This will be used to destroy cleanly CursorChannel and
DisplayChannel.
CursorChannel and DisplayChannel are run in a different
thread. However deregistration of channels and different
steps of destruction should be done in the same thread
so this make possible to join again the 2 threads to
avoid race conditions.
For the moment there is no correct cleanup.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This make compression faster and avoids a warning on newer
lz4 versions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Use dash instead of underscores for file names. This is coherent
with rest of file names.
Rename all tests to test-XXX and remove the spice- prefix when
present. This is coherent with most of the tests.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
self is usually used for GObject, avoid confusion.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The video encoders already have sophisticated bit rate control code that
can react to both stream reports sent by the client, and server frame
drop notifications. Furthermore they can adjust both the frame rate and
the image quality to best match the network conditions.
But if the client cannot send stream reports all this is bypassed and
instead the streaming code performs its own primitive bit rate control
that can only adjust the frame rate.
So this patch removes the code duplication and lets the video encoders
do their job.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The wrong type (RED_TYPE_CHANNEL_CLIENT rather than
TYPE_SMARTCARD_CHANNEL_CLIENT) was used when creating a
SmartcardChannelClient, which meant the _init function was never called,
and SmartcardChannelClient::priv was never initialized.
This caused a crash when trying to connect with --spice-smartcard to a
VM with a smartcard channel configured.
This leaks happen for every connection. Potentially the timer can
be called after the client is closed causing an use after free.
Recently RED_STATISTICS was switched off by default but previous
version have this issue.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This reverts commit c6881ad1a0.
This patch cause the replay utility run at full speed
to slow down a lot and in some cases getting stuck.
I don't understand the reason and when I tested was working
but as we are going to release would be a pity if this test utility
won't work as useful to get feedback and reports.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The following build error occurs when building outside of the source
directory:
glib-mkenums --template spice-server-enums.tmpl.c ../../server/spice-server.h > spice-server-enums.c
Can't open spice-server-enums.tmpl.c: No such file or directory
Makefile:1111: recipe for target 'spice-server-enums.c' failed
Make/Automake uses VPATH to determine that the spice-server-enums.tmpl.c
file listed in the prerequisites for the rule is located in the srcdir.
When we use an automatic variable (e.g. $<), the full path to the
resolved file is used. But when we use the literal filename directly
within the rule definition, this won't happen. So we need to explicitly
specify that the input template file is located in srcdir.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
mmap memory area will remain even if the descriptor is closed.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Avoid to produce loop in the tree removing and adding nodes.
Unlink the node from the containing tree.
This possibly will create unlinked trees if a parent node is
deleted.
What was happening is that the creation of loops inside
the tree caused some statistical function to go into an
infinite loop (and reds_stat tool too).
Nodes were only invalidated but still linked so when reused
the new node could point to an already existing node (like a
sibling) which pointed to the new reused one.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Code is quite independent so move in separate file.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Avoid the following warning when build with disabled gstreamer:
Spice-WARNING **: reds.c:3660:reds_set_video_codecs: spice: unsupported video encoder gstreamer
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Avoid not constant globals.
We started encapsulating all global state into RedsState however
there are still some global variable. This patch remove the
core_public global variable.
To implement this a new SpiceCoreInterface *public_interface
field is added to SpiceCoreInterfaceInternal and the
SpiceCoreInterfaceInternal* argument is passed to callbacks to
understand which external interface to use.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The top down flag can be specified using negative heights.
According to
https://msdn.microsoft.com/en-us/library/windows/desktop/dd183376(v=vs.85).aspx:
"The height of the bitmap, in pixels. If biHeight is positive, the
bitmap is a bottom-up DIB and its origin is the lower-left corner.
If biHeight is negative, the bitmap is a top-down DIB and its origin
is the upper-left corner."
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
These cause compiler errors using RHEL6.
These typedefs are defined in the same file some lines above.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
GCond/GMutex interface is different between Glib 2.32 and
previous versions. Use pthread for compatibility.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These function were really similar.
Factor out a new snd_get_best_rate to avoid code duplication.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This field is common to SpicePlaybackState and SpiceRecordState
which are based on SndWorker.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The common function is supposed to clear the state of SndWorker
so clear even volume which now is in SndWorker
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Include common and libraries includes before local ones as
stated by style.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Syntax checker complained about autoconf variable expansion used
inside Makefile.am.
This patch uses template files instead of options.
This also reduces quoting making template code more readable.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The verify macro used currently has some problem
as it raise a warning in RHEL6.
Use new SPICE_VERIFY macro defined in spice-common
to avoid this issue.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Add a 'self' parameter to all of the char device virtual functions so
that we don't have to play games with the 'opaque' pointer.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Function just return the command, has no ext_cmd parameter to fill.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Pass the new sound channel to these functions rather than setting
worker->connection before calling the function and expecting it to be
set when the function is called. The fewer unenforced assumptions that
must be true for a function to work properly, the easier it will be to
maintain.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Reduce SpiceVolumeState structure size on 64 architectures
swapping the order of two fields.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Reuse more code in snd_send_volume and snd_send_mute.
Reduce conversions in on_new_playback_channel and
on_new_record_channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This field is common to SpicePlaybackState and SpiceRecordState
which are based on SndWorker.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Time is always the the current real time so avoid to compute
it for every call but move to red-record-qxl.c.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The functions declared in that header are all exported by the
library.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Instead of waking up the command loop for every command queued,
handle saved wakeups and replicate these.
This better reproduces what happened in the server.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The QXLMessage has no size so potentially a guest could give an
address that cause the string to overflow out of the video memory.
The current solution is to parse the message, release the resources
associated without printing the message from the client.
This also considering that the QXLMessage usage was deprecated
a while ago (I don't know exactly when).
This patches limit the string to 100000 characters (guest can feed
so much logs in other way) and limit to video memory.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
SpiceBlend and SpiceCopy are just different names for the same
structure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Due to syntax-check limitation this free calls results in
a syntax error.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Use g_param_spec_object() instead of g_param_spec_pointer() for the
'client' and 'channel' properties now that these types are GObjects.
This improves refcounting and typesafety slightly.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Instead of having channel and device object create one the other
create the objects at the beginning and then join them.
This make explicit the code that links the two objects and separate
the objects creation from the linking.
Also remove some boilerplate code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
After renaming the object to RedVmcChannel, the local variables still
used the old 'state' terminology. Changing these variables to 'channel'
makes things a bit more consistent.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This helper function does nothing but cast the return from
red_channel_client_get_channel(), and it has a confusing name
(_get_state(), but returns a channel)
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This move object destructions from spicevmc_device_disconnect
to RedCharDeviceSpiceVmc destructor functions assuring any possible
RedCharDeviceSpiceVmc object free will clear its references.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
No reason why this should be done only on spicevmc_device_disconnect.
red_char_device_write_buffer_release can be called with first pointer
NULL.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Assure field is freed at the end and not used or allocate again.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Store the channel in the char device object, and unref it in dispose.
Previously, we were just creating a channel and potentially allowing it
to leak. There may be better long-term solutions, but this works for
now.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Also move the RedClient struct out of the header to avoid accessing the
internals from other files.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Instead of requiring the channel client to lock the client's lock,
re-arrange things so that all of the locking can be internal to
RedClient methods. So instead of having a pre-create validate function,
we simply move the check to red_client_add_channel() and return an error
if a channel already exists. This encapsulates the client implementation
better and also reduces code duplication in RedChannelClient and
DummyChannelClient.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Switch from a Ring to a GList so that we can hide the internals of
RedClient in a future commit.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It's already defined in the same file some lines above.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Make the order of initialization closer to what it was before
conversion to GObject.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
These fields need just channel to be set to be initialized.
Move their initialization to constructor to make sure
they are initialized as soon as possible.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
There was a chance that on error GQueue were not
initialized but an attempt to destroy it is made.
This ensures GQueue is initialized as soon as
possible. Note that red_channel_client_initable_init
is called after all init and construction callbacks.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Internal types use the 'Red' prefix by convention, rather than the
'Spice' prefix. In addition, this type inherits from RedChannel, so
makes the code a lot clearer to call it a 'channel' rather than a
'state'.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
id field of RedChannelClient is not used and marked as debugging.
Also it's value is quite confusing.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
In commit beec1b41, we manually limited this property value in
_set_property(). But there's a simpler way to do it: via the param spec
for the property.
This also means that we can remove the warning log in red_worker_new()
since GObject will automatically warn if a property is assigned a value
outside of its valid range.
Change the minimum and default value for this property from 0 to 1 so
that we always have a primary surface.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
These functions were implementing the same stuff as empty
messages functions provided by RedChannel so reuse them.
The implementation seems a bit different but the result
is the same. Specifically:
- RedEmptyMsgPipeItem::msg is int while RedVerbItem::verb was
uint16_t however this data goes into the message type which
is uint16_t (a 16 bit on the network protocol);
- red_channel_client_send_empty_msg calls
red_channel_client_begin_send_message while red_marshall_verb
does not. However red_marshall_verb is called only by
cursor_channel_send_item and dcc_send_item which always
calls red_channel_client_begin_send_message.
Note that in dcc_send_item when an empty message is sent
red_channel_client_send_message_pending always returns
true;
- when a PipeItem is created red_channel_client_pipe_add_empty_msg
calls red_channel_client_push while red_pipe_add_verb does not.
This actually make very little difference as this kind of item are
never removed from the queue and a push is forced in every case
running the event handler for the stream watch (see
prepare_pipe_add and red_channel_client_event).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
red_drawable_unref declaration was moved to red-parse-qxl.h.
Result is that only RedDispatcher know of the existence of RedWorker.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
No reason why RedWorker should know the capabilities of
DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
When uncapped x264enc can compress the frames beyond recognition in low
bitrate situation. Beyond the set limit the gains are modest and it is
better to drop frames to reduce the bit rate further.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This was introduced with 96e94c6f32
(Convert RedChannel hierarchy to GObject).
The id of CursorChannel/DisplayChannel were always 0 causing
a wrong assertion on the code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
In this case there is not much change but better
to follow that style as all other constructors
do.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This was introduced with 96e94c6f32
(Convert RedChannel hierarchy to GObject).
The handle-acks settings was TRUE for CursorChannel and DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This was introduced with 96e94c6f32
(Convert RedChannel hierarchy to GObject).
The type for "core-interface" property should be
SpiceCoreInterfaceInternal, not SpiceCoreInterface.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
96e94c6f inadvertantly introduced a regression where an assert was
triggered in red_channel_constructed for DummyChannel since DummyChannel
didn't implement any of the expected RedChannel vfuncs. This patch
avoids the assert by assigning some empty vfuncs.
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
red-common.h included utils.h which included red-common.h
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Allow both compiled and non-compiled tests to be used with "make
check". Compiled tests should be added to check_PROGRAMS, and scripts
that do not need to be built should be added to TESTS.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This makes red_monitors_config_item_new() and
red_monitors_config_item_free() symmetric.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
These are already defined in red-channel.h which is included in
red-channel-client.h header.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
vdi_port_read_buf_release is registered passing data as
RedVDIReadBuf*, not RedPipeItem*. Cast opaque to proper
pointer type to avoid the assumption that first field of
RedVDIReadBuf is a RedPipeItem.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This patch prevents a leak in case the function returns early
Found by coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Silence a warning:
reds.c:150:25: warning: 'lock_cs' defined but not used [-Wunused-variable]
static pthread_mutex_t *lock_cs;
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This fixes a compilation error with gcc 4.4.7 on RHEL 6.8.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In preparation for converting RedChannel to GObject, switch to using
RED_CHANNEL()-type macros for casting. For now they just do a regular
cast, but it helps reduce the size of the GObject patch to make it
easier to review.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Move out of red-worker.c. This requires a little bit of minor
refactoring to avoid accessing some RedWorker internals in the
constructor function, etc.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
CursorChannelClient is already defined in cursor-channel-client.h.
This fixes compilation errors with gcc 4.4.7 on RHEL 6.8.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
When MainChannelClient was split to a separate file, the responsibility
for incrementing this field was supposed to belong to the MainChannel
function (main_channel_connect_semi_seamless()), but by mistake it was
incremented both there and in the client function
(main_channel_client_connect_semi_seamless()).
The bug was introduced in a11b785f19
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Convert the RedChannelClient heirarchy into GObjects. Since the existing
constructors could fail and return NULL, I inherited the base channel
client from GInitable, which introduces a dependency on gio.
When using private structs with GObject, there's a maximum size of (I
think) 64k, which was exceeded by some of the private structs. To avoid
this limitation I changed some members to dynamically allocated.
Attempt to use consistent naming.
Usually we use surface name for RedSurface.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Attempt to use consistent naming.
Usually we use surface name for RedSurface so make sure
code reader do not get confused using a different name
for RedSurfaceCmd.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This function deal only with Stream.
Also the name was misleading and was not clear if it detached the stream
from the DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Currently this is simply avoided by the fact that Virgl with 3d means
Unix socket. Once you enable (in Qemu) tcp sockets this message will
be added to all clients (supposing multiple clients) so potentially
will be in all queues. The same check is done for dcc_gl_scanout_item_new.
dcc_gl_scanout_item_new is called when Qemu calls spice_qxl_gl_draw_async.
Technically a client can support SPICE_DISPLAY_CAP_GL_SCANOUT but server
cannot send the DRM prime directly as this require a unix socket so
if the test for SPICE_DISPLAY_CAP_GL_SCANOUT is done here it make sense
to do the check for the socket type too.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Move large buffer field at the end of structure.
Due to the way machine address memory this usually can reduce code size
and make program sligthly faster.
Actually reduce size by 100 bytes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
In case of invalid value the original compression is unchanged.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
These fields were added in a32e90257e
as part of the multiple client support and were never used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
All other options are documented using initial capital case letter.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
serial was the future serial to send while last_sent_serial was the
last sent.
serial sent started from 1.
To make sure sequence variable is updated just before sending the
message, not every message prepared.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Avoid negative syntax. Also could prevent some memory problem is number
get too big.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Currently on Linux PRIu64 and SCNu64 are the same but just to make
sure in the future use the correct macros.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Do the check after replay_fscanf to make sure everything
is fine before calling red_replay_compat_drawable or
red_replay_native_drawable.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Change the return to ssize_t to be able to distinguish from
empty buffer to error.
Check result returned and avoid continuing potentially
deferencing NULL pointers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
To check fscanf read all needed information a dummy "%n" is appended
to any string and the value stored there is tested. This as scanf family
could return a valid value but not entirely process the string so
adding a "%n" and checking this was processed make sure all expected
string is found.
The code to check for a specific string is now a bit more complicated
as replay_fscanf use a macro which append a constant string.
The "error" field is used to mark any error happened, so in most cases
there is no explicit check beside when this could cause a problem
(for instance results of replay_fscanf used which would result in
uninitialised variable usage).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Allocations are kept into a GList to be able to free in case some
errors happened.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
FOREACH_DCC should be more DisplayChannel related.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
