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>
