If file was compiled with DEBUG defined code did not compile.
Update code of the surrounded section.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
I've never seen used and Dispatcher is quite a code piece of
code, hard to break it.
I also tried to enable and it does not even compile anymore.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
We just created the object, we know is a RedCharDeviceSmartcard,
avoid to cast the pointer using RED_CHAR_DEVICE_SMARTCARD.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
These structures are not used that extensively, do not include
in the common header.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Not used in the header.
Also we reduced a lot the usage of these structures.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
red_channel_client_destroy is not called anymore from RedClient and
should not so update the comments.
red_channel_client_destroy, compared to other XXX_destroy functions
did not unreference the object but was just disconnecting the
client channel so use red_channel_client_disconnect instead.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The object is already destroyed calling red_channel_disconnect_client.
The difference is that red_channel_disconnect_client does it in
the right thread while red_channel_client_destroy here attempts to
do potentially from another thread. This however at this stage
(after already being disconnected) is not an issue, just redundant
and potentially unsafe if red_channel_client_destroy change in
a way to have issue with multiple threads.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The field is only set and never checked.
Cascading cleaning up red_channel_client_set_destroying.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
"RedChannelClient::on_disconnect", which for SpiceVMC is bound to
"spicevmc_red_channel_client_on_disconnect", is called while the
RedChannel object is disconnecting. More precisely is called after
the object has been removed from RedChannel causing
"red_channel_client_is_connected" to be false.
Calling "red_channel_client_destroy" this will lead
to a no-op as function will set "destroying" and call
"red_channel_client_disconnect" which will just exit finding
"red_channel_client_is_connected" false.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Minimal style change.
Allows to easier change the size of the buffer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Francesco Giudici <fgiudici@redhat.com>
Minimal style change.
Allows to easier change the size of the buffer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Francesco Giudici <fgiudici@redhat.com>
Allows to specify abstract Unix sockets addresses.
These Unix sockets are supported on Linux and allows to not
have file system names.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Kevin Pouget <kpouget@redhat.com>
It is not guaranteed that all the messages will be sent on a single
push call, unless this is done very early.
Although currently this function is called very early remove
this hidden assumption in the code.
"ping_id" is just incremented every time a message is sent to
client (in "main_channel_marshall_ping") so the formulae
"mcc->priv->ping_id - 2" was there to get the id of two messages
ago (the first message queued in this function) assuming all
messages were pushed at the time the formulae is used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
FALSE is the default value, no need to set explicitly.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Use new common.m4 include file to make easier to integrate
with spice-common repository.
The new include will allow for instance spice-common to
add additional dependencies without changes (or minor) to
spice-server.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Kevin Pouget <kpouget@redhat.com>
main_channel_client_push_ping is never called with mcc NULL.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
Instead of freeing it in both reds_handle_main_link and
reds_handle_other_links free it in reds_handle_link.
This will reduce the chances code is changed adding a leak in some paths.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Julien Rope <jrope@redhat.com>
Instead of freeing manually the field and then detaching from
the structure just detach only if retained.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Due to aliasing using pointers lead to some sub-optimization.
Use local variable and then write them to output to improve performances.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Julien Rope <jrope@redhat.com>
Since 03d46e9e "build-sys: Raise glib requirement to 2.38" in
2019-02-05
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
g_assert_null, g_assert_true and g_assert_false are all defined in
GLib 2.38
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Basically not run since 03d46e9e "build-sys: Raise glib requirement to
2.38" in 2019-02-05
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Not used since 03d46e9e "build-sys: Raise glib requirement to 2.38" in
2019-02-05
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The record file uses fprintf to record commands and attributes.
On Windows line terminator uses ASCII format ("\r\n") while on
Linux it used UNIX format ("\n"). Open file as binary to use the
same line terminator on both systems.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Francesco Giudici <fgiudici@redhat.com>
Channel client creation can fail in some situation.
For instance if during a migration the client is disconnected.
In most cases this is ignored (this is usually logged in
red_channel_client_initable_init) but not in case of
CursorChannelClient and StreamChannelClient.
This fixes rhbz#1788757.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Julien Rope <jrope@redhat.com>
red_channel_client_shutdown remove the watch and shutdown the
socket. Reuse in red_channel_client_disconnect.
Calling shutdown will close the connection earlier.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
This information is retrieved multiple time.
Avoid to use just g_object_get which is dynamic so potentially
unsafe and slow.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
MainChannelClient is used by other clients to store some data
so should not disappear if other clients are still present.
Keep a owning reference to it and release after RedClient is
released.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Disconnecting a single channel from the client caused the server to
keep a stale channel client (RedChannelClient object) till the client
(RedClient object) entirely disconnected, that is the channel client
is disconnected but still in the list preventing new connections.
Calling red_client_remove_channel from red_channel_client_disconnect
fixes this last issue.
An issue was that was not clear how the ownership were managed. When
red_client_destroy was called red_channel_client_destroy was called
which freed the RedChannelClient object so this should imply
ownership.
However same red_channel_client_destroy call was attempted by
RedChannel using its list (red_channel_destroy). Basically the two
pointers (the one from the channel and the one from the client) were
considered as one ownership. To avoid the confusion now the client
list always decrement the counter.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
The channels list was not protected by a lock in red_client_destroy.
This could cause for instance a RedChannelClient object to be
created while scanning the list so potentially modifying the
list while scanning with all race issues.
Consider a client which attempt to connect to a new channel and
then for some reason close the main channel:
- the new channel connection is handled by the main thread;
- the relative RCC will be passed to red_channel_connect which
can decide to continue the initialization in another thread
(this happens currently for CursorChannelClient and
DisplayChannelClient);
- the main thread will catch the main channel disconnection and
call main_dispatcher_client_disconnect
(from main_channel_client_on_disconnect);
- main thread calls reds_client_disconnect;
- reds_client_disconnect calls red_client_destroy;
- now we have 2 thread which will attempt to change RedClient::channels
list, one protected by the mutex the other not.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Create some traffic on the connection to avoid potential timeout
on some proxies implementation which require some TCP data traffic.
The timeout used by default is quite big (5 minutes) to reduce network
traffic.
In case connectivity monitoring is enabled or latency monitor is
requested the timeout is reduced to the old default.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This is a preparatory patch.
The "latency_monitor" feature allows to measure the latency of a
specific channel client.
Currently the measure is attempted every PING_TEST_TIMEOUT_MS which
is a constant.
To be able to use a different frequency allows to change this for every
channel client.
This feature will be also used to create some traffic on the connection
to allows some sort of keep-alive to overcome some proxy implementation
which requires some TCP data traffic.
This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1719736.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
"&rcc->priv->connectivity_monitor" is cached in "monitor" variable.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
These functions already check for NULL.
They are used mainly for cleanup, so cold path of code so speed
in case of NULL is not important (and usually should not be NULL).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
For some reason under Fedora 31 the g_socket_client_connect call
from test-listen test caused Valgrind to detect a leak.
Note that the like at gtype.c (g_socket_client_connect) specifically
marks a memory block as malloc-like. Not sure if it's an issue
with Valgrind or with GLib, as a workaround disable the leak report.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This suppression was present in former glib.supp version however
the suppression in glib.supp was updated to only catch "reachable"
leaks.
However in test-listen test the allocation is done in a thread
causing the leak to become "definite".
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
OpenGL and Slirp requirement were removed much time ago.
OpenGL was removed by c5c176a5c7
(cfr "server: remove OpenGL", Set 2013).
Slirp was removed by ef9a8bf053
(cfr "Remove tunneling support", Oct 2013).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
The threading API for 1.0 was replaced with the THREADID API.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Missing headers for BN_ and RSA_ functions.
Initialization is deprecated with 1.1.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Allow to modify/cancel timers/watches without having to retrieve
the code interface.
This will make sure that you are not using the wrong interface.
Simplify code to deal with timers/watches.
Remove the requirement to have the core interface available
for removing timers/watches.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Put more event loop code in event-loop.c.
This is a preparation patch for the next one.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The buffer could change inside smartcard_read_buf_prepare.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Using coverage utility exercise more code paths:
- message from channel with wrong type;
- remove message from channel with already removed reader;
- init message from channel (ignored);
- data from devices, ADPU;
- error from device;
- messages split in different ways;
- invalid reader_id values.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Create Smardcard device.
Connect to it and test some messages are parsed and processed
as expected.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Allows to reuse code for emulating a character device.
It will be used for Smardcard test.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This patch handles the scenario when a single read to guest device
brings multiple requests to be handled. When this happens, we will
iterate till all requests are handled and no more requests can be read
from guest device.
If the remaining buffer contains a full request we don't need to read
other bytes (note that there could be no bytes left), just parse the
request.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Use memmove instead of memcpy as the buffer can overlap if the second
request if bigger than the first.
"buf_pos" points to the point of the buffer after we read, if we want
the first part of the next request is "buf_pos - remaining".
Same consideration setting "buf_pos" for the next iteration.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Avoid client to trigger crash. The value of smartcard_readers_get
is checked for NULL so returning it it's not an issue.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
More coherent. Also it's not good for a library to output on
standard output.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
red_channel_client_handle_message is called after parsing the
message so it's not necessary to check it again or parse manually.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
As base messages require parsing better channels should always use
the generated parser.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The generated code handle possible endianess mismatch and check
for message format.
The copy back to "write_buf" allows to use that buffer to send
data back to device.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
"name" parameter of smartcard_channel_client_add_reader it's not
used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
"self-tokens" property is 64 bit and must be passed as 64 bit on
32 bit machines to avoid memory corruptions.
This was introduced by 01de3b8922 ("spicevmc: Avoids DoS if
guest device is not able to get data faster enough"), detected by CI.
It caused this error (split into multiple lines):
(./test-leaks:15879): GLib-GObject-CRITICAL **: 14:03:59.650: \
g_object_new_is_valid_property: object class 'RedCharDeviceSpiceVmc' has \
no property named '\xb0/@\xf3\u0001'
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This fix half (one direction) of
https://gitlab.freedesktop.org/spice/spice/issues/29.
Specifically if you have attempt to transfer a file from the client
using WebDAV.
Previously the queue to the device was unbound. If device was not
getting data fast enough the server started queuing data.
To easily test this you can suspend the WebDAV daemon while transferring
a big file from the client.
To simplify the code and reduce the changes server buffers are
used. This as client token implementation is written with agent
in mind. While when we exhaust server tokens RedCharDevice doesn't
close the client when we exhaust client tokens RedCharDevice closes
the client which in this case it's not wanted.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
If the client is keeping sending data while we can't handle them
(for instance because we need to forward to a device but the
device is not fast enough to receive that amount of data) allows
to stop RedChannelClient to read data.
This after a bit will stop the client sending data as its output
buffer will become full.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This fix half (one direction) of
https://gitlab.freedesktop.org/spice/spice/issues/29.
Specifically if you have attempt to transfer a file to the client
using WebDAV.
Previously the queue to the client was unbound. If client was not
getting data fast enough the server started queuing data.
To easily test this you can use a tool to limit bandwidth and
transfer a big file (I used a "dd if=/dev/zero bs=1M of=test") from
the guest.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
As we don't use any token there's no reason to not queue directly instead
of passing through RedCharDevice.
This will make easier to limit the queue which currently is unlimited.
RedCharDevice flow control has some problems:
- it's really designed with VDI in mind. This for SpiceVMC would cause
code implementation to be confusing having to satisfy the agent token
handling;
- it's using items as unit not allowing counting bytes;
- it duplicates some queue management between RedChannelClient;
- it's broken (see comments in char-device.h);
- it forces "clients" to behave in some way not altering for instance the
queued items (which is very useful if items can be collapsed together);
- it replicates the token handling on the device queue too. This could
seems right but is not that if you have a network card going @ 1 GBit/s
you are able to upload more than 1 Gbit/s just using multiple
connections. The kernel will use a single queue for the network interface
limiting and sharing de facto the various buffers between the multiple
connections.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
It does not make sense to have a graphic card without a monitor.
In spice_qxl_set_max_monitors we prevent to set 0 monitors, do
the same in spice_qxl_set_device_info.
This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1691721.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
As now is an opaque type for RedCharDevice use the type that
better suits us.
This avoid useless conversions or look ups.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
RedClient was an opaque structure for RedCharDevice.
It started to be used when RedsState started to contain all
the global state.
Make it opaque again using a new RedCharDeviceClientOpaque.
The RedCharDeviceClientOpaque define in the header allows users
of the class to override the type to get a more safe type
than RedClient.
The define at the beginning of C file is to make sure we don't
use the opaque type as a specific one.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
No much reason for not inlining it, it's quite small and do
not reduce readability.
Note that spice_server_migrate_switch is deprecated and not
used by Qemu since commit 67be6726b6459472103ee87ceaf2e8e97c154965
(cfr "spice: raise requirement to 0.12" September 2012).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This patch enables the SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE
capability for the stream-channel.
video_stream_parse_preferred_codecs: new function for parsing the
SPICE protocol message. This code used to in inside
dcc_handle_preferred_video_codec_type.
struct StreamChannelClient::client_preferred_video_codecs: new field.
Signed-off-by: Kevin Pouget <kpouget@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
spice_assert is a macro and covscan reports that:
Argument "++twice_remove_called" of spice_assert() has a side effect.
Doesn't matter if there is a side effects or not,
it's a good practice and it makes covscan happy, so
increment the variable one line above.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
There may to be another function (cache_alt_names) between
gnutls_x509_ext_import_subject_alt_names and
gnutls_x509_crt_import
cache_alt_names is a static function in gnutls/lib/x509/x509.c
used only in gnutls_x509_crt_import and may be inlined by
the compiler.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is currently more style patch as the "value" field is the
first field of SpiceStatNode structure, so has offset 0. However
to compute the containing structure it better to use the proper
macro to avoid confusion.
If the offset won't be 0 the subtraction would compute the
wrong pointer as the offset is expressed in bytes but the
element size are uint64_t.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
spice_server_migrate_client_state was removed by
commit 3c6b4e415f
Author: Marc-André Lureau <marcandre.lureau@gmail.com>
Date: Fri Oct 24 17:16:35 2014 +0200
Remove spice-experimental
Remove unneded symbols that nobody should be using anyway.
ABI is modified with this patch, but the library version is not bumped.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
The GLZ code is basically LZ code (in spice-common) sharing image
segments between multiple images.
The code for RLE check in LZ (common/lz_compress_tmpl.c) is dealing
with both RLE and dictionary matches being:
if (!distance) {
/* zero distance means a run */
PIXEL x = *ref;
while ((ip < ip_bound) && (ref < ref_limit)) { // TODO: maybe separate a run from
// the same seg or from different
// ones in order to spare
// ref < ref_limit
in GLZ that part was copied to RLE part removing the need for the
second segment ("ref"). However the comment and setting ref variables
were not removed. Remove the old code to avoid confusions.
This also remove a Covscan warning as ref_limit was set not not used
(reported by Uri Lublin).
The warning was:
CLANG warning: "Value stored to 'ref_limit' is never read"
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
On LLP64 platforms (like Windows) a virtual address cannot be
represented by a "unsigned long" type, so use uintptr_t which is
defined as an integral type large like a pointer.
"address_delta" and "addr_delta" are a difference of pointers so use
same type size.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
On 32 systems pointers are 32 bit while QXLPHYSICAL is always
64 bit.
Using pointer -> intptr_t -> QXLPHYSICAL conversion cause pointers
to have higher 32 bit set to 1 if the address is >= 0x80000000.
This is possible depending on address space.
The QXLPHYSICAL is split in 3 sections:
- slot ID;
- generation;
- virtual address.
Current utility using record file (spice-server-replay) set slot ID
and generation to 0 so if the higher bits become all 1 slot ID and
generation won't be 0 causing the utility to fail.
Use pointer -> uintptr_t -> QXLPHYSICAL conversion to avoid this
issue.
Note that for opposite conversion (QXLPHYSICAL_TO_PTR) the conversion
does not change, type is changed just for consistency.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
Threads are joined by spice_server_destroy, just need to release
last resources.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
There are no much data other than the buffer, reduce the
allocations.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Just a style change, return earlier to avoid some indentation.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Instead of having to manually register the file descriptor and
than need to call dispatcher_handle_recv_read just provide a single
API to create the watch.
This has some advantage:
- replace 2 API with 1;
- code reuse for handling the event (removed 2 functions);
- avoid the caller to use the file descriptor;
- avoid the caller to register wrong events.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The 2 callers red_char_device_send_to_client_tokens_set and
red_char_device_send_to_client_tokens_add are doing mostly
the same thing so put common code to
red_char_device_send_to_client_tokens_absorb.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The function is called only by red_channel_pipes_new_add.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Unused.
Also the devices should be able to release themselves.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
In some configuration _GNU_SOURCE is defined by the compiler
and defining again cause a warning.
Do not define again to avoid the warning.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
These structure contain only bytes, no need for this attribute.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The structure has no holes, adding this attribute could only
decrease efficiency.
Note that HashEntry structure is used for a large (8MB) array so
this won't affect much possible container size.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The decoding is wrong, the Red and QXL structures are different so
code is not doing what is expected. red-parse-qxl translate from QXL
to Red structures, red-record-qxl saves Red structure to file,
red-replay-qxl is supposed to read from file into QXL directly.
If a Quic image is stored inside QXL memory the layout of the QXLImage
in memory is:
- QXLImageDescriptor
- QXLQUICData
- QXLDataChunk
- first chunk data
and all remaining data in linked QXLDataChunk.
red_replay_image was reading the image as data was all contained in
QXLImage->quic.data however "data" should store the first QXLDataChunk
followed by QXLDataChunk data.
Use proper base_size calling red_replay_data_chunks in order to
initialise the image with the first data chunk correctly.
Not easy to reproduce, the only driver is XDDM which means Windows XP
or similars. To enable QUIC encoding I added "image-compression=quic"
and "streaming-video=off" to "-spice" Qemu option in order to force
QUIC encoding in the guest driver (thanks to Uri Lublin for the help
reproducing it).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Avoid having to destroy and create a new GSource every time
we change event mask.
Interfaces required for this patch are available since GLib 2.36
and are specific to Unix.
On Windows use old implementation.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Do not pass unaligned QXLPHYSICAL but pass a valid pointer and
then cast.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Previously we add suppression to glib.supp file (suppressions from
Glib).
Keep the glib.supp file pristine and add another file specific
for SPICE.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
inttypes.h contains macro for format string while
stdint.h more specifically contains type definitions (like uint8_t).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
1. It was renamed in newer versions to 'threads'
2. The default is 1 anyway
Signed-off-by: Snir Sheriber <ssheribe@redhat.com>
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
'l' is being freed within the loop
Found-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In red_pipe_replace_rendered_drawables_with_images, the
value of pipe_item is re-written on the next iteration.
Since a78a7d2510 pipe_item
is no longer used to control the loop.
Found by Covscan.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Interrupt the video streams when the user changes the preferred
video-codecs (dcc_handle_preferred_video_codec_type) or when the host
admin updates the list of video-codecs allowed
(display_channel_set_video_codecs).
The video streaming will be automatically restarted by spice
video-detection rules.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Do not expose multiple functions to fetch different parts of
internal structure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
The 0 result means success however the function (correctly) could
report a failure if the string is incorrect.
This fixes the test after commit b4150de3cd
("spice_server_set_video_codecs: fail when no codec can be installed").
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com
Public function spice_server_set_video_codecs: return -1 if no
encoder/codec has been installed, instead of always returning 0.
Internal function reds_set_video_codecs_from_string: return the number
of invalid encoder/codec entries found in the input list, and update
the installed pointer with the number of encoder/codec pairs
successfully installed.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
spice_server_get_video_codecs: new public function to query the video
encoders/codecs currently enabled in the spice server. It returns a
string that can be fed to the setter counter
(spice_server_set_video_codecs). The string returned by this function
should be released with spice_server_free_video_codecs.
spice_server_free_video_codecs: new public function to free the the
video codec list returned by spice_server_get_video_codecs.
video_codecs_to_string: new private function to transform an array of
RedVideoCodec into a string.
Signed-off-by: Kevin Pouget <kpouget@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Fedora 30 / gcc 9.1.1 20190503 (Red Hat 9.1.1-1) fails to build
because of this error/warning:
> gstreamer-encoder.c: In function 'set_video_bit_rate':
> gstreamer-encoder.c:518:17: error: taking the absolute value of
> unsigned type 'uint64_t' {aka 'long unsigned int'} has no effect
> [-Werror=absolute-value]
> 518 | } else if (abs(bit_rate - encoder->video_bit_rate) > encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN) {
> | ^~~
> gstreamer-encoder.c:518:17: error: absolute value function 'abs'
> given an argument of type 'uint64_t' {aka 'long unsigned int'}
This patches solves these two warnings:
1) cast the substraction to a signed type (int64_t instead of
uint64_t) to preserve the operation meaning;
2) use a custom version of abs() to avoid data truncation and/or
platform-dependent type lengths (abs/labs/llabs)
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Code dealing with nanosecond timestamps normally uses 64 bit integers
and not long long ints. So this makes it easier to print the value of
expressions using these constants.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The attributes in this case are not used to apply the mask.
Doing so avoid sending garbage from the guest which usually
don't initialise the memory in case the mask is missing.
Guest should have cleared these bytes by its own however doing so
on the server fixes the problem too. Considering that these
command should not appears in newer OSes it's surely not a hot path
code.
These garbage came from video memory of the guest so they don't
contain sensitive data.
This fixes https://gitlab.freedesktop.org/spice/spice-server/issues/25.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
"image_surfaces_get" and "drawables_init" are already defined
in the same file earlier.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
The WebSocket protocol allows 0-size frames so a returned lenth of
0 does not only mean an issue but it's perfectly expected.
This is also required by WebSocket specification.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
Allows to specify and get frame type.
Type and flags are returned calling websocket_read and returned
calling websocket_write or websocket_writev.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
Not strictly needed, client can work even without specifying
that.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
Websocket implementations are required to implement such messages.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
Quite rare case, can only happen with congestion, buffers very low
and some space left in the former packet.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
Ignore spaces before "binary" value.
HTTP allows space before and after the value although usually
browsers implementation start the value with a single ASCII space.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
Currently code don't handle if system can't sent the
header in a single write command.
Don't cause abort but just close the connection.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
"len" is not always the full remainder (consider the case when
we are writing a partial frame).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
These were introduced moving code around.
No more reason to copy, just use directly structure fields.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
"type" is just 8 bit.
"frame_ready" and "masked" as booleans.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
Move websocket structure declarations to C file.
Make some functions static as now not used externally.
Introduce a websocket_free function for symmetry.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
Less coupling. This is a preparation for next patch.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
Intention is to make private in websockets.c and reduce
changes in red-stream.c
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
Use g_memdup instead of manual copy.
Trim the original iov if necessary.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
We do this by auto detecting the inbound http(s) 'GET' and probing
for a well formulated WebSocket binary connection, such as used
by the spice-html5 client. If detected, we implement a set of
cover functions that abstract the read/write/writev functions,
in a fashion similar to the SASL implementation.
This includes a limited implementation of the WebSocket protocol,
sufficient for our purposes.
Signed-off-by: Jeremy White <jwhite@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
SSL_CTX_set_ecdh_auto is not defined in some old versions of OpenSSL
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
The G_PID_FORMAT constant is defined only if GLib does not support it.
The constant was wrongly defined.
Jessie Debian 32 shows this issue (printf format error).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jeremy White <jwhite@codeweavers.com>
The formula is here to make sure glyph is aligned to 4 bytes so
tell to the compiler to avoid a warning.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This patch came from some experiments using an emulated MIPS machine.
On such architecture due to not supporting alignment access the
compiler is more strict about conversion complaining with some
pointer casts. Use different conversion to avoid these warnings.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
On Windows Fedora 30 reports these errors:
In file included from /usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/crypto.h:29,
from /usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/bio.h:20,
from /usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/err.h:21,
from red-stream.c:31:
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/x509.h:75:1: error: pasting "stack_st_" and "(" does not give a valid preprocessing token
DEFINE_STACK_OF(X509_NAME)
^~~~~~~~~~~~~~~
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/x509.h:75:17: error: expected ')' before numeric constant
DEFINE_STACK_OF(X509_NAME)
^~~~~~~~~
...
This is due to missing X509_NAME definition by Windows headers.
Including the network header on Windows solves this problem.
This is consistent with reds.c file.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Based on a patch from Hongzhi.Song <hongzhi.song@windriver.com>.
There are following compile errors on Linux 32bit system with -Werror
for gcc.
red-channel.c:207:73: error: format '%x' expects argument of type
'unsigned int', but argument 7 has type 'long unsigned int' [-Werror=format=]
|207| red_channel_debug(self, "thread_id 0x%" G_GSIZE_MODIFIER "x",
~~~~~~~~~~~~~~~~~~~~~^
self->priv->thread_id);
~~~~~~~~~~~~~~~~~~~~~^
pthread_t is an opaque type so there is no easy way to make the printf
format string portable. However the type must be comparable in C so this
(excluding floating point which does not make sense) means an integral type
or a pointer.
Under *BSD this is a pointer so can be converted without loosing precision
to void*.
Under Linux this is a "unsigned long int" type, being Linux ILP32 or LP64
this means that the size of pthread_t is the same as size_t so can be
converted without loosing precision to void*.
Under MingW (the pthread port to Windows) this is a uintptr_t type that is
can be converted without loosing precision to void*.
On any potential future platforms if the integral type is smaller than a
uintptr_t type (which has the same size of void*) the cast should trigger a
warning and if not won't loose precision; the integral type is unlikely to
be bigger than a pointer and likely the cast would trigger a warning.
The cast on read_binary (red-replay-qxl.c) is safe, "*size" is a size_t
while "strm.total_out" is the number of written bytes in a buffer which
cannot be bigger than a size_t.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
On ppc64el and armhf the handling of "1 << mem_info.memslot_id_shift"
will end up beign a zero which breaks the test.
Marking the implicit value 1 as a 64 bit value (to match the uint64_t
target) fixes the issue.
Fixes#31
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This constant fits in a regular 32 bit signed integer so it does not
need the suffix.
It is also not used in expressions that may overflow.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The Cursor/DisplayChannel is not expecting large messages (which are
protocol violations).
This fixes https://gitlab.freedesktop.org/spice/spice-server/issues/11.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This reduces code duplication and passing the MJpegEncoder object
makes it possible to modify the playback calculation without adding
more arguments.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It makes no sense to expect average frame sizes anywhere close to 2GB.
But then make sure to avoid arithmetic overflows.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The source framerate is as important as the resolution when trying to
understand if the system should be fast enough to encode the video
stream in real time.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This header was removed in 2014 (3c6b4e415) as deprecated and added again
in 2015 (2e88eb705) as causing some issue with former Qemu versions.
After 4 years remove again, now there should not be any usage of it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
This way all the minimum delay calculation is in one place and this
makes gstreamer's implementation closer to the mjpeg one.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This reinstates the reds_enable_mm_time() call in do_spice_init()
that was removed by commit c541d7e29d.
We send mm_time adjustments to the client whenever there is no audio
playback. There is no audio playback on startup. Therefore
mm_time_enabled must be true on startup. QED.
This fixes adjusting the client mm_time whenever playing a silent
video (or full desktop stream) when no sound has been played before
such as when using Xspice, booting an OS with no startup or login
jingle, or possibly when migrating a VM (per commit 1c154ea5ec).
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Copy to/from unaligned field to avoid potential unaligned access.
Although it adds a copy it's not in a hot path (migration) and
the structure is pretty small.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This change prevent a warning issued by GCC 9 and potentially
other compilers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Although this feature can be ported to Windows doing so would
require the usage of g_spawn_async_with_fds, which is only available
in GLib 2.58 or some specific Win32 code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Set correctly errno to make callers handle correctly encrypted
traffic.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
test-stream test is passing file descriptor using Unix socket.
test-stat-file needs some porting work of mmap feature.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Replace poll call with select.
As socket is set to non-blocking we must support it so if
we detect an EAGAIN error wait for data.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Allows to easier port socketpair.
Windows does not have this function, we need to create a pair
using 2 internet sockets and connecting one to the other.
The SPICE core interface implementation provided by Qemu under
Windows requires, under Windows, to provide SOCKET handles
so pipes or other Windows handles won't work.
Windows does not provide a socketpair function so use this
replacement.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Between Unix and Windows socket are quite different:
- on Windows sockets have a different namespace from C file
descriptors so you can't use read/write/close or similar functions;
- errors are not stored in errno but you must be read/write the
errors with specific function;
- sometimes sockets are put in non-blocking mode automatically
calling some functions;
- SOCKET type is 64 bit on Windows 64 which does not fit technically
in an int. Is however safe to assume them to fit in an int.
So encapsulate the socket APIs in some definition to make easier
and more safe to deal with them.
Where the portability to Windows would make to code more offuscated a Unix
style was preferred. For instance if errors are detected errno is set from
Windows socket error instead of changing all code handling.
Fortunately on Windows Qemu core interface accepts socket (but not
other types like C file descriptors!).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
"interface" and "MAX_MONITORS" are defined in some Windows system
headers causing garbage code to be fed to the compiler.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This should always be defined and including config.h is a requirement.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
We can not consider the qemu led state to be reliable. It by default has
50ms of delay, so if we want to achieve something reliable, this won't do.
We need to keep our own internal state, and consider it as reliable.
We update it immediately after receiving the key presses, meaning that this
is now the future state of the guest.
When we receive a keymap event, we check against this 'ideal' state and
only update the guest if we 'counted' that it won't have the correct state.
When the guest notifies its change, the modifiers_watch is supposed to
fix any wrong state.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Do not check it after assigning to reds->config->image_compression,
check the value as soon as possible.
This prevent potential invalid settings.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
DisplayChannelClient get them directly from reds (they are changed
only during initialisation so they can be read freely from any
thread).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Rather than having an API to register client callbacks for each channel
type, make them vfuncs.
Since the client callbacks are registered identically for each channel
of the same type, it doesn't make sense for to require these to be
registered separately for each object. It's cleaner to have these be
per-class properties, so they've been converted to virtual functions.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
No reasons to expose red_qxl_is_running, this was used to not
send capability is the state was not running.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This is a preparatory patch that states the running property in QXLState
and provides accessor functions that allows us to check whether the QXL
device is running from different threads.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Currently channel threading/handling is spread between RedQxl,
RedWorker and RedChannel.
Move more to RedChannel simplify RedQxl and RedWorker.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The only way to add new message to Dispatcher was to register
using a number. These numbers corresponded to array indexes.
This is good if the list of messages is allocated statically
and contiguously, on the contrary this method is not that
flexible.
Writing a header of 4 or 16 bytes using system call does not
make much difference so pass all message information in the
payload header.
A new dispatcher_send_message_custom function allows to send
a message passing all message information, including the
pointer to the handler.
This will allow for instance a Dispatcher associate to a given
thread to be reused by different classes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
If worker->qxl->id is bigger than 0x7ffffff (in other words, it's a
negative signed int) then
printf(worker_str, "display[%d]", worker->qxl->id);
will need:
"display[]" -> 9 bytes
%d -> 11 bytes
The trailing \0 will thus overflow our 20 bytes destination.
As QXLInstance::id should be an unsigned int, this commit changes the
format string to use %u. This also switches to snprintf.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
No reason to pass through MainDispatcher, the purpose of
MainDispatcher is to call reds functions from the right thread.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
To avoid potential regressions, check it only if extra checks are
enabled.
This allows to check previous "Move channel registration to constructed
vfunc" commit.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Snir Sheriber <ssheribe@redhat.com>
We should not reuse the same interface twice as doing so will
cause dangling pointers.
Unregister it at every iteration.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
I experienced some crashes with qemu 3.1.0 compiled with libspice-server
0.14.0 on Gentoo.
The problem reproduced reliably with a guest running Ubuntu 18.04.2 LTS.
If I connect a viewer at system startup, I would get a crash just after
the fade-in of the login prompt in GDM.
Interestingly, I usually was unable to reproduce the issue if I waited
to connect until after the greeter was fully displayed.
The patch I used to correct the issue for me applies to the master
branch cleanly, so I suspect the problem may still exist.
The only other references to this issue I could find were two abrt
reports in CentOS:
https://bugs.centos.org/view.php?id=15171https://bugs.centos.org/view.php?id=15441
I'm not sure if the agent->video_encoder is supposed to be guaranteed to
exist when this function is called.
Signed-off-by: Douglas Paul <doug@bogon.ca>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
For the Display Channel and the Cursor channel, move the call to
reds_register_channel() to the _constructed() vfunc rather than calling
it explicitly from RedWorker. This matches what other channels do.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The difference is subtle but compared to what client receives, this
could help identify values set to the wrong head, e.g:
First we received:
| display-channel.c:180:monitors_config_debug: monitors config count:2 max:4
| display-channel.c:184:monitors_config_debug: +0+0 1015x805
| display-channel.c:184:monitors_config_debug: +1015+0 1024x740
And then:
| display-channel.c:180:monitors_config_debug: monitors config count:3 max:4
| display-channel.c:184:monitors_config_debug: +0+0 1015x805
| display-channel.c:184:monitors_config_debug: +0+0 0x0
| display-channel.c:184:monitors_config_debug: +1015+0 1024x740
In the first debug it would be helpful to have "head 0" and "head 1",
to point out the temporary error in monitor's config message.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
For coding style consistency, use 'bool' when we want to represent a
boolean value.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Only send the graphics device display info to agents that advertise the
VD_AGENT_CAP_GRAPHICS_DEVICE_INFO capability
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Make this a RedsState member function rather than a standalone function.
This means that we simply pass RedsState* as an argument rather than the
internal member variables of RedsState. This enables the following
commit which handles the VD_AGENT_CAP_GRAPHICS_DEVICE_INFO capability to
avoid sending graphics device info to agents that do not support it.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
usleep under Windows does not seem to have the required precision.
Use milliseconds and adjust check times according.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Formatting string should be compatible with GLib.
GLib uses formatting types compatible with GNU.
For Linux this is not an issue as both systems (like a printf) and
GLib one uses the same formatting type. However on Windows they
differs potentially causing issues.
This is also make worse as GLib 2.58 changed format attribute from
__printf__ to gnu_printf (Microsoft compatibility formats like %I64d
are still supported but you'll get warnings using GCC/Clang
compilers).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
RedPipeItem is not using the Ring structures anymore.
Also is not using GLib functionality.
Just include base headers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Avoid to register multiple interface with the same ID.
This would result in issues as 2 channels would have the same
(channel_type, channel_id) which must be unique.
Qemu always allocates QXL interface with IDs starting from 0.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The field is only assigned in do_spice_init, surely won't change
in the meanwhile.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This had already been done for autotools in spice-common commit
924f47a653bd87fbd50229ee34b58d7b9a3f1ec8.
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This was added in bd8771adbc.
There's no reason to not use reds function instead.
MainDispatcher needs to listen in the main thread that is the
one provided by reds_core_* functions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Instead of scanning the monitor twice (one to compute the size
and another to build the message) use a single function to
marshal the message.
This also fixes big endian machines (which are not supported).
Marshal function is exported to make easier to test (see following
patch).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Most of the time result is set to AGENT_MSG_FILTER_OK, set at
the beginning and change if necessary.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This finished the work of 90ff154b36
(cfr "Update usage of GObject private structures").
Removes last call to g_type_class_add_private.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
This removes a not really useful switch/case, and changes the function
to exit early on error cases, rather than exiting early in the nominal
case.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This renames the method to smartcard_new_vsc_msg_item as this creates a
new object. This also removes the creation of a temporary VHeader which
is then going to be duplicated.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The function returns NULL if vheader->type is VSC_Init so no need to
check it a second time.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Only RedCharDeviceVDIPortClass implements this vfunc, rather than
forcing every classes deriving from RedCharDeviceClass to implement it,
red_char_device_send_tokens_to_client() can deal with it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
New functions and macros have been added in glib 2.38 to better handle
this case.
c8de2b11bb/NEWS
G_TYPE_INSTANCE_GET_PRIVATE will be deprecated in GLib 2.58.
https://gitlab.gnome.org/GNOME/glib/merge_requests/7/commits
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Bugs such as https://bugzilla.redhat.com/show_bug.cgi?id=1651882 can be
quite tricky to figure out without the detailed OpenSSL error. This
commit adds a detailed dump of the OpenSSL error stack when an OpenSSL
failure happens.
In the bug above, this would have displayed:
(process:13154): Spice-WARNING **: 05:43:10.139: reds.c:2816:reds_init_ssl: Could not load certificates from /etc/pki/libvirt-spice/server-cert.pem
(process:13154): Spice-WARNING **: 05:43:10.140: error:140AB18F:SSL routines:SSL_CTX_use_certificate:ee key too small
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
RedMemSlotInfo keeps an array of groups, and each group contains an
array of slots. Unfortunately, these checks are off by 1, they check
that the index is greater or equal to the number of elements in the
array, while these arrays are 0 based. The check should only check for
strictly greater than the number of elements.
For the group array, this is not a big issue, as these memslot groups
are created by spice-server users (eg QEMU), and the group ids used to
index that array are also generated by the spice-server user, so it
should not be possible for the guest to set them to arbitrary values.
The slot id is more problematic, as it's calculated from a QXLPHYSICAL
address, and such addresses are usually set by the guest QXL driver, so
the guest can set these to arbitrary values, including malicious values,
which are probably easy to build from the guest PCI configuration.
This patch fixes the arrays bound check, and adds a test case for this.
This fixes CVE-2019-3813.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
There should be no reason for the caller to modify the internal
structure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Client process termination did not work for Windows, used Win32
APIs.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Windows does not support Unix sockets.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
MingW does not include this header while including stdint.h so
on Windows you need to include it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
This is a preparatory patch for next portability patches
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
display-channel.h is not using any of the definition from
these headers.
Adding a missing include to red-worker.c.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Avoid using strncpy, considered not secure.
In this case a simple memcpy is used, we are going to terminate
the string in any case on the next line.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Lukáš Hrázký <lhrazky@redhat.com>
Zlib structure take up more than 1MB and it is rarely used nowadays
as it is not much effective.
Initialise it only when necessary saving some memory in the normal
case.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
When a client disconnects, smartcard_channel_client_set_char_device
is called with a NULL "device" argument. In that case there is
no need to take a weak reference to the device.
Without this patch the server complains:
g_object_add_weak_pointer: assertion 'G_IS_OBJECT (object)' failed
and aborts when a second client attempts to connect.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Adds the graphics device info from the streaming device(s) to the
VDAgentGraphicsDeviceInfo message sent to the vd_agent.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Receives the GraphicsDeviceInfo message from the streaming agent and
stores the data in a list on the streaming device.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Sends the device address and device display IDs to the vdagent. The
message is sent either in reaction to the SPICE_MSGC_MAIN_AGENT_START
message or when the graphics device info changes.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Instead of one unsupported example, present two real world examples.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Replace it by spice_qxl_set_device_info. Note we can't use
monitors_count for what's stored in max_monitors, because monitors_count
denotes the length of the device_display_ids array, which
spice_qxl_set_max_monitors doesn't touch.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Adds a function to let QEMU provide information to identify graphics
devices and their monitors in the guest. The function
(spice_qxl_set_device_info) sets the device address (e.g. a PCI path)
and monitor ID -> device display ID mapping of displays exposed by given
QXL interface.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Trace when data is received from the guest and when is sent
to the client.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Instead of
if (serveroutlen) {
...
} else {
red_stream_write_u32_le(stream, serveroutlen);
}
use 'red_stream_write_u32_le(stream, 0);' in the else block as it's
slightly more obvious.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
These constants are meant to be used in format string for pointers
types. Use them for portability.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Due to previous commit "make syntax-check" command reports:
prohibit_signal_without_use
server/tests/test-display-base.c
maint.mk: the above files include signal.h but don't use it
make: *** [maint.mk:639: sc_prohibit_signal_without_use] Error 1
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Use GLib function to launch and wait process exit.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
If input contains the binary record we can't have it modified
during read.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Split level computation, make clear is a sine wave on both channels.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
We are waiting for a client connection, channel is already there
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This changes tests/pki/server-cert.pem and tests/pki/ca-cert.pem to have
2048 bits. These certificates were generated using the
instructions on https://www.spice-space.org/spice-user-manual.html
The -subj args were omitted, and the defaults suggested by openssl used.
The -days parameter was changed to -days 10950, the bits to 2048.
This fixes https://gitlab.freedesktop.org/spice/spice/issues/27.
Some distros are starting to use stricter settings for their openssl
configuration, which forbids 1024 bit keys, and causes test suite
failures.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Now that we have a refcounted RedSurfaceCmd, we can store the command
itself in DisplayChannel rather than copying QXLReleaseInfoExt. This
will let us move the release of the QXL guest resources in red-parse-qxl
in the next commit.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Currently, RedWorker is using stack-allocated variables for RedSurfaceCmd.
Surface commands are rare enough that we can dynamically allocate them
instead, and make the API in red-parse-qxl.h consistent with how other
QXL commands are handled.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Currently, RedUpdateCmd are allocated on the stack, and then
initialized/uninitialized with red_{get,put}_update_cmd
This makes the API inconsistent with what is being done for RedDrawable,
RedCursor and RedMessage. QXLUpdateCmd are not occurring very often,
we can dynamically allocate them instead, and get a consistent API.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Currently, RedMessage are allocated on the stack, and then
initialized/uninitialized with red_{get,put}_message
This makes the API inconsistent with what is being done for RedDrawable
and RedCursor. Since QXLMessage is just a (mostly unused/unsecure) debugging tool,
we can dynamically allocate it instead, and get a consistent API.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Currently, the cursor channel is allocating RedCursorCmd instances itself, and then
calling into red-parse-qxl.h to initialize it, and doing something
similar when releasing the data. This commit moves this common code to
red-parse-qxl.[ch]
The ref/unref are not strictly needed, red_cursor_cmd_free() would
currently be enough, but this makes the API consistent with
red_drawable_{new,ref,unref}.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
At the moment, we'll unconditionally release the guest QXL resources in
red_put_drawable() even if red_get_drawable() failed and did not
initialize drawable->release_info_ext properly.
This commit only sets RedDrawable::qxl once the guest resource have been
successfully retrieved, and only free the guest QXL resources when
RedDrawable::qxl is set.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Rather than needing to call red_drawable_new() and then initialize it
with red_get_drawable(), we can improve slightly red_drawable new so
that red_drawable_{new,ref,unref} is all which is used by code out of
red-parse-qxl.c.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
RedDrawable really is a RedDrawCmd which is parsed by red-parse-qxl.h
Moreover, red_drawable_ref() is already defined inline in
red-parse-qxl.h, and red_drawable_unref() is declared there too even if
its code is still in red-worker.c
This commit moves them close to the other functions creating/unref'ing
QXL commands parsed by red-parse-qxl.h.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Just a style change, on more recent GLib would print a more
friendly error report.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Follow all other char devices implementation (spicevmc, agent,
stream-device) and set the char device state when
connected/disconnected. This allows qemu to discard writes, optimize a
bit the source polling, and will trigger HUP events.
See related qemu "char/spice: discard write() if backend is
disconnected".
Note: sif->state() should probably be handled at the char-device
level. I am not sure what the smartcard channel really brings over
plain spicevmc...
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Without this it's not obvious that a compression setting took effect.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Is possible that port 5913 is already in use as tests that uses
test_new will attempt to use ports from 5912 to 5921 so use a port
not in that range.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
test_new function attempts to detect attempts to listen to tcp ports
already in listening state detecting some messages during
spice_server_init. However the check is wrong (broken in recent
34a44d3e94 "test-display-base: Avoid spurious errors due to listen
failures") and incomplete (missing message).
To better test this conditions put some of the ports in listening
state (like with a "nc -l 5912 & nc -l 5913 &" command) and run
tests in parallel (like with a "make check -j" command).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
To set up a listening socket usually you call in sequence:
- socket;
- bind;
- listen.
If you try to bind() to a port when another socket is already
listening on that port, the bind() will fail.
However, it is possible that the bind() may succeed and the listen()
will fail, as demonstrated in the following sequence:
- socket() create socket 1;
- bind() to port N on socket 1;
- socket() create socket 2;
- bind() to port N on socket 2;
- listen() on socket 1;
- listen() on socket 2 <-- failure.
When running tests (especially multiple tests running in parallel), it
may sometimes happen that there are other tests already listening on
the port that we are trying to use. In this case, we want to ignore
this error and simply try to listen on a different port. We already
attempted to handle this scenario, but we were only ignoring bind()
errors and not listen() errors. So in the scenario mentioned above,
the listen() error was causing the entire test to fail instead of
allowing us to try to listen on another port.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Just cosmetic changes, the static function had underscores to
distinguish from the exported one which was recently renamed.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Lukáš Hrázký <lhrazky@redhat.com>
Adds a function to create a write buffer for sending a message to
vdagent from the server to prevent code duplication.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Instead of having a single red_char_device_write_buffer_get function to
get both client and server buffers and decide by testing client == NULL,
have separate function for a client and for a server. The situation
should always be clear (you're either on the client or on the server
side) and you shouldn't need to parametrize that.
For the server case, add a use_token parameter instead of a separate
red_char_device_write_buffer_get_server_no_token function, as you may
want to parametrize that.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Avoid time adjustment issues.
For instance ticket validity can change when daylight time changes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Lukáš Hrázký <lhrazky@redhat.com>
Do not convert RedStreamSslStatus enumeration type back to int.
This allows compilers to perform some more type safe checks.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This is a thin wrapper over g_get_monotonic_time_ms, and is called only
once, so we can call directly g_get_monotonic_time_ms instead.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Having a single QXL interface is not enough, there can be other (e.g.
streaming) display channels that make the tablet unusable. Add a check for the
number of display channels also being equal to 1. We still need the check for
QXL interaces, because the tablet only works with QXL.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
fix below compile error:
format '%d' expects argument of type 'int', but argument 6 has
type 'long unsigned int' [-Werror=format=]
spice compile failed on 32bit system, since upstream commit
9541cd2fe change %ld to %PRIdPTR, %PRIdPTR is %d, but argument
strm.total_out is uLong.
Signed-off-by: Changqing Li <changqing.li@windriver.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
In a comparison with current autotools build system, meson/ninja
provides a huge improvement in build speed, while keeping the same
functionalities currently available and being considered more user
friendly.
The new system coexists within the same repository with the current one,
so we can do more extensive testing of its functionality before deciding
if the old system can be removed, or for some reason, has to stay for
good.
- Meson: https://mesonbuild.com
This is the equivalent of autogen/configure step in autotools. It
generates the files that will be used by ninja to actually build the
source code.
The project has received lots of traction recently, with many GNOME
projects willing to move to this new build system. The following wiki
page has more details of the status of the many projects being ported:
https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting
Meson has a python-like syntax, easy to read, and the documentation
on the project is very complete, with a dedicated page on how to port
from autotools, explaining how most common use cases can be
implemented using meson.
http://mesonbuild.com/Porting-from-autotools.html
Other important sources of information:
http://mesonbuild.com/howtox.htmlhttp://mesonbuild.com/Syntax.htmlhttp://mesonbuild.com/Reference-manual.html
- Ninja: https://ninja-build.org
Ninja is the equivalent of make in an autotools setup, which actually
builds the source code. It has being used by large and complex
projects such as Google Chrome, Android and LLVM. There is not much to
say about ninja (other than it is much faster than make) because we
won't interact directly with it as much, as meson does the middle man
job here. The reasoning for creating ninja in the first place is
explained on the following post:
http://neugierig.org/software/chromium/notes/2011/02/ninja.html
Also its manual provides more in-depth information about the design
principles:
https://ninja-build.org/manual.html
- Basic workflow:
Meson package is available for most if not all distros, so, taking
Fedora as an example, we only need to run:
# dnf -y install meson ninja-build.
With Meson, building in-tree is not possible at all, so we need to
pass a directory as argument to meson where we want the build to be
done. This has the advantage of creating builds with different options
under the same parent directory, e.g.:
$ meson ./build --prefix=/usr
$ meson ./build-extra -Dextra-checks=true -Dalignment-checks=true
After configuration is done, we call ninja to actually do the build.
$ ninja -C ./build
$ ninja -C ./build install
Ninja defaults to parallel builds, and this can be changed with the -j
flag.
$ ninja -j 10 -C ./build
- Hacking:
* meson.build: Mandatory for the project root and usually found under
each directory you want something to be built.
* meson_options.txt: Options that can interfere with the result of the
build.
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This commit needs an unreleased version of spice-protocol.
The revert is temporary in order to get the spice-server 0.14.1 release
out.
This reverts commit 9f5859c3ba.
The structure usage was removed from commit
2ba69f9f88
("libspice: add surface 0 support").
They were never used by Qemu.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Fixes test-stream-device after adding a log warning about an invalid
message received on the stream device, glib tests fail on unexpected
warning messages.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Following commit fcaf8d1a1, build from tarballs/make distcheck is broken
as spice-server-enums.h is not regenerated when building from tarballs,
and we don't have a -I$(top_srcdir) in our build flags, just
-I$(srcdir). This commit changes #include <server/spice-server-enums.h>
to #include <spice-server-enums.h> which avoids the problem fixed by
commit fcaf8d1a1 without breaking make distcheck.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
When using #include "spice-server-enums.h", it will be looked up first
in the directory containing the file being build, which is going to be
$srcdir when dcc.c includes it. However, spice-server-enums.h is a
generated file, so it will be in $builddir, not in $srcdir. This most of
the time won't be causing any problems, except when you happen to have
an invalid spice-server-enums.h in $srcdir and you are doing an
out-of-tree build.
Using #include <spice-server-enums.h> instead allows the correct
spice-server-enums.h file to be looked up.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Lukáš Hrázký <lhrazky@redhat.com>
The reason for this commit is that Meson expects all submodules to be
placed in this subdirectory, and since autotools build is more flexible
in this case, we make some small adjustments to configure.ac and
Makefile.am files to accommodate for this change.
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Rather than using
#ifdef COMPRESS_DEBUG
spice_info(...);
#endif
we can #define COMPRESS_DEBUG to spice_debug() or to do nothing for a
slight readability improvement. This opportunity is used to replace
these spice_debug() calls with g_debug(). The "do nothing" macro is a bit
convoluted to ensure that we will have a compile-time check for our
g_debug args.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
There was a small regression introduced in get_compression_for_bitmap()
by f401eb07f dcc: Rewrite dcc_image_compress.
If SPICE_IMAGE_COMPRESSION_AUTO_GLZ is specified, and the bitmap has a
stride which is bigger than its width (ie it has padding), then
get_compression_for_bitmap() will return SPICE_IMAGE_COMPRESSION_OFF
while in that case, we used to use QUIC for compression.
This happens because that function in the AUTO_GLZ case first checks if
QUIC should be used, if not, it decides to use GLZ, but then decides it
can't because of the stride, so falls back to OFF, while it used to
fall back to QUIC.
This commit only slightly reworks a preexisting if (!can_lz_compress())
check so that it's unconditional rather than depending on the previous
checks having been unsuccessful.
This issue could be observed by using a spice-html5 without support for
uncompressed bitmaps with end-of-line padding by simply starting a f28
VM and connecting to it/moving the mouse cursor in it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Check that data sent to device are collapsed in a single message.
The StreamChannel object is mocked in the test.
This checks that commit dcc3f995d9
("stream-device: handle_data: send whole message") is doing the
right thing.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If guest sent an empty data message this was not parsed correctly.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
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>
