rpms/spice
7
0
Fork 0
Code Issues Pull Requests Releases Activity

Add a number of misc. bug-fixes from upstream

Browse Source 
- Add a number of misc. bug-fixes from upstream
This commit is contained in:
Hans de Goede 2013-01-21 16:09:59 +01:00
parent c061c4d34c
commit 708002ccbc
7 changed files with 422 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
0001-server-guest_set_client_capabilities-protect-against.patch Normal file
View File

@ -0,0 +1,36 @@
From 1ff42341629948c591621f0a8ddf2859543ca05d Mon Sep 17 00:00:00 2001
From: Uri Lublin <uril@redhat.com>
Date: Mon, 17 Dec 2012 18:34:43 +0200
Subject: [PATCH spice 1/6] server: guest_set_client_capabilities: protect
against NULL worker->display_channel
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"
---
server/red_worker.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/server/red_worker.c b/server/red_worker.c
index e5e3d05..1a9c375 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10342,7 +10342,8 @@ static void guest_set_client_capabilities(RedWorker *worker)
worker->set_client_capabilities_pending = 1;
return;
}
- if (worker->display_channel->common.base.clients_num == 0) {
+ if ((worker->display_channel == NULL) ||
+ (worker->display_channel->common.base.clients_num == 0)) {
worker->qxl->st->qif->set_client_capabilities(worker->qxl, FALSE, caps);
} else {
// Take least common denominator
--
1.8.1

166
0002-red_worker.c-insert-a-drawable-to-its-position-in-th.patch Normal file
View File

@ -0,0 +1,166 @@
From b22e82ad578c835996e184cefe5a26a70378714a Mon Sep 17 00:00:00 2001
From: Yonit Halperin <yhalperi@redhat.com>
Date: Tue, 8 Jan 2013 10:47:53 -0500
Subject: [PATCH spice 2/6] red_worker.c: insert a drawable to its position in
the current tree before calling red_detach_streams_behind
resolves: rhbz#891326
Starting from commit 81fe00b08ad4f, 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).
---
server/red_worker.c | 55 ++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 42 insertions(+), 13 deletions(-)
diff --git a/server/red_worker.c b/server/red_worker.c
index 1a9c375..5e00cb6 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1022,6 +1022,8 @@ static void red_current_flush(RedWorker *worker, int surface_id);
#else
static void red_draw_drawable(RedWorker *worker, Drawable *item);
static void red_update_area(RedWorker *worker, const SpiceRect *area, int surface_id);
+static void red_update_area_till(RedWorker *worker, const SpiceRect *area, int surface_id,
+ Drawable *last);
#endif
static void red_release_cursor(RedWorker *worker, CursorItem *cursor);
static inline void release_drawable(RedWorker *worker, Drawable *item);
@@ -2615,7 +2617,9 @@ static int red_display_drawable_is_in_pipe(DisplayChannelClient *dcc, Drawable *
* after red_display_detach_stream_gracefully is called for all the display channel clients,
* red_detach_stream should be called. See comment (1).
*/
-static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dcc, Stream *stream)
+static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dcc,
+ Stream *stream,
+ Drawable *update_area_limit)
{
int stream_id = get_stream_id(dcc->common.worker, stream);
StreamAgent *agent = &dcc->stream_agents[stream_id];
@@ -2669,27 +2673,41 @@ static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dc
spice_debug("stream %d: upgrade by screenshot. has current %d. box ==>",
stream_id, stream->current != NULL);
rect_debug(&upgrade_area);
- red_update_area(dcc->common.worker, &upgrade_area, 0);
+ if (update_area_limit) {
+ red_update_area_till(dcc->common.worker, &upgrade_area, 0, update_area_limit);
+ } else {
+ red_update_area(dcc->common.worker, &upgrade_area, 0);
+ }
red_add_surface_area_image(dcc, 0, &upgrade_area, NULL, FALSE);
}
}
-static inline void red_detach_stream_gracefully(RedWorker *worker, Stream *stream)
+static inline void red_detach_stream_gracefully(RedWorker *worker, Stream *stream,
+ Drawable *update_area_limit)
{
RingItem *item;
DisplayChannelClient *dcc;
WORKER_FOREACH_DCC(worker, item, dcc) {
- red_display_detach_stream_gracefully(dcc, stream);
+ red_display_detach_stream_gracefully(dcc, stream, update_area_limit);
}
if (stream->current) {
red_detach_stream(worker, stream, TRUE);
}
}
-// region should be a primary surface region
-static void red_detach_streams_behind(RedWorker *worker, QRegion *region)
+/*
+ * region : a primary surface region. Streams that intersects with the given
+ * region will be detached.
+ * drawable: If detaching the stream is triggered by the addition of a new drawable
+ * that is dependent on the given region, and the drawable is already a part
+ * of the "current tree", the drawable parameter should be set with
+ * this drawable, otherwise, it should be NULL. Then, if detaching the stream
+ * involves sending an upgrade image to the client, this drawable won't be rendered
+ * (see red_display_detach_stream_gracefully).
+ */
+static void red_detach_streams_behind(RedWorker *worker, QRegion *region, Drawable *drawable)
{
Ring *ring = &worker->streams;
RingItem *item = ring_get_head(ring);
@@ -2706,7 +2724,7 @@ static void red_detach_streams_behind(RedWorker *worker, QRegion *region)
StreamAgent *agent = &dcc->stream_agents[get_stream_id(worker, stream)];
if (region_intersects(&agent->vis_region, region)) {
- red_display_detach_stream_gracefully(dcc, stream);
+ red_display_detach_stream_gracefully(dcc, stream, drawable);
detach_stream = 1;
spice_debug("stream %d", get_stream_id(worker, stream));
}
@@ -2798,7 +2816,7 @@ static inline void red_handle_streams_timout(RedWorker *worker)
Stream *stream = SPICE_CONTAINEROF(item, Stream, link);
item = ring_next(ring, item);
if (now >= (stream->last_time + RED_STREAM_TIMOUT)) {
- red_detach_stream_gracefully(worker, stream);
+ red_detach_stream_gracefully(worker, stream, NULL);
red_stop_stream(worker, stream);
}
}
@@ -3507,13 +3525,24 @@ static inline int red_current_add(RedWorker *worker, Ring *ring, Drawable *drawa
exclude_region(worker, ring, exclude_base, &exclude_rgn, NULL, drawable);
red_use_stream_trace(worker, drawable);
red_streams_update_visible_region(worker, drawable);
+ /*
+ * Performing the insertion after exclude_region for
+ * safety (todo: Not sure if exclude_region can affect the drawable
+ * if it is added to the tree before calling exclude_region).
+ */
+ __current_add_drawable(worker, drawable, ring);
} else {
+ /*
+ * red_detach_streams_behind can affect the current tree since it may
+ * trigger calls to update_area. Thus, the drawable should be added to the tree
+ * before calling red_detach_streams_behind
+ */
+ __current_add_drawable(worker, drawable, ring);
if (drawable->surface_id == 0) {
- red_detach_streams_behind(worker, &drawable->tree_item.base.rgn);
+ red_detach_streams_behind(worker, &drawable->tree_item.base.rgn, drawable);
}
}
region_destroy(&exclude_rgn);
- __current_add_drawable(worker, drawable, ring);
stat_add(&worker->add_stat, start_time);
return TRUE;
}
@@ -3567,7 +3596,7 @@ static inline int red_current_add_with_shadow(RedWorker *worker, Ring *ring, Dra
// only primary surface streams are supported
if (is_primary_surface(worker, item->surface_id)) {
- red_detach_streams_behind(worker, &shadow->base.rgn);
+ red_detach_streams_behind(worker, &shadow->base.rgn, NULL);
}
ring_add(ring, &shadow->base.siblings_link);
__current_add_drawable(worker, item, ring);
@@ -3579,7 +3608,7 @@ static inline int red_current_add_with_shadow(RedWorker *worker, Ring *ring, Dra
red_streams_update_visible_region(worker, item);
} else {
if (item->surface_id == 0) {
- red_detach_streams_behind(worker, &item->tree_item.base.rgn);
+ red_detach_streams_behind(worker, &item->tree_item.base.rgn, item);
}
}
stat_add(&worker->add_stat, start_time);
@@ -3911,7 +3940,7 @@ static inline int red_handle_surfaces_dependencies(RedWorker *worker, Drawable *
QRegion depend_region;
region_init(&depend_region);
region_add(&depend_region, &drawable->red_drawable->surfaces_rects[x]);
- red_detach_streams_behind(worker, &depend_region);
+ red_detach_streams_behind(worker, &depend_region, NULL);
}
}
}
--
1.8.1

39
0003-red_worker.c-clearing-the-stream-vis_region-after-it.patch Normal file
View File

@ -0,0 +1,39 @@
From 4eb172f6fe9dffe2f3f3045189e84375e54c6cad Mon Sep 17 00:00:00 2001
From: Yonit Halperin <yhalperi@redhat.com>
Date: Tue, 8 Jan 2013 10:51:26 -0500
Subject: [PATCH spice 3/6] red_worker.c: clearing the stream vis_region, after
it has been detached
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.
---
server/red_worker.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/server/red_worker.c b/server/red_worker.c
index 5e00cb6..085e3e6 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -2646,7 +2646,7 @@ static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dc
spice_debug("stream %d: upgrade by linked drawable. sized %d, box ==>",
stream_id, stream->current->sized_stream != NULL);
rect_debug(&stream->current->red_drawable->bbox);
- return;
+ goto clear_vis_region;
}
spice_debug("stream %d: upgrade by drawable. sized %d, box ==>",
stream_id, stream->current->sized_stream != NULL);
@@ -2680,7 +2680,8 @@ static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dc
}
red_add_surface_area_image(dcc, 0, &upgrade_area, NULL, FALSE);
}
-
+clear_vis_region:
+ region_clear(&agent->vis_region);
}
static inline void red_detach_stream_gracefully(RedWorker *worker, Stream *stream,
--
1.8.1

32
0004-link-libspice-server-with-libm-libpthread.patch Normal file
View File

@ -0,0 +1,32 @@
From cf8ebbc48491cf5178e4edc57f49ceded20ead55 Mon Sep 17 00:00:00 2001
From: Michael Tokarev <mjt@tls.msk.ru>
Date: Sat, 2 Jun 2012 13:33:42 +0000
Subject: [PATCH spice 4/6] link libspice server with libm libpthread
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>
---
server/Makefile.am | 1 +
1 file changed, 1 insertion(+)
diff --git a/server/Makefile.am b/server/Makefile.am
index b3fcd2c..8b380fc 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -40,6 +40,7 @@ libspice_server_la_LIBADD = \
$(SLIRP_LIBS) \
$(SSL_LIBS) \
$(Z_LIBS) \
+ $(SPICE_NONPKGCONFIG_LIBS) \
$(NULL)
libspice_server_la_SOURCES = \
--
1.8.1

77
0005-server-Fix-SpiceWorker-CRITICAL-red_worker.c-10968-r.patch Normal file
View File

@ -0,0 +1,77 @@
From d2e1f939fec9b4d852a009cc55c4bbb3d2a94d3b Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Thu, 10 Jan 2013 22:55:51 +0100
Subject: [PATCH spice 5/6] server: Fix SpiceWorker-CRITICAL **:
red_worker.c:10968:red_push_monitors_config: condition `monitors_config !=
NULL' failed
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/red_worker.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/server/red_worker.c b/server/red_worker.c
index 085e3e6..de070a6 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1239,8 +1239,7 @@ static void monitors_config_decref(MonitorsConfig *monitors_config)
return;
}
- spice_debug("removing worker monitors config");
- monitors_config->worker->monitors_config = NULL;
+ spice_debug("freeing monitors config");
free(monitors_config);
}
--
1.8.1

56
0006-worker_update_monitors_config-Drop-bogus-real_count-.patch Normal file
View File

@ -0,0 +1,56 @@
From 50efe1e48d2fcf6a2f12c933ce29e73b6985f599 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Thu, 10 Jan 2013 23:30:34 +0100
Subject: [PATCH spice 6/6] worker_update_monitors_config: Drop bogus
real_count accounting
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>
---
server/red_worker.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/server/red_worker.c b/server/red_worker.c
index de070a6..11fa126 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10951,7 +10951,6 @@ static void worker_update_monitors_config(RedWorker *worker,
{
int heads_size;
MonitorsConfig *monitors_config;
- int real_count = 0;
int i;
if (worker->monitors_config) {
@@ -10968,19 +10967,7 @@ static void worker_update_monitors_config(RedWorker *worker,
dev_monitors_config->heads[i].width,
dev_monitors_config->heads[i].height);
}
-
- // Ignore any empty sized monitors at the end of the config.
- // 4: {w1,h1},{w2,h2},{0,0},{0,0} -> 2: {w1,h1},{w2,h2}
- for (i = dev_monitors_config->count ; i > 0 ; --i) {
- if (dev_monitors_config->heads[i - 1].width > 0 &&
- dev_monitors_config->heads[i - 1].height > 0) {
- real_count = i;
- break;
- }
- }
- heads_size = real_count * sizeof(QXLHead);
- spice_debug("new working monitor config (count: %d, real: %d)",
- dev_monitors_config->count, real_count);
+ heads_size = dev_monitors_config->count * sizeof(QXLHead);
worker->monitors_config = monitors_config =
spice_malloc(sizeof(*monitors_config) + heads_size);
monitors_config->refs = 1;
--
1.8.1

17
spice.spec
View File

@ -8,13 +8,19 @@
Name: spice
Version: 0.12.2
Release: 2%{?dist}
Release: 3%{?dist}
Summary: Implements the SPICE protocol
Group: User Interface/Desktops
License: LGPLv2+
URL: http://www.spice-space.org/
Source0: http://www.spice-space.org/download/releases/%{name}-%{version}.tar.bz2
Source1: spice-xpi-client-spicec
Patch1: 0001-server-guest_set_client_capabilities-protect-against.patch
Patch2: 0002-red_worker.c-insert-a-drawable-to-its-position-in-th.patch
Patch3: 0003-red_worker.c-clearing-the-stream-vis_region-after-it.patch
Patch4: 0004-link-libspice-server-with-libm-libpthread.patch
Patch5: 0005-server-Fix-SpiceWorker-CRITICAL-red_worker.c-10968-r.patch
Patch6: 0006-worker_update_monitors_config-Drop-bogus-real_count-.patch
# https://bugzilla.redhat.com/show_bug.cgi?id=613529
%if 0%{?rhel}
@ -89,6 +95,12 @@ using spice-server, you will need to install spice-server-devel.
%prep
%setup -q
%patch1 -p1
%patch2 -p1
%patch3 -p1
%patch4 -p1
%patch5 -p1
%patch6 -p1
%build
@ -148,6 +160,9 @@ fi
%changelog
* Mon Jan 21 2013 Hans de Goede <hdegoede@redhat.com> - 0.12.2-3
- Add a number of misc. bug-fixes from upstream
* Fri Dec 21 2012 Adam Tkac <atkac redhat com> - 0.12.2-2
- rebuild against new libjpeg