Current code does not free allocated tablet resources.
When a tablet is added some resources are allocated.
Resources should be released either removing the tablet or
freeing spice server object.
Added a test to check these conditions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Currently create device, open it and pass some messages checking
they are handled.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
common/mem.h contains mainly memory allocation functions.
As we decided to move to Glib calls directly avoid to include
function declaration we should not use anymore.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This prevent future problems supporting new channels.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This reduce the attack surface moving some data into read-only
sections.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Missing some names cause some debugging messages to be
generated and some of our tests to fail.
This patch was written by Christophe Fergeau.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The code for reading a StreamDevice message from the streaming agent has
code to handle a situation where you only read a part of the header. If
we've read only a part of the header, we will try to read the remaining
n bytes of the header within a loop until the full header is read.
However, when we try to read the last n bytes, we store it at beginning
of the header struct, which will overwrite the first part of the header.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Include protocol header file defining StreamMsgFormat which is used in a
function signature in this header.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: Frediano Ziglio <fziglio@redhat.com>
This commit adds red_channel_{debug,warning,printerr}() helpers which
will prepend the log message with "channel-name:id (%p)". It also
changes various locations which were doing this manually.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
spice_server_set_channel_security() is already mostly doing that. We can
make its code more generic, and introduce a red_channel_get_name()
method. This method will then be used to make debug messages more
readable by showing the actual channel name rather than its type as
an int.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The objects RedsStream and RedsSASL are currently using the namespace
"Reds" rather than the standard "Red" namespace used throughout the rest
of the project. Change these to be consistent. This also means changing
method names and some related enumeration types.
The files were also renamed to reflect the change:
reds-stream.[ch] -> red-stream.[ch]
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
There are already other debugging code showing channel closure.
Not closed file descriptors can easily be detected with other
tools (like netstat or /proc file system).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
In some cases mouse_mode is a bit field.
However for this structure is used always as a boolean
value.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This field was used just to store a value and retrieve again
while we can just pass it instead.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Setting the capability is not enough, each stream must be enabled
so do so if client support them.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Do not allow the guest to fill host memory.
Also having a huge queue mainly cause to have a higher video
latency.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This allows a better id allocation as devices are created after
fixed ones.
Also will allow to support more easily multiple monitor.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When guest close the device the host device has to be reset too.
This make easier to restart the guest device which can happen in case
of reboot, agent issues or if we want to update the agent.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Currently, red_char_device_reset() stops the device, clears all pending
messages, and clears its device instance. After this function is called,
the char device will not work again until it is assigned a new device
instance and restarted. This is fine for the vdagent char device, which
is currently the only user of this function. But for the stream device,
we want to be able to reset the char device to a working state (e.g.
clear all pending messages, etc) without stopping or disabling the char
device. So this function will now only reset the char device to a clean
working state, and the _stop() and _reset_dev_instance() calls will be
moved up to the caller.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Start showing something when we have a surface and stream
instead of showing a blank screen which is now not useful.
Was useful for debugging purposes to understand that the
new channel was sending messages correctly to client and
client could handle them.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When a new client is connected we must restart the stream so new
clients can receive correct data without having to wait for the
next full screen (which on idle screen could take ages).
On disconnection we should tell the guest to stop streaming
not wasting resources to stream not needed data.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The channel needs to communicate when it receive a new
stream request from the guest.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Remove the fixed size stream and support any display size.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Handle stream data from device sending to the channel.
The StreamChannel will forward the data to the clients using standard
DisplayChannel messages, and will create and destroy streams as
necessary.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
So can be used by the device to communicate with the clients.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Currently only compile, not used and not much sense
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Parse the data sent from the guest to the streaming device.
At the moment, the data is simply discarded after it is parsed.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Add a stub device in guest.
The aim of this device is to make it possible for the guest to send a
stream through a DisplayChannel (in the sense of protocol channel).
This stub allows the guest to send some data and you can see some debug
lines of data arrived on host logs.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This allows the server to add channels after the client is connected.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This function does not make much sense anymore.
Is called by RedVmcChannel which doesn't use RedChannelClient ACKs
so the variable changed are not used.
Moreover, at red_vmc_channel_constructed() time, there will be no
clients yet, so red_channel_init_outgoing_messages() will be a no-op.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Prevent possible buffer reading overflow.
Note that message pointer must be valid and data are checked
value by value so even on overflow you just get an error.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Since commit ef4b1bdb "red-channel-client: Prevent too tight loop
waiting for ACKs", after seamless migration, the display is no longer
updated on the client-side, even though the VM is functional (responds
to keyboard input, reconnecting the client restores the display
functionality, ...).
This is mainly caused because after migration,
red_channel_client_waiting_for_ack() will be true until
red_channel_client_ack_zero_messages_window() is called.
What happens is the following:
The dcc is created, and dcc_start() pushes a RED_PIPE_ITEM_TYPE_SET_ACK
message.
This calls prepare_pipe_add(), which will enable write event on the dcc
watch. red_channel_client_event() will be called, which will trigger a
red_channel_client_push(). Since red_channel_client_waiting_for_ack()
returns true, we won't get any item to push, and (because of commit
ef4b1bdb), we will disable write notifications on the watch.
At this point, rcc->priv->pipe is no longer empty, so prepare_pipe_add()
is not going to reenable the write notifications.
Then red_channel_client_ack_zero_messages_window() is finally called as
part of dcc_handle_migrate_data(), so from this point on,
red_channel_client_waiting_for_ack() is no longer true.
However, nothing ever reenables WRITE events, nor empties
rcc->priv->pipe, so nothing ever gets pushed, causing no display updates
at all after a migration, even if the VM is functional (input, ...)
apart from that.
This commit reenables WRITE events in
red_channel_client_ack_zero_messages_window() if we were waiting for
ack.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Since 2.8, QEMU no longer creates QXL primary surfaces when using GL.
This change broke client-side mouse mode, because Spice server relies on
having a primary surface.
When GL is enabled, use GL scanout informations.
Mouse mode is always client when GL surfaces are used.
This patch and most of the message are based on a patch from
Marc-André Lureau, just moving responsibility from reds to RedQxl.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe de Dinechin <dinechin@redhat.com>
Currently, the port used by most tests is hardcoded to 5912. However,
the test suite can be run in parallel, so if 2 tests run in parallel,
the 2nd one is not going to be able to bind to port 5912 and will fail.
After this commit, test_new() will try to find a free port between 5912
and 5922 and will abort if it can't find any.
The issue can be reproduced by adding a usleep(1000000) to the beginning
of test_destroy().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Start reducing the usage of spice_new*/spice_malloc allocations.
They were designed in a similar way to GLib ones.
Now that we use GLib make sense to remove them.
However the versions we support for GLib can use different memory
allocators so we have to match g_free with GLib allocations
and spice_* ones (which uses always malloc allocator) with free().
This patch remove some easy ones.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The macro will implement most of the boilerplate needed to declare an
object.
Its usage is similar to GLib G_DECLARE_*_TYPE macros.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedChannelClient has a "handle-acks" feature.
If this feature is enabled, after the configured number of messages it
waits for an ACK from the client.
If is waiting for an ACK it stops sending messages.
However the write notification was not disabled, causing the loop event
to always trigger, as the socket in this case is ready to accept data.
Specifically red_channel_client_event is continuously called.
This is noticeable using slow network environments and having
some additional loop instrumentation.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This structure was used to store the cookie for the async
reply and the message for the generic async callback.
Most async messages do not require extra action beside sending back the
cookie for the reply so instead of having a switch on the message type
in red_qxl_async_complete, this commit moves the message-specific
behaviour to the callers, which allows us to store the cookie directly
in RedWorkerMessageAsync rather than needing an intermediate
AsyncCommand structure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Now the push is done automatically when a PipeItem is added
(cfr commit 5c460de1a3
"worker: push data when clients can receive them"),
forcing a push cause only network fragmentation and is required only if
you are handling data in a polling loop (and thus, you are preventing
the default event loop from running).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
It was using g_list_remove_link() to remove an element from the
RedChannel::clients list while it really meant to be using
g_list_delete_link() which frees the memory associated with the link.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
All main test module have this test-XXXX.c naming, make
test-stat coherent.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Put non-trivial programs in separate sections, which makes it easier to
understand the relationship between macros.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
New automake test harness produce *.log and *.trs files for
each test.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This macro is exactly doing what RING_FOREACH just passing streams
ring.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe de Dinechin <cdupontd@redhat.com>
There is exactly one user in RedChannel, and this can be reimplemented
using already public RedChannelClient API. No need for an extra
function very specialized function with a not great name.
This commit thus removes one method from RedChannelClient public API,
and replaces it with an equivalent private helper in RedChannel.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
red_channel_disconnect_if_pending_send() and red_channel_wait_all_sent() are
always called together, we can remove one of the 2 methods.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In case GLib don't provide these functions we use replacements so
there's no need to have a warning if these functions are called.
This potentially capture other compatibility issues in the tests
that would be ignored having all deprecation warnings disabled.
Tested with GLib 2.28 and 2.52.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
When the worker is started it could take a while to start processing
commands.
The reason is that the dispatcher handler is called after the worker
so GLib will receive a FALSE answer to both prepare and check
callbacks of the RedWorkerSource causing GLib to wait till another
event is received.
This is a regression since the introduction of GLib event loop, before
the command processing was always attempted after any events.
Commands (from QXL interface for cursor and display) are processed
during the RedWorkerSource dispatch so if they are not processed just
when the VM is started they will be processed on next event which
could be from dispatcher (main thread requests), from existing
connections or from pending timers. However in the case there are no
clients connected and no other requests from main thread the worker
thread won't process them.
Setting the event_timeout to 0 cause the prepare callback to return
TRUE so GLib will dispatch the RedWorkerSource.
This was discovered attempting to use the tests in server/tests
directory to reproduce a leak in RedWorker.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Pipelines are never freed.
These are detected as leaks by leak detector tools like address sanitizer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Command line options are not freed at the end of the program.
These are detected as leaks by leak detector tools like address sanitizer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
encoder_name is never NULL as already initialized with "mjpeg" value.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If a client is unable to complete the TLS handshake phase
reds_init_client_ssl_connection leaked some memory as the stream is not
correctly freed.
This also causes the stream to send the SPICE_CHANNEL_EVENT_DISCONNECTED
event. Otherwise only SPICE_CHANNEL_EVENT_CONNECTED was sent.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Currently is possible to trigger a leak by passing an invalid
connection.
This can happen if the client opens a connection and then closes it
without writing or reading any data.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The code tests for the presence of RedChannelClient::stream while
initializing RedChannelClient.
However, the check was done too late, and a
RedChannelClient::config_socket implementation (for example
snd_channel_client_config_socket) could have tried to use it before the
check that it's not NULL.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This potentially can also save the copy if there is enough
space to resize the buffer in place.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This field was used only by RedChannelClient to mark when the socket
was shutdown. This condition can simply be tested by RedChannelClient
checking if there's a watch as is the only condition (beside object
destroying/disconnecting) where the watch is removed.
In any case the shutdown was used to understand if there were possible
data still to read.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedQxl and RedWorker are quite bound together running
CursorChannel and DisplayChannel in a separate thread
marshalling (RedQxl) and unmarshalling and executing
(RedWorker) requests.
Make the communication between them private trying
to facilitate maintaining these two files.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
For dispatcher_register_handler(), use 'false' instead of 0 since the
last argument is a bool type now.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
This callback was only executed for message types that were registered
with DISPATCHER_ASYNC ack type. However, the async_done handler was
called immediately after the message-specific handler and was called in
the same thread, so the async_done stuff can just as easily be done from
within the message-specific handler. This allows to simplify the
dispatcher_register_handler() method to simply require a boolean
argument for whether the message type requires an ACK or not.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Pass proper type to callback to avoid having to convert to
the right type for each call.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Is supposed to be used during initialization but is never
used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
AsyncCommand is used to handle asynchronous messages from the
dispatcher.
GL_DRAW_ASYNC is mainly using it to store the cookie.
The value of GL_DRAW_COOKIE_INVALID was choosen to allow implementing
cookies (which basically are handles) either using indexes (where 0 is
valid) or pointers (where 0 is invalid). Currently Qemu uses pointers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These 2 functions were doing the same stuff, calling
client_monitors_config callback in QXLInterface.
The only difference was that red_qxl_use_client_monitors_config
used a NULL value.
Added the check for proper version, QXLInstance before 3.3
did not have this callback.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
You could easily trigger this issue using multiple monitors and a
modified spice-gtk client with this patch:
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1699,6 +1699,7 @@ static gboolean _channel_new(channel_new_t *c)
{
g_return_val_if_fail(c != NULL, FALSE);
+ if (c->type == SPICE_CHANNEL_DISPLAY) c->id = 0;
spice_channel_new(c->session, c->type, c->id);
g_object_unref(c->session);
which cause a crash like
(process:28742): Spice-WARNING **: Failed to create channel client: Client 0x40246f5d0: duplicate channel type 2 id 0
2017-08-24 09:36:57.451+0000: shutting down, reason=crashed
RedChannelClient is an GInitable type, which means that the object is
constructed, and then the _init() function is called, which can fail.
If the _init() fails, the newly-created object will be destroyed. As
part of _init(), we add a new watch for the stream using the core
interface that is associated with the channel. After adding the watch,
our rcc creation fails (due to duplicate ID), and the rcc object is
unreffed. This results in a call to reds_stream_free() (since the rcc
now owns the stream). But in reds_stream_free, we were trying to remove
the watch from the core interface associated with the RedsState. For
most channels, these two core interfaces are equivalent. But for the
Display and Cursor channels, it is the core Glib-based interface
associated with the RedWorker.
The watch in RedsStream by default is bound to the Qemu provided
SpiceCoreInterface while RedChannelClient bound it to Glib one causing
the crash when the watch is deleted from RedsStream. Change the bound
interface.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When a stream is moved from the main thread to a
secondary one the events are potentially registered
using a different core interface. This cause memory
corruption accessing the watch registered in RedsStream.
This patch allows to always use the right interface.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This test runs 2 spice server in one program.
Use two different tcp port to be able to connect to both servers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Add some check that something happened during creation/destruction.
Set as running on "make check".
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Update tests names.
Remove tetris comments, never available and not planned.
Update some notes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
There's no need to not compile this feature, it just enable
a parameters which must be passed in order to change test
behaviour.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This allows to end the loop to end some tests.
Currently different tests enter the loop but never exit from
them.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
In C the sizeof(long) can be different than sizeof(void*),
use proper type.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Do not use calloc and malloc directly without checking
the result. Use instead spice functions to get a nice
error in case of allocation failures.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
For some reasons (documented in cursor_init) the function
uses 128 extra bytes of data causing a reading buffer overflow.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Timers in spice server are supposed to be used in a single thread
context. To avoid problems, protect the usage from multiple
thread with a mutex.
This sometimes caused a crash.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The wakeup timer is used by the worker thread and by the
main thread.
Destroying the object before destroying the worker thread
can lead to use after free.
Destroying the worker thread first makes sure we don't race.
This is detected easily when compiling the test with address sanitizer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
As the indexes are used to compute the index inside an array
using modulo operation when a signed value overflows, the
modulo becames negative, causing a buffer underflow.
Unlikely to happens (take lot of time) but is safer that way.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Cursor resources (basically the shape of it) was retained till
it was used however it was copied so there were no reason to not release
this resource.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
You could easily trigger this issue using multiple monitors and
a modified spice-gtk client with this patch:
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1699,6 +1699,7 @@ static gboolean _channel_new(channel_new_t *c)
{
g_return_val_if_fail(c != NULL, FALSE);
+ if (c->type == SPICE_CHANNEL_DISPLAY) c->id = 0;
spice_channel_new(c->session, c->type, c->id);
g_object_unref(c->session);
This as g_initable_new in this case returns NULL (dcc.c).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Although dispatcher_send_message() does not allow you to send a message
type that is invalid for a dispatcher, it still makes sense to verify
that the type is valid in the receiver. This should only be possible in
the case of some severe problem such as memory corruption, so if it is
invalid, we simply abort.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Is possible to have a leak processing update commands if
the update command is synchronous and the rectangle list
is empty. Note that Qemu always pass an empty list.
If the list is empty display_channel_update fill the list.
This is used to send back the list in case of asynchronous
requests. But in handle_dev_update_async (the callback that
handle the asynchronous case) the list is correctly freed.
This was discovered by accident looking at the code.
Reproduced with a Windows recording file using GCC address
sanitizer and this patch to spice-server-replay:
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -1280,7 +1280,13 @@ static void replay_handle_dev_input(QXLWorker *worker, SpiceReplay *replay,
replay->created_primary = FALSE;
worker->destroy_surfaces(worker);
break;
- case RED_WORKER_MESSAGE_UPDATE:
+ case RED_WORKER_MESSAGE_UPDATE: {
+ static uint8_t count = 0;
+ QXLRect dummy;
+ QXLRect update = { 0, 0, 100, 100 };
+ count ^= 1;
+ worker->update_area(worker, 0, &update, count ? &dummy : NULL, count ? 1 : 0, 0);
+ } break;
// XXX do anything? we record the correct bitmaps already.
case RED_WORKER_MESSAGE_DISPLAY_CONNECT:
// we want to ignore this one - it is sent on client connection, we
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedPipeItem already implements reference counting so
this avoid duplicating code to handle a object with reference
counting that points to another object with reference counting
that holds a RedCursorCmd.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Now the push is done automatically when a PipeItem is added
(cfr commit 5c460de1a3
"worker: push data when clients can receive them"),
forcing a push cause only network fragmentation and is required
only if you are handling data in a loop instead of using the
default loop.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Implements red_channel_pipes_add_type and
red_channel_pipes_add_empty_msg using red_channel_pipes_add.
This avoid duplicating items for each client.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Considering that now RedPipeItem have reference counting
and that lot of items are just used to store constant
data to send, using reference counting instead of creating
different items for each client is easier to do.
So this new red_channel_pipes_add allows to add a single item
to all clients.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Now the push is done automatically when a PipeItem is added
(cfr commit 5c460de1a3
"worker: push data when clients can receive them"),
forcing a push cause only network fragmentation and is required only if
you are handling data in a polling loop (and thus, you are preventing
the default event loop from running).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
display-channel.h contains lots of information used by different
DisplayChannel components.
In the past all RedWorker, CursorChannel and DisplayChannel code was in
a single file. Since lots of code to handle DisplayChannel is still in
RedWorker, display-channel.h contains a lot of declarations so that they
can be accessed from RedWorker.
Moving declarations that are not needed by RedWorker and other external
class components helps to reduce dependencies between RedWorker and
DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This patch allocates VMC IDs by finding the first ID not used
instead of using a global variable and incrementing the value
for each channel created.
This solves some potential issues:
- remove the global state potentially making possible
to use multiple SpiceServer on the same process;
- don't potentially overflow the variable. This can happen if
channels are allocated/deallocated multiple times
(currently not done by Qemu).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This can happen as the connection is asynchronous so (MT main thread,
CT channel thread):
- MT you get a new connection;
- MT a connection is sent to CT;
- MT you get a disconnection of main channel;
- MT red_client_destroy is called;
- CT you attempt to add the RCC to RedClient.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
A RedChannelClient is always attached to a valid RedChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Name will be visible in debugger and /proc filesystem
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The message is asynchronous so to avoid the object to potentially
been released before being processed keep a strong reference to
it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
red_disconnect_display() is duplicating what red_channel_disconnect()
already does, so red_disconnect_display() and red_disconnect_cursor()
are actually identical code-wise. We can directly call
red_channel_disconnect() from flush_commands() rather than passing a
'red_disconnect_t disconnect' argument to that function.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
cursor_channel_disconnect() calls
cursor_channel_client_reset_cursor_cache() on all CursorChannelClient
associated with the current CursorChannel before calling
red_channel_disconnect().
red_channel_disconnect() will iterate over all CursorChannelClient
calling red_channel_client_disconnect(), which will eventually call
CursorChannelClient::on_disconnect. This will in turn
cursor_channel_client_reset_cursor_cache(), so calling it in
cursor_channel_disconnect() before calling red_channel_disconnect() is
redundant.
cursor_channel_disconnect() can thus be replaced by a direct call to
red_channel_disconnect().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
SoundChannelClient has a stub implementation of
RedChannelClient::on_disconnect(), this commit removes the need for it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This vfunc only has a RedChannelClient * argument, and most of the time,
it operates on RedChannelClient, not on RedChannel. Moreover, the only
time it's used is from RedChannelClient. This commit moves the vfunc to
RedChannelClient, which seems like a better fit for it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
ORC library is used internally by GStreamer to generate code
dynamically.
If ORC cannot allocate executable memory, the failure causes
an abort(3) to be called.
This happens on some SELinux configurations that disable executable
memory allocation (execmem boolean).
Check that ORC could work before attempting to use GStreamer to
avoid crashes.
While this check is done, the ORC library outputs an error which will
be well visible in Qemu output.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If you are testing for NULL data this means that variable could be
NULL so avoid to access before the check to make sure the check is hit.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The multimedia time is defined as uint32_t.
Use the proper type instead of int.
Currently no arithmetic is done on this value but
just copies so considering that on the architectures
we support sizeof(int) == sizeof(uint32_t) there's
no change in the resulting machine code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
A RedClient can be freed from the main thread following a main channel
disconnection (reds_client_disconnect). This can happen while another
thread is allocating a new channel client for that client.
To prevent the usage of a pointer which can be invalid
take ownership of the pointer.
Note that we don't need this when disconnecting as disconnection is
done synchronously (the dispatch messages are registered with
DISPATCH_ACK).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Trace the number of loops done processing display commands
and the number of loops in which the queue was full.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
If a DisplayChannelClient cannot be instantiated capabilities
are not released correctly.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
display variable already contains the DCC_TO_DC(dcc) value so
reuse it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Some RedChannelClient data members were marked as int when they only
hold booleans.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
reds_get_n_clients is a single line and is used only by
spice_server_get_num_clients.
The 2 functions have very similar names so inlining
reds_get_n_clients does not make code less readable.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The leak detector we use currently is not enough to detect
some kind of leak in DisplayChannel so manually test.
These tests are enabled only when --enable-extra-checks is passed
to configure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Mostly of red_channel_destroy calls were preceded by
a call to unregister the channel.
The only exception was the main channel as this channel is
always present and its initialisation is a bit different.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
RedWorker should not handle directly to client but
defer the job to DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Make easier to understant the value to use in the code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
From spice_server_port_event API you can send port events to
any char device. Although currently this is used only for "port"
devices implemented in spicevmc.c this will allow to support
such events using different objects.
This will be used for instance for a streaming device which
will be a specific SpicePort implementation.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When a RedCursorCmd is passed to cursor_channel_process_cmd(), it
constructs a new CursorItem which takes ownership of that command. If
the cursor_cmd->type falls through to the default case of the switch
statement, we will print a warning and return without freeing the
CursorItem (and thus the RedCursorCmd).
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Now the push is done automatically when a PipeItem is added
(cfr commit 5c460de1a3
"worker: push data when clients can receive them"),
forcing a push cause only network fragmentation and is required
only if you are handling data in a loop instead of using the
default loop.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Under error: 'link' fields are being accessed, so it's
wrong to goto error with link == NULL.
Instead, return immediately.
Found by coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
If qxl->descriptor.type is QUIC, red_replay_data_chunks_free
frees qxl (data), so no need to free it again at the bottom
of the function.
Found by coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
It was also possible for a malicious client to set
VDAgentMonitorsConfig::num_of_monitors to a number larger
than the actual size of VDAgentMOnitorsConfig::monitors.
This would lead to buffer overflows, which could allow the guest to
read part of the host memory. This might cause write overflows in the
host as well, but controlling the content of such buffers seems
complicated.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Total message size received from the client was unlimited. There is
a 2kiB size check on individual agent messages, but the MonitorsConfig
message can be split in multiple chunks, and the size of the
non-chunked MonitorsConfig message was never checked. This could easily
lead to memory exhaustion on the host.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
To help avoid stream.c and dcc.c to access display-channel private
structure to get the nth Stream structure pointer.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
As we have a function for that, don't do the math elsewhere.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
When compiling, -Werror=missing-field-initializers is enabled.
However, some gcc versions (like Gentoo 4.9.4 one) fail to see
that all the members of the SpiceBaseInterface struct are
initialized:
test-display-base.c:844:5: error: missing initializer for field
'description' of 'SpiceBaseInterface'
[-Werror=missing-field-initializers] .base.description = "test
spice virtual channel char device",
The solution is to initialize .base member as a structure at once
instead of multiple times per each member.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
As discussed recently the usage of domain for logging has
different issues (they are not filtered and handled coherently)
and are not widely used in the code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This was not done until now, and it's only going to be needed if we receive
a partial ClientMonitorsConfig message.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The function name is always prepended by the spice_log macro, so we
don't need to explicitly add it in debug messages.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
RedsClientMonitorsConfig duplicates what SpiceBuffer does,
so using we can replace it with SpiceBuffer and make
reds_on_main_agent_monitors_config() simpler.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is causing issues with potential improvements to the logging
system, and I've always found this usage a bit odd anyway.
Using spice_debug(""); was not possible as this triggers
-Wformat-zero-length warnings from our use of -Wall.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In the past CursorItem structure was stored in
RedCursorCmd::device_data field so the check was there to check if the
structure fit into that field.
Since 2ba69f9f88
("libspice: add surface 0 support") the structure is no more stored in
the field so there's no reason for this check causing only confusion.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The maximum value for a 32 bit variable is UINT32_MAX and not
UINT_MAX. Currently all supported platforms have these two
constants having the same value so this patch don't change
nothing in the generated code but potentially this could
change in a future.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
File transfer and Copy & Paste can be disabled on the server even when
they're supported by the guest agent. Tell it the client by adjusting
the agent capabilities.
Related: rhbz#1373725
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
When a guest is rebooted, the QXL driver gets unloaded at some point in
the reboot process. When the driver is unloaded, the spice server sets a
single flag to FALSE: RedWorker::driver_cap_monitors_config. This flag
indicates whether the driver is capable of sending its own monitors config
messages to the client.
The only place this flag is used is when a new primary surface is created. If
this flag is true, the server assumes that the driver will send its own
monitors config very soon after the surface is created. If it's false, the
server directly sends its own temporary monitors config message to the client
based on the size of the just-created primary surface. This temporary monitors
config message always has a maximum monitor count of 1.
This flag is set to false at startup so that in early boot (e.g. vga text
mode), the server will send out these 'temporary' monitor config messages to
the client whenever the primary surface is destroyed and re-created. This
causes the client to resize its window to match the size of the guest. When the
QXL driver is loaded and starts taking over the monitors config
responsibilities, we set this flag to true and the server stops sending
monitors config messages out on its own.
If we reboot and set this flag to false, it will result in the server sending a
monitors config message to the client indicating that the guest now supports a
maximum of 1 monitor. If the guest happens to have more than one display window
open, it will destroy those extra windows because they exceed the maximum
allowed number of monitors. This means that if you reboot a guest with 4
monitors, after reboot it will only have 1 monitor.
To avoid this, we assume that if we had the ability to support multiple
monitors at some point, that will return at some point. So when the server
constructs its own monitors config message to send to the client (when the
driver_cap_monitors_config flag is false), we send the previous maximum monitor
count instead of always sending a maximum of 1.
Resolves: rhbz#1274447
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The variable is used to store a boolean type.
Update style for this boolean variable.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Use a #define rather than a magic number to make the code a bit more
readable.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
rcc is already deferenced in red_channel_client_get_client so
checking for NULL after that is uselss.
Also this call is generated from red_channel_client_disconnect
which requires the rcc pointer to be valid.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The base RedChannel already keeps a list of channel clients, so there's
no need for the SndChannel to also keep track of the client itself.
Since the SndChannel only supports a single client (whereas other
channels may have some partial support for multiple clients), I've
provided a convenience function for getting the client and warning if
there is more than one.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The content of these functions almost exclusively deals with channel
client functionality except one line where the channel's active state is
set to TRUE.
These functions are called in two different places.
The first place is from the public API spice_server_record_start() and
spice_server_playback_start(). These functions should alter the
channel's active state, and then set the associated channel client to
active.
The second place is when a new channel client is created. In this
case, it is only called if the channel is already active, so it doesn't
make much sense to set the channel's active state inside of the
function.
To simplify things (and enable some future refactoring), this function
now only deals with the SndChannelClient. The functions have also been
renamed to reflect this fact. The SndChannel's active state is now only
modified from the public API functions.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Instead of putting a 'next' link within the channel structure itself,
just use a generic GList structure to keep a list of active sound
channels.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
When a new PlaybackChannelClient or RecordChannelClient is created,
there are several places where we make decisions based on whether the
client is active or not. But these checks are done before the 'active'
flag is ever set, so this code is effectively dead. This has been the
case since commit 6fdcb931 and d351bb35, so this code has not been
executed for approximately 7 years now.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
It is only called from the constructor, so move all of the code into
that function.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This function is only called from the constructor, so move all of that
code into the constructor.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Reverse return values of the various bool methods so that 'true' means
success, and 'false' failure rather than the opposite.
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The red_get_* methods in red-parse-qxl.c return a boolean, even though
their return type is an int, and they return 1/0. This commit changes
this to the more explicit bool/true/false.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This use to be an anonymous enum, used as an int in
RedCharDeviceWriteBufferPrivate::origin.
Introducing a typedef clarifies what kind of value it's holding.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
This is intended to hold the fields that only char-device.c has a use
for, but for now this only adds the boilerplate for it, the next commit
will move the relevant field there.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
red_channel_client_clear_sent_item() will clear send_data.blocked, so no
need to do it in red_channel_client_msg_sent() as well.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Now that red_channel_client_set_blocked() is no longer changing the
events we are watching for, we can call it from
red_channel_client_push().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Since 5c460d, we need to watch for WATCH_EVENT_WRITE as long as there are
items queued waiting to be sent, this does not need to be done only when
the network queue is full.
When red_channel_client_set_blocked() is called, as a message is being
sent, WATCH_EVENT_WRITE will already be set, so it does not need to set
it again.
red_channel_client_msg_sent() removes WATCH_EVENT_WRITE, but this will
be done later anyway by red_channel_client_push() if needed.
Since it's redundant, we can remove this.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
red_channel_client_msg_sent() calls red_channel_client_clear_sent_item()
which will clear the rcc->priv->send_data.blocked flag. Later on, it
contains a check for !red_channel_client_is_blocked(), which will always
be true as nothing in between could have set it again. This commit
removes this unneeded check.
This check was already redundant when it was introduced in
9a62a9a809
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Before this commit, red_channel_client_start_ping_timer() and
red_channel_client_cancel_ping_timer() had checks to be no-ops when
rcc->priv->latency_monitor.timer is NULL.
This commit adds a similar check to
red_channel_client_restart_ping_timer(), and removes explicit NULL
checks before calls to red_channel_client_{cancel,restart,start}_ping_timer()
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
rcc is just used on the next line so cannot be NULL.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Many function already used bool as boolean type.
Change last gboolean occurrencies and values.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Commit 590acf3c55 introduced a regression after migration:
PlaybackChannel::connection is never set even if a client is connected,
which means the playback channel is non-functional until a client
reconnects as all the snd_channel_set_xxx() method checks for a non-NULL
PlaybackChannel::connection before sending data to the client.
This commit slightly changes the code flow in
on_new_playback_channel_client()/playback_channel_client_constructed()
so that PlaybackChannel::connection is set regardless of what
red_client_during_migrate_at_target() returns. This is what was done
prior to 590acf3c55.
This resolves https://bugs.freedesktop.org/show_bug.cgi?id=100136
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This is a particularly opaque part of the code for managing pending
Drawable operations. This patch adds documentation atempting to explain
these functions.
It can be useful for debug to know what the codec of the playback and
record channels are, so add a debug-level print statement indicating
this.
Resolves: rhbz#1436251
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Main-channel-client.c has code to send pings when the statistics code is
enabled. This is done through a timer calling a ping_timer_cb callback.
However, this timer is created but never started, so this code is
actually dead-code.
Since this code does not seem to be doing much apart from sending the
pings (which can be achieved by simply setting monitor-latency to TRUE
in main_channel_client_create()), this commit removes it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Their uint32_t value is never used, all that matters is whether we
received data or not.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
in main_channel_push_agent_connected(), it used the convenience function
red_channel_test_remote_cap() to determine whether to send the
AGENT_CONNECTED_TOKENS message, or the AGENT_CONNECTED message. However,
this function returns false if *any* one client doesn't support the
capability. We should instead check each individual client separately to
see if they support the capability.
This was a convenience function around
red_channel_client_test_remote_common_cap() that simply iterated over
each client (currently we only really support a single client anyway)
and returned TRUE only if all clients supported the capability. But
nobody ever called this wrapper function.
Both _test_remote_cap() and _test_remote_common_cap() are used as
boolean values, so change the return type from int to bool to follow our
coding standard.
This is a setting for determining whether to compress the audio playback
channel or not. It is variously typed as int or uint32_t. Convert it to
a 'bool' to make it more clear that it is a true/false value rather than
an enumeration or something like that.
The DUMP_BITMAP compile option enable some debugging code to
output image bitmaps in a subdirectory of /tmp.
However if this directory does not exists the server will crash
as not able to create a file in it.
Try to create directory before creating the file.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
- glz_enc_dictionary_reset
- monitors_config_new
- red_channel_any_blocked
- red_channel_no_item_being_sent
- red_client_get_channel
are only used in the file where they are defined, so they can as well be
static.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is only available in newer FreeBSD releases (9.1 and later), and
will cause build errors or older versions
This fixes https://bugs.freedesktop.org/show_bug.cgi?id=99213
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This new function removes one place outside of RedsStream which needs to
access RedsStream::socket
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This allows to move some low-level code out of reds.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This allows to move some low-level code out of reds.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The code to enable/disable on a TCP socket is duplicated in multiple
places in the code base, this commit replaces this duplicated code with
a helper in RedsStream.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Doing a memset(0) on a SpiceStatNode does not create an empty/unattached
node, but instead creates a node '0'. This node could be non-existing,
or totally unrelated.
This patch creates a default '0' node as soon as the file is created.
This will make the behaviour of 0-filled nodes more in line with what
one would expect.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This changes the header guards in all .h files to follow this format:
/*
* Licensing block
*/
/* ... */
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This is the default value, and the property is marked as
_CONSTRUCT_ONLY, so we don't need to explicitly force the default when
instantiating a RedChannelClient.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
InputsChannelClient::new and SmartcardChannelClient::new both accept a
"monitor_latency" argument, which is always FALSE. It can be removed.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Before GLib 2.36 you should call g_type_init before attempting
any GLib type usage.
As constructor function are called before even main we need
to call g_type_init much before do_spice_init.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Stress a bit video encoders.
This check different combination of
- encoder type;
- image formats;
- image clipping (encoding partial frames);
- handling frames split into chunks.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Handle single chunk writev as normal write.
From some test more than 60% of the times writev is called with 1 as
counter. We can easily and very cheaply turn this call to a simpler
write avoiding the need to pass the array to the kernel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
As pointed out by danpb, tests/pki/server-cert.pem is only valid until
Fri Nov 29 15:51:44 UTC 2019
This changes tests/pki/server-cert.pem and tests/pki/ca-cert.pem to be
valid until 2048. These certificates were generated using the
instructions on https://www.spice-space.org/page/SSLConnection
The -subj args were omitted, and the defaults suggested by openssl used.
The -days parameter was changed to -days 10950.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This allows to remove a small hack in server/Makefile.am where we were
using make check-valgrind-memcheck rather than make check-valgrind to
make sure we get a non-0 exit code on failures.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This allows to run automatically our test-suite with valgrind to test
for memory leaks.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
test-qxl-parsing is really a series of several tests. Porting it to
GTest makes this more obvious. This also has the side-effect of making
it more friendly to 'make check-valgrind' (which would fail on SIGALRM,
and on unexpected g_warning()).
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
On my 64 bit Fedora 25, CMSG_SPACE() adds 4 bytes of padding after the
file descriptor in the control data. This causes warnings when ran under
valgrind as we set msg_controllen to CMSG_SPACE().
This commit fills the control data to 0 to avoid these warnings.
==30301== Syscall param sendmsg(msg.msg_control) points to uninitialised byte(s)
==30301== at 0x8127367: sendmsg (sendmsg.c:28)
==30301== by 0x41880B: reds_stream_send_msgfd (reds-stream.c:295)
==30301== by 0x40953F: main (test-stream.c:121)
==30301== Address 0xffefff1b4 is on thread 1's stack
==30301== in frame #1, created by reds_stream_send_msgfd (reds-stream.c:263)
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This reverts commit c3d237075b.
When you call gst_buffer_add_video_meta_full GStreamer assumes that
buffer is contiguous. Specifically GStreamer assumes that the first
chunk is big enough to hold a whole frame. This results usually in
some pixel shifts in the video. The pixel shifts you can see are
artifacts due to how the guest sends the frames.
Assuming you allocate the 2 chunks with 2 malloc:
p1 = malloc(size);
...
p2 = malloc(size);
Usually the memory allocator tend to allocate linearly if there are
space adding a prefix to describe next block leading to a memory
arrangement as:
+-------------------+
| p1 prefix |
+-------------------+
| p1 buffer |
+-------------------+
| p2 prefix |
+-------------------+
| p2 buffer |
+-------------------+
now if you take p1 pointer and you assume it points to a 2 * size
buffer you will get p2 prefix in the middle, this prefix is the pixel
shifts.
Problems happens specifically in gst_video_frame_map_id.
This bug is reported in https://bugzilla.gnome.org/show_bug.cgi?id=779524.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
A few dispose/finalize implementations were not chaining up to their
parent classes, causing memory leaks.
This fixes leaks like the following:
==16240== Memcheck, a memory error detector
==16240== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==16240== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==16240== Command: ./test-leaks
==16240==
==16240==
==16240== HEAP SUMMARY:
==16240== in use at exit: 245,490 bytes in 3,638 blocks
==16240== total heap usage: 5,418 allocs, 1,780 frees, 17,676,968 bytes allocated
==16240==
==16240== 16 bytes in 1 blocks are definitely lost in loss record 203 of 771
==16240== at 0x4C2DADE: malloc (vg_replace_malloc.c:298)
==16240== by 0x4C2FC91: realloc (vg_replace_malloc.c:785)
==16240== by 0x52864E: dispatcher_register_handler (dispatcher.c:374)
==16240== by 0x5293E0: main_dispatcher_constructed (main-dispatcher.c:315)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x4095B1: server_leaks (test-leaks.c:45)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== 16 bytes in 1 blocks are definitely lost in loss record 204 of 771
==16240== at 0x4C2DADE: malloc (vg_replace_malloc.c:298)
==16240== by 0x4C2FC91: realloc (vg_replace_malloc.c:785)
==16240== by 0x52864E: dispatcher_register_handler (dispatcher.c:374)
==16240== by 0x5293E0: main_dispatcher_constructed (main-dispatcher.c:315)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x40BFD4: test_new (test-display-base.c:902)
==16240== by 0x40979D: vmc_leaks (test-leaks.c:92)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== 96 bytes in 1 blocks are definitely lost in loss record 638 of 771
==16240== at 0x4C2FA50: calloc (vg_replace_malloc.c:711)
==16240== by 0x8427D3C: g_malloc0 (gmem.c:124)
==16240== by 0x842801F: g_malloc0_n (gmem.c:355)
==16240== by 0x527B44: dispatcher_constructed (dispatcher.c:141)
==16240== by 0x529321: main_dispatcher_constructed (main-dispatcher.c:307)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x4095B1: server_leaks (test-leaks.c:45)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== 96 bytes in 1 blocks are definitely lost in loss record 639 of 771
==16240== at 0x4C2FA50: calloc (vg_replace_malloc.c:711)
==16240== by 0x8427D3C: g_malloc0 (gmem.c:124)
==16240== by 0x842801F: g_malloc0_n (gmem.c:355)
==16240== by 0x527B44: dispatcher_constructed (dispatcher.c:141)
==16240== by 0x529321: main_dispatcher_constructed (main-dispatcher.c:307)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x40BFD4: test_new (test-display-base.c:902)
==16240== by 0x40979D: vmc_leaks (test-leaks.c:92)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== LEAK SUMMARY:
==16240== definitely lost: 224 bytes in 4 blocks
==16240== indirectly lost: 0 bytes in 0 blocks
==16240== possibly lost: 0 bytes in 0 blocks
==16240== still reachable: 207,718 bytes in 3,312 blocks
==16240== of which reachable via heuristic:
==16240== newarray : 1,536 bytes in 16 blocks
==16240== suppressed: 34,548 bytes in 302 blocks
==16240== Reachable blocks (those to which a pointer was found) are not shown.
==16240== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==16240==
==16240== For counts of detected and suppressed errors, rerun with: -v
==16240== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 20 from 20)
FAIL test-leaks (exit status: 1)
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The code that manages pending QXL Drawable operations is fairly complex
and difficult to understand. This is an attempt to start documenting
that code to save time when we have to work on this code in the future.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This commit changes all functions returning TRUE/FALSE from having an
'int' return value to 'bool'.
This way it's obvious that such a function is not going to return
anything else than TRUE or FALSE.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
config_socket is configuring the client stream socket.
As is responsibility of RedChannelClient to handle the stream
it make more sense to have the function in this object.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Verify stuff are freed correctly (like TLS context).
The different PKI file required are generated with
base values (localhost and rsa 1024).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Add and remove some vmc device to check for leaking.
These combination assure that currently implemented type
of devices (webdav, usb and generic) are checked.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The nested if statements could be confusing, no needs for them.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Send header and reply together.
This potentially save up to 160 bytes on the network which
is a considerable amount considering that the message is
about 50 bytes.
This as sending multiple chunks you can have different framing,
specifically:
- if you use TLS every chunk get encrypted separately
(reds-stream, currently usually 29 bytes for every chunks);
- tcp settings and no delay on socket. More likely with fast
connections or better network cards. The tcp framing is
usually about 80 bytes;
- additional tcp acknowledge (usually 64 bytes).
So 80 + 29 + 64 = 173 bytes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These vfuncs are more appropriate in RedChannelClient.
The buffer they allocated are related to the client stream
which is managed directly by RedChannelClient.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This prepare for the next patch.
The network recieve buffer should be per-client rather than per-channel.
The following patch will make this change, but this common base class
will allow the cursor client and the display client to share a common
implementation.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
As the counters are shared there is no reason why not
handling the byte count from RedChannelClient directly.
This remove a dependency and avoid some function calls.
The only visible difference at user level is that the
counters are created when a client connects.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Show messages sent to clients.
This is useful to understand the message number as an high
message number can affects performance and is not easy to
understand the message count from the byte count (which is
available).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Instead of disabling the code use the compatibility functions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
[0] SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE
This message provides a list of video codecs based on client's order
of preference.
We duplicate the video codecs array from reds.c and sort it using the
order of codecs as reference.
This message will not change an ongoing streaming but it could change
newly created streams depending the rank value of each video codec
that can be set by spice_server_set_video_codecs()
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Allows to use recording function for multiple purposes.
This will allow to register multiple screen VM or recording
additional stuff like sound.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The synchronization code is required to avoid mixing writing
from multiple threads.
Following patches will add this feature.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
For each channel there are two set of capabilities, one
for the common ones and one for the specific ones.
A single set were almost always passed using 2 arguments,
a number of elements and an array but then before using
these were converted to a GArray.
Use a single structure (already available) to pass all
channel capabilites using a single argument.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Add function to initialize and destroy this type.
Add GType type for boxing it.
These changes a in preparation for next patch.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These properties are not read and code is broken (the content of
the array would be uninitialized).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Watch was added but never removed.
The added basic_event_loop_destroy() addition allows to see that
this leak is gone).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This switches the test to using the GTest API, and add several tests
related to https://bugzilla.redhat.com/show_bug.cgi?id=1411194
This uses some API not available in glib 2.28, so this checks we have a
new enough glib before building this test, and disables warnings when
using too new glib API when building it.
The "multiple-vmc-devices" is based off code written by Frediano Ziglio.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Using spice_info() gets in the way of tests using
g_test_expect_message() as all the messages emitted using
a non-debug log level must be listed as expected, otherwise we get a
critical about an expected message not having been logged.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Currently, the network sockets opened by reds_init_net() are not closed
on destruction, in other words they are leaked.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This allows to chain several test cases by using
test_new()/test_destroy().
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Do not assume the device passed as an argument to
spice_server_char_device_remove_interface() is the same as the current
Reds::vdagent instance. This commit adds a check that this is the case,
and returns early if not.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
spice_new is a bit more safe as return a properly typed pointer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The usage was removed with commit 7ea1f2c133
("sound: Use RedChannelClient to receive/send data").
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Using X == TRUE is fragile, since the input value is a uint8_t. It would be
surprising if the value was set to 2 or 0xFF and the test failed.
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Some constants/macros/function prototypes are defined in
red-channel-client.h while they are only used by red-channel-client.c.
This commit moves them to the .c file to make them private.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
RedChannelClient is responsible for talking to the client so it knows
if is connected or not.
These vfuncs where used by DummyChannel used by SoundChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Most channels don't need to do specific settings for the client socket
so avoid the need to do this setting making easier to setup the client
channnel.
Some improvements and commit subject suggested by Christophe Fergeau.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
TCP_NODELAY flag is set by default for all connection inside
reds.c so there's no need to set again for the single
client channel.
Note that there are still some calls to setsockopt to change this
option.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Since c3d237 "gstreamer: Avoid memory copy if strides are different" is
only needed when zero copy is disabled. This moves the function
definition to an already existing #ifdef block.
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This is a revert of b76e561d.
For a SpicePlaybackInstance, the base interface must be a
SpicePlaybackInterface instance, not a SpiceBaseInterface instance, or
spice-server code will end up reading out of bounds.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is a revert of 93b4f4050^ and 93b4f4050.
For a SpiceCharDeviceInstance, the base interface must be a
SpiceCharDeviceInterface instance, not a SpiceBaseInterface instance, or
spice-server code will end up reading out of bounds.
vmc_state/vmc_write/vmc_read implementations also have to be provided.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is a revert of 93b4f4050^ and 93b4f4050.
For a SpiceCharDeviceInstance, the base interface must be a
SpiceCharDeviceInterface instance, not a SpiceBaseInterface instance, or
spice-server code will end up reading out of bounds.
vmc_state/vmc_write/vmc_read implementations also have to be provided.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Use new structures and functions to implement the statistics code.
Use inline functions instead of macros for increased type-safety.
If statistics are disabled, the structures and functions become
empty. This confines the configuration-specific #defines to the
statistics implementation itself and avoids the need for #defines in
the calling functions. This greatly reduces the chance of accidentally
breaking the build for one configuration or the other. The reds option
was removed from stat_inc_counter() as it was not used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Non blocking flag is set for all connection inside reds.c so
there's no need to set again for the single client channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Do not make it assume vec contains IOV_MAX elements.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
They no longer contain any vfuncs, so calling them "handler" does not
make a lot of sense. This commit renames them to
OutgoingMessageBuffer/IncomingMessageBuffer.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Nothing outside of RedChannelClient needs access to data contained in
RedChannelClientPrivate, so we can move all the type definitions to the
.c file to make it fully opaque rather than relying on a private header.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The methods previously used by OutgoingHandlerInterface and
IncomingHandlerInterface are no longer used as generic callbacks,
but are directly called from RedChannelClient code. We can be explicit
about the type of their first argument (RedChannelClient *) rather than
using a generic void * pointer.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
There is only one implementation of OutgoingHandler which relies
OutgoingHandler::opaque being a RedChannelClient. This commit makes this
explicit in order to get rid of the OutgoingHandler::opaque data member.
This renames red_peer_handle_outgoing() to
red_channel_client_handle_outgoing() as the method is now very much tied
to RedChannelClient.
If we want to keep some genericity, we could return error codes from
red_channel_client_handle_outgoing() and handle RedChannelClient
disconnection/... from the caller rather than directly in the
_handle_outgoing() method. This would probably allow to move the
data reading logic to reds-stream.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
There is only one implementation of IncomingHandler which relies
IncomingHandler::opaque to be a RedChannlClient. This commit makes this
explicit. This allows to get rid of the IncomingHandler::opaque data
member.
This renames red_peer_handle_incoming() to
red_channel_client_handle_incoming() as the method is now very much tied
to RedChannelClient.
If we want to keep some genericity, we could return error codes from
red_channel_client_handle_incoming() and handle RedChannelClient
disconnection/... from the caller rather than directly in the
_handle_incoming() method. This would probably allow to move the
data reading logic to reds-stream.c
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This commit removes what remains of IncomingHandlerInterface. The
remaining function pointers were pointing to RedChannel vfuncs.
Moreover the IncomingHandlerInterface abstraction is unused, ie the
codebase only has a single implementation for it, so we can directly
call the relevant methods and make them static instead.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This commit removes IncomingHandlerInterface::on_error and
IncomingHandlerInterface::on_input. As with previous commits, these
vfuncs are always calling the method, and RedChannel::init sets them to
point to RedChannelClient methods, which RedChannelClient is then going
to call indirectly through the IncomingHandlerInterface instance.
This commit changes this to direct calls to the corresponding methods.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Similarly to the previous commits, this removes an indirection level,
IncomingHandlerInterface stores pointers to
alloc_recv_buf/release_recv_buf vfuncs, but these are always set to
RedChannel::alloc_recv_buf/RedChannel::release_recv_buf, which are also
vfuncs which can be overridden if needed. This commit removes the
indirection and directly calls the relevant methods.
These vfuncs really should be in RedChannelClient rather than
RedChannel.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
RedChannel uses OutgoingHandlerInterface to provide constant pointers to
RedChannelClient methods. This OutgoingHandlerInterface structure is
then used in RedChannelClient to indirectly call these methods.
The OutgoingHandlerInterface abstraction is unused, ie the codebase
only has a single implementation for it, so we can directly call the
relevant methods and make them static instead.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <figlio@redhat.com>
Have the RedChannelClient callback call into a RedChannel callback
rather than doing the opposite. This will be useful in some subsequent
refactoring of this code.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
red_channel_client_parse() currently does roughly:
if (klass->parser) {
parsed = klass->parser(msg, msg_size, &parsed_size);
klass->handle_parsed(rcc, parsed_size, msg_type, parsed);
} else {
klass->handle_message(rcc, msg_type, msg, msg_size);
}
The handle_parsed implementation expects a void * 'parsed' argument,
which will then be cast to the correct type. There is not really a need
to provide distinct handle_parsed/handle_message vfuncs, instead we can
say that if a RedChannel subclass provides a 'parser' vfunc, then it's
'handle_message' vfunc will be called with the parsed message, otherwise
it will be called with unparsed data (ie what handle_message currently
expects).
This makes the code slightly easier to follow as messages will always be
handled by the same vfunc.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
smartcard_channel_client_pipe_add_push was just calling
red_channel_client_pipe_add_push without any cast or other
changes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This makes the code slightly more readable as this means less local
variables to keep track of when taking a high level view of that code.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
read_binary() attaches 'mem' to the SpiceReplay::allocated list.
On failure, SpiceReplay::allocated and its content are freed by
spice_replay_free().
SpiceReplay::primary_mem is also freed, which causes a double free
as replay_handle_create_primary() added 'mem' both to
SpiceReplay::primary_mem and SpiceReplay::allocated.
This commit avoids this by ensuring SpiceReplay::primary_mem is not
kept in the SpiceReplay::allocated list.
Note that this double free can happen only on currupted or wrong
record images.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The Surface and Display channels each have a 'current_list' Ring, and
Surface also has a 'current' Ring. these names are confusing, so at
minimum, add a comment indicating the type of object they hold. The
DisplayChannel::current_list already had a comment, but it was
incorrect.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Caller is supposed the function return a buffer able to store
size bytes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The limits for capabilities are specified using 32 bit unsigned integers.
This could cause possible integer overflows causing buffer overflows.
For instance the sum of num_common_caps and num_caps can be 0 avoiding
additional checks.
As the link message is now capped to 4096 and the capabilities are
contained in the link message limit the capabilities to 1024
(capabilities are expressed in number of uint32_t items).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The limit for link message is specified using a 32 bit unsigned integer.
This could cause possible DoS due to excessive memory allocations and
some possible crashes.
For instance a value >= 2^31 causes a spice_assert to be triggered in
async_read_handler (reds-stream.c) due to an integer overflow at this
line:
int n = async->end - async->now;
This could be easily triggered with a program like
#!/usr/bin/env python
import socket
import time
from struct import pack
server = '127.0.0.1'
port = 5900
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect((server, port))
data = pack('<4sIII', 'REDQ', 2, 2, 0xaaaaaaaa)
s.send(data)
time.sleep(1)
without requiring any authentication (the same can be done
with TLS).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
red_channel_client_handle_message can handle base messages
so reuse it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Some gcc version reports this error:
stat-file.c: In function 'stat_file_add_node':
stat-file.c:180:15: error: 'node' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
g_strlcpy(node->name, name, sizeof(node->name));
^~~~
cc1: all warnings being treated as errors
This warning is a false positive as this loop:
for (ref = 0; ref <= stat_file->max_nodes; ref++) {
node = &stat_file->stat->nodes[ref];
...
}
will always iterate at least once.
This patch rewrite the loop in order to make more compilers
understand that node variable is always initialized.
There are two checks apparently removed in the patch:
- check for stat_file->stat not being NULL. This was worthless as the
field was already used in the function. Also this field is never
NULL (unless memory corruption happened);
- stat_file->stat->num_of_nodes >= stat_file->max_nodes. It's implicit
in the loop. If num_of_nodes >= max_nodes means that there are no
free nodes so all nodes should have SPICE_STAT_NODE_FLAG_ENABLED set,
loop will exit and function will return INVALID_STAT_REF.
Reported-by: Christophe Fergeau <cfergeau@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The stat file contains an array of max_nodes elements
so we must stay in [0, max_nodes) range, not [0, max_nodes].
There are no spice path that lead to these overflows but
it's better to have them fixed before creating one.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Adding conditional for having gstreamer_0_10 or gstreamer_1_0,
removing the previous conditionals and update relevant ifdefs
with the newly defined conditional
We support only a single client so don't waste code just
to check this.
The worst stuff can happen is that we'll migrate multiple
connections.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Move the freeing of SndChannel data members from snd_detach_common() to
the finalize function to encapsulate things a bit more cleanly. It
doesn't really change the behavior or order of destruction since
snd_detach_common() destroys the channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Sound implementation used internal RedChannelClient data while now
it just uses the public interface not thouching RedChannelClient
internal state so now is possible to make this field private.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Now that SndChannelClient has switched from using its own code for
sending data to using RedChannelClient, it's very close to being an
actual RedChannelClient.
This commit makes it directly inherit from RedChannelClient rather than
having a channel_client field. This allows to get rid of the whole
DummyChannel/DummyChannelClient code.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
snd_set_command()/snd_send() are higher level methods which take care of
scheduling calls to the corresponding snd_*_send_*() methods when
appropriate. This commit switches a few direct snd_*_send_*() calls to
snd_set_command()/snd_send().
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
You can see that SndChannelClient has much less field
as the code to read/write from/to client is reused from
RedChannelClient instead of creating a fake RedChannelClient
just to make the system happy.
One of the different between the old sound code and all other
RedChannelClient objects was that the sound channel don't use
a queue while RedChannelClient use RedPipeItem object. This was
the main reason why RedChannelClient was not used. To implement
the old behaviour a "persistent_pipe_item" is used. This RedPipeItem
will be queued to RedChannelClient (only one!) so signal code we
have data to send. The {playback,record}_channel_send_item will
then send the messages to the client using RedChannelClient functions.
For this reason snd_reset_send_data is replaced by a call to
red_channel_client_init_send_data and snd_begin_send_message is
replaced by red_channel_client_begin_send_message.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
The removed code was trying to read data when
spice_server_record_get_samples() is called. Since reading of data is
event-driven anyway (see snd_event), it's redundant to try
again to read more data.
This commit removes this code as this will some refactoring easier in
the next commits.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
We can get it from our DummyChannelClient rather than storing it in
SndChannelClient.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
We can use the marshaller provided by the dummy RedChannelClient
associated with SndChannelClient.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Filter out commands which should not happen. Should it be a
g_warn_if_fail() or such instead?
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is in preparation for switching SndChannelClient into a proper
RedChannelClient. The prototype of the new helper matches what is
expected from the RedChannel::config_socket vfunc.
To be able to achieve that, this commit associates the sound channel
RedsStream instance with the DummyChannelClient instance we have, and
then call snd_channel_config_socket() on that instance.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The main goal of this commit is to avoid to dereference 'client' before
it's checked for NULL. This meant splitting one error condition in 2
separate ones.
This also sets default values for the 'frame' and 'num-samples'
out parameters so that we just have to return on error conditions.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This structure is used to store format information for
both Gstreamer 0.10 and 1.0 however the two format uses
different fields from it.
Use a macro to filter only needed fields.
This currently also fixes a compile error using Gstreamer 0.10
(GST_VIDEO_FORMAT_RGB15 not defined as not available).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
The structure is used only to allocate private data.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Currently, calling spice_server_remove_interface() twice in a row with
the same SPICE_INTERFACE_CHAR_DEVICE is going to cause a crash when
calling red_char_device_get_server(char_device->st); because
char_device->st will have been set to NULL by the first call.
This commit adds a few sanity checks before trying to use the various
'st' members of the interfaces.
This should avoid the crash described in
https://bugzilla.redhat.com/show_bug.cgi?id=1411194 even though it's not
clear how we got in that situation.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Avoid to use g_object_get if not necessary.
red_char_device_get_server is more type safe and we are
not bound to dynamic fields.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This field is used only for debugging.
Remove it reducing a bit all these "current" fields around.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
If bitmap stride and stream stride are different copy was used.
Using GStreamer 1.0 you can avoid the copy setting correctly
image offset and stride.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Handling read returning 0 (usually end of connection/pipe)
is the same of handling an error (read result -1) with errno == 0
so merge the two paths to reuse code and simplify.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This allows the connection to early fail in case initial bytes
are not correct.
This allows for instance VNC client to graceful fail connecting
to a spice-server. This happens easily as the two protocols
share the same range of ports.
This resolves rhbz#1416692.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Tested-by: Daniel P. Berrange <berrange@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Code to read and process display commands were the same
so use a common function for better reuse.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This format is required to add metadata to the source buffer
using gst_buffer_add_video_meta_full.
This metadata can be used to pass strides/offsets, or
dmabuf-specific information.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This happened during VM resume.
RedSurfaceCmd were allocated but never freed.
We don't need to malloc the RedSurfaceCmd used in handle_dev_close()
as display_channel_process_surface_cmd() will not try to reference
it after it has returned.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Code to read and process cursor commands were the same
so use a common function for better reuse.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This allows to do some possible statistics or graph.
Currently the report contains encoded sizes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Currently MUTE and VOLUME commands use the same VOLUME mask. This commit
introduces a separate SND_MUTE_MASK for MUTE commands to make things
a bit more clear.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This is no longer used since "sound: Convert SndChannel to GObject"
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It's already defined before in the same source file.
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Based on a patch from Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It became unused in 26027036c 'red_channel: remove unused migrate flag
from RedChannel' but was never removed from the function prototype.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This became obsolete when RedChannel became GObject-based.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
These formats were tested manually using test-gst utility
in server/tests.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Add an utility to make possible to check various features of
VideoEncoder.
2 GStreamer plugins are used in a chain like this:
(1) input pipeline -> (2) video encoder -> (3) output pipeline
While converting output from (1) is compared with output of (3)
making sure the streaming is working correctly.
You can set various options:
- part of the input pipeline description to allow specifying different
video from GStreamer test ones to a video file;
- the encoder to use;
- different image properties to use for (2) input:
- different bit depth;
- top/down or down/up;
- initial bitrate.
The idea is to use this helper in combination with a shell script
and some video sources to make able to test various settings.
Also can be used to extend the current encoder list.
As an example you can use a command like
$ ./test-gst -e gstreamer:vp8 -i \
'filesrc location=bbb_sunflower_1080p_30fps_normal.mp4 \
! decodebin ! videoconvert'
to check vp8 encoding.
Currently it does not emulate bandwidth changes as stream reports
from the client are not coded.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Small refactor. As reds_get_video_codecs() returns the video codecs as
GArray, we should match reds_set_video_codecs() to have a GArray as
parameter instead of string.
reds_set_video_codecs_from_string() seems more appropriate for the
previous function.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Previously, the object we now call SndChannel was named SndWorker, and
the object we now call SndChannelClient was called SndChannel. When
these names were changed, the functions
on_new_(record|playback)_channel() were not updated, so the function
names and the arguments are both a bit confusing now. Update them to
match the new names.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
When the initial image was sent to the client the reference
was not incremented leading to some user after free.
This regression was introduced in
3bde2e570c
("DCC: remove more init_send_data() arguments").
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This fixes a regression caused by
a43c21b6bc
("DCC: change how fill_bits() marshalls data by reference").
Before the mentioned patch there were a few references to Drawable
structure so an uint8_t was enough.
Now that every chunk of the image is accounted you can easily
get an overflow.
This fixes https://bugs.freedesktop.org/show_bug.cgi?id=99258.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Fix compilation error due to -Werror=maybe-uninitialized:
CC test-display-base.o
test-display-base.c: In function 'do_wakeup':
test-display-base.c:579:13: error: 'update' may be used uninitialized...
push_command(&update->ext);
Signed-off-by: Snir Sheriber <ssheribe@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This third argument (and the 'item' member of
RedChannelClient::priv::send_data) was a somewhat roundabout way to keep
the RedPipeItem alive until a message is sent, just in case some data
owned by that pipeitem was added to the marshaller by reference. This
was a rather confusing mechanism, however, since it did not have any
obvious connection to the _add_by_ref() call. It was never very clear
whether you needed to pass an item to this function or not. The previous
series of patches made this parameter unnecessary since the referencing
of the pipe item (or other related structure) is now more explicitly
connected to the calls to spice_marshaller_add_by_ref_full().
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The fill_bits() function marshalls some data by reference. This data is
owned by the RedDrawable that is owned by the Drawable that is owned by
the RedDrawablePipeItem. Instead of keeping the RedPipeItem alive by
passing it to red_channel_client_init_send_data(), simply reference the
Drawable and marshall it with _add_by_ref_full(). This means that we
can't use the _add_chunks_by_ref() convenience function since that
function doesn't allow us to pass a free function to clean up the data
after it is sent.
This change is not perfect since the fill_bits() function makes an
assumption that 'simage' is owned by the 'drawable'. On the other hand,
the previous code made a much bigger assumption: that the caller would
ensure that the data would be kept alive
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Use spice_marshaller_add_by_ref_full() instead of _add_by_ref() to
handle the referenced data properly rather than passing the pipe item to
red_channel_client_init_send_data() to keep the pipe item alive
indirectly.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Use spice_marshaller_add_by_ref_full() instead of
spice_marshaller_add_by_ref() to allow the marshaller to manage the
lifetime of the referenced data buffer rather than having to manage it
by passing a PipeItem to red_channel_client_init_send_data(). Since the
data is owned by CursorItem (which is not in fact a RedPipeItem, but is
owned by a pipe item and is itself refcounted), we take a reference on
the CursorItem when adding the data buf to the marshaller, and then
unreference it in the marshaller free func.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The only time that the pipe item needs to be passed as the third
argument to red_channel_client_init_send_data() is when the pipe item
holds a data buffer that has been added to the marshaller by reference
(spice_marshaller_add_by_ref()) and needs to be kept alive until the
data has been sent. In all other cases, the item does not need to be
kept alive, so we can safely pass NULL for this third parameter.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Any data that is added to the marshaller by reference (using e.g.
spice_marshaller_add_by_ref_full()) is freed during
spice_marshaller_reset(). But the marshaller is not currently reset
until we begin to send the next message (in
red_channel_client_send_item()). This means that the sent message data
lives longer than expected and can violate some assumptions in other
parts of the code.
To make sure that the data is cleaned up right after being sent, I've
added a reset call to clear_sent_item() and called that function from
_on_out_msg_done(). This means that _restore_main_sender() no longer
needs to reset the marshaller, and we no longer need to call
_reset_sent_item() within _on_out_msg_done() (since this function is
called from _clear_sent_item()).
This prepares the way for refactoring
red_channel_client_init_send_data() to change how we keep the
RedPipeItem alive while sending a message.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
We should replace the video_codecs GArray only after the parsing of
input is done, otherwise we might lose previous configuration.
Tests were updated to match this situation. Input that fails to
replace video_codecs are considered bad.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This make more obvious which directory they refer
and potentially avoid ignoring unwanted files.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The partial expression "MSEC_PER_SEC * size * 8" can overflow if
size is 536870 or more. This as the operation is done using
32 bit unsigned integers. Being the size potentially double of
a compressed frame size the limit can be easily reached.
As get_average_frame_size already return a 64 bit use 64 bit
even for the size to avoid the integer overflow.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Francois Gouget <fgouget@codeweavers.com>
This function is supposed to add an item to the queue to
be sent before all other queued items.
Was never used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
refs was used before GObject for reference counting.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
All RedWorker messages starts with RedWorker except
SpiceMsgDisplayGlDraw.
For coherence introduce a RedWorkerMessageGlDraw structure
holding just SpiceMsgDisplayGlDraw. This also allows possible
extensions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
The spice_marshaller_add_ref() family of functions is confusing since it
sounds like you're incrementing a reference on the marshaller. What it
is actually doing is adding a data buffer to the marshaller by reference
rather than by value. Changing the function names to _add_by_ref() makes
this clearer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
spice_debug was called for not-EINTR case, move
it to the right place.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
man 2 dup2 specifies:
The close-on-exec flag (FD_CLOEXEC; see fcntl(2)) for
the duplicate descriptor is off.
Since the purpose of the fcntl call is to turn off FD_CLOEXEC
flag, and it's already done, just remove this call.
Suggested-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Spice coding style suggests to use curly braces
for while blocks.
Some prefer the block to not be empty so continue
is untouched.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This patch prevents possible memory leak.
Found by coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In both image_encoders_restore_glz_dictionary() and
image_encoders_get_glz_dictionary() shared-dict may
be NULL if size is too large, and the server gets
size from the network.
Both functions end up calling glz_enc_dictionary_create()
that calls glz_dictionary_window_create() where size is
checked.
Found by coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It shadows the outer one.
Renamed also the outer 'container' variable.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Allow to catch minor issue with test code.
Although test usually are coded with less care than production
code the current changes required are not so extensive and
can catch different issues.
Most of the patch is making functions static to avoid warnings for
undeclared functions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This patch creates display_channel_get_video_codecs() helper to let
stream.c get the video-codecs without accessing the internal structure
of DisplayChannel.
As video-codecs is a property of DisplayChannel, this change means
making this property readable as well.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Allow to dynamically remove QXL interfaces. This could be used to
support hot swapping of QXL cards.
This code is actually not used in any way.
QXL interfaces are destroyed by spice_server_destroy automatically.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Make finalization of DisplayChannel consistent with other code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Reduce GObject changes coming in the next commit since we'll need to
change how we access the marshaller anyway. This will make the
following commits easier to review.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
As data is packae in a single piece of memory send it
altogether.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Including g_clear_pointer() in glib-compat.h and using it to avoid
warnings in odd situations.
Signed-off-by: Victor Toso <victortoso@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
When qemu (for example) delivers audio samples to the spice server, it
does so by requesting a buffer from the spice server
(spice_server_playback_get_buffer()), filling them with audio data, and
then calling spice_server_playback_put_samples() to send them to the
client. Between the call to _get_buffer() and the call to
_put_samples(), we need to ensure that the buffer remains valid. Since
this buffer is allocated within PlaybackChannelClient, we did this by
incrementing a ref on the PlaybackChannelClient in _get_buffer(), and
decrementing the ref in _put_samples(). This has the effect of
potentially keeping the PlaybackChannelClient alive after the spice
client has disconnected.
This was not a problem when PlaybackChannelClient was a simple helper
class. But we intend to change PlaybackChannelClient so that it
inherits from RedChannelClient. In that case, the reference taken in
_get_buffer() would result in the RedChannelClient (and associated
RedChannel, etc) being kept alive longer than expected. To avoid this,
we add an additional ref-counted adapter class (AudioFrameContainer)
that owns the allocated audio frames and can outlive the
RedChannelClient if necessary. When the client is freed, the
AudioFrameContainer is just unreferenced.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Stops using the dummy channel.
Data handling still goes through DummyChannelClient which is why
empty implementation of some required vfuncs is working.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Try to arrange destruction in the opposite order of the creation
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The check to limit too low bit rates was setting encoder->bit_rate
instead of bit_rate. However after some lines bit_rate was used
to set encoder->bit_rate basically removing the lower threshold.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Francois Gouget <fgouget@codeweavers.com>
By removing the stream's video encoder we force the stream to send
future frames using the fallback code, that is as regular screen
updates.
However note that we keep the stream object: we have to. Otherwise
future frames would trigger the creation of a new stream object with a
new video encoder which would again try to stream the video and fail
again and again.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Reviewed-by: Victor Toso <victortoso@redhat.com>
Make easier to understand that they refer to client and not
all channel.
Specifically:
- RecordChannel -> RecordChannelClient
- PlaybackChannel -> PlaybackChannelClient
- playback_channel -> playback_client
- record_channel -> record_client
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
SndWorker has been historically based on RedChannel, initial git commit
has:
struct SndWorker {
Channel base;
...
};
SndChannel, contrary to what its name may imply is more focused on
marshalling/sending of sound data, which is the responsibility of
RedChannelClient for the other SPICE channels.
This commit and following ones make the naming of these 2 types more
consistent with how the rest of the code is structured.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
SpicePlaybackState and SpiceRecordState have same structures
changing only slightly the behaviour.
Using SndWorker instead allows some minor simplification and
more code reuse.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Most of the times the check is done externally
so moving inside the function reduce the code.
This is similar to the way free(3) works.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This function assume there is only one client.
Was used only by some obsolete functions.
Avoid to use such function in the future.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This function wrongly close the first client.
Wrongly as closing the file descriptor cause a dandling
file descriptor in the object potentially leading
to closing another file descriptor open later.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This will close all clients and release the channel properly
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Now that RedStatFile is private there is no need
to include some headers in stat-file.h.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
These functions are not used since years and are not supporting
multiple clients.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Use same constants for common commands.
This will allow code reuse between Record and Playback.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Allows to close worker thread.
This will be used to destroy cleanly CursorChannel and
DisplayChannel.
CursorChannel and DisplayChannel are run in a different
thread. However deregistration of channels and different
steps of destruction should be done in the same thread
so this make possible to join again the 2 threads to
avoid race conditions.
For the moment there is no correct cleanup.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This make compression faster and avoids a warning on newer
lz4 versions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Use dash instead of underscores for file names. This is coherent
with rest of file names.
Rename all tests to test-XXX and remove the spice- prefix when
present. This is coherent with most of the tests.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
self is usually used for GObject, avoid confusion.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The video encoders already have sophisticated bit rate control code that
can react to both stream reports sent by the client, and server frame
drop notifications. Furthermore they can adjust both the frame rate and
the image quality to best match the network conditions.
But if the client cannot send stream reports all this is bypassed and
instead the streaming code performs its own primitive bit rate control
that can only adjust the frame rate.
So this patch removes the code duplication and lets the video encoders
do their job.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The wrong type (RED_TYPE_CHANNEL_CLIENT rather than
TYPE_SMARTCARD_CHANNEL_CLIENT) was used when creating a
SmartcardChannelClient, which meant the _init function was never called,
and SmartcardChannelClient::priv was never initialized.
This caused a crash when trying to connect with --spice-smartcard to a
VM with a smartcard channel configured.
This leaks happen for every connection. Potentially the timer can
be called after the client is closed causing an use after free.
Recently RED_STATISTICS was switched off by default but previous
version have this issue.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This reverts commit c6881ad1a0.
This patch cause the replay utility run at full speed
to slow down a lot and in some cases getting stuck.
I don't understand the reason and when I tested was working
but as we are going to release would be a pity if this test utility
won't work as useful to get feedback and reports.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The following build error occurs when building outside of the source
directory:
glib-mkenums --template spice-server-enums.tmpl.c ../../server/spice-server.h > spice-server-enums.c
Can't open spice-server-enums.tmpl.c: No such file or directory
Makefile:1111: recipe for target 'spice-server-enums.c' failed
Make/Automake uses VPATH to determine that the spice-server-enums.tmpl.c
file listed in the prerequisites for the rule is located in the srcdir.
When we use an automatic variable (e.g. $<), the full path to the
resolved file is used. But when we use the literal filename directly
within the rule definition, this won't happen. So we need to explicitly
specify that the input template file is located in srcdir.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
mmap memory area will remain even if the descriptor is closed.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Avoid to produce loop in the tree removing and adding nodes.
Unlink the node from the containing tree.
This possibly will create unlinked trees if a parent node is
deleted.
What was happening is that the creation of loops inside
the tree caused some statistical function to go into an
infinite loop (and reds_stat tool too).
Nodes were only invalidated but still linked so when reused
the new node could point to an already existing node (like a
sibling) which pointed to the new reused one.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Code is quite independent so move in separate file.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Avoid the following warning when build with disabled gstreamer:
Spice-WARNING **: reds.c:3660:reds_set_video_codecs: spice: unsupported video encoder gstreamer
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Avoid not constant globals.
We started encapsulating all global state into RedsState however
there are still some global variable. This patch remove the
core_public global variable.
To implement this a new SpiceCoreInterface *public_interface
field is added to SpiceCoreInterfaceInternal and the
SpiceCoreInterfaceInternal* argument is passed to callbacks to
understand which external interface to use.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The top down flag can be specified using negative heights.
According to
https://msdn.microsoft.com/en-us/library/windows/desktop/dd183376(v=vs.85).aspx:
"The height of the bitmap, in pixels. If biHeight is positive, the
bitmap is a bottom-up DIB and its origin is the lower-left corner.
If biHeight is negative, the bitmap is a top-down DIB and its origin
is the upper-left corner."
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
These cause compiler errors using RHEL6.
These typedefs are defined in the same file some lines above.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
GCond/GMutex interface is different between Glib 2.32 and
previous versions. Use pthread for compatibility.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
These function were really similar.
Factor out a new snd_get_best_rate to avoid code duplication.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This field is common to SpicePlaybackState and SpiceRecordState
which are based on SndWorker.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The common function is supposed to clear the state of SndWorker
so clear even volume which now is in SndWorker
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Include common and libraries includes before local ones as
stated by style.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Syntax checker complained about autoconf variable expansion used
inside Makefile.am.
This patch uses template files instead of options.
This also reduces quoting making template code more readable.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The verify macro used currently has some problem
as it raise a warning in RHEL6.
Use new SPICE_VERIFY macro defined in spice-common
to avoid this issue.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Add a 'self' parameter to all of the char device virtual functions so
that we don't have to play games with the 'opaque' pointer.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Function just return the command, has no ext_cmd parameter to fill.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Pass the new sound channel to these functions rather than setting
worker->connection before calling the function and expecting it to be
set when the function is called. The fewer unenforced assumptions that
must be true for a function to work properly, the easier it will be to
maintain.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Reduce SpiceVolumeState structure size on 64 architectures
swapping the order of two fields.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Reuse more code in snd_send_volume and snd_send_mute.
Reduce conversions in on_new_playback_channel and
on_new_record_channel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This field is common to SpicePlaybackState and SpiceRecordState
which are based on SndWorker.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Time is always the the current real time so avoid to compute
it for every call but move to red-record-qxl.c.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The functions declared in that header are all exported by the
library.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Instead of waking up the command loop for every command queued,
handle saved wakeups and replicate these.
This better reproduces what happened in the server.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The QXLMessage has no size so potentially a guest could give an
address that cause the string to overflow out of the video memory.
The current solution is to parse the message, release the resources
associated without printing the message from the client.
This also considering that the QXLMessage usage was deprecated
a while ago (I don't know exactly when).
This patches limit the string to 100000 characters (guest can feed
so much logs in other way) and limit to video memory.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
SpiceBlend and SpiceCopy are just different names for the same
structure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Due to syntax-check limitation this free calls results in
a syntax error.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Use g_param_spec_object() instead of g_param_spec_pointer() for the
'client' and 'channel' properties now that these types are GObjects.
This improves refcounting and typesafety slightly.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Instead of having channel and device object create one the other
create the objects at the beginning and then join them.
This make explicit the code that links the two objects and separate
the objects creation from the linking.
Also remove some boilerplate code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
After renaming the object to RedVmcChannel, the local variables still
used the old 'state' terminology. Changing these variables to 'channel'
makes things a bit more consistent.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This helper function does nothing but cast the return from
red_channel_client_get_channel(), and it has a confusing name
(_get_state(), but returns a channel)
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This move object destructions from spicevmc_device_disconnect
to RedCharDeviceSpiceVmc destructor functions assuring any possible
RedCharDeviceSpiceVmc object free will clear its references.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
No reason why this should be done only on spicevmc_device_disconnect.
red_char_device_write_buffer_release can be called with first pointer
NULL.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Assure field is freed at the end and not used or allocate again.
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Store the channel in the char device object, and unref it in dispose.
Previously, we were just creating a channel and potentially allowing it
to leak. There may be better long-term solutions, but this works for
now.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Also move the RedClient struct out of the header to avoid accessing the
internals from other files.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Instead of requiring the channel client to lock the client's lock,
re-arrange things so that all of the locking can be internal to
RedClient methods. So instead of having a pre-create validate function,
we simply move the check to red_client_add_channel() and return an error
if a channel already exists. This encapsulates the client implementation
better and also reduces code duplication in RedChannelClient and
DummyChannelClient.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Switch from a Ring to a GList so that we can hide the internals of
RedClient in a future commit.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It's already defined in the same file some lines above.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Make the order of initialization closer to what it was before
conversion to GObject.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
These fields need just channel to be set to be initialized.
Move their initialization to constructor to make sure
they are initialized as soon as possible.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
There was a chance that on error GQueue were not
initialized but an attempt to destroy it is made.
This ensures GQueue is initialized as soon as
possible. Note that red_channel_client_initable_init
is called after all init and construction callbacks.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Internal types use the 'Red' prefix by convention, rather than the
'Spice' prefix. In addition, this type inherits from RedChannel, so
makes the code a lot clearer to call it a 'channel' rather than a
'state'.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
id field of RedChannelClient is not used and marked as debugging.
Also it's value is quite confusing.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
In commit beec1b41, we manually limited this property value in
_set_property(). But there's a simpler way to do it: via the param spec
for the property.
This also means that we can remove the warning log in red_worker_new()
since GObject will automatically warn if a property is assigned a value
outside of its valid range.
Change the minimum and default value for this property from 0 to 1 so
that we always have a primary surface.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
These functions were implementing the same stuff as empty
messages functions provided by RedChannel so reuse them.
The implementation seems a bit different but the result
is the same. Specifically:
- RedEmptyMsgPipeItem::msg is int while RedVerbItem::verb was
uint16_t however this data goes into the message type which
is uint16_t (a 16 bit on the network protocol);
- red_channel_client_send_empty_msg calls
red_channel_client_begin_send_message while red_marshall_verb
does not. However red_marshall_verb is called only by
cursor_channel_send_item and dcc_send_item which always
calls red_channel_client_begin_send_message.
Note that in dcc_send_item when an empty message is sent
red_channel_client_send_message_pending always returns
true;
- when a PipeItem is created red_channel_client_pipe_add_empty_msg
calls red_channel_client_push while red_pipe_add_verb does not.
This actually make very little difference as this kind of item are
never removed from the queue and a push is forced in every case
running the event handler for the stream watch (see
prepare_pipe_add and red_channel_client_event).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
red_drawable_unref declaration was moved to red-parse-qxl.h.
Result is that only RedDispatcher know of the existence of RedWorker.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
No reason why RedWorker should know the capabilities of
DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
When uncapped x264enc can compress the frames beyond recognition in low
bitrate situation. Beyond the set limit the gains are modest and it is
better to drop frames to reduce the bit rate further.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This was introduced with 96e94c6f32
(Convert RedChannel hierarchy to GObject).
The id of CursorChannel/DisplayChannel were always 0 causing
a wrong assertion on the code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
In this case there is not much change but better
to follow that style as all other constructors
do.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This was introduced with 96e94c6f32
(Convert RedChannel hierarchy to GObject).
The handle-acks settings was TRUE for CursorChannel and DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This was introduced with 96e94c6f32
(Convert RedChannel hierarchy to GObject).
The type for "core-interface" property should be
SpiceCoreInterfaceInternal, not SpiceCoreInterface.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
96e94c6f inadvertantly introduced a regression where an assert was
triggered in red_channel_constructed for DummyChannel since DummyChannel
didn't implement any of the expected RedChannel vfuncs. This patch
avoids the assert by assigning some empty vfuncs.
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
red-common.h included utils.h which included red-common.h
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Allow both compiled and non-compiled tests to be used with "make
check". Compiled tests should be added to check_PROGRAMS, and scripts
that do not need to be built should be added to TESTS.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This makes red_monitors_config_item_new() and
red_monitors_config_item_free() symmetric.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
These are already defined in red-channel.h which is included in
red-channel-client.h header.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
vdi_port_read_buf_release is registered passing data as
RedVDIReadBuf*, not RedPipeItem*. Cast opaque to proper
pointer type to avoid the assumption that first field of
RedVDIReadBuf is a RedPipeItem.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This patch prevents a leak in case the function returns early
Found by coverity.
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Silence a warning:
reds.c:150:25: warning: 'lock_cs' defined but not used [-Wunused-variable]
static pthread_mutex_t *lock_cs;
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This fixes a compilation error with gcc 4.4.7 on RHEL 6.8.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
In preparation for converting RedChannel to GObject, switch to using
RED_CHANNEL()-type macros for casting. For now they just do a regular
cast, but it helps reduce the size of the GObject patch to make it
easier to review.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Move out of red-worker.c. This requires a little bit of minor
refactoring to avoid accessing some RedWorker internals in the
constructor function, etc.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
CursorChannelClient is already defined in cursor-channel-client.h.
This fixes compilation errors with gcc 4.4.7 on RHEL 6.8.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
When MainChannelClient was split to a separate file, the responsibility
for incrementing this field was supposed to belong to the MainChannel
function (main_channel_connect_semi_seamless()), but by mistake it was
incremented both there and in the client function
(main_channel_client_connect_semi_seamless()).
The bug was introduced in a11b785f19
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Convert the RedChannelClient heirarchy into GObjects. Since the existing
constructors could fail and return NULL, I inherited the base channel
client from GInitable, which introduces a dependency on gio.
When using private structs with GObject, there's a maximum size of (I
think) 64k, which was exceeded by some of the private structs. To avoid
this limitation I changed some members to dynamically allocated.
Attempt to use consistent naming.
Usually we use surface name for RedSurface.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Attempt to use consistent naming.
Usually we use surface name for RedSurface so make sure
code reader do not get confused using a different name
for RedSurfaceCmd.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This function deal only with Stream.
Also the name was misleading and was not clear if it detached the stream
from the DisplayChannel.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Currently this is simply avoided by the fact that Virgl with 3d means
Unix socket. Once you enable (in Qemu) tcp sockets this message will
be added to all clients (supposing multiple clients) so potentially
will be in all queues. The same check is done for dcc_gl_scanout_item_new.
dcc_gl_scanout_item_new is called when Qemu calls spice_qxl_gl_draw_async.
Technically a client can support SPICE_DISPLAY_CAP_GL_SCANOUT but server
cannot send the DRM prime directly as this require a unix socket so
if the test for SPICE_DISPLAY_CAP_GL_SCANOUT is done here it make sense
to do the check for the socket type too.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Move large buffer field at the end of structure.
Due to the way machine address memory this usually can reduce code size
and make program sligthly faster.
Actually reduce size by 100 bytes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
In case of invalid value the original compression is unchanged.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
These fields were added in a32e90257e
as part of the multiple client support and were never used.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
All other options are documented using initial capital case letter.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
serial was the future serial to send while last_sent_serial was the
last sent.
serial sent started from 1.
To make sure sequence variable is updated just before sending the
message, not every message prepared.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Avoid negative syntax. Also could prevent some memory problem is number
get too big.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Currently on Linux PRIu64 and SCNu64 are the same but just to make
sure in the future use the correct macros.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Do the check after replay_fscanf to make sure everything
is fine before calling red_replay_compat_drawable or
red_replay_native_drawable.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Change the return to ssize_t to be able to distinguish from
empty buffer to error.
Check result returned and avoid continuing potentially
deferencing NULL pointers.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
To check fscanf read all needed information a dummy "%n" is appended
to any string and the value stored there is tested. This as scanf family
could return a valid value but not entirely process the string so
adding a "%n" and checking this was processed make sure all expected
string is found.
The code to check for a specific string is now a bit more complicated
as replay_fscanf use a macro which append a constant string.
The "error" field is used to mark any error happened, so in most cases
there is no explicit check beside when this could cause a problem
(for instance results of replay_fscanf used which would result in
uninitialised variable usage).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Allocations are kept into a GList to be able to free in case some
errors happened.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
FOREACH_DCC should be more DisplayChannel related.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Noting that coding by hand these loop introduced some regression
I'm trying to introduce back from macros.
Before trying something harder to make possible to bind the type of
the content I'm trying some simple macro as were before.
I added the type to avoid some blindly void* casts.
Also the GListIter is introduced to avoid the possibility to exchange
easily some parameters.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The check for free_list->wait.header.wait_count was always true.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
In anticipation of porting to GObject, use casting macros (e.g.
MAIN_CHANNEL_CLIENT()) to cast RedChannelClient types. This will help
reduce the changeset slightly porting to GObject and thus make it easier
to review those upcoming changes.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Instead of using a Ring (and having a ring item link in every pipe
item), store them in a GQueue. This also necesitated changing
RedCharDeviceVDIPort->priv->read_bufs to a GList as well.
Also Optimise client pipe by passing pipe position instead of data.
This avoids having the search the data scanning all the queue changing
the order of these operations from O(n) to O(1).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Move all of the DisplayChannel data memembers into a private struct to
encapsulate things better. This necessitated a few new 'public' methods
and a small bit of refactoring to avoid poking into DisplayChannel
internals from too many places. The DisplayChannel and the
DisplayChannelClient are still far too intertwined to completely avoid
accessing private data, so at the moment the private struct is defined
in display-channel.h and the DisplayChannelClient implementation
still accesses it sometimes.
The eof variable and enumeration will be used for all errors
so avoid confusion.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
read_binary do not allocate a buffer for no-NULL pointers.
Avoid using uninitialized data and allocate proper buffer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Explicitely discard AGENT_MSG_FILTER_MONITORS_CONFIG messages
from the agent.
Also remove unused AGENT_MSG_FILTER_END
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Make RedsState::mig_target_clients into a GList to improve encapsulation
and maintainability. Also RedsMigTargetClient::pending_links. With
GList, a type implementation can be ignorant of whether they're
contained within a list or not.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Encapsulate private data of CursorChannel in a private struct. This
isn't very useful at the moment, but it will help prepare the way for
porting the RedChannel heirarchy to GObject.
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Both src->stride and src->y are uint32_t
Fixed by making one of them uint64_t
Found by coverity
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Previously we were creating a variable named 'dev_state' and then
apparently not using it. Well, we *were* actually using it, but in a
convoluted sort of way. Creating a new RedCharDevice has a
side-effect of setting itself as the 'st' attribute of
SpiceCharDeviceInstance. So 'dev_state' and 'char_device->st' are in
fact the same variable. But they were being used interchangeably, which
was rather confusing. For example
if (dev_state)
// do something with char_device->st
So this patch doesn't actually change anything, but it makes the code a
bit easier to follow.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
main_channel_client_start_net_test() checks MainChannelClient already
which is the single reason why main_channel_client_push_ping() could
fail. Reorganizing this code a little bit.
Acked-by: Frediano Ziglio <fziglio@redhat.com
These need to be introduced at the same time since cache-item.tmpl.c
assumes that both of these classes will have a cache in the same place:
either within the channel client struct itself or (now) within a priv
struct owned by the channel client.
This encapsulates private data and prepares for porting to GObject.
Acked-by: Frediano Ziglio <fziglio@redhat.com
Agent properties like file transfer or copy & paste can be disabled by
calling spice_server_set_agent_{copypaste, file_xfer} before the spice
server is initialized. In that case the call crashes the server because
the agent device is created after the initialization.
To avoid the crash this commit introduce a helper function for setting
the agent properties after the server is initialized.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
More preparation for separating RedChannelClient into a separate file.
Rename function slightly for improved readability:
red_channel_client_blocked() -> red_channel_client_is_blocked()
Encapsulate the structure a bit more and avoid poking into it from other
class implementations. Getting ready to split RedChannelClient out to a
separate file.
Instead of having all other classes poke into the RedChannelClient
struct to get the RedClient associated with the channel client, call the
accessor function. This commit allows us to encapsulate RedChannelClient
and move it to its own file soon.
Don't poke into the structure to get the channel
This prepares for encapsulating RedChannelClient a bit more and
separating it into its own source file.
spice-server.h cannot include glib.h because it is a public header and
is used by projects that do not use GLib.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This avoid a check for NULL.
Also will be used to catch invalid values when table will be extended.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Reduce structure length using static allocated string inside the
structure.
This will also avoid using .data.rel.ro section and relocations
reducing even more library size.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
The field is only used by DisplayChannelClient, not by CursorChannelClient
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The function was just calling another function with same parameters
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This reduces the number of arguments that need to be passed to the
function, and also avoids changing a different object within a function
that is supposed to just create a new object
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Add a few more methods and accessors so that other files don't need to
manipulate the struct members directly. Move the struct definition to a
private header which only the dcc-* files will include.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This allow option string to contain separator so you could set as
"field=value ! format".
This is useful as some encoders use the output format to specify
compression types (for instance with some H264 encoders you can
specify the profile to use).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Do not handle them as normal keys.
State is not saved for these keys.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
key and key_ext in SpiceKbdState are indexed using
state[scan & 0x7f]
where scan is a 8 bit value got from client. In theory client can send
any value causing scan & 0x7f to be 0x7f. However these arrays contains
only 0x7f values so 0x7f cause a off one overflow.
This potentially cause key_ext to overflow in reds pointer following.
Happily this is not exploitable in either 32 or 64 bit environment.
On 64 bit key_ext is followed by a 4 byte (sizeof(bool) == 4) padding
which is written by the possible overflow.
On 32 bit reds will be overwritten with either 0 or 1 which will cause
a SIGSEGV leading to a DoS. Considering that you have to have access
to the machine with a client you are just shutting down only guests you
can access to.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Eventually, during a seamless migration, qemu may finish to migrate
before the spice client even finished to connect all channels to
destination and informed the server. In this case,
main_channel_client_migrate_src_complete() will fall back to
switch-host method, and reds_mig_fill_wait_disconnect() is called to
complete the migration (disconnecting all channels).
reds_mig_cleanup() is called when all channels are disconnected, but
reds->mig_wait_connect is still TRUE, and it will call
migrate_connect_complete() instead of the expected
migrate_end_complete(). Setting reds->mig_wait_connect to FALSE when
reds_mig_fill_wait_disconnect() solves the issue.
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1352836
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Without a video encoder no frame will be streamed anyway.
This fixes the case where the client does not support any of the
server's codecs, such as if it was compiled with --enable-gstvideo=no
--disable-builtin-mjpeg.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
g_object_new is a variadic function which takes property values.
As the compiler cannot check if these property values are correct,
make sure they are using casts.
This actually fixes a crash in reds.c for 32 bit architectures.
Based on a patch by Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
This happens during VM shutdown.
Qemu should never send a draw command without scanout.
This should be fixed in Qemu or in Virgl driver.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This is related to CVE-2016-0749
==529== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60040009c098 at pc 0x7fffee0eda6d bp 0x7fffffffcd00 sp 0x7fffffffccf0
WRITE of size 4 at 0x60040009c098 thread T0
#0 0x7fffee0eda6c in smartcard_char_device_notify_reader_add /home/elmarco/pkg/spice/spice-0.12.4/server/smartcard.c:334
#1 0x7fffee0ef783 in smartcard_add_reader /home/elmarco/pkg/spice/spice-0.12.4/server/smartcard.c:642
#2 0x7fffee0f0568 in smartcard_channel_handle_message /home/elmarco/pkg/spice/spice-0.12.4/server/smartcard.c:757
#3 0x7fffee032f3f in red_peer_handle_incoming /home/elmarco/pkg/spice/spice-0.12.4/server/red_channel.c:304
#4 0x7fffee033216 in red_channel_client_receive /home/elmarco/pkg/spice/spice-0.12.4/server/red_channel.c:322
#5 0x7fffee03bf1f in red_channel_client_event /home/elmarco/pkg/spice/spice-0.12.4/server/red_channel.c:1561
#6 0x555555c3c53d in qemu_iohandler_poll /home/elmarco/src/qemu/iohandler.c:143
#7 0x555555c3b800 in main_loop_wait /home/elmarco/src/qemu/main-loop.c:504
#8 0x5555556f160c in main_loop /home/elmarco/src/qemu/vl.c:1818
#9 0x5555556f160c in main /home/elmarco/src/qemu/vl.c:4394
#10 0x7fffed80eb14 in __libc_start_main /usr/src/debug/glibc-2.17-c758a686/csu/libc-start.c:274
#11 0x5555556f9c20 in _start (/home/elmarco/src/qemu/x86_64-softmmu/qemu-system-x86_64+0x1a5c20)
0x60040009c098 is located 0 bytes to the right of 8-byte region [0x60040009c090,0x60040009c098)
allocated by thread T0 here:
#0 0x7ffff4e612be in __interceptor_realloc /usr/src/debug/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/x86_64-redhat-linux/libsanitizer/asan/../../../../libsanitizer/asan/asan_malloc_linux.cc:92
#1 0x7fffee121308 in spice_realloc /home/elmarco/pkg/spice/spice-0.12.4/spice-common/common/mem.c:123
#2 0x7fffee004a48 in __spice_char_device_write_buffer_get /home/elmarco/pkg/spice/spice-0.12.4/server/char_device.c:516
#3 0x7fffee004e87 in spice_char_device_write_buffer_get /home/elmarco/pkg/spice/spice-0.12.4/server/char_device.c:557
#4 0x7fffee0ed8b9 in smartcard_char_device_notify_reader_add /home/elmarco/pkg/spice/spice-0.12.4/server/smartcard.c:325
#5 0x7fffee0ef783 in smartcard_add_reader /home/elmarco/pkg/spice/spice-0.12.4/server/smartcard.c:642
#6 0x7fffee0f0568 in smartcard_channel_handle_message /home/elmarco/pkg/spice/spice-0.12.4/server/smartcard.c:757
#7 0x7fffee032f3f in red_peer_handle_incoming /home/elmarco/pkg/spice/spice-0.12.4/server/red_channel.c:304
#8 0x7fffee033216 in red_channel_client_receive /home/elmarco/pkg/spice/spice-0.12.4/server/red_channel.c:322
#9 0x7fffee03bf1f in red_channel_client_event /home/elmarco/pkg/spice/spice-0.12.4/server/red_channel.c:1561
#10 0x555555c3c53d in qemu_iohandler_poll /home/elmarco/src/qemu/iohandler.c:143
SUMMARY: AddressSanitizer: heap-buffer-overflow /home/elmarco/pkg/spice/spice-0.12.4/server/smartcard.c:334 smartcard_char_device_notify_reader_add
Signed-off-by: Marc-Andre Lureau <marcandre.lureau@redhat.com>
There is an unref when the message is sent.
This is related to CVE-2016-0749
==17204== ERROR: AddressSanitizer: heap-use-after-free on address 0x6008000144a8 at pc 0x7fffee0ce245 bp 0x7fffffffc630 sp 0x7fffffffc620
READ of size 4 at 0x6008000144a8 thread T0
#0 0x7fffee0ce244 in smartcard_unref_vsc_msg_item /home/elmarco/src/spice/spice/server/smartcard.c:608
#1 0x7fffee0cb451 in smartcard_unref_msg_to_client /home/elmarco/src/spice/spice/server/smartcard.c:178
#2 0x7fffedfcdf14 in spice_char_device_read_from_device /home/elmarco/src/spice/spice/server/char-device.c:330
#3 0x7fffedfd1763 in spice_char_device_wakeup /home/elmarco/src/spice/spice/server/char-device.c:901
#4 0x7fffee05da98 in spice_server_char_device_wakeup /home/elmarco/src/spice/spice/server/reds.c:2990
#5 0x55555593fa34 in spice_chr_write /home/elmarco/src/qemu/spice-qemu-char.c:189
#6 0x5555559375f1 in qemu_chr_fe_write /home/elmarco/src/qemu/qemu-char.c:220
#7 0x555555b3b682 in ccid_card_vscard_send_msg.isra.2 /home/elmarco/src/qemu/hw/usb/ccid-card-passthru.c:76
#8 0x555555b3c466 in ccid_card_vscard_send_error /home/elmarco/src/qemu/hw/usb/ccid-card-passthru.c:91
#9 0x555555b3c466 in ccid_card_vscard_handle_message /home/elmarco/src/qemu/hw/usb/ccid-card-passthru.c:242
#10 0x555555b3c466 in ccid_card_vscard_read /home/elmarco/src/qemu/hw/usb/ccid-card-passthru.c:289
#11 0x55555593f169 in vmc_write /home/elmarco/src/qemu/spice-qemu-char.c:41
#12 0x7fffedfcee6d in spice_char_device_write_to_device /home/elmarco/src/spice/spice/server/char-device.c:477
#13 0x7fffedfcfd31 in spice_char_device_write_buffer_add /home/elmarco/src/spice/spice/server/char-device.c:629
#14 0x7fffee0ce9df in smartcard_channel_write_to_reader /home/elmarco/src/spice/spice/server/smartcard.c:675
#15 0x7fffee0cc7db in smartcard_char_device_notify_reader_add /home/elmarco/src/spice/spice/server/smartcard.c:341
#16 0x7fffee0ce4f3 in smartcard_add_reader /home/elmarco/src/spice/spice/server/smartcard.c:648
#17 0x7fffee0cf2e2 in smartcard_channel_handle_message /home/elmarco/src/spice/spice/server/smartcard.c:763
#18 0x7fffedffe21f in red_peer_handle_incoming /home/elmarco/src/spice/spice/server/red-channel.c:307
#19 0x7fffedffe4f6 in red_channel_client_receive /home/elmarco/src/spice/spice/server/red-channel.c:325
#20 0x7fffee00726c in red_channel_client_event /home/elmarco/src/spice/spice/server/red-channel.c:1566
#21 0x555555c3c53d in qemu_iohandler_poll /home/elmarco/src/qemu/iohandler.c:143
#22 0x555555c3b800 in main_loop_wait /home/elmarco/src/qemu/main-loop.c:504
#23 0x5555556f160c in main_loop /home/elmarco/src/qemu/vl.c:1818
#24 0x5555556f160c in main /home/elmarco/src/qemu/vl.c:4394
#25 0x7fffed7d0b14 in __libc_start_main /usr/src/debug/glibc-2.17-c758a686/csu/libc-start.c:274
#26 0x5555556f9c20 in _start (/home/elmarco/src/qemu/x86_64-softmmu/qemu-system-x86_64+0x1a5c20)
0x6008000144a8 is located 24 bytes inside of 40-byte region [0x600800014490,0x6008000144b8)
freed by thread T0 here:
#0 0x7ffff4e61009 in __interceptor_free /usr/src/debug/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/x86_64-redhat-linux/libsanitizer/asan/../../../../libsanitizer/asan/asan_malloc_linux.cc:61
#1 0x7fffee0ce2a1 in smartcard_unref_vsc_msg_item /home/elmarco/src/spice/spice/server/smartcard.c:610
#2 0x7fffee0cdd58 in smartcard_channel_release_pipe_item /home/elmarco/src/spice/spice/server/smartcard.c:548
#3 0x7fffee000668 in red_channel_client_release_item /home/elmarco/src/spice/spice/server/red-channel.c:602
#4 0x7fffee0006ef in red_channel_client_release_sent_item /home/elmarco/src/spice/spice/server/red-channel.c:609
#5 0x7fffee0007b5 in red_channel_peer_on_out_msg_done /home/elmarco/src/spice/spice/server/red-channel.c:620
#6 0x7fffedffed7e in red_peer_handle_outgoing /home/elmarco/src/spice/spice/server/red-channel.c:385
#7 0x7fffee0057bb in red_channel_client_send /home/elmarco/src/spice/spice/server/red-channel.c:1294
#8 0x7fffee0076e6 in red_channel_client_begin_send_message /home/elmarco/src/spice/spice/server/red-channel.c:1605
#9 0x7fffee0cdccd in smartcard_channel_send_item /home/elmarco/src/spice/spice/server/smartcard.c:541
#10 0x7fffee000570 in red_channel_client_send_item /home/elmarco/src/spice/spice/server/red-channel.c:588
#11 0x7fffee005bfb in red_channel_client_push /home/elmarco/src/spice/spice/server/red-channel.c:1347
#12 0x7fffee007ef7 in red_channel_client_pipe_add_push /home/elmarco/src/spice/spice/server/red-channel.c:1673
#13 0x7fffee0cde4d in smartcard_channel_client_pipe_add_push /home/elmarco/src/spice/spice/server/smartcard.c:571
#14 0x7fffee0cb567 in smartcard_send_msg_to_client /home/elmarco/src/spice/spice/server/smartcard.c:187
#15 0x7fffedfcdba2 in spice_char_device_send_msg_to_clients /home/elmarco/src/spice/spice/server/char-device.c:282
#16 0x7fffedfcdea4 in spice_char_device_read_from_device /home/elmarco/src/spice/spice/server/char-device.c:329
#17 0x7fffedfd1763 in spice_char_device_wakeup /home/elmarco/src/spice/spice/server/char-device.c:901
#18 0x7fffee05da98 in spice_server_char_device_wakeup /home/elmarco/src/spice/spice/server/reds.c:2990
#19 0x55555593fa34 in spice_chr_write /home/elmarco/src/qemu/spice-qemu-char.c:189
Signed-off-by: Marc-Andre Lureau <marcandre.lureau@redhat.com>
Users should not change the list of supported subtypes.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Was used as write variable only for testing.
Avoid usage of not constant globals.
Making globals constants avoid future race condition
usages.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Avoid multiple initializations of this library.
Also initialize using thread safe code to avoid possible race
conditions.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Current Linux pthread implementation should use futex so there should
be no leak but calling destroy avoid possible future leaks.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Encoding image requires a RedDrawable (where the data is stored) and
a Ring where to store information to free Glz structures.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Remove some coupling, we mainly need to store a list of RedGlzDrawables.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Do not access too much encoders data.
Slightly different as now if glz is frozen lz compression is used.
Glz is frozen only during migration.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
RedCompressBuf are no longer pooled.
The usage was removed in 92d9b782bd.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Let's follow the 'standard' for optional components. This commit also
drops (now) unecessary #ifdef USE_LZ4 from lz4-encode.c, as the decision
to build this file is now made in Makefile.
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
gpointer definition was not included causing the header to fails to
compile if included first.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Francois Gouget <fgouget@codeweavers.com>
Introduced by commit 903c91cd30, this
variable is used only for LZ4 code. Move the declaration to the proper
block of code.
Build log:
spicevmc.c: In function 'handle_compressed_msg':
spicevmc.c:346:14: error: variable 'decompressed' set but not used [-Werror=unused-but-set-variable]
uint8_t *decompressed;
^
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This was introduced by commit 903c91cd30.
To fix it, we simply protect the try_compress_lz4 function with proper
ifdef guards.
Build log:
spicevmc.c: In function 'try_compress_lz4':
spicevmc.c:143:5: error: implicit declaration of function 'LZ4_compress_default' [-Werror=implicit-function-declaration]
compressed_data_count = LZ4_compress_default((char*)&msg_item->buf,
^
spicevmc.c:143:5: error: nested extern declaration of 'LZ4_compress_default' [-Werror=nested-externs]
spicevmc.c: At top level:
spicevmc.c:124:24: error: 'try_compress_lz4' defined but not used [-Werror=unused-function]
static RedVmcPipeItem* try_compress_lz4(SpiceVmcState *state, int n, RedVmcPipeItem *msg_item)
^
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This buffer was just written and then used, no reason to store into
a more persistent structure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Rename this function to red_glz_drawable_free() and remove the
ImageEncoders argument since the RedGlzDrawable already holds a pointer
to the ImageEncoders structure
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Rename this function to glz_drawable_instance_item_free() and remove the
ImageEncoders argument since the RedGlzDrawable already holds a pointer
to the ImageEncoders structure.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The field was used just as a flag.
This has the advantage to make clear to not use the pointer as we don't
have ownership.
Also makes the structure a bit smaller.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
configure will use GStreamer 1.0 if present and fall back to
GStreamer 0.10 otherwise.
ffenc_mjpeg takes its bitrate as a long so extend set_gstenc_bitrate().
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
This is faster and lets the encoder leverage past bitrate shaping
history to attain the target faster.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
This typically happens when sending very small frames (less than
16 pixels in one dimension) to the x264enc encoder.
This avoids repeatedly wasting time rebuilding the pipeline.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
The video encoder uses the client reports and/or notifications of
server frame drops as its feedback mechanisms. In particular it keeps
track of the maximum video margin and reduces the bit rate whenever the
margin goes below certain thresholds or decreases too sharply.
It uses these to figure out the lowest bit rate that causes negative
feedback, and the highest bit rate that allows a return to positive
feedbacks. It then works to narrow this range and settles on the lower
end once the spread has gone below a given threshold.
All the while it monitors the effective bit rate to ensure the target
bit rate does not grow significantly beyond what the GStreamer encoder
will produce: this avoids target bit rate 'bubbles' which would
invariably be followed by a bit rate crash with accompanying frame loss.
As soon as the network feedback indicates a significant degradation the
bit rate is lowered to minimize the risk of frame loss and/or long
freezes.
It also relies on the existing shaping of the GStreamer output bit rate
to minimize the pipeline reconfigurations.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
The GStreamer codecs don't follow the specified bit rate very closely:
they can decide to exceed it for ten seconds or more if they consider
the scene deserves it. Such long bursts are enough to cause network
congestion, resulting in many lost frames which cause significant
display corruption.
So the GStreamer video encoder now uses a short 300ms virtual buffer
to shape the compressed video output and ensure we don't exceed the
target bit rate for any significant length of time.
It could instead rely on the network feedback (when available) to lower
the bit rate. However frequent GStreamer bit rate changes lower the
overall compression level and also result in a lower average bit rate,
both of which result in lower video quality.
The GStreamer video encoder also keeps track of the encoded frame size
so it can gather statistics and call update_client_playback_delay()
with accurate information and also annotate the client report debug
traces with the corresponding bit rate information.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Note that we can only avoid copies for the first 1 Mpixels or so.
That's because Spice splits larger frames into more chunks than we can
fit GstMemory fragments in a GStreamer buffer. So if there are more
pixels we will avoid copies for the first 3840 KB and copy the rest.
Furthermore, while in practice the GStreamer encoder will only modify
the RedDrawable refcount during the encode_frame(), in theory the
refcount could be decremented from the GStreamer thread after
encode_frame() returns.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
If an error occurs for whatever reason (e.g. codec not supporting odd
frame sizes), the GStreamer pipeline will drop the current buffer,
causing the encoder to be stuck waiting for the sample. So this patch
tracks error notifications and ensures we don't wait for a sample if
none will come.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
This way the video encoder is not forced to use malloc()/free().
This also allows more flexibility in how the video encoder manages the
buffer which allows for a zero-copy implementation in both video
encoders.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
The Spice server administrator can specify the encoder and codec
preferences to optimize for CPU or bandwidth usage. Preferences are
described in a semi-colon separated list of encoder:codec pairs.
The server has a default preference list which can explicitly be
selected by specifying 'auto'.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
The server picks a codec supported by the client based on the following
new client capabilities:
* SPICE_DISPLAY_CAP_MULTI_CODEC which denotes a recent client that
supports multiple codecs. This capability is needed to not have to
hardcode that MJPEG is supported. This makes it possible to write
clients that don't support MJPEG.
* SPICE_DISPLAY_CAP_CODEC_XXX, where XXX is a supported codec. Note
that for now the server only supports the MJPEG codec.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
This introduces a pared down GStreamer-based video encoder to serve as
the basis for later enhancements.
In this form the new encoder supports both regular and sized streams
but lacks any rate control. It should still work fine if bandwidth is
sufficient such as on LANs.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
encode_frame() needs the QXL_DRAW_COPY operation's SpiceCopy.src_area
field anyway, so the width and height parameters were redundant.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Video frames correspond to QXL_DRAW_COPY operations where the frame area
is defined by the SpiceCopy.src_area field.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
After the removal of Drawable::sized_stream, we no longer need to detect
if the stream changes size in is_next_stream_frame() so it can return a
boolean rather than a value from an enum.
Only red_marshall_stream_data() needs to know whether to send the frame
using a SpiceMsgDisplayStreamDataSized or a regular StreamData message.
So check whether we have a sized frame there and simplify the rest of
the code.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
This code just refactors the function without doing any functional
changes. The actual changes will be in the next commit, and this will
make the next commit much more obvious.
Usually the RedDrawable bbox dimensions match the src_area dimensions
so that checking that the bbox matches the stream's original dest_area
should be enough to determine if sized stream support is needed to
send the frame.
But making the bbox different could be used to have the scaling be
performed on the client side. So it's better not to assume the bbox and
src_area have the same dimensions.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Compressed message type is CompressedData which contains compression
type (1 byte) followed by the uncompressed data size (4 bytes - exists
only if data was compressed) followed by the compressed data
If SPICE_USBREDIR_CAP_DATA_COMPRESS_LZ4 capability is available &&
data_size > COMPRESS_THRESHOLD && !AF_LOCAL data will be sent
compressed otherwise data will be sent uncompressed (also if
compression has failed)
Update the required protocol to 0.12.12
Signed-off-by: Snir Sheriber <ssheribe@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Encoders function should not use DisplayChannelClient
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This change is less clean the other similar patches as zlib and
glz require more steps.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
dcc-encoders should be in change of allocate and free the structure
don't put internal assumptions (which functions are used for memory
management) in different files.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Use red_time_t to make sure timestamp has the right precision even
on 32 bit systems.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Primary surface, as additional surfaces, can be used to access
host memory from the guest using invalid parameters.
The removed warning is not enough to prevent all cases. Also a warning
is not enough to stop an escalation to happen.
The red_validate_surface do different checks to make sure surface
request is valid and not cause possible buffer/integer overflows:
- format is valid;
- width is not large to cause overflow compared to stride;
- stride is not -2^31 (a number which negate is still <0);
- stride * height does not overflow.
This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1312980.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Make possible to reuse it outside red-parse-qxl.c.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
There is no reason to initialize these fields in two different places.
Note that an usage between dcc_new and the function where these
field were initialised would have resulted in a crash.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This function is never used outside this compile unit.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
As the tokens counter were not being reset you could enter in a
situation where client thinks it has more tokens then server which
would eventually lead to client's disconnection from 0c5eca97f1
onwards (before it was crashing).
It is easy to check the above situation if you track the amount of
tokens you have in the client and simply kill and restart the agent
while doing some file transfer: the client could reach more then 13
tokens which should not really be possible.
Based on patch from Frediano Ziglio <fziglio@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Use red_record_cursor_cmd to be able to record cursor commands.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The segment_size computation on 32 bit can lead to big numbers which
can lead to negative offset. Let'ss avoid to underrun the buffer,
as it's not checked.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Fabiano Fidêncio <fidencio@redhat.com>
Use 64 bit arithmetic to avoid overflows.
The multiplication between count and a constant can overflow.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Fabiano Fidêncio <fidencio@redhat.com>
The source area should not extend outside the source bitmap, or have
swapped coordinates.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This variable was always the same value as
dispatcher_allows_client_mouse.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Possibly used for debugging or an initial recursive lock.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This fixes a regression introduced by
4028fb1c79, where clients_num
was not updated correctly anymore.
There is no reason to cache the number of elements on that list,
as the expected number of clients is quite small.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
SpiceCharDeviceInstance is not a GObject, the GObject is char_device->st
as the above line is stating.
The crash never happen as Qemu never calls spice_server_remove_interface
for smartcards.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Setting "sin" property is equivalent to call
red_char_device_reset_dev_instance so there is no need for a if/else
as the code is doing mostly (beside setting agent_attached) the
same thing
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
RedCharDevice used for the agent has flow control enabled.
This make possible for red_char_device_write_buffer_get to return NULL.
Handle such situation without crashing avoiding NULL dereference.
This fixes https://bugs.freedesktop.org/show_bug.cgi?id=95416.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Upcast conversions are by definition unsafe. There are however some
location where such conversion are more safe. In this case send_item
callback is registered specifically for this type of RedChannel making
the conversion more reliable.
The other conversion (CursorChannel -> RedChannel) became safe.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Avoid converting from RedChannelClient* to DisplayChannelClient* and
DisplayChannelClient* to RedChannelClient* just to use RedChannelClient*
('display_channel->clients' is a list of RedChannelClients).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Avoid converting RedChannelClient* to CursorChannelClient* and then
CursorChannelClient* to RedChannelClient* just to use RedChannelClient*.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Rename red_record_dev_input_primary_surface_create to
red_record_primary_surface_create.
Name is more concise and was not clear what "dev_input" was supposed
to specify.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This allows compression using external programs or any type
of filters.
To use it set SPICE_WORKER_RECORD_FILTER environment to the
filter command you want to use. The command is executed with
g_spawn_async_with_pipes (which uses execve) so is not a shell
command although the command is parsed using g_shell_parse_argv
which split arguments as shell does.
One easy way to use it is to just use a compressor like gzip with
export SPICE_WORKER_RECORD_FILENAME=/tmp/qemu_record.gz
export SPICE_WORKER_RECORD_FILTER=gzip
qemu ...
The filter will receive the recording on standard input and is
supposed to write in output filename (which is the standard output).
You can use additional arguments in SPICE_WORKER_RECORD_FILTER to
specify for instance compression level.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Remove global/static from red_record_qxl.c.
Defined a structure and use it to hold record state.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The existence of this class can be hidden to user of CursorChannel class
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This function was handling mainly CursorChannel state
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Actually not exactly the same as this fix a bug if base is not the
first element (in this case if dpi_ring_item is NULL dpi is not)
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
The id type is already uint64_t, no need to cast using some low
level casts.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This reduce a bit the structure size and make clear that
the field is used only when the item is a cache item.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
since display_channel_print_stats is now static, the compiler complains
when RED_WORKER_STAT is undefined since this function is not used:
../../server/display-channel.c:853:13: error: 'display_channel_print_stats' defined but not used [-Werror=unused-function]
To fix, don't define the function when RED_WORKER_STAT is undefined.
Acked-by: Victor Toso <victortoso@redhat.com>
Instead of using a Ring, use a GList to store the list of channel
clients. This allows us to iterate the clients without poking inside of
the client struct to get the channel_link. This is required in order to
make the RedChannelClient struct private.
This was proposed by Christophe as improvement over some typesafe
patches.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Scan remaining code searching for problems with structure
layout assumptions in the code.
Where code required some restructuring put some verify checks
to make sure code won't compile if these assumptions are not
in place anymore.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Make code more type safe. This allow to move or delete structure
fields more safely
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
All other classes using RedPipeItem as base use base as parent name
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This avoids having to retain a pointer just to check item is still in
the queue with ring_item_is_linked(&item->link).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Remove the need to release the item inside send_item callbacks.
This looks like a partial rollback of previous patch but is
to make clear the intention of the change.
The lifetime of items could extend a bit further but there
are no cases this small lag should cause problems.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This is quite confusing and prone to errors.
Use RedPipeItem reference counting instead.
To compensate for the additional reference due to red_pipe_item_ref
in RedChannel sub class with empty hold_item have to add a
red_pipe_item_unref call in send_item.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
There is some inconsistent naming of RedChannelClient methods. Some
include the word 'peer' (which normally seems to refer to RedsStream).
Remove the 'peer' terminology from those functions that are basically
RedChannelClient methods.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Following commit e8017dd366 here
are some more ifdef simplification.
For PIXEL_ID and PIXEL_DIST macros,
the ifndef LZ_PLT defines are special cases of the LZ_PLT
(#else) defines, when pix_per_byte is 1
Now do_match can accept pix_per_byte param for all different
definitions (implementations).
Signed-off-by: Uri Lublin <uril@redhat.com>
Acked-by: Fabiano Fidêncio <fidencio@redhat.com>
This is a public RedClient API that handles updating itself rather than
having the RedChannelClient poke around at the internal structure in
rec_channel_client_seamless_migration_done().
spice-server-replay is built with the same settings as tests.
Currently the tests links to a convenience spice-server library.
A convenience library is a static library build with object file
for a dynamic library.
This make all tests contain all spice-server library code and not
linking to the spice-server dynamic library (the .so).
It's not a problem for most tests however this make impossible for
spice-server-replay utility to be used with a dynamic spice-server
library (you have to recompile it including the library).
This is a pity as you could for instance compare your system
library behavior with the library you are developing.
Also if we decide to install in a future the utility for testing
we would have lot of code duplication.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
With my compile switches (which are mostly derived from RedHat spec
file) I'm getting this warning:
stream-test.c: In function 'sock_fd_read':
stream-test.c:66:43: warning: dereferencing type-punned pointer will
break strict-aliasing rules [-Wstrict-aliasing]
*fd = *((int *) CMSG_DATA(cmsg));
^
memcpy could decrease performance but as this is a test it's not
an issue.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Make sure is possible to include any headers alone.
I used this script to check independence (run under server directory):
----
#!/bin/bash
set -e
# check not already modified
if grep -q libheaders.la Makefile.am; then
echo "Header library already prepared" >&2
exit 1
fi
add_lib() {
local hdr="$1"
hdr=${hdr%.h}
c="mao_${hdr}.c"
echo "#include \"${hdr}.h\"" > $c
echo -e "\t$c \\" >&3
}
# add library to compile all headers alones
exec 3>> Makefile.am
echo "
noinst_LTLIBRARIES += libheaders.la
libheaders_la_SOURCES = \\" >&3
for hdr in *.h; do
case $hdr in
spice-bitmap-utils.h)
add_lib $hdr
;;
spice*.h)
;;
*)
add_lib $hdr
;;
esac
done
echo -e "\t\$(NULL)" >&3
exec 3>&-
----
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Avoid having to provide a lot of empty implementations
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
All RedPipeItems can be freed with red_pipe_item_unref.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
The items of these pipe items were allocated staticaly inside the
StreamAgent structure. All others RedPipeItem are allocated dynamically.
This could solve possible future maintenance as the life of these
item is more easier to understand.
It's more easier to understand why reference where incremented.
It also make the StreamAgent structure a bit smaller.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Putting base as first item the default free function is fine.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Move ring_remove to the function to release this type of item.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Make macro have same prototype even if we need an extra argument.
This avoids having different calls for LZ_PLT.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Change from %f to %g. This allows printf to choose format based on
order of numbers.
Also increase precision making sure that number of precision is
respected, not matter the order of the number.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
Use some utility function to show and compute statistics.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This regression was introduced by commit
18d3876b78
("UpgradeItem: use base PipeItem for refcounting")
This fix https://bugs.freedesktop.org/show_bug.cgi?id=95365.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This doesn't cause problems at the moment, because the files that
include this file also include the other files that are needed, but just
for completeness...
Acked-by: Pavel Grunt <pgrunt@redhat.com>
This function was wrongly marked as deprecated in commit b41220b144.
Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Removing an interface cause SpiceBaseInstance->st to be set to NULL.
This pointer was then deferenced in agent code.
As SpiceBaseInstance should not be used after this call make sure
we don't keep pointers to it.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This code make easier to be sure we don't have dangling pointers
resetting in the function which free the structure.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
The include directory is specified with the -I which is the directory
used directly by #include<>.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
When a client disconnects remove it from the list of clients connected
to the spice char-device.
This was caused by commit 1cec1c5118
("reds: Make VDIPortState a GObject") as the lifespan of RedCharDevice
was changed.
This could be reproduced with:
- start rhel7 machine
- connect remote viewer (RV)
- RV: login
- connect ssh
- SSH: stop agent
- disconnect RV
- SSH: start agent
- connect to RV
and caused (using address sanitizer):
main_channel_handle_parsed: agent start
=================================================================
==29592==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c00001cff0 at pc 0x7fa85b6e8595 bp 0x7ffde3801940 sp 0x7ffde3801930
READ of size 8 at 0x60c00001cff0 thread T0
#0 0x7fa85b6e8594 in red_client_get_main /home/freddy/work/spice-server/server/red-channel.c:2190
#1 0x7fa85b7311e6 in vdi_port_send_msg_to_client /home/freddy/work/spice-server/server/reds.c:880
#2 0x7fa85b69383e in red_char_device_send_msg_to_client /home/freddy/work/spice-server/server/char-device.c:138
#3 0x7fa85b69383e in red_char_device_send_msg_to_clients /home/freddy/work/spice-server/server/char-device.c:356
#4 0x7fa85b69383e in red_char_device_read_from_device /home/freddy/work/spice-server/server/char-device.c:403
#5 0x55a2633b81c1 (/usr/bin/qemu-system-x86_64+0x5561c1)
#6 0x55a2633afe7a (/usr/bin/qemu-system-x86_64+0x54de7a)
#7 0x55a2634cb7b1 (/usr/bin/qemu-system-x86_64+0x6697b1)
#8 0x55a2632078d0 (/usr/bin/qemu-system-x86_64+0x3a58d0)
#9 0x55a26379b2e8 (/usr/bin/qemu-system-x86_64+0x9392e8)
#10 0x55a26379a7a0 (/usr/bin/qemu-system-x86_64+0x9387a0)
#11 0x55a26313fb78 in main (/usr/bin/qemu-system-x86_64+0x2ddb78)
#12 0x7fa85a3cc57f in __libc_start_main (/lib64/libc.so.6+0x2057f)
#13 0x55a26314b0c8 (/usr/bin/qemu-system-x86_64+0x2e90c8)
0x60c00001cff0 is located 48 bytes inside of 128-byte region [0x60c00001cfc0,0x60c00001d040)
freed by thread T0 here:
#0 0x7fa869e3667a in __interceptor_free (/lib64/libasan.so.2+0x9867a)
#1 0x7fa85b6d75f7 in red_client_unref /home/freddy/work/spice-server/server/red-channel.c:2076
#2 0x7fa85b6ead74 in dispatcher_handle_single_read /home/freddy/work/spice-server/server/dispatcher.c:291
#3 0x7fa85b6ead74 in dispatcher_handle_recv_read /home/freddy/work/spice-server/server/dispatcher.c:314
#4 0x55a26379b2e8 (/usr/bin/qemu-system-x86_64+0x9392e8)
#5 0x55a26379a7a0 (/usr/bin/qemu-system-x86_64+0x9387a0)
#6 0x55a26313fb78 in main (/usr/bin/qemu-system-x86_64+0x2ddb78)
#7 0x7fa85a3cc57f in __libc_start_main (/lib64/libc.so.6+0x2057f)
previously allocated by thread T0 here:
#0 0x7fa869e36b19 in __interceptor_calloc (/lib64/libasan.so.2+0x98b19)
#1 0x7fa85b7d6858 in spice_malloc0 /home/freddy/work/spice-server/spice-common/common/mem.c:109
#2 0x7fa85b6e760c in red_client_new /home/freddy/work/spice-server/server/red-channel.c:2053
#3 0x7fa85b7449e4 in reds_handle_main_link /home/freddy/work/spice-server/server/reds.c:1762
#4 0x7fa85b7449e4 in reds_handle_link /home/freddy/work/spice-server/server/reds.c:2002
#5 0x7fa85b745d3a in reds_handle_ticket /home/freddy/work/spice-server/server/reds.c:2056
#6 0x55a26379b2e8 (/usr/bin/qemu-system-x86_64+0x9392e8)
#7 0x55a26379a7a0 (/usr/bin/qemu-system-x86_64+0x9387a0)
#8 0x55a26313fb78 in main (/usr/bin/qemu-system-x86_64+0x2ddb78)
#9 0x7fa85a3cc57f in __libc_start_main (/lib64/libc.so.6+0x2057f)
SUMMARY: AddressSanitizer: heap-use-after-free /home/freddy/work/spice-server/server/red-channel.c:2190 red_client_get_main
Shadow bytes around the buggy address:
0x0c187fffb9a0: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
0x0c187fffb9b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c187fffb9c0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x0c187fffb9d0: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
0x0c187fffb9e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c187fffb9f0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd[fd]fd
0x0c187fffba00: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
0x0c187fffba10: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c187fffba20: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x0c187fffba30: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
0x0c187fffba40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
This replaces the original mjpeg_encoder API with a VideoEncoder base
class which can be reimplemented by other encoders.
This also renames the members and enums from mjpeg_* to video_*.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
We may get stream reports after the stream has been closed, and even, in
extreme cases, after a new stream has started reusing the same stream
id. Neither case should cause the server to crash.
So restore dcc_handle_stream_report() to how it was before being moved
to dcc.c.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
Acked-by: Victor Toso <victortoso@redhat.com>
