[Spice-commits] 8 commits - server/main_channel.c server/main_channel.h server/main_dispatcher.c server/red_channel.c server/red_channel.h server/reds.c server/reds.h server/red_worker.c
Yonit Halperin
yhalperi at kemper.freedesktop.org
Wed May 8 08:27:31 PDT 2013
server/main_channel.c | 10 ++++++
server/main_channel.h | 6 ++++
server/main_dispatcher.c | 5 ---
server/red_channel.c | 7 +++-
server/red_channel.h | 11 +++++++
server/red_worker.c | 68 +++++++++++++++++++++++++++++------------------
server/reds.c | 20 +++++++++----
server/reds.h | 4 ++
8 files changed, 93 insertions(+), 38 deletions(-)
New commits:
commit 313e2622d91675b15a6231151f2e9509ccf1dc96
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue May 7 13:50:20 2013 -0400
reds: fix memory leak when core->base.minor_version < 3
diff --git a/server/reds.c b/server/reds.c
index a378f80..f6a1ce9 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -196,10 +196,9 @@ static void reds_stream_push_channel_event(RedsStream *s, int event)
void reds_handle_channel_event(int event, SpiceChannelEventInfo *info)
{
- if (core->base.minor_version < 3 || core->channel_event == NULL)
- return;
+ if (core->base.minor_version >= 3 && core->channel_event != NULL)
+ core->channel_event(event, info);
- core->channel_event(event, info);
if (event == SPICE_CHANNEL_EVENT_DISCONNECTED) {
free(info);
}
commit 5fb3d2557ee37be4ce4f6eb041148d3eb922978a
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue May 7 12:26:10 2013 -0400
reds: move handle_channel_event logic from main_dispatcher to reds
main_dispactcher role is to pass events to the main thread.
The logic that handles the event better not be inside main_dispatcher.
diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c
index 8402402..92b0791 100644
--- a/server/main_dispatcher.c
+++ b/server/main_dispatcher.c
@@ -64,10 +64,7 @@ static void main_dispatcher_self_handle_channel_event(
int event,
SpiceChannelEventInfo *info)
{
- main_dispatcher.core->channel_event(event, info);
- if (event == SPICE_CHANNEL_EVENT_DISCONNECTED) {
- free(info);
- }
+ reds_handle_channel_event(event, info);
}
static void main_dispatcher_handle_channel_event(void *opaque,
diff --git a/server/reds.c b/server/reds.c
index b8db905..a378f80 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -189,11 +189,20 @@ static ChannelSecurityOptions *find_channel_security(int id)
return now;
}
-static void reds_stream_channel_event(RedsStream *s, int event)
+static void reds_stream_push_channel_event(RedsStream *s, int event)
+{
+ main_dispatcher_channel_event(event, s->info);
+}
+
+void reds_handle_channel_event(int event, SpiceChannelEventInfo *info)
{
if (core->base.minor_version < 3 || core->channel_event == NULL)
return;
- main_dispatcher_channel_event(event, s->info);
+
+ core->channel_event(event, info);
+ if (event == SPICE_CHANNEL_EVENT_DISCONNECTED) {
+ free(info);
+ }
}
static ssize_t stream_write_cb(RedsStream *s, const void *buf, size_t size)
@@ -1524,7 +1533,7 @@ static void reds_info_new_channel(RedLinkInfo *link, int connection_id)
link->stream->info->connection_id = connection_id;
link->stream->info->type = link->link_mess->channel_type;
link->stream->info->id = link->link_mess->channel_id;
- reds_stream_channel_event(link->stream, SPICE_CHANNEL_EVENT_INITIALIZED);
+ reds_stream_push_channel_event(link->stream, SPICE_CHANNEL_EVENT_INITIALIZED);
}
static void reds_send_link_result(RedLinkInfo *link, uint32_t error)
@@ -2845,7 +2854,7 @@ static RedLinkInfo *reds_init_client_connection(int socket)
getpeername(stream->socket, (struct sockaddr*)(&stream->info->paddr_ext),
&stream->info->plen_ext);
- reds_stream_channel_event(stream, SPICE_CHANNEL_EVENT_CONNECTED);
+ reds_stream_push_channel_event(stream, SPICE_CHANNEL_EVENT_CONNECTED);
openssl_init(link);
@@ -4573,7 +4582,7 @@ void reds_stream_free(RedsStream *s)
return;
}
- reds_stream_channel_event(s, SPICE_CHANNEL_EVENT_DISCONNECTED);
+ reds_stream_push_channel_event(s, SPICE_CHANNEL_EVENT_DISCONNECTED);
#if HAVE_SASL
if (s->sasl.conn) {
diff --git a/server/reds.h b/server/reds.h
index 59f13ce..c5c557d 100644
--- a/server/reds.h
+++ b/server/reds.h
@@ -111,11 +111,15 @@ typedef struct RedsMigSpice {
int sport;
} RedsMigSpice;
+/* any thread */
ssize_t reds_stream_read(RedsStream *s, void *buf, size_t nbyte);
ssize_t reds_stream_write(RedsStream *s, const void *buf, size_t nbyte);
ssize_t reds_stream_writev(RedsStream *s, const struct iovec *iov, int iovcnt);
void reds_stream_free(RedsStream *s);
+/* main thread only */
+void reds_handle_channel_event(int event, SpiceChannelEventInfo *info);
+
void reds_disable_mm_timer(void);
void reds_enable_mm_timer(void);
void reds_update_mm_timer(uint32_t mm_time);
commit 20cc9567643dcd33166359fa2226ae15030939b3
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue May 7 14:55:29 2013 -0400
red_worker: fail handle_migrate_data instead of aborting when there is an error during restoration of surfaces
diff --git a/server/red_worker.c b/server/red_worker.c
index 247f153..f12d8f8 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10099,16 +10099,20 @@ static uint64_t display_channel_handle_migrate_data_get_serial(
return migrate_data->message_serial;
}
-static void display_channel_client_restore_surface(DisplayChannelClient *dcc, uint32_t surface_id)
+static int display_channel_client_restore_surface(DisplayChannelClient *dcc, uint32_t surface_id)
{
/* we don't process commands till we receive the migration data, thus,
* we should have not sent any surface to the client. */
- spice_assert(!dcc->surface_client_created[surface_id]);
+ if (dcc->surface_client_created[surface_id]) {
+ spice_warning("surface %u is already marked as client_created", surface_id);
+ return FALSE;
+ }
dcc->surface_client_created[surface_id] = TRUE;
+ return TRUE;
}
-static void display_channel_client_restore_surfaces_lossless(DisplayChannelClient *dcc,
- MigrateDisplaySurfacesAtClientLossless *mig_surfaces)
+static int display_channel_client_restore_surfaces_lossless(DisplayChannelClient *dcc,
+ MigrateDisplaySurfacesAtClientLossless *mig_surfaces)
{
uint32_t i;
@@ -10116,11 +10120,14 @@ static void display_channel_client_restore_surfaces_lossless(DisplayChannelClien
for (i = 0; i < mig_surfaces->num_surfaces; i++) {
uint32_t surface_id = mig_surfaces->surfaces[i].id;
- display_channel_client_restore_surface(dcc, surface_id);
+ if (!display_channel_client_restore_surface(dcc, surface_id)) {
+ return FALSE;
+ }
}
+ return TRUE;
}
-static void display_channel_client_restore_surfaces_lossy(DisplayChannelClient *dcc,
+static int display_channel_client_restore_surfaces_lossy(DisplayChannelClient *dcc,
MigrateDisplaySurfacesAtClientLossy *mig_surfaces)
{
uint32_t i;
@@ -10131,7 +10138,9 @@ static void display_channel_client_restore_surfaces_lossy(DisplayChannelClient *
SpiceMigrateDataRect *mig_lossy_rect;
SpiceRect lossy_rect;
- display_channel_client_restore_surface(dcc, surface_id);
+ if (!display_channel_client_restore_surface(dcc, surface_id)) {
+ return FALSE;
+ }
spice_assert(dcc->surface_client_created[surface_id]);
mig_lossy_rect = &mig_surfaces->surfaces[i].lossy_rect;
@@ -10142,6 +10151,7 @@ static void display_channel_client_restore_surfaces_lossy(DisplayChannelClient *
region_init(&dcc->surface_client_lossy_region[surface_id]);
region_add(&dcc->surface_client_lossy_region[surface_id], &lossy_rect);
}
+ return TRUE;
}
static int display_channel_handle_migrate_data(RedChannelClient *rcc, uint32_t size,
void *message)
@@ -10151,6 +10161,7 @@ static int display_channel_handle_migrate_data(RedChannelClient *rcc, uint32_t s
DisplayChannel *display_channel = SPICE_CONTAINEROF(rcc->channel, DisplayChannel, common.base);
DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
uint8_t *surfaces;
+ int surfaces_restored = FALSE;
int i;
spice_debug(NULL);
@@ -10214,13 +10225,16 @@ static int display_channel_handle_migrate_data(RedChannelClient *rcc, uint32_t s
surfaces = (uint8_t *)message + migrate_data->surfaces_at_client_ptr;
if (display_channel->enable_jpeg) {
- display_channel_client_restore_surfaces_lossy(dcc,
- (MigrateDisplaySurfacesAtClientLossy *)surfaces);
+ surfaces_restored = display_channel_client_restore_surfaces_lossy(dcc,
+ (MigrateDisplaySurfacesAtClientLossy *)surfaces);
} else {
- display_channel_client_restore_surfaces_lossless(dcc,
- (MigrateDisplaySurfacesAtClientLossless*)surfaces);
+ surfaces_restored = display_channel_client_restore_surfaces_lossless(dcc,
+ (MigrateDisplaySurfacesAtClientLossless*)surfaces);
}
+ if (!surfaces_restored) {
+ return FALSE;
+ }
red_channel_client_pipe_add_type(rcc, PIPE_ITEM_TYPE_INVAL_PALLET_CACHE);
/* enable sending messages */
red_channel_client_ack_zero_messages_window(rcc);
commit b82351f7119d3c9058ebafd1de130f3b078d0c74
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue May 7 14:54:40 2013 -0400
red_channel: notify and shutdown a channel client when its handle_migrate_data fails
diff --git a/server/red_channel.c b/server/red_channel.c
index 9d71543..119e5e5 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -1275,14 +1275,17 @@ static void red_channel_handle_migrate_data(RedChannelClient *rcc, uint32_t size
return;
}
if (!red_channel_client_waits_for_migrate_data(rcc)) {
- spice_error("unexcpected");
+ spice_channel_client_error(rcc, "unexpected");
return;
}
if (rcc->channel->channel_cbs.handle_migrate_data_get_serial) {
red_channel_client_set_message_serial(rcc,
rcc->channel->channel_cbs.handle_migrate_data_get_serial(rcc, size, message));
}
- rcc->channel->channel_cbs.handle_migrate_data(rcc, size, message);
+ if (!rcc->channel->channel_cbs.handle_migrate_data(rcc, size, message)) {
+ spice_channel_client_error(rcc, "handle_migrate_data failed");
+ return;
+ }
red_channel_client_seamless_migration_done(rcc);
}
commit dbb99a6517bb7a1d059d7962ddba5ae5524abee3
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue May 7 14:39:12 2013 -0400
red_channel: add spice_channel_client_error
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.
diff --git a/server/red_channel.h b/server/red_channel.h
index f770510..ba299b6 100644
--- a/server/red_channel.h
+++ b/server/red_channel.h
@@ -328,6 +328,17 @@ struct RedChannel {
#endif
};
+/*
+ * When an error occurs over a channel, we treat it as a warning
+ * for spice-server and shutdown the channel.
+ */
+#define spice_channel_client_error(rcc, format, ...) \
+ do { \
+ spice_warning("rcc %p type %u id %u: " format, rcc, \
+ rcc->channel->type, rcc->channel->id, ## __VA_ARGS__); \
+ red_channel_client_shutdown(rcc); \
+ } while (0)
+
/* if one of the callbacks should cause disconnect, use red_channel_shutdown and don't
* explicitly destroy the channel */
RedChannel *red_channel_create(int size,
commit f50827e5274cecbfca8cd5a4995215087d8368fc
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue May 7 11:04:59 2013 -0400
red_worker: fix incorrect is_low_bandwidth after migrating a low bandwidth connection
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 at redhat.com>
diff --git a/server/red_worker.c b/server/red_worker.c
index be53c1d..247f153 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10199,6 +10199,9 @@ static int display_channel_handle_migrate_data(RedChannelClient *rcc, uint32_t s
} else {
spice_critical("restoring global lz dictionary failed");
}
+
+ dcc->common.is_low_bandwidth = migrate_data->low_bandwidth_setting;
+
if (migrate_data->low_bandwidth_setting) {
red_channel_client_ack_set_client_window(rcc, WIDE_CLIENT_ACK_WINDOW);
if (dcc->common.worker->jpeg_state == SPICE_WAN_COMPRESSION_AUTO) {
commit 0a4d29b2e134a6330dd78c5ba46dcc652c4db33b
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue May 7 11:01:15 2013 -0400
red_worker: cleanup: add is_low_bandwidth flag to CommonChannelClient
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.
diff --git a/server/red_worker.c b/server/red_worker.c
index decbfbb..be53c1d 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -665,6 +665,7 @@ typedef struct CommonChannelClient {
RedChannelClient base;
uint32_t id;
struct RedWorker *worker;
+ int is_low_bandwidth;
} CommonChannelClient;
/* Each drawable can refer to at most 3 images: src, brush and mask */
@@ -3287,12 +3288,6 @@ static inline int red_is_next_stream_frame(RedWorker *worker, const Drawable *ca
FALSE);
}
-static int display_channel_client_is_low_bandwidth(DisplayChannelClient *dcc)
-{
- return main_channel_client_is_low_bandwidth(
- red_client_get_main(red_channel_client_get_client(&dcc->common.base)));
-}
-
static inline void pre_stream_item_swap(RedWorker *worker, Stream *stream, Drawable *new_frame)
{
DrawablePipeItem *dpi;
@@ -3318,7 +3313,7 @@ static inline void pre_stream_item_swap(RedWorker *worker, Stream *stream, Drawa
agent = &dcc->stream_agents[index];
if (!dcc->use_mjpeg_encoder_rate_control &&
- !display_channel_client_is_low_bandwidth(dcc)) {
+ !dcc->common.is_low_bandwidth) {
continue;
}
@@ -8768,7 +8763,7 @@ static void display_channel_marshall_migrate_data(RedChannelClient *rcc,
MIGRATE_DATA_DISPLAY_MAX_CACHE_CLIENTS == MAX_CACHE_CLIENTS);
display_data.message_serial = red_channel_client_get_message_serial(rcc);
- display_data.low_bandwidth_setting = display_channel_client_is_low_bandwidth(dcc);
+ display_data.low_bandwidth_setting = dcc->common.is_low_bandwidth;
display_data.pixmap_cache_freezer = pixmap_cache_freeze(dcc->pixmap_cache);
display_data.pixmap_cache_id = dcc->pixmap_cache->id;
@@ -10286,6 +10281,7 @@ static int common_channel_config_socket(RedChannelClient *rcc)
RedClient *client = red_channel_client_get_client(rcc);
MainChannelClient *mcc = red_client_get_main(client);
RedsStream *stream = red_channel_client_get_stream(rcc);
+ CommonChannelClient *ccc = SPICE_CONTAINEROF(rcc, CommonChannelClient, base);
int flags;
int delay_val;
@@ -10300,7 +10296,14 @@ static int common_channel_config_socket(RedChannelClient *rcc)
}
// TODO - this should be dynamic, not one time at channel creation
- delay_val = main_channel_client_is_low_bandwidth(mcc) ? 0 : 1;
+ ccc->is_low_bandwidth = main_channel_client_is_low_bandwidth(mcc);
+ delay_val = ccc->is_low_bandwidth ? 0 : 1;
+ /* FIXME: Using Nagle's Algorithm can lead to apparent delays, depending
+ * on the delayed ack timeout on the other side.
+ * Instead of using Nagle's, we need to implement message buffering on
+ * the application level.
+ * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
+ */
if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, &delay_val,
sizeof(delay_val)) == -1) {
if (errno != ENOTSUP) {
@@ -10402,7 +10405,6 @@ static CommonChannelClient *common_channel_client_create(int size,
uint32_t *caps,
int num_caps)
{
- MainChannelClient *mcc = red_client_get_main(client);
RedChannelClient *rcc =
red_channel_client_create(size, &common->base, client, stream, monitor_latency,
num_common_caps, common_caps, num_caps, caps);
@@ -10416,7 +10418,7 @@ static CommonChannelClient *common_channel_client_create(int size,
// TODO: move wide/narrow ack setting to red_channel.
red_channel_client_ack_set_client_window(rcc,
- main_channel_client_is_low_bandwidth(mcc) ?
+ common_cc->is_low_bandwidth ?
WIDE_CLIENT_ACK_WINDOW : NARROW_CLIENT_ACK_WINDOW);
return common_cc;
}
@@ -10749,7 +10751,6 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
DisplayChannel *display_channel;
DisplayChannelClient *dcc;
size_t stream_buf_size;
- int is_low_bandwidth = main_channel_client_is_low_bandwidth(red_client_get_main(client));
if (!worker->display_channel) {
spice_warning("Display channel was not created");
@@ -10776,7 +10777,7 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
dcc->send_data.free_list.res_size = DISPLAY_FREE_LIST_DEFAULT_SIZE;
if (worker->jpeg_state == SPICE_WAN_COMPRESSION_AUTO) {
- display_channel->enable_jpeg = is_low_bandwidth;
+ display_channel->enable_jpeg = dcc->common.is_low_bandwidth;
} else {
display_channel->enable_jpeg = (worker->jpeg_state == SPICE_WAN_COMPRESSION_ALWAYS);
}
@@ -10785,7 +10786,7 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
display_channel->jpeg_quality = 85;
if (worker->zlib_glz_state == SPICE_WAN_COMPRESSION_AUTO) {
- display_channel->enable_zlib_glz_wrap = is_low_bandwidth;
+ display_channel->enable_zlib_glz_wrap = dcc->common.is_low_bandwidth;
} else {
display_channel->enable_zlib_glz_wrap = (worker->zlib_glz_state ==
SPICE_WAN_COMPRESSION_ALWAYS);
commit dd9f882aedfffec3dab3ec97a85b1fab6459e4bc
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue May 7 10:45:13 2013 -0400
main_channel: add routine for checking if a network test had been conducted and completed
diff --git a/server/main_channel.c b/server/main_channel.c
index dd927ab..4cf7e19 100644
--- a/server/main_channel.c
+++ b/server/main_channel.c
@@ -164,6 +164,7 @@ enum NetTestStage {
NET_TEST_STAGE_WARMUP,
NET_TEST_STAGE_LATENCY,
NET_TEST_STAGE_RATE,
+ NET_TEST_STAGE_COMPLETE,
};
static void main_channel_release_pipe_item(RedChannelClient *rcc,
@@ -962,6 +963,7 @@ static int main_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, uint
if (roundtrip <= mcc->latency) {
// probably high load on client or server result with incorrect values
mcc->latency = 0;
+ mcc->net_test_stage = NET_TEST_STAGE_INVALID;
spice_printerr("net test: invalid values, latency %" PRIu64
" roundtrip %" PRIu64 ". assuming high"
"bandwidth", mcc->latency, roundtrip);
@@ -969,18 +971,19 @@ static int main_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, uint
}
mcc->bitrate_per_sec = (uint64_t)(NET_TEST_BYTES * 8) * 1000000
/ (roundtrip - mcc->latency);
+ mcc->net_test_stage = NET_TEST_STAGE_COMPLETE;
spice_printerr("net test: latency %f ms, bitrate %"PRIu64" bps (%f Mbps)%s",
(double)mcc->latency / 1000,
mcc->bitrate_per_sec,
(double)mcc->bitrate_per_sec / 1024 / 1024,
main_channel_client_is_low_bandwidth(mcc) ? " LOW BANDWIDTH" : "");
- mcc->net_test_stage = NET_TEST_STAGE_INVALID;
break;
default:
spice_printerr("invalid net test stage, ping id %d test id %d stage %d",
ping->id,
mcc->net_test_id,
mcc->net_test_stage);
+ mcc->net_test_stage = NET_TEST_STAGE_INVALID;
}
break;
}
@@ -1139,6 +1142,11 @@ void main_channel_close(MainChannel *main_chan)
}
}
+int main_channel_client_is_network_info_initialized(MainChannelClient *mcc)
+{
+ return mcc->net_test_stage == NET_TEST_STAGE_COMPLETE;
+}
+
int main_channel_client_is_low_bandwidth(MainChannelClient *mcc)
{
// TODO: configurable?
diff --git a/server/main_channel.h b/server/main_channel.h
index b2f0e6f..27367a4 100644
--- a/server/main_channel.h
+++ b/server/main_channel.h
@@ -67,9 +67,15 @@ int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa, sockle
int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa, socklen_t *salen);
uint32_t main_channel_client_get_link_id(MainChannelClient *mcc);
+/*
+ * return TRUE if network test had been completed successfully.
+ * If FALSE, bitrate_per_sec is set to MAX_UINT64 and the roundtrip is set to 0
+ */
+int main_channel_client_is_network_info_initialized(MainChannelClient *mcc);
int main_channel_client_is_low_bandwidth(MainChannelClient *mcc);
uint64_t main_channel_client_get_bitrate_per_sec(MainChannelClient *mcc);
uint64_t main_channel_client_get_roundtrip_ms(MainChannelClient *mcc);
+
int main_channel_is_connected(MainChannel *main_chan);
RedChannelClient* main_channel_client_get_base(MainChannelClient* mcc);
More information about the Spice-commits
mailing list