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>
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
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>
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>
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>}
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>
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>
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>
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>
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.
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.
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
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
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.
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.
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.
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.
