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.
