rhbz#1004443
The methods that trigger waitings on the client pipe require that
the waiting will succeed in order to continue, or otherwise, that
all the living pipe items will be released (e.g., when
we must destroy a surface, we need that all its related pipe items will
be released). Shutdown of the socket will eventually trigger
red_channel_client_disconnect (*), which will empty the pipe. However,
if the blocking method failed, we need to empty the pipe synchronously.
It is not safe(**) to call red_channel_client_disconnect from ChannelCbs
, but all the blocking calls in red_worker are done from callbacks that
are triggered from the device.
To summarize, calling red_channel_client_disconnect instead of calling
red_channel_client_shutdown will immediately release all the pipe items that are
held by the channel client (by calling red_channel_client_pipe_clear).
If red_clear_surface_drawables_from_pipe timeouts,
red_channel_client_disconnect will make sure that the surface we wish to
release is not referenced by any pipe-item.
(*) After a shutdown of a socket, we expect that later, when
red_peer_handle_incoming is called, it will encounter a socket
error and will call the channel's on_error callback which calls
red_channel_client_disconnect.
(**) I believe it was not safe before commit 2d2121a170 (before adding ref
count to ChannelClient). However, I think it might still be unsafe, because
red_channel_client_disconnect sets rcc->stream to NULL, and rcc->stream
may be referred later inside a red_channel_client method unsafely. So instead
of checking if (stream != NULL) after calling callbacks, we try to avoid
calling red_channel_client_disconnect from callbacks.
(1) merge 'force' and 'wait_for_outgoing_item' to one parameter.
'wait_for_outgoing_item' is a derivative of 'force'.
(2) move the call to red_wait_outgoing_item to red_clear_surface_drawables_from_pipe
client/Makefile.am:199: warning: 'INCLUDES' is the old name for 'AM_CPPFLAGS' (or '*_CPPFLAGS')
server/tests/Makefile.am:3: warning: 'INCLUDES' is the old name for 'AM_CPPFLAGS' (or '*_CPPFLAGS')
After eb09c25c, red_parse_qxl.c still has some spice_error() which
will kill the server even though the code is trying to return an error
when the spice_error() is hit.
This commit replaces these occurrences with a spice_warning() which
will not kill spice-server.
bitmap_consistent should return true or false.
Currently it aborts instead of returning false, due to spice_error.
Replacing spice_error with spice_warning, provides information and returns
false, as expected.
This fixes Fedora bz#997932
rhbz#994175
When a client connection is closed surprisingly (i.e., without a FIN
segment), we cannot identify it by a socket error (which is the only
way by which we identified disconnections so far).
This patch allows a channel client to periodically check the state of
the connection and identify surprise disconnections.
For channels that don't run as part of the main loop, we use
spice_timer_queue, while for the other channels we use
qemu timers support. The callbacks for setting timers are supplied to
red_channel via SpiceCoreInterface, and their behavior should be
consistent. qemu timers are called only once per each call to
timer_start. This patch assigns the same behaviour to spice_timer_queue.
Three blocking functions, one was split to leave the display channel
specific referencing of the DrawablePipeItem being sent inside
red_worker, but the rest (most) of the timeout logic was moved to
red_channel, including the associated constants.
Moved functions:
red_channel_client_wait_pipe_item_sent
red_wait_outgoing_item
red_wait_all_sent
Introduces red_time.h & red_time.c for a small helper function dealing
with time.h
setting DRAW_ALL define doesn't produce correct rendering. Using
update_area instead of red_draw_qxl_drawable will work but it shouldn't
be required. This is not work I intend to do right now, so marking it
for anyone looking at this in the future.
150 seconds is way too long period for holding the guest driver and
waiting for a response for the client. This timeout was 15 seconds, but
when off-screen surfaces ware introduced it was arbitrarily multiplied by
10.
Other existing related bugs emphasize why it is important to decrease
the timeout:
(1) 994211 - the qxl driver waits for an async-io reponse for 60 seconds
and after that, it switches to sync-io mode. Not only that the
driver might use invalid data (since it didn't wait for the query to
complete), falling back to sync-io mode introduces other errors.
(2) 994175 - spice server sometimes doesn't recognize that the client
has disconnected.
(3) There might be cache inconsistency between the client and the server,
and then the display channel waits indefinitely for a cache item (e.g., bug
977998)
This patch changes the timeout to 30 seconds. I tested it under wifi +emulating 2.5Mbps network,
together with playing video on the guest and changing resolutions in a loop. The timeout didn't expired
during my tests.
This bug is related to rhbz#964136 (but from rhbz#964136 info it is still not
clear why the client wasn't responsive).
The snd channels has one reference as long as their socket is active.
The playback channel has an additional reference for each frame that is
currently filled by the sound device.
Once the channel is disconnected (the socket has been freed and the
first reference is released) snd_disconnect_channel shouldn't release
a reference again.
When the sequence of calls bellow occurs, the PlaybackChannel
is not released (snd_channel_put is not called for the
samples that refer to the channel).
spice_server_playback_get_buffer
snd_channel_disconnect
spice_server_playback_put_samples
When we want to disconnect the main channel from a callback, it is
safer to use red_channel_client_shutdown, instead of directly
destroying the client. It is also more consistent with how other
channels treat errors.
red_channel_client_shutdown will trigger socket error in the main channel.
Then, main_channel_client_on_disconnect will be called,
and eventually, main_dispatcher_client_disconnect.
I didn't replace calls to reds_disconnect/reds_client_disconnect in
places where those calls were safe && that might need immediate client
disconnection.
Fixes rhbz#918169
Some channels make direct calls to reds/main_channel routines. If
these routines try to read/write to the socket, and they get socket
error, main_channel_client_on_disconnect is called, and triggers
red_client_destroy. In order to prevent accessing expired references
to RedClient, RedChannelClient, or other objects (inside the original call, after
red_client_destroy has been called) I made the call to
red_client_destroy asynchronous with respect to main_channel_client_on_disconnect.
I added MAIN_DISPATCHER_CLIENT_DISCONNECT to main_dispatcher.
main_channel_client_on_disconnect pushes this msg to the dispatcher,
instead of calling directly to reds_client_disconnect.
The patch uses RedClient ref-count in order to handle a case where
reds_client_disconnect is called directly (e.g., when a new client connects while
another one is connected), while there is already CLIENT_DISCONNECT msg
pending in the main_dispatcher.
Examples:
(1) snd_worker.c
snd_disconnect_channel()
channel->cleanup() //snd_playback_cleanup
reds_enable_mm_timer()
.
.
main_channel_push_multi_media_time()...socket_error
.
.
red_client_destory()
.
.
snd_disconnect_channel()
channel->cleanup()
celt051_encoder_destroy()
celt051_encoder_destory() // double release
Note that this bug could have been solved by changing the order of
calls: e.g., channel->stream = NULL before calling cleanup, and
some other changes + reference counting. However, I found other
places in the code with similar problems, and I looked for a general
solution, at least till we redesign red_channel to handle reference
counting more consistently.
(2) inputs_channel.c
inputs_connect()
main_channel_client_push_notify()...socket_error
.
.
red_client_destory()
.
.
red_channel_client_create() // refers to client which is already destroyed
(3) reds.c
reds_handle_main_link()
main_channel_push_init() ...socket error
.
.
red_client_destory()
.
.
main_channel_client_start_net_test(mcc) // refers to mcc which is already destroyed
This can explain the assert in rhbz#964136, comment #1 (but not the hang that occurred before).
Fixes: leaks of pipe items & "red_client_destroy: assertion `rcc->send_data.size == 0'"
red_channel_disconnect clears the pipe. It is called only once. After,
it was called, not items should be added to the pipe.
An example of when this assert can occur:
on_new_cursor_channel (red_worker.c), pushes 2 pipe items.
When it pushes the first pipe item, if the client has disconnected,
it can hit a socket error, and then, red_channel_client_disconnect is called.
The second call to adding a pipe item, will add the item to
the pipe. red_channel_client_pipe_add_type also calls
red_channel_client_push, which will update the send_data.size.
Then, the push will also hit a socket error, but red_channel_client_disconnect
won't clear the pending pipe item again, since it was already called.
When red_client_destory is called, we hit assertion `rcc->send_data.size
== 0'.
Note that if a pipe item is added to the pipe after
red_channel_client_disconnect was called, but without pushing it,
we should hit "spice_assert(rcc->pipe_size == 0)".
The ioctl on sockets is actually named SIOCOUTQ though its value
is identical to TIOCOUTQ which is for terminals.
SIOCOUTQ is linux specific so we add a header check and ifdef based
on the presence of the header
This prevents bogus ioctls on non-Linux platforms
In this case, make syntax-check is wrong, and we actually do
need the cast.
A cast is needed when types are uint64_t <--> pointer
Using a local "ptr" variable makes both gcc and syntax-check happy.
Earlier in this function, test->target_surface is set to 1, which
is the only allowed non-primary surface currently.
If surface parameters are given (and specifically data is checked)
they are being used, otherwise a default surface is used.
Earlier in this function, "command" is set to a non-NULL value.
Thus, the else part was unreachable code, which is fixed now.
When surface_id == 0, primary is used.
Otherwise (currently 1), secondary is used.
Also, remove unused test_width and test_height.
Since commit caea769943,
test->width and test->height are used.
This was originally intended to fix the problem fixed by
commit 53488f0275.
What is left are FOREACH loops that are at less risk and maybe safe (no
read/write or disconnect/destroy are called from within them).
Specifically, the loop in red_pipes_add_draw can cause spice to abort.
In red_worker.c (WORKER_FOREACH_DCC):
red_pipes_add_drawable
red_pipe_add_drawable
red_handle_drawable_surfaces_client_synced
red_push_surface_image
red_channel_client_push
red_channel_client_send
red_peer_handle_outgoing
reds_stream_writev (if fails -- EPIPE)
handler->cb->on_error = red_channel_client_disconnect()
red_channel_remove_client()
ring_remove() -- of rcc from channel.clients ring.
RCC_FOREACH may be dangerous
The following patches replace FOREACH loops with a SAFE version.
Using unsafe loops may cause spice-server to abort (assert fails).
Specifically a read/write fail in those loops, may cause the client
to disconnect, removing the node currently iterated, which cause spice
to abort in ring_next():
-- assertion `pos->next != NULL && pos->prev != NULL' failed
Currently, both red_channel_pipes_add_type() and
red_channel_pipes_add_empty_msg() use plaing RING_FOREACH() which is not
safe versus removals from the ring within the loop body.
Although it's rare, such a removal can occur in both cases. In the case
of red_channel_pipes_add_type() we have:
red_channel_pipes_add_type()
-> red_channel_client_pipe_add_type()
-> red_channel_client_push()
And in the case of red_channel_client_pipes_add_empty_msg() we have:
red_channel_client_pipes_add_empty_msg()
-> red_channel_client_pipe_add_empty_msg()
-> red_channel_client_push()
But red_channel_client_push() can cause a removal from the clients ring if
a network error occurs:
red_channel_client_push()
-> red_channel_client_send()
-> red_peer_handle_outgoing()
-> handler->cb->on_error callback
= red_channel_client_default_peer_on_error()
-> red_channel_client_disconnect()
-> red_channel_remove_client()
-> ring_remove()
When this error path does occur, the assertion in RING_FOREACH()'s
ring_next() trips, and the process containing the spice server is aborted.
i.e. your whole VM dies, as a result of an unfortunately timed network
error on the spice channel.
Please apply.
Signed-off-by: David Gibson <dgibson@redhat.com>
The image descriptor flags shouldn't be copied as is from the flags that
were set by the driver. Specifically, the CACHE_ME flag shouldn't be copied,
since it is possible that (a) the image won't be cached (b) the image
is already cached, but in its lossy version, and we may want to set the bit for
CACHE_REPLACE_ME, in order to cache it in its lossless version.
In case (b), the client first looks for the CACHE_ME flag, and only if
it is not set it looks for CACHE_REPLACE_ME (see canvas_base.c). Since both flags where set,
the client ignored REPLACE_ME, and didn't turned off the lossy flag of the
cach item. Then, when a request from this lossles item reached the
client (FROM_CACHE_LOSSLESS), the client display channel waited
endlessly for the lossless version of the image.
The assert:
spice_assert(pthread_equal(pthread_self(), client->thread_id))
and the assert:
spice_assert(pthread_equal(pthread_self(), rcc->channel->thread_id))
were coded in order to protect data that is accessed from the main
context (red_client and most of the channels), from
access by threads of other channels (namely, the display and cursor
channels), and vice versa.
However, some of the calls to the sound channel interface,
and also the char_device interface, can be done from the vcpu thread.
It doesn't endanger these channels internal data, since qemu use global
mutex for the vcpu and io threads.
Thus, pthread_self() can be != channel->thread_id, if one of them is
the vcpu thread and the other is the io-thread, and we shouldn't assert.
Future plans: A more complete and complicated solution would be to manage our own thread for
spice-channels, and push input from qemu to this thread, instead of
counting on the global mutex of qemu
rhbz#823472
If client_migrate_info was called once with cert-host-subject and
then again without cert-host-subject, on a third call to
client_migrate info, the cert-host-subject from the first call would
have been freed for the second time.
It's not always obvious what address spice-server will bind to,
in particular when the 'addr' parameter is omitted on QEMU
commandline. The decision of what address to bind to is made
in reds_init_socket with a call to getaddrinfo. Surprisingly,
that function had a call to getnameinfo() already, but it does
not seem to be using the result of that call in any way.
This commit moves this call after the socket is successfully bound
and add a log message to indicate which address it's bound to.
When setting an initial video stream bit rate, if the bit rate
wasn't calculated by main_channel_client, and we don't have
estimation from previos streams, use some default values.
The patch also removes updating dcc->streams_max_bit_rate when
the bit_rate held by the main_channel is larger than it. It is not necessary
since we compare those 2 values each time we set the initial bit rate
for a stream.
spice_channel_client_error prints warning and shutdowns the
channel_client that hit the error.
This macro is useful for errors that are specific for one session
and that are unrecoverable only with respect to this session.
Prefer disconnecting a client over aborting when possible.
rhbz#956345
After a spice session has been migrated, we don't retest the network
(user experience considerations). Instead, we obtain the is_low_bandwidth flag
from the src-server, via the migration data.
Before this patch, if we migrated from server s1 to s2 and then to s3,
and if the connection to s1 was a low bandwidth one, we erroneously
passed is_low_bandwidth=FALSE from s2 to s3.
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Replace the mixed calls to display_channel_client_is_low_bandwidth
and to main_channel_client_is_low_bandwidth, with one flag in
CommonChannelClient that is set upon channel creation.
red_create_stream is called even without any client but there is no
encoding since the mjpeg encoder is now associated with StreamAgent
which is only created when we have a client.
The connection to the target server is established before migration
starts. However, the client reads and replies to messages from the server only after
migration completes. Thus, we better not send ping msgs from the target
before migration completes (because the observed roundtrip duration will
be bigger than the real one).
This bug results in the client dropping all the video frames after
migration in case that (1) the hosts involved in migration have different
mm-time; and that (2) there is no audio playback.
This is relvant only for the client that was connected during the
migration.
rhbz#958276
When a client disconnects, red_channel_client_pipe_clear is called.
Releasing pipe items of type == MIGRATE||EMPTY_MSG||PING
wasn't handled, and was passed to channel_cbs.release_item.
There, an error occured since the pipe items were not recognized.
With a SPICE_DISPLAY_CAP_MONITORS_CONFIG capable client, the client needs to
know what part of the primary to use for each monitor. If the guest driver
does not support this, the server sends messages to the client for a
single monitor spanning the entire primary.
As soon as the guest calls spice_qxl_monitors_config_async once, we set
the red_worker driver_has_monitors_config flag and stop doing this.
This is a problem when the driver gets unloaded, for example after a reboot
or when switching to a text vc with usermode mode-setting under Linux.
To reproduce this start a multi-mon capable Linux guest which uses
usermode mode-setting and then once X has started switch to a text vc. Note
how the client window does not only not resize, if you try to resize it
manually you always keep blackborders since the aspect is wrong.
This patch is the spice-server side of fixing this, it adds a new
spice_qxl_driver_unload method which clears the driver_has_monitors_config
flag.
The other patch needed to fix this is in qemu, and will calls this new method
from qxl_enter_vga_mode.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
mjpeg_encoder modify the initial bit we supply it, according to the
client feedback. If it reaches a bit rate which is higher than the
initial one, we use the higher bit rate as the new bit rate estimation.
When there is no audio playback, we set the mm_time in the client to be older
than the one in the server by at least the requested latency (the delta is
actually bigger, due to the network latency).
When there is an audio playback, we adjust the mm_time in the client by
adjusting the playback buffer using SPICE_MSG_PLAYBACK_LATENCY.
A frame can be dropped if a new frame was added during the same
call to red_process_command (we didn't attempt to send the older
frame). Such drops are ignored.
This patch only employs setting the stream parameters based on
the initial given bit-rate, the latency, and the encoding size.
Later patches will also employ mjpeg_encoder response to client reports,
and its control over frame drops.
The patch also removes old stream bit rate calculations that weren't
used.
The stream starts after lossless frames were sent to the client,
and without rate control (except for pipe congestion). Thus, on the beginning
of the stream, we might observe frame drops on the client and server side which
are not necessarily related to mis-estimation of the bit rate, and we would
like to wait till the stream stabilizes.
The actual frames distribution does not necessarily fit the
condition "at least one frame every (1000/rate_contorl->fps)
milliseconds".
For keeping the average frame rate close to the defined fps, we
periodically measure the current average fps, and modify
rate_control->adjusted_fps accordingly. Then, we use
(1000/rate_control->adjusted_fps) as the interval between the
frames.
The required client playback latency is assessed based on the current
estimation of the bit rate, the network latency, and the encoding size
of the frames. When the playback delay that is reported by the client
seems too small, or when the stream parameters change, we send the
client an updated playback latency estimation.
mjpeg_encoder can receive periodic reports about the playback status on
the client side. Then, mjpeg_encoder analyses the report and can
increase or decrease the stream bit rate, depending on the report.
When the bit rate is changed, the quality and frame rate of the stream
are re-evaluated.
Previously, the mjpeg quality was always 70. The frame rate was
tuned according to the frames' congestion in the pipe.
This patch sets the quality and frame rate according to
a given bit rate and the size of the first encoded frames.
The following patches will introduce an adaptive video streaming, in which
the bit rate, the quality, and the frame rate, change in response to
different parameters.
Patches that make red_worker adopt this feature will also follow.
The mjpeg_encoder should be client specific, and not shared between
different clients**, for the following reasons:
(1) Since we use abbreviated jpeg datastream for mjpeg, employing the same
mjpeg_encoder for different clients might cause errors when the
clients decode the jpeg data.
(2) The next patch introduces bit rate control to the mjpeg_encoder.
This feature depends on the bandwidth available, which is client
specific.
** at least till we change multi-clients not to re-encode the same
streams.
My commit 71315b2e "snd_worker: Don't send empty audio-volume messages",
fixes only one case of sending an empty volume message, if the client connects
to a vm early during its boot sequence, while the snd hardware is being reset
by the guest driver, qemu will call spice_server_playback_set_volume() with
0 channels from the reset handler.
This patch also applies both fixes to the record channel.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
When qemu migration completes, we need to stop the streams, and to send
the corresponding upgrade_items to the client.
Otherwise, (1) the client might display lossy regions that we don't track
(streams are not part of the migration data).
(2) streams_timeout may occur after MSG_MIGRATE has been sent, leading
to messages being sent to the client after MSG_MIGRATE and before
MSG_MIGRATE_DATA (e.g., STREAM_CLIP, STREAM_DESTROY, DRAW_COPY).
No message besides MSG_MIGRATE_DATA should be sent after
MSG_MIGRATE.
When a msg other than MIGRATE_DATA reached spice-gtk after MSG_MIGRATE,
spice-gtk sent it to dest server as the migration data, and the dest
server crashed with a "bad message size" assert.
If no volume has been set it, we end up sending a volume message with
audio-volume for 0 channels (iow an empty message). This is not useful
and triggers the following warning in spice-gtk:
(remote-viewer:8726): GSpice-WARNING **: set_sink_input_volume() failed:
Invalid argument
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
2 closely related changes in one:
1) When leaving the read or write loop because the chardev has been stopped
active should not be updated. It has been set to FALSE by
spice_char_device_stop and should stay FALSE
2) The updating of dev->active should be done *before* unref-ing dev
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The write-retry timer should not be set when we're leaving
spice_char_device_write_to_device because the char-dev has been stopped.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Before this patch the write-loop in spice_char_device_write_to_device would
break on running becoming 0, after having written some data, without updating
the buffer status, causing the same data to be written *again* when started.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
This fixes spice-gtk printing message like these on migration:
(remote-viewer:18402): GSpice-CRITICAL **: spice_channel_iterate_read: assertion `c->state != SPICE_CHANNEL_STATE_MIGRATING' failed
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
This is clearly something which should be handled in the inputs_channel code,
rather then having a special case for it in the generic channel handling
code in reds.c. Moving it here also fixes the TODO we had on only sending
this message to new clients.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Currently main_channel_push_notify only gets passed a static string, but
chances are in the future it may get passed dynamically allocated strings,
prepare it for this.
While at it also make clear that its argument is a string, and simplify
things a bit by making use of this knowledge (pushing the strlen call down).
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Client -> agent messages can spawn multiple VDIChunks. When this happens
the agent re-assembles the chunks into a complete VDAgentMessage before
processing it. The server only guarentees coherency at the chunk level,
so it is not possible for a partial chunk to get delivered to the agent.
But it is possible for some chunks of a VDAgentMessage to be delivered to
the agent followed by a client to disconnect without the rest of the
VDAgentMessage being delivered!
This will leave the agent in a wrong state, and the first messages send to it
by the next client to connect will get seen as the rest of the VDAgentMessage
from the previous client.
This patch sends the agent a new VD_AGENT_CLIENT_DISCONNECTED message from the
VDP_SERVER_PORT, on which the agent can then reset its VDP_CLIENT_PORT state.
Note that no capability check is done for this, since the capabilities are
something negotiated between client and agent. The server will simply always
send this message on client disconnect, relying on older agents discarding the
message since it has an unknown type (which both the windows and linux agents
already do).
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
To allow the server to send agent messages without needing to wait for a
self-token. IE for sending VD_AGENT_CLIENT_DISCONNECTED messages.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
These messages are printed when the server tries to push a mouse event to
the agent before the previous one has been flushed. This is a normal condition
(which gets tracked by the reds->pending_mouse_event boolean), and as such
it should *not* trigger the printing of error messages.
I've seen these messages occasionally before, but with agent file-xfer they
are trivial to trigger, simply send a large file to the agent and while it
is transferring move the mouse over the client window. Note that due to the
client tokens not allowing the client to completely saturate the agent
channel mouse events do still get send to the agent, just with a slightly
larger interval. So everything is working as designed and this spice_printerr
is just leading to people chasing ghosts.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
1) This does not buy us much, as red_marshall_monitors_config() also
removes 0x0 sized monitors and does a much better job at it
(also removing intermediate ones, not only tailing ones)
2) The code is wrong, as it allocs space for real_count heads, where
real_count always <= monitors_config->count and then stores
monitors_config->count in worker->monitors_config->count, causing
red_marshall_monitors_config to potentially walk
worker->monitors_config->heads past its boundaries.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
During my dynamic monitor support testing today, I hit the following assert
in red_worker.c:
"red_push_monitors_config: condition `monitors_config != NULL' failed"
This is caused by the following scenario:
1) Guest causes handle_dev_monitors_config_async() to be called
2) handle_dev_monitors_config_async() calls worker_update_monitors_config()
3) handle_dev_monitors_config_async() pushes worker->monitors_config, this
takes a ref on the current monitors_config
4) Guest causes handle_dev_monitors_config_async() to be called *again*
5) handle_dev_monitors_config_async() calls worker_update_monitors_config()
6) worker_update_monitors_config() does a decref on worker->monitors_config,
releasing the workers reference, this monitor_config from step 2 is
not yet free-ed though as the pipe-item still holds a ref
7) worker_update_monitors_config() creates a new monitors_config with an
initial ref-count of 1 and stores that in worker->monitors_config
8) The pipe-item of the *first* monitors_config is send, upon completion
a decref is done on the monitors_config, and monitors_config_decref not
only frees the monitor_config, but *also* sets worker->monitors_config
to NULL, even though worker->monitors_config no longer refers to the
monitor_config being freed, it refers to the 2nd monitor_config!
9) The client which was connected when this all happened disconnects
10) A new client connects, leading to the assert:
at red_worker.c:9519
num_common_caps=1, common_caps=0x5555569b6f60, migrate=0,
stream=<optimized out>, client=<optimized out>, worker=<optimized out>)
at red_worker.c:10423
at red_worker.c:11301
Note that red_worker.c:9519 is:
red_push_monitors_config(dcc);
gdb does not point to the actual line of the assert because the function gets
inlined.
The fix is easy and obvious, don't set worker->monitors_config to NULL in
monitors_config_decref. I'm a bit baffled as to why that code is there in
the first place, the whole point of ref-counting is to not have one single
unique place to store the reference...
This fix should not have any adverse side-effects as the 4 callers of
monitors_config_decref fall into 2 categories:
1) Code which immediately after the decref replaces worker->monitors_config
with a new monitors_config:
worker_update_monitors_config()
set_monitors_config_to_primary()
2) pipe-item freeing code, which should not touch the worker state at all
to being with
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
server/Makefile apparently forgot to link libspice-server
with -lm -lpthread, but it uses symbols from these libraries
directly. These libs are detected by configure and stored in
$(SPICE_NONPKGCONFIG_LIBS) make variable, but this variable
is never referenced at link time. Add it to server/Makefile.am,
to libspice_server_la_LIBADD variable.
Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
The stream vis_region should be cleared after the stream region was sent
to the client losslessly. Otherwise, we might send redundant stream upgrades
if we process more drawables that are dependent on the stream region.
resolves: rhbz#891326
Starting from commit 81fe00b08a, red_detach_streams_behind can
trigger modifications in the current tree (by update_area calls). Thus,
after calling red_detach_streams_behind it is not safe to access tree
entries that were calculated before the call.
This patch inserts the drawable to the tree before the call to
red_detach_streams_behind. This change also requires making sure
that rendering operations that can be triggered by
red_detach_streams_behind will not include this drawable (which is now part of the tree).
Reported-by: Michal Luscon <mluscon@redhat.com>
Found by a Coverity scan:
in handle_dev_start -
Checking "worker->display_channel" implies that "worker->display_channel"
might be NULL.
Passing "worker" to function "guest_set_client_capabilities"
in guest_set_client_capabilities -
Directly dereferencing parameter "worker->display_channel"
reds.c is using strncpy with a length one byte less than the
destination buffer size, and is relying on the fact that the
destination buffers are static global variables.
Now that we depend on glib, we can use g_strlcpy instead, which
avoids relying on such a subtle trick to get a nul-terminated
string.
We currently output a warning when getaddrinfo fails, but then
we go on trying to use the information it couldn't read. Make
sure we bail out of reds_init_socket if getaddrinfo fails.
spice_server_set_ticket and spice_server_set_addr get (library)
user-provided strings as arguments, and copy them to fixed-size
buffers using strncpy. However, if these strings are too long,
the copied string will not be 0-terminated, which will cause issues
later. This commit copies one byte less than the size of the
destination buffer. In both cases, this buffer is a static global
variable, so its memory will be set to 0.
red_proccess_commands calls were added after calling
guest_set_client_capabilities in order to cleanup the command ring from
old commands that the client might not be able to handle.
However, calling red_process_commands at this stage does send messages
to the client.
In addition, since setting the client capabilities at the guest is not
synchronized, emptying the command ring is not enough in order to make
sure the following commands will be supported by the client.
The call to red_proccess_commands before initializing the display
streams (the call to red_display_start_streams), caused inconsistencies
related to video streaming upon reconnecting (rhbz#883564).
I'm reverting this patch till another solution for the capabilities
mismatch is introduced.
Resolves: rhbz#883564
A Spice port channel carry arbitrary data between the Spice client and
the Spice server. It may be used to provide additional services on top
of a Spice connection. For example, a channel can be associated with
the qemu monitor for the client to interact with it, just like any
qemu chardev. Or it may be used with various protocols, such as the
Spice Controller.
A port kind is identified simply by its fqdn, such as org.qemu.monitor,
org.spice.spicy.test or org.ovirt.controller...
The channel is based on Spicevmc which simply tunnels data between
client and server, with a few additional messages.
See the description of the channel protocol in spice-common history.
The server can receive from the client agent data even when the agent
is disconnected. This can happen if the client sends the agent data
before it receives the AGENT_DISCONNECTED msg. We should receive and handle such msgs, instead
of disconnecting the client.
This bug can also lead to a server crash if the agent gets reconnected
fast enough, and it receives an agent data msg from the client before MSGC_AGENT_START.
upstream bz#55726
rhbz#881980
Internal images are just read from the surface, compressed, and sent to the client.
Then, they are destroyed. I can't find any reason for aligning their memory.
rhbz#876685
The current lz implementation does not support such bitmaps.
The following patch will actually prevent allocating stride > bpp*width
for internal images.
Previously, there was no check for the size of the message received from
the client, and all messages were read into a buffer of size 1024.
However, migration data can be bigger than 1024. In such cases, memory
corruption occurred.
red_wait_outgoing_item only waits till the currently outgoing msg is
completely sent.
red_wait_outgoing_items does the same for multi-clients. handle_dev_stop erroneously called
red_wait_outgoing_items, instead of waiting till all the items in the
pipes are sent.
This waiting is necessary because after drawables are sent to the client, we release them from the
device. The device might have been stopped due to moving to the non-live
phase of migration. Accessing the device memory during this phase can lead
to inconsistencies.
Also, MSG_MIGRATE should be the last message sent to the client, before
MSG_MIGRATE_DATA. Due to this bug, msgs were marshalled and sent after
handle_dev_stop and after handle_dev_display_migrate which sometimes led
to the release of surfaces, and inserting MSG_DISPLAY_DESTROY_SURFACE
after MSG_MIGRATE.
This patch also removes the calls to red_wait_outgoing_items, from
dev_flush_surfaces. They were unnecessary.
The current solution just copy the buffer. Currently data that is read
from the guest is always copied before sending it to the client. When we
will have ref count for these buffers, we can also use it for marshalling
the migration data.
The ref count is used in order to keep buffers that were in the write
queue and now are part of migration data, in case the char_device state
is destroyed before we complete sending the migration data.
fix: rhbz#866929
At migration destination side, we need to restore the client's surfaces
state, before sending surfaces related messages.
Before this patch, we stopped the processing of only the cmd ring, till migration data
arrived.
However, some QXL_IOs require reading and rendering the cmd ring (e.g.,
update_area). Moreover, when the device is reset, after destroying all
surfaces, we assert (in qemu) if the cmd ring is not empty (see
rhbz#866929).
This fix makes the red_worker thread wait till the migration data arrives
(or till a timeout), and not process any input from the device after the
vm is started.
We try to inject an interrupt to the vm in this case, which we cannot do
if it is stopped. Instead log this and update when vm restarts.
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=870972
(that bz is on qemu, it will be cloned or just changed, not
sure yet)
New API: spice_server_set_ws_ports
This adds an optional dependency on libwebsockets. You need to get my
patched 0.0.3 version here:
git://people.freedesktop.org/~alon/libwebsockets
There is no qemu patches yet, to test change in reds.c the default value
of spice_ws_port to 5959 (for the default of spice-html5).
For testing there is an online client at
http://spice-space.org/spice-html5/spice.html
Known issues:
1. The tester (server/tests/test_display_no_ssl) gets into dropping all
data after a few seconds, I think it's an issue with the implemented
watches, but haven't figured it out.
2. libwebsocket's read interface is inverted to what our code expects,
i.e. there is no libwebsocket_read, so there is an additional copy
involved (see RedsWebSocket). This can be fixed.
3. Listening on a separate port. Since the headers are different, we
could listen on the same port (first three bytes RED/GET). I don't know
if we want to?
Todos:
1. SSL not implemented yet. Needs some thought as to how.
2. Serve spice-html5 when accessed as a http server. Nice to have.
This solves a problem with new spice-server and old qemu-kvm, where spice thinks
qif->client_monitors_config exists, while it does not exist in qemu-kvm.
Also "major > required_major" was added to the condition.
Also only the specific RedDispatcher is checked (and not all dispatchers).
The client will send 0x000000## codes for regular keys, and 0x0000##e0 codes
for extended keys. The current code which simply walks the uint32_t code in
memory order relies on the memory order being little endian, which will
clearly fail on big endian machines, this fixes this.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
snd_channel_put freed "channel", and then channel->worker was accessed.
It caused segmentation faults during connections and disconnections of the client.
This caused a jenkins build failure:
snd_worker.c:148: error: redefinition of typedef 'PlaybackChannel'
snd_worker.c:126: note: previous declaration of 'PlaybackChannel' was here
The client of _get_buffer() holds a ref to the SndChannel, and we
should access that SndChannel when _put_samples() is called, not the one
that happens to currently be attached to the Interface.
When we release the SndChannel reference during
snd_disconnect_channel(), we need to set the pointer to NULL so it
doesn't get released again on client reconnect during
snd_set_playback_peer(). This can happen when a reference is held from
_playback_get_buffer().
This used to abort with spice_error. The caller currently does
not check spice_server_char_device_add_interface return value, but
it's still cleaner to report an error in this case.
Errors occurring in reds_init_ssl used to be fatal through the use
of spice_error, but this was downgraded to non-fatal spice_warning
calls recently. This means we no longer error out when invalid SSL
(certificates, ...) parameters are passed by the user.
This commit changes reds_init_ssl return value from void to int so
that errors can be reported to the caller.
We used to be aborting in such situations, but this was changed
during the big spice_error/printerr cleanup. We are currently
outputting a warning but not reporting the error with the caller
when reds_init_net fails to register listening watches with the
mainloop. As it's unlikely that things will work as expected in
such cases, better to error out of the function instead of pretending
everything is all right.
BN_new returns NULL on allocation failures. Given that we abort
on malloc allocation failures, we should also abort here. The
current code will segfault when BN_new fails as it immediatly tries
to use the NULL pointer.
If the guest supports client monitors config we pass it the
VDAgentMonitorsConfig message via the
QXLInterface::client_monitors_config api instead of via the vdagent.
Adds two functions:
- red_dispatcher_use_client_monitors_config:
check that QXLInterface supports client_monitors_config and that it's
functional.
- red_dispatcher_client_monitors_config:
send the client monitors configuration to the guest.
Used to implement guest monitor configuration change similarly to real
hardware in conjunction with the new qemu interrupt
QXL_INTERRUPT_CLIENT_MONITORS_CONFIG. client_monitors_config is also
used to probe the support by the interface. If it is not supported we
send the message to the guest agent.
This makes a linux qxl driver similar to existing kms drivers.
The logic is:
For every received VDAgentMonitorsConfig:
if client_monitors_config(NULL):
write client configuration to pci rom BAR.
send interrupt to guest
guest kernel reads configuration from rom BAR.
guest kernel issues event to user space
user space reads (libdrm) and reconfigures (libXRandr)
else: (current implementation)
write message to guest agent
guest agent issues reconfiguration via XRandr / windows Escape ioctl to kernel
No new symbols are added, but there is an addition to QXLInterface:
void (*set_client_capabilities)(QXLInstance *qin,
uint8_t client_present,
uint8_t caps[58]);
When a new client connects, there may be commands in the ring that it
can't understand, so we need to process these before forwarding new
commands to the client. By doing this after changing the capability
bits we ensure that the new client will never see a command that it
doesn't understand (under the assumption that the guest will read and
obey the capability bits).
Acked-by: Alon Levy <alonl@redhat.com>
A new interface
set_client_capabilities (QXLInstance *qin,
uint8_t client_present,
uint8_t caps[58]);
is added to QXLInstance, and spice server is changed to call it
whenever a client connects or disconnects. The QXL device in response
is expected to update the client capability bits in the ROM of the
device and raise the QXL_INTERRUPT_CLIENT interrupt.
There is a potential race condition in the case where a client
disconnects and a new client with fewer capabilities connects. There
may be commands in the ring that the new client can't handle. This
case is handled by first changing the capability bits, then processing
all commands in the ring, and then start forwarding commands to the
new client. As long as the guest obeys the capability bits, the new
client will never see anything it doesn't understand.
Just checks stride vs width times bpp.
This fixes a potential abort on guest generated bad images in
glz_encoder.
Other files touched to move some consts to red_common, they are
static so no problem to be defined in both red_worker.c and
red_parse_qxl.c .
replace add_ref with add for stack allocated SpiceMigrateDataDisplay.
This fixes wrong MIGRATE_DATA message in display channel (symptom is
glz_encoder_max being way too big, and malloc failure at target) seen on
F18 with gcc-4.7.1-5.fc18.x86_64 and glibc-2.16-8.fc18.x86_64 (didn't
appear on RHEL 6).
Storing the motion count in uint16_t and not in uint32_t since
the exact count is not important, just its division in
SPICE_INPUT_MOTION_ACK_BUNCH (see the next 2 patches).
red_channel_client_set_message_serial is called for setting
the serial of the display channel messages after migration (on the
destination side). The serial is retrieved from the migration data.
The playback and record channel send SPICE_MSG_MIGRATE to the client.
Both playback and record channel does not have a state to restore:
while in the legacy migration implementation the record channel
used to restore the mode and start time, it looks unnecessary since
the client receives from the src MSG_RECORD_STOP before the migration
completion notification (when the vm is stopped). Afterwards, when the vm
starts on the dest side, the client receives MSG_RECORD_START.
Due to the fix in the previous patch, snd_disconnect_channel can be
called both when there is write/read error in the channel, or from
red_client_destroy (which calls client_cbs.disconnect).
Multiple calls to snd_disconnect_channel resulted in calling
channel->cleanup(channel) more than once (double release).
snd channel wasn't added to be part of the client's channels list.
As a result, when the client was destroyed, or migrated, snd channel
client wasn't destroy, or its migration callback wasn't called.
However, due to adding dummy channels to the client, we need special
handling for calls to disconnecting dummy channel clients.
TODO: we need to refactor snd_worker to use red_channel
A channel pipe item type must start from PIPE_ITEM_TYPE_CHANNEL_BASE.
SPICE_MSG_MIGRATE value eq. PIPE_ITEM_TYPE_SET_ACK. Setting a pipe item
type to SPICE_MSG_MIGRATE, leads to red_channel handling PIPE_ITEM_TYPE_SET_ACK.
If reading/writing from the device have occured before migration data
has arrived, the migration data might no longer be relvant, and we
disconnect the client.
Before sending the above msg, if there is a pending partial msg that
has been read from the agent, we send it to the client. The alternative
was to keep the msg as part of the migration data, and then
to send it to the destination server via the client and to wait there
for the msg chunk completion, before sending it to the client. Of
course, the latter is less efficient.
A channel pipe item type must start from PIPE_ITEM_TYPE_CHANNEL_BASE.
SPICE_MSG_MIGRATE value eq. PIPE_ITEM_TYPE_SET_ACK. Setting a pipe item
type to SPICE_MSG_MIGRATE, leads to red_channel handling PIPE_ITEM_TYPE_SET_ACK.
Also removed sending SPICE_MSG_MIGRATE. It will be handled in the next
patch.
Attach/detach a client to a SpiceCharDeviceState upon its
connection/disconnection, instead of upon reader_add/remove messages.
When the client is removed from a SpiceCharDeviceState, all the
messages from this client are removed from the device write queue.
This shouldn't happen when we only receive reader_remove and the
client is still connected.
When restoring migration data, we also restore data that is addressed to
the device, and that might have been originated from more than 1
message. When the write buffer that is assoicated with this data is
released, we need to free all the relevant tokens.
Tracking the channels that wait for migration data. If there
is a new migration process pending, when all the channels have
restored their state, we begin the new migration.
In semi-seamless, SPICE_MSG_MAIN_MIGRATE_END is sent.
In seamless, each channel migrates separately.
The src waits till all the clients are disconnected (or a timeout), and
then it notifies qemu that spice migration has completed.
The patch doesn't include the per-channel logic for seamless migration
(sending MSG_MIGRATE, MIGRATE_DATA, etc.).
- handle SPICE_MSGC_MAIN_MIGRATE_DST_DO_SEAMLESS
- reply with SPICE_MSG_MAIN_MIGRATE_DST_SEAMLESS_ACK/NACK
- prepare the channels for migration according to the migration
type (semi/seamless)
see spice-protocol for more details:
commit 3838ad140a046c4ddf42fef58c9727ecfdc09f9f
sending SPICE_MSG_MAIN_MIGRATE_BEGIN_SEAMLESS and handling
SPICE_MSGC_MAIN_MIGRATE_CONNECTED_SEAMLESS
The src side signals the client to establish a connection
to the destination.
In seamless migration, the client is also used to perform
a sort of handshake with the destination, for verifying
if seamless migration can be supported.
see spice-protocol for more details:
commit 3838ad140a046c4ddf42fef58c9727ecfdc09f9f
The file will hold the declarations of the different migration
data messages (depending on the channel), that will be passed
from the src server to the dst server, via the client, using
SPICE_MSG_MIGRATE_DATA.
This new call is used in order to identify whether qemu, or
the management (e.g. libvirt), support seamless migration.
If it is supported, qemu spice cmd-line configuration should have
seamless-migration=on.
In addition, we disable seamless migration support if multiple clients
are allowed. Currently, only one client is supported.
Till now, red_worker was notfied about vm status changes via QXLWorker->start/stop
(or spice_qxl_start/stop).
Newer qemu, that supports calling spice_server_vm_start/stop, will call only
these routines, and won't call QXLWorker->start/stop.
When vm state changes (started/stopped), we notify all the
attached SpiceCharDeviceStates about the change. This is mainly required
for avoiding writing/reading to/from the device during the non-live
stage of migration.
spice version will be bumped in one of the following patches.
The list of attached char_devices will be used in the next patch
for notifying each instance of SpiceCharDeviceState when the vm
is started or stopped.
Graduality is irrelevant for A8 images, so instead of using RGB-ness
as a short-cut, add a new macro BITMAP_FMT_HAS_GRADUALITY() that
returns true for the existing RGB images, but false for A8.
After marshalling MSG_STREAM_CREATE, there is no need to ref and
unref stream->current before and after completing the sending of the
message (correspondingly). The referencing is unnecessary because all
the data that is required from the drawable (the clipping), is copied
during marshalling, and no field in the drawable is referenced (see
spice_marshall_msg_display_stream_create).
Moreover, the referencing was bugous:
While display_channel_hold_pipe_item and
display_channel_client_release_item_after_push referenced and
dereferenced, correspondingly, stream->current, stream->current might
have changed in between these calls, and then we ended up with one drawable
leaking, and one drawable released before its time has come (which
of course led to critical errors).
a SpiceMsgDisplayMonitorsConfig is sent on two occasions:
* as a result of a spice_qxl_monitors_config_async
* whenever a client connects and there is a previously set monitors
config
Sending the new message is protected by a new cap,
SPICE_DISPLAY_CAP_MONITORS_CONFIG
More elaborately:
spice_qxl_monitors_config_async receives a QXLPHYSICAL address of a
QXLMonitorsConfig struct and reads it, caching it in the RedWorker, and
sending it to all clients. Whenever a new client connects it receives
a SpiceMsgDisplayMonitorsConfig message as well.
prevents division by zero later (SIGFPE, Arithmetic exception) in
spice-common code, at spice-common/common/canvas_base.c:646
for both client and server (server only upon rendering).
Rules for replacing spice_printerr:
* if it's a client drop / agent drop, spice_error
* if it's important, spice_warning
* else, spice_info.
The fourth rule is spice_debug if it's going to fill up the log, but
nothing of the sort here.
Other things this patch does:
* changed reds_show_new_channel to reds_info_new_channel in an attempt to
advertise it doesn't always show anything, unless SPICE_DEBUG_LOG_LEVEL
>= SPICE_DEVUG_LOG_INFO (==3)
* removes two lines of whitespace.
* added "crashy" to multiple client warning (only warning introduced,
the rest are errors).
Specifically all those that the previous patch converted to spice_debug.
spice_debug contains very verbose stuff like update_area that drowns out
those relatively rare (client connect / disconnect generated) messages.
With SpiceCharDeviceState, the smartcard code now supports partial writes,
and storing data that is received from the client after the device is
stopped, instead of attempting to write it to the guest.
The lifetime of the channel is not necessarily correlated to the life
time of the device. In the next patch, we need to keep a reference
to SpiceCharDeviceWriteBuffer, which might be in use even if the
SpiceCharDeviceState is destroyed, but the channel is still connected.
The next patch keeps this reference inside SmartCardChannelClient.
This patch also removes the routine smartcard_readers_detach_all(rcc), which
is unnecessary since we don't support multiple readers; even when
we do support them, each channel client should be associated with only
one reader (i.e., we will have different channels for different
readers).
This patch and the following one do not introduce tokening to the smartcard
channel. But this can be done easily later, by setting the appropriate
variables in SpiceCharDeviceState (after adding the appropriate protocol messages,
and implementing this in the client side).
With SpiceCharDeviceState, spicevmc now supports partial writes,
and storing data that is received from the client after the device is
stopped, instead of attempting to write it to the guest.
This patch and the following one do not introduce tokening to the
spicevmc channel. But this can be done easily later, by setting the appropriate
variables in SpiceCharDeviceState (after adding
the appropriate protocol messages, and implementing this in the client
side).
This code is never called, it was relevant for seamless migration.
Most of the data that needs to be migrated was moved to
SpiceCharDeviceState. When we implement seamless migration,
we will have routines in char_device.c for migrating the relevant data.
- Allow sending tokens to a specific client.
- Do not ignore tokens that are sent from the client to the server.
The tokens support for multiple clients and for server side tokens
is still broken in reds. It will be fixed in following patches, when
the server-side agent code will use the SpiceCharDeviceState api.
Notice that ignoring the server-side tokens didn't introduce a problem
since both the client and the server set it to ~0.
SpiceCharDeviceState manages the (1) write-to-device queue
(2) wakeup and reading from the device (3) client tokens (4)
sending messages from the device to the client/s, considering the
available tokens.
SpiceCharDeviceState can be also stopped and started. When the device
is stopped, no reading or writing is done from/to the device. Messages
addressed from the client to the device are being queued.
Later, an api for stop/start will be added to spice.h and it should
be called from qemu.
This patch does not yet remove the wakeup callback from
SpiceCharDeviceState, but once all the char devices (agent/spicevmc/smartcard)
code will switch to the new implementation, SpiceCharDeviceState
will be moved to the c file and its reference to the wakeup callback will be removed.
Current bumped and age bumped for new intefaces only (no backward
incompatible changes).
New libtool version is 2.0.1, using --version-info instead of
--version-name. Doing the version change and --version-name to
--version-info change here to avoid changing the libtool version twice.
Added interfaces:
spice_server_set_name
spice_server_set_uuid
spice_server_set_listen_socket_fd
spice_server_is_server_mouse
New library name in linux:
libspice-server.so.1.1.0
Old:
libspice-server.so.1.0.2
The red_channel_client_event call to red_channel_client_receive might result
in a disconnected channel client. The following call to
red_channel_client_push may call to red_peer_handle_outgoing with a
disconnected socket.
Added ref count for RedChannel and RedChannelClient.
red_channel.c/red_peer_handle_incoming call to
handler->cb->handle_message might lead to the release of the channel
client, and the following call to handler->cb->release_msg_buf will be
a violation.
This bug can be produced by causing main_channel_handle_parsed
call red_client_destory, e.g., by some violation in
reds_on_main_agent_data that will result in a call to reds_disconnect.
Resolves: rhbz#824384
red_wait_pipe_item_sent mistakingly returned without waiting for sending the given pipe item
when the channel wasn't blocked. As a result, we failed when we had to
destroy a surface (e.g., QXL_IO_DESTROY_ALL_SURFACES) and to release all
the drawables that are depended on it (by removing them or waiting they will be sent).
In addition, red_wait_pipe_item_sent increased and decreased the reference to the pipe item
using channel_cbs->hold_item, and channel_cbs->release_item. However,
these calls can be called only by red_channel, otherwise
display_channel_client_release_item_before_push is called twice and
leads to a double call to ring_remove(&dpi->base).
Instead ref/put_drawable_pipe_item should be called.
The above routine was risky, since red_channel_client_init_send_data
can also be called with item==NULL. Thus, not all pipe items can be tracked.
The one call that was made for this routine was not necessary.
Resolves: rhbz#820669
Fix a segfault caused by a call to __red_is_next_stream_frame made by
red_stream_maintenance, with a drawable that is not a DRAW_COPY one.
The segfault is a reault of __red_is_next_stream_frame accessing
red_drawable->u.copy.src_bitmap for a non DRAW_COPY drawable.
Before, we tested only higher frames, while wider frames would have
triggered a bug in mjpeg_encoder, when spice is linked with libjpeg and
not libjpeg-turbo.
Simplify keeping count of self_bitmap_image by putting it in
RedDrawable. It is allocated on reading from the command pipe and
deallocated when the last reference to the RedDrawable is dropped,
instead of keeping track of it in GlzDrawable and Drawable.
After the previous patch moving self_bitmap freeing inside red_drawable
ref count, we have a possible self_bitmap leak:
red_process_commands
red_get_drawable | red_drawable #1, red_drawable->self_bitmap == 1
red_process_drawable | rd #2, d #1, d->self_bitmap != NULL
release_drawable | rd #1, d# = 0, try to release self_bitmap, but
blocked by rd #1
put_red_drawable | rd #0
This patch moves the call to release_drawable after the call to
put_red_drawable, thereby fixing the above situation.
rhbz #813826
When playing a youtube video on Windows guest, the driver sometimes(**) sends
images which contain the video frames, but also other parts of the
screen (e.g., the youtube process bar). In order to prevent glitches, we send these
images as part of the stream, using SPICE_MSG_DISPLAY_STREAM_DATA_SIZED.
(**) It happens regularly with the you tube html5 player. With flash,
it occurs when moving the cursor in the player area.
spice-common changes: STREAM_DATA_SIZED message was added in order to support
video streams with frames that their size is different from the initial size
that the stream was created with.
This patch also includes server and client adjustments to the new
SpiceMsgDisplayStreamData.
The previous patch took care that streams will be upgraded by a surface
screenshot and not the last frame, if necessary. Thus, there is no
more a reason for detaching drawables from streams when they are
rendered. Moreover, detaching such drawables can cause glitches, in
case they are still in the pipe, and red_update_area is called
frequently and leads to stream frames rendering.
Upgrading a stream: When the stream's visible region is bigger than the one of the last
frame, we will send an updated screenshot of the visible region, instead
of sending the last frame losslessly.
Differentiate between the clipping of the video stream, and the region
that currently displays fragments of the video stream (henceforth,
vis_region). The latter equals or contains the former one. For example,
let c1 be the clip area at time t1, and c2 be the clip area at time t2,
where t1 < t2. If c1 contains c2, and at least part of c1/c2, hasn't been
covered by a non-video images, vis_region will contain c2, and also the part
of c1/c2 that still displays fragments of the video.
When we consider if a stream should be "upgraded" (1), due to its area
being used by a rendering operation, or due to stopping the video, we
should take into account the vis_region, and not the clip region (next
patch: not upgrade by the last frame, but rather by vis_region).
This fix will be more necessary when sized frames are introduced (see the
following patches). Then, the vis_region might be larger
than the last frame, and contain it, more frequently than before.
(1) "upgrading a stream" stands for sending its last frame losslessly. Or more
precisely, lossless resending of all the currently displayed lossy areas, that were
sent as part of the stream.
Methods which longjump, unconditionally raise an
exception, or call _exit() cannot return control
to the caller so should be annotated with 'noreturn'
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The 3 part of the conditional in the is_equal_brush method
compared the b1->u.color field to itself, instead of b2->u.color
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Tell the compiler that was really do intend to cast from int
to pointer, to prevent warnings about implicit casts
* server/tests/test_display_base.c: Add explicit casts
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
If a 'goto' statement jumps across a variable declaration
which also has an initializer, the variable is in an undefined
state. Splitting the the declaration & initialization doesn't
change that, but the compiler can at least now detect use of
the unintialized variable
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The red_worker_main method allocates a RedWorker struct instance
on the stack. This struct is a full 2 MB in size which is not
at all resonable to allocate on the stack.
* server/red_worker.c: Move RedWorker struct to the heap
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
When using setjmp/longjmp the state of local variables can be
undefined in certain scenarios:
[quote man(longjmp)]
The values of automatic variables are unspecified after a
call to longjmp() if they meet all the following criteria:
· they are local to the function that made the correspond‐
ing setjmp(3) call;
· their values are changed between the calls to setjmp(3)
and longjmp(); and
· they are not declared as volatile.
[/quote]
* server/red_worker.c: Mark some vars as volatile
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
* client/red_channel.cpp: AbortTrigger::on_event can't return
given its current impl
* server/red_worker.c: red_worker_main can't return
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
All printf var-args style methods should be annotation with
their format. All format strings must be const strings.
* client/application.cpp, client/cmd_line_parser.cpp,
client/hot_keys.cpp: Avoid non-const format
* client/client_net_socket.cpp: Fix broken format specifier
* client/red_peer.cpp: Fix missing format specifier
* client/platform.h: Add SPICE_GNUC_PRINTF annotation to term_printf
* client/utils.h: Add SPICE_GNUC_PRINTF annotation to string_printf
* server/glz_encoder_config.h, server/red_worker.c: Add
SPICE_GNUC_PRINTF annotation to warning callbacks
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
* server/red_worker.c: Add missing const for return type
* server/reds.c: Static strings must be declared const
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Arithmetic on void * types is non-portable & trivially avoided
* server/dispatcher.c: Use uint8_t for arithmetic
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This patch changed getvirt to continue working even if spice_critical
doesn't abort (i.e. SPICE_ABORT_LEVEL != -1). This is in preparation to
make getvirt not abort at all. The reason is that getvirt is run on
guest provided memory, so a bad driver can crash the vm.
This patch will replace the common/ directory with the spice-common
project. It is for now a simple project subdirectory shared with
spice-gtk, but the goal is to make it a proper library later on.
With this change, the spice-server build is broken. The following
commits fix the build, and have been seperated to ease the review.
v2
- moves all the generated marshallers to spice-common library
- don't attempt to fix windows VS build, which should somehow be
splitted with spice-common (or built from tarball only to avoid
generation tools/libs deps)
v3
- uses libspice-common-client
- fix a mutex.h inclusion reported by Alon
cb_get_virt and cb_validate_virt have disappeared a long time ago,
not needed since:
commit 5ac88aa79f
Author: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu Jul 1 17:55:33 2010 +0200
Properly parse QXLImage to the new-world SpiceImage
SpiceImage now replaces RedImage and has all image types in it.
All image data are now chunked (and as such not copied when demarshalling).
If we run out of watches slots, we return NULL from watch_add, which
means that the other watch_foo functions may get called with a NULL
parameter, protect them against this.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
If we allow listening on arbitrary sockets like unix sockets,
we can get ENOPROTOOPT errors from setsockopt calls that set TCP
specific options. This should be allowed to happen.
Commit 143a1df24e changed red_worker_main
from epoll to poll. But epoll has edge triggered semantics (when requested
and we requested them), where as poll is always level triggered. And
red_worker was relying on the edge triggered semantics, as it was always
polling for POLLOUT, which, when edge triggered, would only cause poll
to register an event after we had blocked on a write. But after the
switch to regular poll, with its level triggered semantics, the POLLOUT
condition would almost always be true, causing red_worker_main to not
block on the poll and burn CPU as fast as it can as soon as a client was
connected.
Luckily we already have a mechanism to switch from polling for read only
to polling for read+write and back again in the form of watches. So this
patch changes the red_worker dummy watch implementation into a proper watch
implementation, and drops the entire EventListener concept since that then is
no longer needed.
This fixes spice-server using 400% CPU on my quad core machine as soon as
a client was connected to a multi head vm, and as an added bonus is a nice
cleanup IMHO.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
We allow channels to have different core implementations, but we were
relying on reds_stream_free to remove the stream watch on disconnect,
and reds_stream_free always uses the qemu core implementation.
So far we were getting away with this since all the alternative core
implementations always return NULL from watch_add.
But:
1) The code before this patch clearly was not correct, since it was matching
a channel-core watch_add with a qemu-core watch_remove
2) I plan to move red_worker over to actually using an alternative watch
implementation at which point this becomes a real problem
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The red_worker EventListener struct is either embedded in one of:
1) DisplayChannelClient
2) CursorChannelClient
3) RedWorker
And as such gets destroyed when these get destroyed, in case 1 & 2 through
a call to red_channel_client_destroy().
So free-ing it when the ref-count becomes 0 is wrong, for cases:
1) and 2) this will lead to a double free;
3) this will lead to passing memory to free which was not returned by malloc.
This is not causing any issues as the ref-count never gets decremented, other
then in red_worker_main where it gets incremented before it gets decremented,
so it never becomes 0.
So we might just as well completely remove it.
Notes:
1) This is mainly a preparation patch for fixing issues introduced by
the move from epoll to poll
2) Since removing the ref-counting removes the one code path where listeners
would get set to NULL, this patch moves the setting of NULL to
pre_disconnect, where it should have been done in the first place since
red_client_destroy calls red_channel_client_disconnect
(through the dispatcher) followed by red_channel_client_destroy, so
after pre_disconnect the listener may be gone.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
While git-bisecting another issue I ended up hitting and not recognizing
the bug fixed by commit 7a079b452b.
While fixing this (again) I noticed that (even after the fix) not all
users of ChannelCbs first zero it. So this patch ensures that all users of
ChannelCbs first zero it, and does the same for ClientCbs while at it.
Since before this patch there were multiple zero-ing styles, some using
memset and other using a zero initializer this patch also unifies all
the zero-ing to use a NULL initializer for the first element.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
This fixes a core dumped observed once by repeated migration. So far 100
migrations and no recurrence.
Core was generated by `/home/alon/spice/upstream/bin/qemu-system-x86_64 --enable-kvm -qmp unix:/tmp/mi'.
Program terminated with signal 11, Segmentation fault.
11197 if (evt_listener && evt_listener->refs > 1) {
Missing separate debuginfos, use: debuginfo-install bluez-libs-4.98-3.fc17.x86_64 brlapi-0.5.6-4.fc17.x86_64 bzip2-libs-1.0.6-4.fc17.x86_64 cryptopp-5.6.1-6.fc17.x86_64 keyutils-libs-1.5.5-2.fc17.x86_64 libssh2-1.4.0-1.fc17.x86_64 nss-softokn-freebl-3.13.1-20.fc17.x86_64 xen-libs-4.1.2-11.fc17.x86_64 xz-libs-5.1.1-2alpha.fc17.x86_64
(gdb) bt
(gdb) l
11192 for (i = 0; i < MAX_EVENT_SOURCES; i++) {
11193 struct pollfd *pfd = worker.poll_fds + i;
11194 if (pfd->revents) {
11195 EventListener *evt_listener = worker.listeners[i];
11196
11197 if (evt_listener && evt_listener->refs > 1) {
11198 evt_listener->action(evt_listener, pfd);
11199 if (--evt_listener->refs) {
11200 continue;
11201 }
(gdb) p evt_listener
$1 = (EventListener *) 0x7f15a9a5d1e0
(gdb) p *evt_listener
Cannot access memory at address 0x7f15a9a5d1e0
(gdb) p i
$2 = 2
(gdb) p worker.listeners
$3 = {0x7f15bc832520, 0x7f15a406e1a0, 0x7f15a9a5d1e0, 0x0 <repeats 17 times>}
Add spice_server_set_name() and spice_server_set_uuid() that allows
the client to identify a Spice server (useful to associate settings
with a particular server)
The SPICE_MSG_MAIN_NAME and SPICE_MSG_MAIN_UUID messages are only sent
to capable clients, announcing SPICE_MAIN_CAP_NAME_AND_UUID.
Currently, when a ticket has already expired, or is invalid, there is
no qemu log to tell what went wrong. This commit adds such a log.
Fixes rhbz#787669
- Do not refer to .c files managed by another makefile (this will fail
make distclean)
- Do not refer to files by relative path (should use $top_srcdir for ex)
- Use LDADD for object linking instead of LDFLAGS, for linker flags
Now, cursor is being shown in all tests as a white rectangle and is
running in the screen doing a diagonal movement. It's a very simple
way to test cursor commands and is sufficient for our tests.
This is provided by <limits.h> on all platforms as long as _XOPEN_SOURCE
is defined. On Linux, this is 1024, on Solaris, this is 16, and on any
other platform, we now respect the value supported by the OS.
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
Solaris has a pitiful maximum writev vector size of only 16, so the ping
request at initial startup destroyed this call and broke things
immediately. Reimplement stream_writev_cb() to respect IOV_MAX and break
the writev() calls into chunks as necessary. Care was taken to return
the correct values as necessary so the EAGAIN handling logic can
determine where to resume the writev call the next time around.
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
This removes the epoll dependency we had in red_worker, which was the
last Linux-specific call we were using in the entire Spice server. Given
we never have more than 10 file descriptors involved, there is little
performance gain had here by using epoll() over poll().
The biggest change is introduction of a new pre_disconnect callback;
this is because poll, unlike epoll, cannot automatically remove file
descriptors as they are closed from the pollfd set. This cannot be done
in the existing on_disconnect callback; that is too late as the stream
has already been closed and the file descriptor lost. The on_disconnect
callback can not be moved before the close and other operations easily
because of some behavior that relies on client_num being set to a
certain value.
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
Rather than assign the callbacks one-by-one, we can just memcpy the
struct into the one we have allocated in our RedChannel object, which is
much more efficient, not to mention future-proof when more callbacks are
added.
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
We had multiple stub methods that simply called other disconnect
methods, making my head hurt with the indirection. Call the right
methods at the right time and rip out the stub methods; if they are
truely needed later they can be added again.
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
With future patches in mind that will allow for some other
non-Linux-specific event polling sytem to be used, rename this to a more
generic name. All of the select/poll/epoll/kqueue family of calls are
related to evented I/O, so 'event_' makes sense in this case.
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
This is supported by the GNU linker, but not the Solaris linker, which
is used as the default on that platform even when compiling with GCC.
Omit passing the option to the linker on platforms that do not support
it.
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
spicevmc calls red_channel_client_destroy() on the rcc when it disconnects
since we don't want to delay the destroy until the session gets closed as
spicevmc channels can be opened, closed and opened again during a single
session.
This causes red_channel_client_destroy() to get called twice, triggering
an assert, when a connected channel gets destroyed.
This was fixed with commit ffc4de01e6 for
the case where: a spicevmc channel was open on client disconnected, and
the main channel disconnect gets handled first.
But the channel can also be destroyed when the chardev gets unregistered
with the spice-server. This path still triggers the assert.
This patch fixes this by adding a destroying flag to the rcc struct, and
also moves the previous fix over to the same, more clean, method of
detecting this special case.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
fixes rhbz 790749 use after free of SpiceChannelEventInfo.
The lifetime of the SpiceChannelEventInfo was that of RedsStream, but it
is used by main_dispatcher_handle_channel_event after the RedsStream is
freed for the cursor and display channels. Making SCEI allocation be at
RedsStream allocation, and deallocation after the DESTROY event is
processed by core->channel_event, fixes use after free.
red_printf() takes care of adding a newline to all messages; remove the
extra newline from all messages and macros that were doubling them up.
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
As suggested by Alon, a simple automated test to try to find
regressions in Spice code.
To use this, compile Spice with --enable-automated-tests and
run test_display_streaming passing --automated-tests as parameter.
Some non-Linux platforms return a (caddr_t *) result for the return
value of mmap(), which is very unfortunate. Add a (void *) cast to
explicitly avoid the warning when compiling with -Werror.
For the IO vector related stuff, signed vs. unsigned comes into play so
adding a (void *) cast here is technically correct for all platforms.
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
dc7855967f did this for the TCP_NODELAY and IP_TOS calls; we should do
it for priority as well if necessary.
We also #ifdef the setting of the low-level socket priority based on
whether we have a definition of SO_PRIORITY available. This option is
not available on Illumos/Solaris platforms; however, since we set IP_TOS
anyway it is not a big deal to omit it here.
output to send a SIGIO signal to the running program. However, we don't
handle this signal anywhere in the code, so setting the option is
unnecessary.
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
With Daniel P. Berrange's patches to allow use of pre-supplied fd's
as channels, we can no longer be sure that our connections are TCP
sockets, so it makes no sense to complain if a TCP/IP specific
setsockopt fails with an errno of ENOTSUP.
Note that this extends Daniel's commit 492ddb5d1d
which already added the same check to server/inputs_channel.c
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Since free() takes a void* parameters do not need to be cast.
The existing code here is actally fine, but it trips up the
syntax-check rule, so tweak it to an equivalent construct
which passes the syntax check
Support for a header without a serial and without sub list.
red_channel: Support the two types of headers.
Keep a consistent consecutive messages serial.
red_worker: use urgent marshaller instead of sub list.
snd_worker: Sound channels need special support since they still don't use
red_channel for sending & receiving.
When red_channel::red_channel_client_begin_send_message is called,
the message that is pending in the urgent marshaller will be sent before
the one in the main channel.
The urgent marshaller should be used if in the middle of marshalling one message,
you find out you need to send another message before. This functionality
is equivalent to the sub_list messages. It will replace them in the following
patches, when sub_list is removed from Spice data header.
If setting the TCP_NODELAY socket option fails with ENOTSUP,
then don't treat this is a fatal error. SPICE is likely just
running over a UNIX socket instead.
* server/inputs_channel.c: Ignore TCP_NODELAY socket opt fails
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
When an applications passes in a pre-accepted socket for a
client, they may well have already performed suitable authentication
out of band. They should thus have the option to request that any
spice authentication is skipped.
* server/reds.c, spice.h: Add flag for skipping auth
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Allow applications to pass a pre-accepted client socket file
descriptor in. The new APIs are spice_server_add_ssl_client
and spice_server_add_client
* server/reds.c: Implement new APIs
* server/spice.h: Define new APIs
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
To allow setup of an SSL client, from a passed in client
socket, move all the SSL client initialization code out
of reds_accept_ssl_connection and into a new method called
reds_init_client_ssl_connection
* server/reds.c: Introduce reds_init_client_ssl_connection
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Remove the accept() call from __reds_accept_connection and
rename it to reds_init_client_connection. The caller is now
responsible for accepting the new socket. The method
reds_init_client_connection merely initializes it for
usage.
* server/reds.c: Add reds_init_client_connection
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Neither reds_accept_connection or reds_accept are very long,
so the split is pointless & increases code size for no gain.
Merge them together to reduce code size
* server/reds.c: Merge reds_accept_connection into reds_accept
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The display channel was unnecessarily set to NULL when we disconnect all the clients
(on flush display commands timeout).
As a result, we recreated the display channel when a new client was connected.
The display channel was created with default red_channel.client_cbs, while its
correct client_cbs are the ones that are set by the red_dispatcher when it creates
the first display_channel.
This fix enforces a single creation of the display channel (per qxl), via the red_dispatcher.
Currently when the main channel disconnects while a spicevmc channel
(such as a usbredir channel) is still connected, qemu will abort with the
following message:
ring_remove: ASSERT item->next != NULL && item->prev != NULL failed
This is caused by red_client_destroy() first calling:
rcc->channel->client_cbs.disconnect(rcc);
And then calling:
red_channel_client_destroy(rcc);
For each channel. This is fine, but the spicevmc disconnect code does a
red_channel_client_destroy(rcc) itself since as usb devices are added
/ removed, the channels carrying their traffic get connected / disconnected
and they get re-used for new devices, which won't work if the old channel is
still there when the new connection comes in.
This patch fixes the double destroy when there are still spicevmc channels
connected by not doing the red_channel_client_destroy from the spicevmc
disconnect code when not just the channel, but the entire client is
disconnecting.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Also fixup the header of server/red_parse_qxl.c, which still contained
some GPL (program rather then library) text in its header.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
spice_server_migrate_connect is in 0.8.3 in the released 0.8 branch,
and so should not be changed in 0.10. This doesn't break the 0.9.1
release which didn't contain this symbol at all, only 0.9.2 release
that hopefully no one actually packaged.
This patch reuses Dispatcher in RedDispatcher. It adds two helpers
to red_worker to keep RedWorker opaque to the outside. The dispatcher is
abused in three places that use the underlying socket directly:
once sending a READY after red_init completes
once for each channel creation, replying with the RedChannel instance
for cursor and display.
FDO Bugzilla: 42463
rfc->v1:
* move callbacks to red_worker.c including registration (Yonit)
* rename dispatcher to red_dispatcher in red_worker.c and red_dispatcher.c
* add accessor red_dispatcher_get_dispatcher
* s/dispatcher_handle_recv/dispatcher_handle_recv_read/ and change sig to
just Dispatcher *dispatcher (was the SpiceCoreInterface one)
* remove SpiceCoreInterface parameter from dispatcher_init (Yonit)
* main_dispatcher needed it for channel_event so it has it in
struct MainDispatcher
* add dispatcher_get_recv_fd for red_worker
used for main_dispatcher only in this patch.
Dispatcher is meant to be used for Main<->any low frequency messages.
It's interface is meant to include the red_dispatcher usage:
fixed size messages per message type
some messages require an ack
Some methods are added to be used by RedDispatcher later:
dispatcher_handle_read - to be called directly by RedDispatcher epoll
based loop
dispatcher_set_opaque - to be set from red_worker pthread
dispatcher_init - allow NULL core as used by red_worker
Read and Write behavior:
Sender: blocking write, blocking read for ack (if any).
Reader: poll for any data, if such then blocking read for a
message_type and following message. repeat until poll returns
with no pending data to read.
FDO Bugzilla: 42463
This is part of the dispatcher update, extracting the dispatcher routine
from red_dispatcher and main_dispatcher into dispatcher.
Supporting multiple async operations will make it natural to support
async monitor commands and async guest io requests that could overlap in
time.
Use a Ring for AsyncCommands.
Free Desktop Bugzilla: 42463
Related FD: 41622
We will add a qemu call to spice_server_migrate_start when migration starts.
For now, it does nothing, but we may need this notification in the future.
(cherry picked from commit b821316771 branch 0.8)
(1) not sending anything to a migrated client till we recieve SPICE_MSGC_MIGRATE_END
(2) start a new client migration (handle client_migrate_info) only after SPICE_MSGC_MIGRATE_END
from the previous migration was received for this client
(3) use the correct ticket
Note: we assume the same channles are linked before and ater migration. i.e.,
SPICE_MSGC_MAIN_ATTACH_CHANNELS is not sent from the clients.
If the migration has completed successfully:
(1) send MSG_MAIN_MIGRATE_END to the clients that are connected to the target
(2) send MSG_MAIN_SWITCH_HOST to all the other clients
If the migration failed, send MSG_MAIN_MIGRATE_CANCEL to clients that are
connected to the target.
(cherry picked from commit 4b82580fc3 branch 0.8;
Was modified to support multiple clients, and the separation of main_channel from reds)
Conflicts:
server/reds.c
(1) send SPICE_MSG_MAIN_MIGRATE_BEGIN upon spice_server_migrate_connect
(to all the clients that support it)
(2) wait for SPICE_MSGC_MAIN_MIGRATE_(CONNECTED|CONNECT_ERROR) from all the relevant clients,
or a timeout, in order to complete client_migrate_info monitor command
(cherry picked from commit 5560c56ef0 branch 0.8;
Was modified to support the separation of main channel from reds, and multiple clients)
Conflicts:
server/reds.c
semi-seamless migration details:
migration source side
---------------------
(1) spice_server_migrate_connect (*): tell client to link
to the target side - send SPICE_MSG_MAIN_MIGRATE_BEGIN.
This should be called upon client_migrate_info cmd.
client_migrate_info is asynchronous.
(2) Complete spice_server_migrate_connect only when the client has been connected
to the target - wait for SPICE_MSGC_MAIN_MIGRATE_(CONNECTED|CONNECT_ERROR) or a timeout.
(3) spice_server_migrate_end: tell client migration it can switch to the target - send
SPICE_MSG_MAIN_MIGRATE_END.
(4) client cleans up all data related to the connection to the source and switches to the target.
It sends SPICE_MSGC_MAIN_MIGRATE_END.
migration target side
---------------------
(1) the server identifies itself as a migraiton target since the client is linked with (connection_id != 0)
(2) server doesn't start the channels' logic (channel->link) till it receives SPICE_MSGC_MAIN_MIGRATE_END
from the client.
* After migration starts, the target qemu is blocked and cannot accept new spice client
connections. Thus, we trigger the connection to the target upon client_migrate_info
command.
(cherry picked from commit 6e56bea67c branch 0.8)
Conflicts:
server/spice.h
The code for setting and testing channel capabilities was
unnecessarily duplicated. Now it is in red_channel.
RedsChannel was dropped from Reds; It was used only for holding
the channels common capabilities, which are now held in RedChannel.
add main_dispatcher, a message passing mechanism for sending messages to
the main thread. The main thread is the thread that implements
SpiceCoreInterface, which is assumed to be a single thread.
Similar to the async operation of red_worker, a socket pair is created
and used to pass messages. The messages are a fixed size to ease
parsing. A single message is defined to pass a channel_event.
RHBZ: 746950
FDBZ: 41858
This patch is 0.8 branch only, for the master branch there should be a
better approach to share code with red_dispatcher and ready the way for
later adding more threads.
cherry-pick from 0.8 80caf07e09
Conflicts:
server/reds.c
When running some xinerama tests, I got several
glz_usr_free_image: error
messages. Looking at the code, this error is reported when this
function is called from a different DisplayChannelClient than the
one which created the glz compressed image.
When this happens, the backtrace is
at glz_encoder_dictionary.c:362
0x7fff940b6670) at glz_encoder_dictionary.c:449
image_type=LZ_IMAGE_TYPE_RGB32, image_width=512, image_height=256, image_stride=2048, first_lines=0x0,
num_first_lines=0, usr_image_context=0x7fff7420da40, image_head_dist=0x7fff9b2a3194)
at glz_encoder_dictionary.c:570
top_down=4, lines=0x0, num_lines=0, stride=2048, io_ptr=0x7fff740ea7c0 " ZL", num_io_bytes=65536, usr_context=
0x7fff7420da40, o_enc_dict_context=0x7fff7420da60) at glz_encoder.c:255
drawable=0x7fff9b46bc08, o_comp_data=0x7fff9b2a3350) at red_worker.c:5753
0x7fff9b46bc08, can_lossy=0, o_comp_data=0x7fff9b2a3350) at red_worker.c:6211
0x7fff9b46bc08, can_lossy=0) at red_worker.c:6344
0x7fff74085c50, dpi=0x7fff7445b890, src_allowed_lossy=0) at red_worker.c:7046
0x7fff7445b890) at red_worker.c:7720
at red_worker.c:7964
at red_worker.c:8431
Since the glz dictionary is shared between all the
DisplayChannelClient instances that belong to the same client, it can
happen that the glz dictionary code decides to free an image from one
thread while it was added from another thread (thread ==
DisplayChannelClient), so the error message that is printed is not an
actual error. This commit removes this message and adds a comment
explaining what's going on.
Several functions in server/ were not specifying an argument list,
ie they were declared as void foo(); When compiling with
-Wstrict-prototypes, this leads to:
test_playback.c:93:5: erreur: function declaration isn’t a prototype
[-Werror=strict-prototypes]
red_display_marshall_stream_start initializes a
SpiceMsgDisplayStreamCreate structure before marshalling it and
sending it on the wire. However, it never fills
SpiceMsgDisplayStreamCreate::stamp which then causes a complaint
from valgrind. This patch sets this value to 0, it's not used
by the client so the value shouldn't matter.
create_test_primary_surface::test_display_base.c creates a
QXLDevSurfaceCreate structure and initialize it, but doesn't set
the position field. Moreover, this structure has 4 bytes of padding
to the end (as shown by pahole from dwarves), so initialize the whole
structure to 0 before using it.
Issue found by the Coverity scanner.
HDG: Fixup don't free RGB24_line if it was not allocated by do_jpeg_encode
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
While discussing various things with Alon in Vancouver, it came up that
having a channel which simply passes through data coming out of a qemu
chardev frontend unmodified, like the usbredir channel does, can be used
for a lot of other cases too. To facilitate this the usbredir channel code
will be turned into a generic spicevmc channel, which is just a passthrough
to the client, from the spicevmc chardev.
This patch renames usbredir.c to spicevmc.c and changes the prefix of all
functions / structs to match. This should make clear that the code is not
usbredir specific.
Some examples of why having a generic spicevmc pass through is good:
1) We could add a monitor channel, allowing access to the qemu monitor from
the spice client, since the monitor is a chardev frontend we could re-use
the generic spicevmc channel server code, so all that is needed to add this
(server side) would be reserving a new channel id for this.
2) We could allow users to come up with new channels of their own, without
requiring qemu or server modification. The idea is to allow doing something
like this on the qemu startup cmdline:
-chardev spicevmc,name=generic,channelid=128
To ensure these new "generic" channels cannot conflict with newly added
official types, they must start at the SPICE_CHANNEL_USER_DEFINED_START value
(128).
These new user defined channels could then either be used with a special
modified client, with client plugins (if we add support for those), or
by exporting them on the client side for use by an external ap, see below.
3) We could also add support to the client to make user-defined channels
end in a unix socket / pipe, allowing handling of the data by an external app,
we could for example have a new spice client cmdline argument like this:
--custom-channel unixsocket=/tmp/mysocket,channelid=128
This would allow for something like:
$random app on guest -> virtio-serial -> spicevmc chardev ->
-> spicevmc channel -> unix socket -> $random app on client
4) On hind sight this could also have been used for the smartcard stuff,
with a 1 channel / reader model, rather then the current multiplexing code
where we've our own multiplexing protocol wrapper over the libcacard
smartcard protocol.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Now that the Channel struct is gone and the RedChannel has the same lifetime
as the chardev interface there is no need to have these 2 separate.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Fixes a valgrind discovered possible bug in spice-server - valgrind on
test_playback saw it, didn't see it happen with qemu.
The problem is that the frames buffers returned by spice_server_playback_get_buffer
are part of the malloc'ed SndChannel, whose lifetime is smaller then that of SndWorker.
As a result a pointer to a previously returned spice_server_playback_get_buffer could
remain and be used after SndChannel has been freed because the client disconnected.
Merging the functionality of reds::channel, into RedChannel.
In addition, cleanup and fix disconnection code: before this patch,
red_dispatcher_disconnect_display_client
could have been called from the red_worker thread
(and it must be called only from the io thread).
RedChannel holds only connected channel clients. RedClient holds all the
channel clients that were created till it is destroyed
(and then it destroys them as well).
Note: snd_channel still doesn't use red_channel, however it
creates dummy channel and channel clients, in order to register itself
in reds.
server/red_channel.c: a channel is connected if it holds at least one channel client
Previously I changed RedChannel to hold only connected channel clients and
RedClient, to hold all the channel clients as long as it is not destroyed.
usbredir: multichannel has not been tested, it just compiles.
client_cbs are supposed to be called from client context (reds). This patch will be used
in future patches for relacing reds::Channel with RedChannel in order to eliminate redundancy.
introduces ref_red_drawable and put_red_drawable (rename from free_red_drawable)
RedDrawable is already references by Drawable and RedGlzDrawable, with
a hack to NULL the drawable field in RedGlzDrawable to indicate RedGlzDrawable
is the last reference holder. Using an explicit reference count instead.
CursorPipeItems and their corresponding cursor_item were not
freed when they were removed from the pipe without sending them.
In addition cursor_channel_hold_pipe_item used wrong conversion
to (CursorItem*) for a (CursorPipeItem*).
Required to support multiple clients.
Also changes somewhat the way we produce PIPE_ITEM_TYPE_LOCAL_CURSOR. Btw,
I haven't managed to see when we actually produce such an item during my
tests.
Previously we had a single pipe item per CursorItem, this is impossible
with two pipes, which happens when we have two clients.
Also adds Drawable pipes and glz rings.
main_channel and red_worker had several locations that still accessed rcc
directly, so they had to be touched too, but the changes are minimal.
Most changes are in red_channel: drop the single client reference in RedChannel
and add a ring of channels.
Things missing / not done right in red_worker:
* StreamAgents are in DCC - right/wrong?
* GlzDict is multiplied - multiple compressions.
We still are missing:
* remove the disconnect calls on new connections
There is no inter-client shared dictionary and cache yet.
At this point the display channel can be used by multiple clients.
You can still crash on lack of Drawables or CursorItems due to the slower
clients pipe growing uncontrollably.
This patch compiles but breaks spice.
Split both display and cursor channels to a client part and channel part.
Introduce DisplayChannelClient, CursorChannelClient, CommonChannelClient.
don't disconnect channel on client disconnect.
Move all caches to the ChannelClient's.
Remove reference counting of the channel.
No new functionality introduced.
NOTE: Introduces a crash in disconnections, a regression, resulting from
incorrect thread access, that is fixed in the patch titled:
"server: registering RedChannel in reds, instead of Channel"
each client supplying a smartcard channel gets it's own smartcard. If
there are not enough smartcards provided by the server (read: qemu)
then it will be as though there are none.
currently disabled - later patches that enable smartcard don't make
this channel available to any but the first client.
s/TunnelChannel/TunnelChannelClient/
That's about it. this is probably the wrong way to do it. Not tested
at all. What do we want, a separate interface per client? same interface
for all clients? probably the later. This doesn't do that. Not tested,
so probably doesn't even work.
changes red_channel_pipe_item_is_linked to red_channel_client_pipe_item_is_linked,
since tunnel channel is the only user, must be done in patch to not break compilation.
The main channel deals with connecting new clients, announcing mouse mode
changes, and the agent channel. The implementation is currently done without
any changes to the protocol, so everything has to be either broadcast or
to a specific client.
channels list - specific client
mouse mode - broadcast
agent - broadcast
notify - broadcast (should have two modes, and use the appropriate)
Notable TODOs:
* migration testing
* agent tokens are wrongly sent (or did I fix that? check)
Introduce functions to add (via producer method) the same item to multiple
pipes, all for the same channel.
Note: Right now there is only a single channel, but the next patches will do the
per-channel breakdown to channel and channel_client before actually introducing
a ring in RedChannel, this makes it easier to make smaller changes - the
channel->rcc link will exist until removed in the ring introducing patch.
on red_channel_peer_on_incoming_error, if we are already shutdown, do not
call the channel's error handler. Since the channel has been shutdown, we
assume this is a second or later error, and handling has already occured.
Expose additional api to find a client given a connection_id. The connection_id
is first set when the first channel connects, which is the main channel.
It could also be kept in the RedClient instead, not sure.
TODO:
multiple todo's added for multiclient handling. I don't remember why
I wrote them exactly, and besides if I did any migration tests. So: TODO.
cleanup only. Note that the ping function is half used since the opt parameter
stopped being called with anything but NULL, should be returned at some point,
specifically when we drop the 250kbyte ping on start and do a continuous check
for latency and bandwidth.
See:
81945d897 - server: add new vd interface QTerm2Interface, Yaniv Kamay
introducing the usage of ping with a non NULL opt
3f7ea8e7a - zap qterm interfaces, Gerd Hoffman
removing it
They were globals before. This introduces api for other channels
to query the low bandwidth status. The queries themselves are still done
from the wrong context (channel and not channel client) but that's because
the decoupling of channel and channel client will be done in the following
patches.
Note that snd_worker.c got two copied function declarations that belong to
main_channel.h but can't be easily dragged into snd_worker.c since it still
uses it's own RedChannel struct.
Secondary channels are those that don't support multiple clients. The
support added in this patch just doesn't let the second or more connected
client receive the unsupported channels in the channels list sent by the
server to the client. This doesn't handle the situation where:
client A connects (gets all channels)
client B connects (gets supported multiple client channels)
client A disconnects (Suboptimal 1: B doesn't get new channels at this point)
client C connects (Suboptimal 2: C doesn't get the full list of channels, but
the partial one)
Specifically the channels that only support a single client are:
sound (both playback and record channels)
smartcard
tunnel
That means RedClient tracks a ring of channels. Right now there will be only
a single client because of the disconnection mechanism - whenever a new
client comes we disconnect all existing clients. But this patch adds already
a ring of clients to reds.c (stored in RedServer).
There is a known problem handling many connections and disconnections at the
same time, trigerrable easily by the following script:
export NEW_DISPLAY=:3.0
Xephyr $NEW_DISPLAY -noreset &
for ((i = 0 ; i < 5; ++i)); do
for ((j = 0 ; j < 10; ++j)); do
DISPLAY=$NEW_DISPLAY c_win7x86_qxl_tests &
done
sleep 2;
done
I fixed a few of the problems resulting from this in the same patch. This
required already introducing a few other changes:
* make sure all removal of channels happens in the main thread, for that
two additional dispatcher calls are added to remove a specific channel
client (RED_WORKER_MESSAGE_CURSOR_DISCONNECT_CLIENT and
RED_WORKER_MESSAGE_DISPLAY_DISCONNECT_CLIENT).
* change some asserts in input channel.
* make main channel disconnect not recursive
* introduce disconnect call back to red_channel_create_parser
The remaining abort is from a double free in the main channel, still can't
find it (doesn't happen when running under valgrind - probably due to the
slowness resulting from that), but is easy to see when running under gdb.
use MainChannel* instead of Channel* for a many functions in main_channel.h
(affects main_channel.c and reds.c).
some one liner fixes are hidden in here too.
Another cleanup patch, no change to behavior (still one client, and it
disconnects previous client if any).
The implementation for multiple client is straightforward: the pipe
remains per (channel,client) pair, so it needs to move from the RedChannel
that to RedChannelClient. Implementation using a single pipe with multiple
consumers (to reflect different latencies) doesn't fit well with pipe rewriting
that is used by the display channel. Additionally this approach is much simpler
to verify. Lastly it doesn't add considerable overhead (but see the display
channel changes in a later patch for a real place to rethink).
This patch is just technical, changing signatures to reflect the first
argument (oop style) so red_channel becomes red_channel_client. Some places
may seem odd but they should be fixed with later comits where the channels
grow to support multiple clients.
Sound (playback/record) channels are the only ones not touched - this is
consistent with previous patches, since they have been left out of the
RedChannel refactoring. That is left as future work. (note that they don't use
a pipe, which was the reason for not refactoring).
This commit adds a RedChannelClient that now owns the stream connection,
but still doesn't own the pipe. There is only a single RCC per RC
right now (and RC still means RedChannel, RedClient will be introduced
later). All internal api changes are in server/red_channel.h, hence
the need to update all channels. red_worker.c is affected the most because
it makes use of direct access to some of RedChannel still.
API changes:
1. red_channel_client_create added.
rec_channel_create -> (red_channel_create, red_channel_client_create)
2. two way connection: rcc->channel, channel->rcc (later channel will
hold a list, and there will be a RedClient to hold the list of channels
per client)
3. seperation of channel disconnect and channel_client_disconnect
TODO:
usbredir added untested.
The only difference between them being that the later also does a push.
I don't believe that to be a problem, but if it does I can always introduce
a push'less version.
rename types - we use _proc suffix mostly to indicate function pointer types,
use it for some function pointers that were missing it.
s/channel_handle_migrate_flush_mark/channel_handle_migrate_flush_mark_proc/
s/channel_handle_migrate_data_get_serial/channel_handle_migrate_data_get_serial_proc/
s/channel_handle_migrate_data/channel_handle_migrate_data_proc/
and spice_server_playback_put_samples. The former retrieves a buffer from a free
list with spice_server_playback_get_buffer, and should be used once via
spice_server_playback_put_samples. The tester previously reused the same buffer
a number of times.
After the changes to add libjpeg-turbo support to spice-server mjpeg
compression code, it's relatively easy to hit an assertion from
libjpeg in spice-server about "too few scanlines transferred" when
the mjpeg streaming code triggers. This assertion brings down qemu,
which is bad :)
This is because when we first initialize the mjpeg encoder, we do:
stream_width = SPICE_ALIGN(src_rect->right - src_rect->left, 2);
stream_height = SPICE_ALIGN(src_rect->bottom - src_rect->top, 2);
stream->mjpeg_encoder = mjpeg_encoder_new(stream_width, stream_height);
and then when we iterate over the image scanlines to feed them to
libjpeg, we do:
const int image_height = src->bottom - src->top;
const int image_width = src->right - src->left;
for (i = 0; i < image_height; i++) {
mjpeg_encoder_encode_scanline(...);
}
mjpeg_encoder_end_frame(...);
When stream_height is odd, the mjpeg_encoder will be created with
an height that is 1 more than the number of lines we encode. Then
libjpeg asserts when we tell it we're done with the compression
while it's still waiting for one more scanline.
Looking through git history, this rounding seems to be an artifact
from when we were using ffmpeg for the mjpeg encoding. Since
spicec and spicy (the latter needs a few fixes) can handle streams
with odd height/width, the best way to solve this issue is to stop
rounding up the height and width of the streams we create. This
even saves some bandwidth :)
when changing resolutions due to the new async code paths the surface
creation command was kept by reference, and later, when the red_worker
signaled completion by calling async_complete the mouse mode was updated
using the reference. This caused the wrong values to be read resulting in wrong
resolutions set and a non working mouse pointer. Fix this by keeping a copy of
the surface creation command instead of a reference.
No bz. Found in testing.
It is a bit early to bump, since a 0.9.1 release is not happening yet,
but this allows me to test if the vdagent SpiceCharInterface state callback
fixes are present or not in qemu code, and thus disabling the ugly vdagent
specific workaround from spice-qemu-char.c when compiling against a new
enough spice-server.
It was sending the wrong data, the memory right after the VCSMsgHeader
which was actually not where the data was.
Fixed by having the header and data (VSCError, 4 bytes of the error code)
embedded in the ErrorItem pipe item.
It's not used when we use jpeg-turbo colorspaces, so it's better
to allocate it when we know we'll need it rather than always
allocating it even if it won't be used.
After the refactoring to optionally use libjpeg-turbo, some
of the functions that mjpeg-encoder used to provide are now no
longer used. This commit removes them.
When libjpeg-turbo is available, we can use the BGR and BGRX
colorspaces that it provides to avoid extra conversions of the
data we want to compress to mjpeg
The main point is to move the pixel conversion code into
the MjpegEncoder class to be able to make use libjpeg-turbo
additional pixel formats without the reds_worker code noticing.
This API is meant to allow us to move the pixel format conversion
into MjpegEncoder. This will allow us to be able to use the
additional pixel formats from libjpeg-turbo when available.
It takes a lot of arguments, "id" is unused, "frame" and
"frame_size" can be obtained from the "stream" argument, so
can get rid of 3 arguments to make things more readable.
When encoding a frame, red_worker passes an allocated buffer to
libjpeg where it should encode the frame. When it fails, a new
bigger buffer is allocated and the encoding is restarted from
scratch. However, it's possible to use libjpeg to realloc this
buffer if it gets too small during the encoding process. Make use
of this feature, especially since it will make it easier to encore
one line at a time instead of a full frame in subsequent commits.
When encoding to mjpeg, the on screen data have to be converted
to 24bpp RGB since that's the format that libjpeg expects. Factor
as much code as possible for the 3 formats we handle.
The check this patch removes causes us to not set vdagent to NULL, nor
update the mouse mode when the guest agent disconnects when no client is
attached. Which leads to a non working mouse, and on agent reconnect a
"spice_server_char_device_add_interface: vdagent already attached" message
instead of a successful re-add of the agent interface .
hansg: Note this is commit 443994ba from the 0.8 branch, which I did
not forward port back then because it seemed unnecessary on master, but it
turns out that the (wrong) check was just hidden in another place on master.
This does the following, all to remove any referenced memory on the pci bars:
flush_all_qxl_commands(worker);
flush_all_surfaces(worker);
red_wait_outgoing_item((RedChannel *)worker->display_channel);
red_wait_outgoing_item((RedChannel *)worker->cursor_channel);
The added api is specifically async, i.e. it calls async_complete
when done.
The new _ASYNC io's in qxl_dev listed at the end get six new api
functions, and an additional callback function "async_complete". When
the async version of a specific io is used, completion is notified by
calling async_complete, and no READY message is written or expected by
the dispatcher.
update_area has been changed to push QXLRects to the worker thread, where
the conversion to SpiceRect takes place.
A cookie has been added to each async call to QXLWorker, and is passed back via
async_complete.
Added api:
QXLWorker:
update_area_async
add_memslot_async
destroy_surfaces_async
destroy_primary_surface_async
create_primary_surface_async
destroy_surface_wait_async
QXLInterface:
async_complete
For each callback in QXLWorker, for example QXLWorker::update_area, add
a direct call named spice_qxl_update_area.
This will (a) remove the pointless indirection and (b) make shared
library versioning alot easier as we'll get new linker symbols which
we can tag with the version they appeared in the shared library.
This patch adds symbol versions to the spice server library. Each
symbol which is exported by libspice-server gets tagged with the
(stable) version where it appeared first. This way the linker and rpm
are able to figure which version of the spice-server libary is required
by a particular qemu binary/package.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
When qemu creates a channel, reds.c contains code to check the
minor/major channel versions known to QEMU (ie the ones that were
current in spice-server when QEMU was compiled) and to compare these
versions against the current ones the currently installed spice-server
version.
According to kraxel [1], the rules for these interface numbers are:
"The purpose of the versions is exactly to avoid the need for a new
soname. The rules are basically:
(1) You add stuff to the interface, strictly append-only to not break
binary compatibility.
(2) You bump the minor version of the interface.
(3) You check the minor version at runtime to figure whenever the
added fields contain valid stuff or not.
An example is here (core interface, minor goes from 2 to 3, new
channel_event callback):
http://cgit.freedesktop.org/spice/spice/commit/?id=97f33fa86aa6edd25111b173dc0d9599ac29f879
"
The code currently refuses to create a channel if QEMU minor version is
less than the current spice-server version. This does not correspond
to the intended behaviour, this patch changes to fail is qemu was compiled
with a spice-server that is *newer* than the one currently installed. This
case is something we cannot support nicely.
[1] http://lists.freedesktop.org/archives/spice-devel/2011-July/004440.html
red_handle_drawable_surfaces_client_synced was called only from red_pipe_add_drawable, while it
should also be called from red_pipe_add_drawable_after. Otherwise, the client
might receive a command with a reference to a surface it doesn't hold and crash.
red_pipe_add_drawable can lead to removal of drawables from current tree
(since it calls red_handle_drawable_surfaces_client_synced), which can
also lead to releasing these drawables.
Before the fix, red_current_add_equal, called red_pipe_add_drawable,
without assuring afterwards that the drawables it refers to are still alive or
still in the current tree.
qemu calls spice_server_migrate_switch even if it didn't do a
spice_server_migrate_info first. Fix the resulting error by not pushing
a switch host tag to the pipe in this case, and add a check anyway in the
marshalling code just in case.
fixes "display_channel_release_item: panic: invalid item type"
Before changing the red_worker to use the red_channel interface, there
was a devoted red_pipe_clear routine for the display channel and cursor channel.
However, clearing the pipe in red_channel, uses the release_item callback
the worker provided. This callback has handled only resources that need to be released
after the pipe item was enqueued from the pipe, and only for pipe items that were set in
red_channel_init_send_data.
This fix changes the display channel release_item callback to handle all types of
pipe items, and also handles differently pushed and non-pushed pipe items.
On migration, destroy_surfaces is called from qxl (qxl_hard_reset), before the device was loaded (on destination).
handle_dev_destroy_surfaces led to red_process_commands, which read the qxl command ring
(which appeared to be not empty), and then when processing the command
it accessed unmapped memory.
In addition (1) make handle_dev_destroy_surfaces call red_release_cursor
(2) call red_wait_outgoing_item(cursor_channel) only after adding msgs to pipe
When the worker was stoped, the cursor was copied from guest ram to the host ram,
and its corresponding qxl command was released.
This is unecessary, since the qxl ram still exists (we keep references
to the surfaces in the same manner).
It also led to BSOD on guest upon migration: the device tracks cursor set commands and it stores
a reference to the last one. Then, it replays it to the destination server when migrating to it.
However, the command the qxl replayed has already been released from the pci by the original
worker, upon STOP.
This reverts commit 456ff9f8d5.
That patch just disabled the smartcard channel completely because
the check was done *before* the initialization of the qemu smartcard
devices, not after.
According to spice.proto the smartcard channel can receive acks and any
other message defined in BaseChannel. While the spicec implementation didn't
send an ACK spice-gtk does, so handle it.
These messages allow the guest to send the audio device volume to the
client. It uses an arbitrary scale of 16bits, which works good enough
for now.
Save VolumeState in {Playback,Record}State, so that we can send the
current volume on channel connection.
Note about future improvements:
- add exact dB support
- add client to guest volume change
Updated since v2:
- bumped record and playback interface minor version to allow
conditional compilation
Updated since v1:
- sync record volume on connection too
spice_common.h provides an ASSERT macro, no need to duplicate it
in many places. For now client/debug.h keeps its own copy since
debug.h and spice_common.h have clashes on other macros which are
trickier to unify.
With -Wpointer-arith, gcc complains about void pointer arithmetic.
This is not a big deal with gcc, but could be with other compilers,
so it's better to cast to char */uint8_t * before doing the
arithmetic on such pointers.
When using config.h, it must be the very first include in all source
files since it contains #define that may change the compilation process
(eg libc structure layout changes when it's used to enable large file
support on 32 bit x86 archs). This commit adds it at the beginning
of all .c and .cpp files
spice client and spice server shares code from
common/{gdi,gl,sw}_canvas.[ch]. However, while most of the code is
shared, the server code wants a canvas compiled with
SW_CANVAS_IMAGE_CACHE defined while the client code wants a canvas
compiled with SW_CANVAS_CACHE.
The initial autotools refactoring didn't take that into account,
this is now fixed by this commit. After this commit, the canvas
files from common/ are no longer compiled as part of the
libspice-common.la convenience library. Instead, there are "proxy"
canvas source files in client/ and server/ which #include the
appropriate C files after defining the relevant #define for the
binary that is being built.
To prevent misuse of the canvas c files and headers in common/,
SPICE_CANVAS_INTERNAL must be set when including the canvas headers
from common/ or when building the c files from common/ otherwise
the build will error out.
spice Makefile.am setup is a bit confusing, with source file
names being listed several times in different Makefile.am
(generally, once in EXTRA_DIST and another time in another
Makefile.am in _SOURCES). The client binaries are built
by client/x11/Makefile.am, which means recursing into client,
then into x11 to finally build spicec. This Makefile.am is
also referencing files from common/ and client/, which is
a bit unusual with autotools.
This patch attempts to simplify the build process to get
something more usual from an autotools point of view.
The source from common/ are compiled into a libtool convenience
library, which the server and the client links against which avoids
referencing source files from common/ when building the server and
the client. The client is built in client/Makefile.am and directly
builds files from x11/ windows/ and gui/ if needed (without
recursing in these subdirectories).
This makes the build simpler to understand, and also makes it
possible to list source files once, which avoids potential
make distcheck breakage when adding new files.
There is a regression in this patch with respect to
sw_canvas/gl_canvas/gdi_canvas. They should be built with
different preprocessor #defines resulting in different behaviour
of the canvas for the client and the server. However, this is not
currently the case, both the client and the server will use the same
code for now (which probably means one of them is broken). This will
be fixed in a subsequent commit.
make distcheck passes, but compilation on windows using the
autotools build system hasn't been tested, which means it's likely
to be broken. It shouldn't be too hard ot fix it though, just let
me know of any issues with this.
When compiling spice with make CFLAGS="-g3 -ggdb3 -O0 -Wall -Werror",
the build broken because of a few unused variables/missing returns.
This patch fixes these warnings.
The check this patch removes causes us to not set vdagent to NULL, nor
update the mouse mode when the guest agent disconnects when no client is
attached. Which leads to a non working mouse, and on agent reconnect a
"spice_server_char_device_add_interface: vdagent already attached" message
instead of a successful re-add of the agent interface .
This ensures that if the client or agent connects to the client-agent
"tunnel" while the other side is halfway through sending a multi part
message, the rest of the message gets discarded, and the connecting
party starts getting data at the beginning of the next message.
The agent message filter keeps track of messages as they are being send
reset the relevant filter to its initial state when one of the 2 ends
of the agent<->client "tunnel" disconnects.
read_from_vdi_port calls dispatch_vdi_port data, which will disconnect
the guest agent if it sends invalid data. It would then try to read more
data from the disconnected guest agent resulting in a NULL ptr dereference,
this patch fixes this.
write_to_vdi_port() was checking for reds->agent_state.connected to determine
wether it could write queued data. But agent_state.connected reflects if
*both* ends are connected. If the client has disconnected, but the guest agent
is still connected and some data is still pending (like a final clipboard
release from the client), then this data should be written to the guest agent.
We were calling reds_reset_vdp on client disconnect, which is not a good
idea. reds_reset_vdp does 3 things:
1) It resets the state related to reading chunks from the spicevmc virtio
port. If the client disconnects while the guest agent is in the middle
of sending a chunk, this will lead to an inconsistent state, and lots
of printing of "dispatch_vdi_port_data: invalid port" messages caused
by this inconsistent state sometimes followed by a segfault.
This can be triggered by copy and pasting something large (say
a screenshot) from the guest to the spice-gtk client, as the spice-gtk
client currently has a bug causing it to crash when receiving a multi
chunk vdagent messages. Without this patch (and with the spice-gtk bug
present) I can consistently reproduce this.
2) It clears any buffered writes from the client to the guest still pending
because the virtio port cannot consume data fast enough. Since the agent
itself is still running fine, throwing away writes for it because the
client has disconnected makes no sense. Esp, since on clean exit the
client may very well send a clipboard release message directly
before closing the connection, and this may get lost this way.
3) It sets client_agent_started to false, this is the only thing which
actually makes sense to do on client disconnect.
Note that since we no longer reset the vdp state on client disconnect, we
must now reset it on agent disconnect even if we don't have a client. So
the reds_reset_vdp call in reds_agent_remove() gets moved to the top,
above both the agent_state.connected and reds->peer checks which will
both fail in the no client case.
dispatch_vdi_port_data, was calling vdi_read_buf_release when no client
is connected to free the passed in buf. The only difference between
vdi_read_buf_release and directly adding the buffer back to the ring
with ring_add, is that vdi_read_buf_release calls read_from_vdi_port
after adding the buffer back. But dispatch_vdi_port_data only gets called
from read_from_vdi_port itself, thus this would lead to recursing into
read_from_vdi_port. read_from_vdi_port is protected against recursion and
will immediately return if called recursively. Thus calling
vdi_read_buf_release from dispatch_vdi_port_data is pointless, instead
simply putting the buffer back in the ring suffices.
Also bump SPICE_SERVER_VERSION to 0x000801 as 0.8.1 will be the
first version with the new API for this, and we need to be able to
detect the presence of this API in qemu.
The current assert(reds->agent_state.connected) tiggers if when
the agent disconnected there was still data waiting to be sent (for
instance if there is a bug in the client handling clipboard and it
is killed while a large clipboard transfer is in progress). So first
call to reds_agent_remove happens from spice_server_char_device_remove_interface,
and then it is called again (triggering the assert) from free_item_data
from read_from_vdi_port because of the channel destruction.
Other option would be to not call it from one of the paths - but that
is suboptimal:
* if there is no data in the pipe, the second call never happens.
* the second call has to be there anyway, because it may fail during
parsing data from the agent.
This patch fixes a segfault on this assert when a client starts passing
from guest agent to client a large clipboard and dies in the middle. There
is still another assert happening occasionally at marshaller which I don't
have a fix for (but it doesn't seem to be related).
Current master is calling red_channel_destroy() on incoming error, but
reds Channels still references it, which causes a double free() later
on (see valgrind report below).
Instead, on error condition, do like the rest of the channels and call
reds_disconnect(), which remove the references and call shutdown(),
which then call red_channel_destroy() and finally free the channel
with red_channel_destroy().
Note: the previous code intention was certainly to be able to keep the
rest of the channels connected when input channel has errors. This is
not addressed by this patch.
red_channel_shutdown:
==29792== Invalid read of size 8
==29792== at 0x4C6F063: red_channel_shutdown (red_channel.c:460)
==29792== by 0x4C51EFA: inputs_shutdown (inputs_channel.c:463)
==29792== by 0x4C48445: reds_shatdown_channels (reds.c:539)
==29792== by 0x4C4868A: reds_disconnect (reds.c:603)
==29792== by 0x4C519E9: main_channel_on_error (main_channel.c:765)
==29792== by 0x4C6E80A: red_channel_peer_on_incoming_error (red_channel.c:215)
==29792== by 0x4C6E22D: red_peer_handle_incoming (red_channel.c:87)
==29792== by 0x4C6E551: red_channel_receive (red_channel.c:154)
==29792== by 0x4C6F329: red_channel_event (red_channel.c:531)
==29792== by 0x41CB8C: main_loop_wait (vl.c:1365)
==29792== by 0x437CDE: kvm_main_loop (qemu-kvm.c:1589)
==29792== by 0x41FE9A: main (vl.c:1411)
==29792== Address 0x30b0f6d0 is 0 bytes inside a block of size 28,648 free'd
==29792== at 0x4A05372: free (vg_replace_malloc.c:366)
==29792== by 0x4C6F032: red_channel_destroy (red_channel.c:454)
==29792== by 0x4C6E80A: red_channel_peer_on_incoming_error (red_channel.c:215)
==29792== by 0x4C6E22D: red_peer_handle_incoming (red_channel.c:87)
==29792== by 0x4C6E551: red_channel_receive (red_channel.c:154)
==29792== by 0x4C6F329: red_channel_event (red_channel.c:531)
==29792== by 0x41CB8C: main_loop_wait (vl.c:1365)
==29792== by 0x437CDE: kvm_main_loop (qemu-kvm.c:1589)
==29792== by 0x41FE9A: main (vl.c:1411)
https://bugs.freedesktop.org/show_bug.cgi?id=34971
This reverts commit 5062433d8a.
red_channel_receive() can call red_channel_destroy() which frees
channel.
The condition bellow is then checked, which can access a freed
channel:
if (event & SPICE_WATCH_EVENT_WRITE || channel->send_data.blocked)
Reverting this commit solves the issue without any apparent
bugs/drawbacks, which kind of clears out the weird TODO.
handle_dev_input: cursor connect
==11826== Invalid read of size 4
==11826== at 0x4C6F83C: red_channel_event (red_channel.c:535)
==11826== by 0x41CB8C: main_loop_wait (vl.c:1365)
==11826== by 0x437CDE: kvm_main_loop (qemu-kvm.c:1589)
==11826== by 0x41FE9A: main (vl.c:1411)
==11826== Address 0x31fb00f0 is 96 bytes inside a block of size 28,648 free'd
==11826== at 0x4A05372: free (vg_replace_malloc.c:366)
==11826== by 0x4C6F536: red_channel_destroy (red_channel.c:453)
==11826== by 0x4C52B5D: inputs_channel_on_incoming_error (inputs_channel.c:449)
==11826== by 0x4C6ED0E: red_channel_peer_on_incoming_error (red_channel.c:215)
==11826== by 0x4C6E731: red_peer_handle_incoming (red_channel.c:87)
==11826== by 0x4C6EA55: red_channel_receive (red_channel.c:154)
==11826== by 0x4C6F82D: red_channel_event (red_channel.c:530)
==11826== by 0x41CB8C: main_loop_wait (vl.c:1365)
==11826== by 0x437CDE: kvm_main_loop (qemu-kvm.c:1589)
==11826== by 0x41FE9A: main (vl.c:1411)
==11826==
https://bugs.freedesktop.org/show_bug.cgi?id=34971
This allows later to have the callback table under RedChannel when
the callbacks actually get used by RedChannelClient. Since the cb's
are identical for different clients of the same channel it makes sense
to store the callback pointers in one place per channel. The rest of
the incoming and outgoing struct just gets moved to RedChannelClient.
Changes in display channel for a code size win.
A note about this and the previous cursor change: it will appear that we are
now (with these changes) releasing resources too early. This is not so - send
always has the option of blocking, which means after send you can not release
resources anyway, that's what the release_item callback is for. So both the
code before and now are doing the same accounting.
Use in main_channel. This is just for backward portability later
when multiple clients are introduced - needs to be considered (which
sockets do we want to export from libspiceserver?)
Introduce SpiceMarshaller param to all send's that do add_buf
Next patch will use marshaller in all functions that currently don't by
replacing red_channel_add_buf with marshaller add_ref. Note - currently
tunnel is broken due to wrong size in messages.
move all the ASSERT/PANIC/PANIC_ON/red_error/red_printf* macros
to a common file to be used with ring.h that is going to be used externally
(by spice-gtk).
Handling done in red_channel instead of per channel, using call backs
for the channel specific part.
Intended to reduce furthur reliance of channels on RedChannel struct.
The commit makes the code harder to understand because of the artificial
get_serial stuff, should later be fixed by having a joint migration
header with the serial (since all channels pass it).
For ussage in the send_item callback. It's only valid during this
time anyway (should make it return NULL in other occasions?)
No more direct usage of RedChannel.send_data.marshaller by channels.
The renames are part of refactoring red_worker's RedChannel to reuse
red_channel.h's RedChannel at the end.
s/red_send_data/red_channel_send/
s/red_pipe_get/red_channel_pipe_get/
s/recive_data/incoming/
s/red_receive/red_channel_receive/
s/channel_handle_message/red_channel_handle_message/
s/channel_is_connected/red_channel_is_connected/
s/red_pipe_add_type/red_channel_pipe_add_type/
We introduce 2 public functions to integrate with the library user.
spice_server_set_sasl() - turn on SASL
spice_server_set_sasl_appname() - specify the name of the app (It is
used for where to find the default configuration file)
The patch for QEMU is on its way.
https://bugs.freedesktop.org/show_bug.cgi?id=34795
Try to have a common base dispose() method for channels. For now, it
just free the caps.
Make use of it in snd_worker, and in sync_write() - sync_write() is
going to have default caps later on.
https://bugs.freedesktop.org/show_bug.cgi?id=34795
This is stylish change again. We are talking about a RedStream object,
so let's just name the variable "stream" everywhere, to avoid
confusion with a non existent RedPeer object.
https://bugs.freedesktop.org/show_bug.cgi?id=34795
This patch make it easier to spot warnings in compilation. It should
work with older versions of automake that don't support silent rules.
If you want verbose build, make V=1.
Signed-off-by: Uri Lublin <uril@redhat.com>
https://bugs.freedesktop.org/show_bug.cgi?id=34795
s/disconnect_channel_proc/channel_disconnect_proc/
s/release_item_proc/channel_release_pipe_item_proc/
s/handle_message_proc/channel_handle_parsed_proc/
Adds RedChannel* channel as first parameter to hold_pipe_item_proc
with everything (almost) not in red_channel's RedChannel
As a result of CommonChannel a free cb is added to EventHandler,
to take care of non zero offset for embedded EventHandler.
The device already sends one. There are actually two connections going
on:
server to client - this is the smartcard channel, it reuses the VSC protocol.
server to device - this is an internal connection using VSC too.
We generally just passthrough all messages from the client to the device,
and from the device to the client. We only rewrite the reader_id because
the device knows about a single id (it is actually a card id), and we
may manage more then one in the future.
Bottom line is that there was an extra VSC_Error message reaching the client.
try to push either on signal (write available) or when blocked
and read signaled. From red_worker, copied for compatibility when
switching later to RedChannel in red_worker. Doesn't make a lot of
sense (but works), see comment in patch.
MAX_SEND_VEC was 100 for DisplayChannel's RedChannel implementation which is being replaced
with RedChannel in red_channel. So changing from 50 to 100 in red_channel
(make this configurble?) - effectively increased memory usage by:
(100-50)*sizeof(iovec)*(num_of_channels-2) ==(arch 64bit) 50*16*6 ~ 5k
Not terrible.
hold_item called on init_send_data, matching release.
This is not the behavior of red_worker - we ref++ (==hold_item) when
sending the item, and --refs when releasing it, instead of only holding
if the send is blocked.
Note 1: Naming: hold_pipe_item is the proc name, the variable is called
hold_item, this is similar to release_item/release_pipe_item naming.
Note 2: All channels have empty implementation, we later use this when
red_worker get's RedChannelized.
While we are at it: There is no reason for chardev
support to stay in the experimental area, so move it out.
qemu should not need the "spice-experimental.h" file.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Implement server-side support for switch-host client migration. Client
side support is present already in the tree.
Setting the migration information is done using the existing
spice_server_migrate_info() function. A new
spice_server_migrate_switch() function has been added which triggers
sending out the switch-host message.
Seamless migration functions are left there for now.
spice_server_migrate_start() has been chamnged to just fail
unconditionally though as seamless migration is broken anyway.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
drawable_count was becoming negative. It tracks the number of
items in the worker->current_list ring. It was decremented correctly,
but incremented only in several cases. The cases it wasn't incremented
where:
red_current_add_equal found an equivalent drawable
by moving the increment to where the item is added to current_list, in
__current_add_drawable, the accounting remains correct.
This has no affect other then correct accounting, as drawable_count isn't
used for anything.
We should pass up errors instead of aborting. Do that at least
for bind() failures which actually happen in real live due to the
tcp port being busy.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
We erronously ignored data from guest on the serial channel if no client is
connected. This leads to an assert when the guest writes a second time, since
there is still data unconsumed by us (the host).
Fix by reading data anyway, and discarding it after parsing (and reading) whole
messages from the guest.
Net affect is that any messages the agent sends while no client is connected
get discarded, but only full messages are discarded.
This fixes an abort if booting a winxp guest with vdagent without a connected
client.
updates taken from spice vga mode updates, i.e. non cacheable, glz compressed
(depends on whatever settings you apply to the server) opaque draw operations.
+ completed the SpiceCoreInterface implementation (timers)
v1->v2:
removed test_util.c (Hans)
replaced mallocz with calloc (Hans)
We are keeping track of tokens for sending agent data to the client, but
the client send an initial value of ~0, and never gives us new send tokens
so this is all rather useless -> remove it.
Note that it is kept in the migration data struct for compatibility reasons.
read_from_vdi_port(), called from vdagent_char_device_wakeup() may
fail to consume all data because no buffers are available in the
read_bufs ring. When this happens we would fail to ever read more data
from the agent on the guest as the port is throttled and stays throttled
until we've consumed all data from the current buffer.
This patch re-enables the call to read_from_vdi_port() from
vdi_read_buf_release(), so that we will try the read again when space
becomes available in the read_bufs ring.
Together with another nasty hack in the linux guest virtio_console
driver, where it waits for a write to be acked by the host before
continuing with the next one, this can lead to a linux guest
getting stuck / hang (until the write is read by the spice-server
which never happens becaus of the above issues).
Note that even with this patch, the guest will still gets stuck due to
a bug in watch_update_mask in spice-core in qemu, which causes writing
to the client to never resume once it blocked. A patch for this has been
submitted to qemu.
read_from_vdi_port() MUST always be called in a while loop until it returns 0.
This is needed because it can cause new data available events and its
recursion protection causes those to get lost. Calling it until it returns 0
ensures that all data has been consumed.
Example scenario where we can fail to read the available data otherwise:
- server/reds.c: vdagent_char_device_wakeup get called
by hw/spice-vmc.c because data has arrived from the guest,
- hw/spice-vmc.c: vmc_read get calls
- If the vmc_read call depletes the current buffer it calls
virtio_serial_throttle_port(&svc->port, false)
- This causes vmc_have_data to get called, which if in the
mean time another buffer has arrived causes
vdagent_char_device_wakeup to gets called again
(so recursively)
- vdagent_char_device_wakeup is protected against recursive
calling and ignores the second call (a nasty hack)
- if no other data arrives, the arrived data will not get read
This patch adds a channel event callback to the spice core interface.
This new callback will be called for three events:
(1) A new connection has been established.
(2) The channel is ready (i.e. authentication is done,
link message verification passed all tests, channel
is ready to use).
(3) Channel was disconnected.
Qemu will use this to send notifications to the management app.
When drawing a drawable with a NULL src bitmap that means we should
be using the previously generated self_bitmap. Not doing this causes
a segfault due to accessing the NULL.
The self_bitmap is the size of self_bitmap_area, not the bbox.
This is especially important since we later copy the self_bitmap_area
into the new bitmap, and if that is larger than bbox then we will
overwrite random memory.
We really need to flush the ring to ensure that we push something on the
release ring. If we don't do this and the ring is not pushed for other
reasons we will timeout in the guest driver waiting for the ring.
We've changed how resources are released so they are now being
freed continuosly, rather than on OOM, since we want to free as early
possible to avoid fragmentation. So, OOM situations should be a bit
less common now and signify a real memory shortage, so we should try
to free up more resources.
When upgrading a cluster of machines you typically do this by
upgrading a set of machines at a time, making the new machines run
the new software version, but in a fashion compatible with the old
versions (in terms of e.g. migration). Then when all machines are
upgrades, any new features in the new version can be enabled.
This API allows qemu to limit the set of features that spice uses to
those compatible with an older version, in order to do an upgrade like
this. Right now it doesn't really do much, since we don't keep compat
with 0.4.0 atm (although that may be added later).
There is no guarantee that any future version of spice support
being compatible with any previous version. However, we will always
support compatibility with the previous major version so that clusters
can be upgraded step by step.
This will prevent: 1) rendering problems (commands sent to the client in the wrong order)
2) sending commands for surfaces that haven't been created yet on the client side.
This used to be a callback for the vdi_port "data ready" interrupt,
which did indicate either data ready to read or data ready to write, but
this is no longer the case now that virtio-serial is used.
This seemingly simple fix prevents a race that needs to be fixed with
another patch, see freedesktop bz #29903
The vdi_port_write_timer_started flag was not being reset, which prevented
another vdi_port_write_timer_start from actually starting the timer. Fix
is to change order of lines. This happens in the callback of the timer, so
no chance of double timer set.
A side effect of the previous red_current_flush, which flushed all the surfaces, and was called on a new display channel connection, was
that red_handle_drawable_surfaces_client_synced sent the most updated surfaces images when needed. However, now, it should
explicitly call red_current_flush.
Moreover, since red_current_flush was called on a new display channel connection only if there was a primary surface,
if the connection of the display channel occurred at the moment of no primary surface, red_handle_drawable_surfaces_client_synced was buggy.
Update #define in server/spice.h in preparation for the 0.6.0 release.
We also got some new functions, thus we have to increate the shared
lib minor number for spice-server.
The actual bitmap data was added to the main marshaller rather than
the submarshaller that pointed to the SpiceImage part. This made us
send too short messages failing demarshalling in the client.
When the we reset qxl, we destroy all srufaces. Since surfaces and glz
drawables are no longer dependent, we need to call red_display_clear_glz_drawables explicitly
in order to clear all our drawables references in the server.
Remove all uses of @end in the marshaller, instead just using
the C struct array-at-end-of-struct. To make this work we also remove
all use of @end for switches (making them C unions).
We drop the zero member of the notify message so that we can avoid this
use of @end for a primitive in the marshaller (plus its useless to send
over the wire).
We change the offsets and stuff in the migration messages to real pointers.
There is no need to check the pci ids or revisions. Thats a contract
between qemu and the driver, and spice need not care, as long as
we get the right data from qemu.
I still don't have commit access (can't ssh to anarchy) so if someone could commit this (alex) thanks,
Alon
Fix for no opengl patch - required to compile the server (fixes missing symbol gl_canvas_init).
Internally and in the network protocol (for the new version) we
now store the actual number of segments rather than the size of the
full segments array in bytes. This change consists of multiple changes
to handle this:
* Make the qxl parser calculate num_segments
* Make the canvas stroke code handle the new SpicePath layout.
* Fix up is_equal_path in red_worker.c for the new layout
* replace multiple calls to spice_marshall_PathSegment with a single
spice_marshall_Path call
* Make the byte_size() array size handling do the conversion from
network size to number of elements when marshalling/demarshalling.
* Update the current spice protocol to send the segment count rather than
the size
* Update the old spice protocol to use the new byte_size functionallity
to calculate the size sent and the number of elements recieved
red_parse_qxl.c starts to follow QXLPHYSICAL references and build up
data structures. Can zap a bunch of get_virt calls in red_worker.c,
followed by cleanups.
(de-) marshaller needs updates to deal with that. Also I suspect with
the get_virt() calls being gone we can offload more work to generated
marshaller code.
client doesn't build.
when SPICE_MSG_DISPLAY_RESET was sent, SPICE_MSG_DISPLAY_SURFACE_DESTROY had already
been sent for all surfaces.
It also caused a client crash since DisplayChannel::handle_reset assumes that screen
exists.
This is required because we don't want to free messages that just
refer to the unparsed message (like SpiceMsgData).
Also, in the future we might need it for more complex demarshalling.
We move all message structs from spice-protocol to spice as
we want to be able to change these as needed internally. The
on-network format is no longer defined by these structures anyway,
but rather by the spice protocol description.
When a surface is sent to the client using red_send_surface_image, operations were already
performed on it. Thus it may combine, especially if it is a primary surface, both "picture-like" areas
and areas that are more "artificial". In order to avoid noticeable artifacts, such surface will be sent lossless.
The code also handles cases in which the server doesn't hold anymore these surfaces parts, i.e., when
it holds a more updated version of them. This scenario is handled by replacing commands that were rendered, with images.
1) add an option to determine if a bitmap can be sent lossy to the client
2) when required, replacing lossy cache items with their correspending
lossless bitmaps
Supposed to be used for work-in-progress bits,
where interfaces are not finalized yet.
Moved over vdi port interface, tunnel interface
and spice client migration functions.
Add worker->loadvm_commands. qemu will uses this to send a series of
commands needed to restore state after savevm/loadvm and migration.
That will be one create-surface command per surface and one cursor-set
command for the local pointer.
The worker->save/load functions are not needed any more.
Likewise the interface->{get,set}_save_data callbacks.
Surfaces created via loadvm_commands *will* not be cleared. Also
primary surfaces are not cleared any more (unconditionally, although
we could do that conditionally on loadvm using the flags field in
QXLSurfaceCreate).
With this patch applied the spice server will not release surface create
commands for the whole lifecycle of the surface. When the surface is
destroyed both create and destroy commands are released.
This has the effect that the surface metadata (size, depth, ...) is kept
in qxl device memory. This in turn makes it alot easier for qemu to
handle savevm/loadvm. It just needs to do some minimal command parsing
and maintain pointers to the create commands for the active surfaces.
Pretty straight forward.
One thing we should think about is if and how we are going to deal
with multiple ports here?
With vdi port using virtio-serial as communication channel to the guest
it is easy to have multiple ports, i.e. we might want to use a second
instance for clipboard data. That implies that we need support for
multiple channels all the way through the stack ...
This is the direction I wanna take with all interfaces: Clearly
separate interface (aka version information and function pointers)
and state information. SpiceKbdInterface defines the interface,
SpiceKbdInstance maintains per-instance state information. Keyboard
hasn't much beside a pointer to SpiceKbdInterface, for other
interfaces this very likely will be different.
VDInterface has been renamed to SpiceBaseInterface. Dropped base_version
element, shlib versioning should be used instead. Dropped id element,
it is passed to spice_server_add_interface() instead. Now
SpiceBaseInterface has static information only, multiple interface
instances can share it.
Added SpiceBaseInstance struct for maintaining per-instance state
information. Adapted spice_server_{add,remove}_interface() functions
to the new world.
This patch adds a new file handle watch interface to libspice, featuring
three callbacks:
(1) watch_add() -- create a new file watch.
(2) watch_update_mask() -- change event mask. spice frequently
enables/disables write notification.
(3) watch_remove() -- remove a file watch.
libspice users must implement these functions to allow libspice
monitoring file handles.
The old interface (set_file_handlers) doesn't explicitly express the
lifecycle of a watch. Also it maps 1:1 to a qemu-internal function.
In case the way qemu implements file watches changes (someone sayed
QemuIONotifier?) this will break horribly. Beside that it is very
bad style.
Follwing patches will switch over users one by one to the new interface
and finally zap the old one.
Interfaces must be registered after spice_server_init().
The "next" callback is used to discover interfaces
registered before spice_server_init(). Which is a empty
list and thus pretty pointless. Remove it.
- drop spice_channel_name_t enum, use spice-protocol defines instead.
- switch spice_server_set_channel_security() channel parameter from
enum to string.
- drop spice_server_set_default_channel_security(), use
spice_server_set_channel_security with channel == NULL instead.
Adds sanity check to iovec setup. In theory this should never ever
trigger. In practice guest driver bugs can make it trigger. This
patch avoids qemu burning cpu in a endless loop, instead we'll print a
message and abort. Not sure whenever there is a more graceful way to
handle the situation ...
Surface creation now specifies the exact format, not only the bit depth
of each surface which is used for rendering.
Additionally we now actually store the surfaces in that format, instead
of converting everything to 32bpp when drawing or e.g. handling palettes.
We now support 16bit format pixmaps as well as the old ones. Including
both 555 and 565 modes.
We drop the palette argument for pixmap construction as it was only
used for black/white anyway.
Canvas creation is simplified so that there is no separate set_mode
state. Canvases are already created in the right mode and never change.
The new command return dirty area to be used
by users that want spice to render localy or
into some framebuffer (sdl / vnc)
Signed-off-by: Izik Eidus <ieidus@redhat.com>
The idea is that we can try to defer some stuff to be later
send in the pipe if the pipe is not fulled yet, moreover
we will then walk on the pipe using:
red_clear_surface_drawables_from_pipe() and will try to
remove the uneeded objects of this surface
Still some room to improvment but later...
Signed-off-by: Izik Eidus <ieidus@redhat.com>
Just skip the items of destroyed surface that are found in the pipe
before we skip them, we check if they are needed by other users...
Signed-off-by: Izik Eidus <ieidus@redhat.com>
Now we can send commands from the server to the client
to destroy surfaces (right now just the primary surface)
Needed for offscreens support)
Another patch`s on the way.
Signed-off-by: Izik Eidus <ieidus@redhat.com>
Also change include dir to "spice-server" for consistency.
libspice.so conflicted with the tclspice package, and its also
a clarification for when we create a spice client library.
In 'filter' video streaming mode, use a more permissive threshold for distinguishing
'realistic' streams from 'textaul'/'artificial' streams. The previous threshold classified
streams that were scaled on the guest as artificial and thus they were not recoginized as videos.
I added a lower limit to the size of images that are being streamed.
The limit is only active in "filter" video streaming mode.
This will prevent blurry animated icons.
Every place that does a regular malloc/calloc and aborts on failure
should use spice_malloc/spice_mallo0 instead, which is leaner and cleaner.
Allocations of dynamically sized arrays can use g_malloc_n or g_new etc
which correctly handle multiplication overflow if some of the arguments
are not trusted.
Add new functions to configure spice port and ticketing. Yes, this is
incomplete, it includes just the most important bits to get something
up'n'running.
These functions are supposed to replace both spice_parse_args() and
the monitor interaction via qterm interface.
The implementation can't handle multiple spice server instances at the
same time right now. The API allows this though, so if we fixup the
implementation some day we don't have to change the API.
quic_usr_more_lines_unstable() assumes it can allways copy a complete
scanline. Well, it can't. In case the screen rectangle which needs
updating has an x-offset greater than zero *and* includes the last
scanline of the screen it will overflow the source buffer by
x-offset * bytes-per-pixel bytes.
Instead of having two virtualizations of the canvas we push the
virtualization into the canvas code itself. This not only avoids
the duplication of this code, it also makes the exposed API for the
canvas much smaller (in terms of exported API).
It also lets us use the virtualization to implement basic support
for operations in canvas_base which is then overridden by each canvas
implementation.
pixman_region32_t is an efficient well tested region implementation (its
the one used in X) that we already depend on via pixman and use in
some places. No need to have a custom region implementation.
Instead of passing a bunch of function pointer and an opaque
pointer we make a real type and add a vtable pointer to it.
This means we can simplify all the canvas constructors, etc.
This includes:
* pixman region from SpiceRects
* rop2 enum
* solid fill
* solid fill with rop
* tiled fill
* tiled fill with rop
* blit
* blit with rop
* copy rect
3 available mechanisms: by public key, by host name, and by certificate subject name.
In the former method, chain of trust verification is not performed.
The CA certificate files are looked for under <spice-config-dir>/spice_truststore.pem
windows <spice-config-dir>=%APPDATA%\spicec\
linux <spice-config-dir>=$HOME/.spicec/
Increase client timeout in order to prevent unnecessary
disconnecting of client while the connection is over WAN.
Tested by changing WinXP resolution (with desktop background) while
connecting over WAN (1.5Mbit 150Kbit)
