Leak detectors did not manage to find leaks, possibly as double list
have all elements likely with a pointer to them.
The reference from the agent is necessary for inserting it into
the list.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
CentOS 7 compiler generate this warning:
glz-encoder-dict.c: In function 'glz_dictionary_pre_encode':
glz-encoder-dict.c:516:30: error: 'prev_seg_id' may be used uninitialized in this function [-Werror=maybe-uninitialized]
dict->window.segs[prev_seg_id].next = seg_id;
^
glz-encoder-dict.c:492:22: note: 'prev_seg_id' was declared here
uint32_t seg_id, prev_seg_id;
^
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
G_PID_FORMAT was only added in glib 2.50.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
SPICE expects to have each frame in a single message.
So far the stream-device did not do that.
That works fine for H264 streams but not for MJPEG.
The client handles by itself MJPEG streams, and not via
gstreamer, and is assuming that a message contains the
whole frame. Since it currently not, using spice-streaming-agent
with MJPEG plugin, confuses the client which burns CPU
till it fails and keeps complaining:
"GSpice-CRITICAL **: 15:53:36.984: need more input data"
This patch fixes that, by reading the whole message from the
device (the streaming agent) and sending it over to the client.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It's currently using a GPLv2+ header, which was probably a mistake given
the project overall license. It was created by a Red Hat employee, and
only modified by Red Hat employees since then, so the (c) Red Hat is
correct, and there are no other copyright holders to contact.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
self_bitmap flag is used for some complex drawing not possible
by QXL_DRAW_COPY commands. Having this flag set causes
spice-server do draw part of the screen, copy that part on new
allocated image and reduce network optimisations with no visual
changes.
Some drivers (like Windows 10 DOD) set this flag by mistake for
this command so reset it.
More details follow.
The self_bitmap flag is used for some drawing command requiring to mix
the frame buffer with some other image. For this specific
QXL_DRAW_COPY command self_bitmap is used by spice-server code during
cachine/sending (the reason for the cache is to cache images sent to
client so the relationship between the two parts of the code).
However the self_bitmap_image (an image created in spice-server if
this flags is set) is used only if src_bitmap of SpiceCopy structure
(the structure used to store the QXL_DRAW_COPY command inside
spice-server) is NULL. But in red_get_copy_ptr (red-parse-qxl.c, the
function that parse the QXL_DRAW_COPY command form the QXL device)
not having a src_bitmap is considered an error so the
self_bitmap_image won't be used.
Why this flag affects network performance?
When spice-server see this flag it update the frame buffer according
to the pending commands (commands to be sent or still to be drawn on
frame buffer). spice-server maintain a tree of commands used to reduce
rendering and command to send. More or less if a command is covering
other commands (for instance filling the entire screen with a single
color) the pending commands can be removed from the queue and not sent
to the client. However when an update of the frame buffer is requested
spice-server update the frame buffer removing the commands from the
tree but not from the client queue.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Pointers to memory allocated in user space are never NULL.
The only exception can be if you explicitly map memory at zero.
There is however no reasons for such requirement and this practise
was also removed from Linux due to security reasons.
This API looks copied from a kernel environment where valid virtual
addresses can be NULL.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
In case we pass something like "spice:mjpeg$%*" the last part is
ignore making the string parse correctly.
A single pair should end by either string terminator or pair terminator.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These functions are in the standard C library, not well known
but quite useful for parsing strings.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Now warnings are printed through g_warning which causes the test to
fail. We need to use g_test_expect_message() to prevent that failure.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It was only used twice, for what looks like adhoc debugging. This commit
removes it, similarly to what was done for some spice_printerr() calls.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The remaining occurrences of spice_printerr() are warnings when
something unexpected happens, they can be replaced with g_warning() so
that users of spice-server can redirect them with
g_log_set_default_handler().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Depending on the context, we want to output a warning or just a debug
log.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
These calls seem to have been added for debugging for a very specific
purpose. At the very least, they should have been using g_debug() rather
than spice_printerr(). This commit removes these.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
With last changes are just used once and are straight forward.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Do not extract all components and compare one by one, can be easily
compared together.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The macros for both depth are the same, reuse the definition.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When a new record channel is added, the code relies on a snd_send() call
in record_channel_client_constructed() to send RECORD_START to the
client. However, at this point, snd_send() is non-functional because
the red_channel_client_pipe_add() call it makes is a no-op because
prepare_pipe_add() makes a connection check through
red_channel_client_is_connected() queueing the item. This connection
check returns FALSE at ::constructed() time as the channel client will
only become connected towards the end of
red_channel_client_initable_init() which runs after the object
instantiation is complete.
This causes a bug where starting recording and then
disconnecting/reconnecting the client does not successfully reenable
recording. This is a regression introduced by commit d8dc09
'sound: Convert SndChannelClient to RedChannelClient'
This commit solves this issue by making PlaybackChannelClient and
RecordChannelClient implement GInitable, and move the code interacting
with the client in their _initable_init() function, as at this point the
objects will be able to send data.
https://bugzilla.redhat.com/show_bug.cgi?id=1549132
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The result of this function is always cast to a pointer, there
is no reason to return an integer.
This API looks copied from a kernel environment where virtual
addresses can have different sizes compare to pointers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Define JpegEncoderContext as an abstract structure.
This allows to reduce casts.
Also remove some alignment warnings on some architecture like mips.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
On some systems you need to call g_spawn_close_pid after
spawning a process to avoid leaks (currently Windows).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
In some environment pthread.h is not defined but its definitions
are used in some headers.
Actually happens using MingW.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Just style change. Invert the if to exit earlier.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe de Dinechin <dinechin@redhat.com>
These constants are meant to be used in format string for size_t
types. Use them for portability.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
It was probably meant to be used as a "user_data" argument for the
various callbacks, but turns out not to be used.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Client callbacks in sound channels do not use registered
data so don't pass a valid pointer making clear from
source that the parameter is not used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Without an explicit call to SSL_CTX_set_ecdh_auto(reds->ctx, 1), OpenSSL
1.0 (still used by el7) would not use ECDH ciphers (this is now
automatic with OpenSSL 1.1.0). This commit adds this missing call. It's
based on a suggestion from David Jasa
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1566597
Most of pipe items use this name for the base field.
This also allows to use SPICE_UPCAST macros instead of a long
SPICE_CONTAINEROF.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Although capabilities inside link message are handled as arrays
of 4 bytes unsigned integers we don't need capabilities to be
aligned to 4 bytes just to call g_memdup so use a pointer to
uint8_t instead.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This array is just used locally in red_channel_client_handle_outgoing
so declare it there.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Removes debug messages that are logged on every draw, spamming the log
excessively when debugging.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
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>
