> On 27 Mar 2018, at 10:12, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
>
> With the right patch attached this time.. ;) I've only compile-tested
> this as this really is just a proof of concept at this point.
If I understand correctly, you break the ABI twice, and you rely on a “side effect” of changing the variable name to avoid conflicts that would otherwise arise with the version number alone?
It makes the history fo the code harder to track, and the changes more “subtle". At the very least, I would add a commit log explaining that since you can’t rely on version numbers alone since the version numbering scheme changes, you renamed the variable used to track version numbering.
Also, that means you need to follow the same patterns in locksteps for the plugins, so you are not just making the history of the agent complicated, but also the history of the plugins.
So overall, a lot of additional complication. What is the benefit?
Christophe
> Christophe
>
> On Mon, Mar 26, 2018 at 06:47:08PM +0200, Christophe Fergeau wrote:
>> On Mon, Mar 26, 2018 at 11:27:19AM +0200, Christophe de Dinechin wrote:
>>>
>>>
>>>> On 26 Mar 2018, at 11:25, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
>>>>
>>>> Hey,
>>>>
>>>> On Fri, Mar 23, 2018 at 01:05:22PM +0100, Christophe de Dinechin wrote:
>>>>> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>>>>>
>>>>> Ensure that plugin version checking cannot be bypassed.
>>>>> Implement version checking without violating the C++ ODR.
>>>>> Improve ABI version numbering when incompatibilites are detected post-release.
>>>>> Add macro to make it easier to generate the ABI interface.
>>>>>
>>>>> Christophe de Dinechin (2):
>>>>> Ensure that plugins cannot bypass version check
>>>>> Add a macro to deal with the boilerplate of writing a streaming agent
>>>>> plugin
>>>>
>>>> Could this be split in 4 patches each implementing one of these changes?
>>>
>>> I don’t see how. Any idea on how this could be done?
>>
>> I would at least split the change regarding how the versioning is
>> managed, see attached patch for a proof of concept (most likely needs
>> some adjustment, no proper logs, ..). The ODR change/ensure that plugin
>> cannot bypass version check can probably be considered two faces of the
>> same coin).
>>
>> Christophe
>
>> From f3b8d49752949c8b24efb3bc6515cb31cc81ebb0 Mon Sep 17 00:00:00 2001
>> From: Frediano Ziglio <fziglio@xxxxxxxxxx>
>> Date: Fri, 16 Mar 2018 11:31:41 +0100
>> Subject: [spice-server 1/3] Preparation work
>>
>> ---
>> server/stream-channel.c | 35 ++++++++++++++++++++++++-----------
>> 1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/server/stream-channel.c b/server/stream-channel.c
>> index 88f859f6d..3fab28ae1 100644
>> --- a/server/stream-channel.c
>> +++ b/server/stream-channel.c
>> @@ -508,17 +508,10 @@ data_item_free(RedPipeItem *base)
>> g_free(pipe_item);
>> }
>>
>> -void
>> -stream_channel_send_data(StreamChannel *channel, const void *data, size_t size, uint32_t mm_time)
>> +static StreamDataItem*
>> +stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_time)
>> {
>> - if (channel->stream_id < 0) {
>> - // this condition can happen if the guest didn't handle
>> - // the format stop that we send so think the stream is still
>> - // started
>> - return;
>> - }
>> -
>> - RedChannel *red_channel = RED_CHANNEL(channel);
>> + stream_channel_unref_data_item(channel);
>>
>> StreamDataItem *item = g_malloc(sizeof(*item) + size);
>> red_pipe_item_init_full(&item->base, RED_PIPE_ITEM_TYPE_STREAM_DATA,
>> @@ -527,9 +520,29 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
>> item->data.base.multi_media_time = mm_time;
>> item->data.data_size = size;
>> item->channel = channel;
>> - stream_channel_update_queue_stat(channel, 1, size);
>> +
>> + return item;
>> +}
>> +
>> +void
>> +stream_channel_send_data(StreamChannel *channel, const void *data, size_t size, uint32_t mm_time)
>> +{
>> + RedChannel *red_channel = RED_CHANNEL(channel);
>> +
>> + if (channel->stream_id < 0) {
>> + // this condition can happen if the guest didn't handle
>> + // the format stop that we send so think the stream is still
>> + // started
>> + return;
>> + }
>> +
>> + StreamDataItem *item;
>> +
>> + item = stream_channel_new_data_item(channel, size, mm_time);
>> +
>> // TODO try to optimize avoiding the copy
>> memcpy(item->data.data, data, size);
>> + stream_channel_update_queue_stat(channel, 1, size);
>> red_channel_pipes_add(red_channel, &item->base);
>> }
>>
>> --
>> 2.14.3
>>
>>
>> From c8c204c5482e3bfa0038ea9829fb96923274ee29 Mon Sep 17 00:00:00 2001
>> From: Frediano Ziglio <fziglio@xxxxxxxxxx>
>> Date: Fri, 16 Mar 2018 11:44:21 +0100
>> Subject: [spice-server 2/3] more preparation work
>>
>> ---
>> server/stream-channel.c | 57 +++++++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 48 insertions(+), 9 deletions(-)
>>
>> diff --git a/server/stream-channel.c b/server/stream-channel.c
>> index 3fab28ae1..b5652e5ff 100644
>> --- a/server/stream-channel.c
>> +++ b/server/stream-channel.c
>> @@ -44,6 +44,7 @@
>>
>> typedef struct StreamChannelClient StreamChannelClient;
>> typedef struct StreamChannelClientClass StreamChannelClientClass;
>> +typedef struct StreamDataItem StreamDataItem;
>>
>> /* we need to inherit from CommonGraphicsChannelClient
>> * to get buffer handling */
>> @@ -74,6 +75,10 @@ struct StreamChannel {
>>
>> StreamQueueStat queue_stat;
>>
>> + /* pending partial data item */
>> + StreamDataItem *data_item;
>> + uint32_t data_item_pos;
>> +
>> /* callback to notify when a stream should be started or stopped */
>> stream_channel_start_proc start_cb;
>> void *start_opaque;
>> @@ -104,12 +109,12 @@ typedef struct StreamCreateItem {
>> SpiceMsgDisplayStreamCreate stream_create;
>> } StreamCreateItem;
>>
>> -typedef struct StreamDataItem {
>> +struct StreamDataItem {
>> RedPipeItem base;
>> StreamChannel *channel;
>> // NOTE: this must be the last field in the structure
>> SpiceMsgDisplayStreamData data;
>> -} StreamDataItem;
>> +};
>>
>> #define PRIMARY_SURFACE_ID 0
>>
>> @@ -129,6 +134,16 @@ stream_channel_client_init(StreamChannelClient *client)
>> client->stream_id = -1;
>> }
>>
>> +static void
>> +stream_channel_unref_data_item(StreamChannel *channel)
>> +{
>> + if (channel->data_item) {
>> + red_pipe_item_unref(&channel->data_item->base);
>> + channel->data_item = NULL;
>> + channel->data_item_pos = 0;
>> + }
>> +}
>> +
>> static void
>> request_new_stream(StreamChannel *channel, StreamMsgStartStop *start)
>> {
>> @@ -152,6 +167,7 @@ stream_channel_client_on_disconnect(RedChannelClient *rcc)
>> channel->stream_id = -1;
>> channel->width = 0;
>> channel->height = 0;
>> + stream_channel_unref_data_item(channel);
>>
>> // send stream stop to device
>> StreamMsgStartStop stop = { 0, };
>> @@ -424,6 +440,16 @@ stream_channel_constructed(GObject *object)
>> reds_register_channel(reds, red_channel);
>> }
>>
>> +static void
>> +stream_channel_finalize(GObject *object)
>> +{
>> + StreamChannel *channel = STREAM_CHANNEL(object);
>> +
>> + stream_channel_unref_data_item(channel);
>> +
>> + G_OBJECT_CLASS(stream_channel_parent_class)->finalize(object);
>> +}
>> +
>> static void
>> stream_channel_class_init(StreamChannelClass *klass)
>> {
>> @@ -431,6 +457,7 @@ stream_channel_class_init(StreamChannelClass *klass)
>> RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
>>
>> object_class->constructed = stream_channel_constructed;
>> + object_class->finalize = stream_channel_finalize;
>>
>> channel_class->parser = spice_get_client_channel_parser(SPICE_CHANNEL_DISPLAY, NULL);
>> channel_class->handle_message = handle_message;
>> @@ -503,7 +530,9 @@ data_item_free(RedPipeItem *base)
>> {
>> StreamDataItem *pipe_item = SPICE_UPCAST(StreamDataItem, base);
>>
>> - stream_channel_update_queue_stat(pipe_item->channel, -1, -pipe_item->data.data_size);
>> + if (pipe_item->channel->data_item != pipe_item) {
>> + stream_channel_update_queue_stat(pipe_item->channel, -1, -pipe_item->data.data_size);
>> + }
>>
>> g_free(pipe_item);
>> }
>> @@ -521,6 +550,9 @@ stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_ti
>> item->data.data_size = size;
>> item->channel = channel;
>>
>> + channel->data_item = item;
>> + channel->data_item_pos = 0;
>> +
>> return item;
>> }
>>
>> @@ -536,14 +568,20 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
>> return;
>> }
>>
>> - StreamDataItem *item;
>> + while (size) {
>> + StreamDataItem *item = channel->data_item;
>>
>> - item = stream_channel_new_data_item(channel, size, mm_time);
>> + if (!item) {
>> + item = stream_channel_new_data_item(channel, size, mm_time);
>> + }
>>
>> - // TODO try to optimize avoiding the copy
>> - memcpy(item->data.data, data, size);
>> - stream_channel_update_queue_stat(channel, 1, size);
>> - red_channel_pipes_add(red_channel, &item->base);
>> + size_t copy_size = size;
>> + // TODO try to optimize avoiding the copy
>> + memcpy(item->data.data, data, copy_size);
>> + size -= copy_size;
>> + stream_channel_update_queue_stat(channel, 1, size);
>> + red_channel_pipes_add(red_channel, &item->base);
>> + }
>> }
>>
>> void
>> @@ -583,6 +621,7 @@ stream_channel_reset(StreamChannel *channel)
>> channel->stream_id = -1;
>> channel->width = 0;
>> channel->height = 0;
>> + stream_channel_unref_data_item(channel);
>>
>> if (!red_channel_is_connected(red_channel)) {
>> return;
>> --
>> 2.14.3
>>
>>
>> From 43b50f11428313d395c7c01bb3c93356c76f9ac8 Mon Sep 17 00:00:00 2001
>> From: Frediano Ziglio <fziglio@xxxxxxxxxx>
>> Date: Wed, 7 Mar 2018 12:01:49 +0000
>> Subject: [spice-server 3/3] stream-channel: Send the full frame in a single
>> message
>>
>> The current implementation of server and client assumes that a single
>> data message contains an encoded frame.
>> This is not a problem for most encoding but for MJPEG this causes
>> the client to fail decoding.
>> Collapse frame data into a single message before sending to the client.
>> This is done in the channel as the channel code is responsible to take care
>> of client protocol details. This allows for instance to support chunked
>> transfer to client if implemented.
>>
>> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
>> ---
>> server/red-stream-device.c | 10 ++++++----
>> server/stream-channel.c | 27 +++++++++++++++++++++++----
>> server/stream-channel.h | 12 ++++++++++++
>> 3 files changed, 41 insertions(+), 8 deletions(-)
>>
>> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
>> index c87c4899d..81336e549 100644
>> --- a/server/red-stream-device.c
>> +++ b/server/red-stream-device.c
>> @@ -271,11 +271,13 @@ handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
>> if (n <= 0) {
>> break;
>> }
>> - // TODO collect all message ??
>> - // up: we send a single frame together
>> - // down: guest can cause a crash
>> - stream_channel_send_data(dev->stream_channel, buf, n, reds_get_mm_time());
>> + uint32_t mm_time = reds_get_mm_time();
>> + if (dev->msg_pos == 0) {
>> + stream_channel_start_data(dev->stream_channel, dev->hdr.size, mm_time);
>> + }
>> + stream_channel_send_data(dev->stream_channel, buf, n, mm_time);
>> dev->hdr.size -= n;
>> + dev->msg_pos += n;
>> }
>>
>> return dev->hdr.size == 0;
>> diff --git a/server/stream-channel.c b/server/stream-channel.c
>> index b5652e5ff..75dd1f867 100644
>> --- a/server/stream-channel.c
>> +++ b/server/stream-channel.c
>> @@ -556,6 +556,20 @@ stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_ti
>> return item;
>> }
>>
>> +void
>> +stream_channel_start_data(StreamChannel *channel, size_t size, uint32_t mm_time)
>> +{
>> + // see stream_channel_send_data comment
>> + if (channel->stream_id < 0) {
>> + return;
>> + }
>> +
>> + // TODO this collects all chunks in a single message
>> + // up: we send a single frame together (more compatible)
>> + // down: guest can cause a crash due to DoS. As a safe measure we limit the maximum message
>> + stream_channel_new_data_item(channel, MIN(size, 32*1024*1024), mm_time);
>> +}
>> +
>> void
>> stream_channel_send_data(StreamChannel *channel, const void *data, size_t size, uint32_t mm_time)
>> {
>> @@ -575,12 +589,17 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
>> item = stream_channel_new_data_item(channel, size, mm_time);
>> }
>>
>> - size_t copy_size = size;
>> + size_t copy_size = item->data.data_size - channel->data_item_pos;
>> + copy_size = MIN(copy_size, size);
>> // TODO try to optimize avoiding the copy
>> - memcpy(item->data.data, data, copy_size);
>> + memcpy(item->data.data + channel->data_item_pos, data, copy_size);
>> size -= copy_size;
>> - stream_channel_update_queue_stat(channel, 1, size);
>> - red_channel_pipes_add(red_channel, &item->base);
>> + channel->data_item_pos += copy_size;
>> + if (channel->data_item_pos == item->data.data_size) {
>> + channel->data_item = NULL;
>> + stream_channel_update_queue_stat(channel, 1, item->data.data_size);
>> + red_channel_pipes_add(red_channel, &item->base);
>> + }
>> }
>> }
>>
>> diff --git a/server/stream-channel.h b/server/stream-channel.h
>> index e8bec80bd..18a1bdead 100644
>> --- a/server/stream-channel.h
>> +++ b/server/stream-channel.h
>> @@ -60,6 +60,18 @@ struct StreamMsgStartStop;
>>
>> void stream_channel_change_format(StreamChannel *channel,
>> const struct StreamMsgFormat *fmt);
>> +
>> +/**
>> + * Tell the channel that a new data packet is starting.
>> + * This can be used to group all chunks together.
>> + */
>> +void stream_channel_start_data(StreamChannel *channel,
>> + size_t size,
>> + uint32_t mm_time);
>> +
>> +/**
>> + * Send to channel a chunk of data.
>> + */
>> void stream_channel_send_data(StreamChannel *channel,
>> const void *data, size_t size,
>> uint32_t mm_time);
>> --
>> 2.14.3
>>
>
>
>
>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> <split.patch>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel