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>
