Re: [Spice-devel] [PATCH spice-server 07/10] Propagate running property from RedWorker to DisplayChannel
Somehow this information doesn't really seem to belong to
DisplayChannel, but I can't put my finger on exactly why it feels out
of place...
On Wed, 2019-03-20 at 09:59 +, Frediano Ziglio wrote:
> This is a preparatory patch to allows DisplayChannel to check
> if device is running.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/display-channel-private.h | 1 +
> server/display-channel.c | 8
> server/display-channel.h | 2 ++
> server/red-worker.c | 2 ++
> 4 files changed, 13 insertions(+)
>
> diff --git a/server/display-channel-private.h b/server/display-
> channel-private.h
> index 58179531..067c6418 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -89,6 +89,7 @@ struct DisplayChannelPrivate
> uint32_t renderer;
> int enable_jpeg;
> int enable_zlib_glz_wrap;
> +bool running;
At minimum, this seems like way too generic of a name. I will probably
look at this later and wonder what it means for a DisplayChannel to be
'running'. But in reality this variable indicates that the QXL device
is started? Actually, after looking briefly at this handle_dev_start()
function, I can't even figure out what causes this function to be
called...
>
> /* A ring of pending drawables for this DisplayChannel,
> regardless of which
> * surface they're associated with. This list is mainly used to
> flush older
> diff --git a/server/display-channel.c b/server/display-channel.c
> index cb052bfc..49a2bd48 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2540,3 +2540,11 @@ void display_channel_debug_oom(DisplayChannel
> *display, const char *msg)
> ring_get_length(>priv->current_list),
> red_channel_sum_pipes_size(channel));
> }
> +
> +void display_channel_set_running(DisplayChannel *display, bool
> running)
> +{
> +if (running == display->priv->running) {
> +return;
> +}
> +display->priv->running = running;
> +}
> diff --git a/server/display-channel.h b/server/display-channel.h
> index f64da0bd..bdf2f059 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -158,6 +158,8 @@ void
> display_channel_reset_image_cache(DisplayChannel *self);
>
> void display_channel_debug_oom(DisplayChannel *display, const char
> *msg);
>
> +void display_channel_set_running(DisplayChannel *display, bool
> running);
> +
> G_END_DECLS
>
> #endif /* DISPLAY_CHANNEL_H_ */
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 07329b17..c9c47133 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -588,6 +588,7 @@ static void handle_dev_stop(void *opaque, void
> *payload)
> spice_assert(worker->running);
>
> worker->running = FALSE;
> +display_channel_set_running(worker->display_channel, false);
>
> display_channel_free_glz_drawables(worker->display_channel);
> display_channel_flush_all_surfaces(worker->display_channel);
> @@ -616,6 +617,7 @@ static void handle_dev_start(void *opaque, void
> *payload)
> display_channel_wait_for_migrate_data(worker-
> >display_channel);
> }
> worker->running = TRUE;
> +display_channel_set_running(worker->display_channel, true);
> worker->event_timeout = 0;
> guest_set_client_capabilities(worker);
> }
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 06/10] Move thread/dispatching handling to RedChannel
On Wed, 2019-03-20 at 09:59 +, Frediano Ziglio wrote:
> Currently channel threading/handling is spread between RedQxl,
> RedWorker and RedChannel.
> Move more to RedChannel simplify RedQxl and RedWorker.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/cursor-channel.c| 4 +-
> server/cursor-channel.h| 4 +-
> server/display-channel.c | 2 +
> server/display-channel.h | 1 +
> server/red-channel.c | 111 +--
> server/red-qxl.c | 110 +-
> server/red-replay-qxl.c| 3 -
> server/red-stream-device.c | 2 +-
> server/red-worker.c| 133 ++-
> --
> server/red-worker.h| 46 ++---
> 10 files changed, 160 insertions(+), 256 deletions(-)
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 4220084f..e8af01b0 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -228,7 +228,8 @@ static void
> cursor_channel_send_item(RedChannelClient *rcc, RedPipeItem *pipe_it
> }
>
> CursorChannel* cursor_channel_new(RedsState *server, int id,
> - const SpiceCoreInterfaceInternal
> *core)
> + const SpiceCoreInterfaceInternal
> *core,
> + Dispatcher *dispatcher)
> {
> spice_debug("create cursor channel");
> return g_object_new(TYPE_CURSOR_CHANNEL,
> @@ -238,6 +239,7 @@ CursorChannel* cursor_channel_new(RedsState
> *server, int id,
> "id", id,
> "migration-flags", 0,
> "handle-acks", TRUE,
> +"dispatcher", dispatcher,
> NULL);
> }
>
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 603c2c0a..767325ea 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -21,6 +21,7 @@
>
> #include "common-graphics-channel.h"
> #include "red-parse-qxl.h"
> +#include "dispatcher.h"
>
> G_BEGIN_DECLS
>
> @@ -57,7 +58,8 @@ GType cursor_channel_get_type(void) G_GNUC_CONST;
> * CursorChannel thread.
> */
> CursorChannel* cursor_channel_new(RedsState *server, int id,
> - const SpiceCoreInterfaceInternal
> *core);
> + const SpiceCoreInterfaceInternal
> *core,
> + Dispatcher *dispatcher);
>
> void cursor_channel_reset (CursorChannel
> *cursor);
> void cursor_channel_do_init (CursorChannel
> *cursor);
> diff --git a/server/display-channel.c b/server/display-channel.c
> index e179abfd..cb052bfc 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2227,6 +2227,7 @@ static SpiceCanvas
> *image_surfaces_get(SpiceImageSurfaces *surfaces, uint32_t su
> DisplayChannel* display_channel_new(RedsState *reds,
> QXLInstance *qxl,
> const SpiceCoreInterfaceInternal
> *core,
> +Dispatcher *dispatcher,
> int migrate, int stream_video,
> GArray *video_codecs,
> uint32_t n_surfaces)
> @@ -2246,6 +2247,7 @@ DisplayChannel* display_channel_new(RedsState
> *reds,
> "n-surfaces", n_surfaces,
> "video-codecs", video_codecs,
> "handle-acks", TRUE,
> + "dispatcher", dispatcher,
> NULL);
> if (display) {
> display_channel_set_stream_video(display, stream_video);
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 948018cf..f64da0bd 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -99,6 +99,7 @@ struct Drawable {
> DisplayChannel*display_channel_new
> (RedsState *reds,
>
> QXLInstance *qxl,
>
> const SpiceCoreInterfaceInternal *core,
> +
> Dispatcher *dispatcher,
>
> int migrate,
>
> int stream_video,
>
> GArray *video_codecs,
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 1d88739e..8d05c971 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -70,6 +70,9 @@ struct RedChannelPrivate
> uint32_t type;
> uint32_t id;
>
> +/* "core" interface to register events.
> + * Can be thread specific.
Re: [Spice-devel] [PATCH spice-server 03/10] main-dispatcher: Remove main_dispatcher_self_handle_channel_event
>
> That's a weird function, but seems rather unrelated to the current
> series.
>
Agreed, was on the "dispatcher" line of change. Maybe the title should be
"main-dispatcher: Inline main_dispatcher_self_handle_channel_event"
(Inline instead of Remove) ?
> Acked-by: Jonathon Jongsma
>
>
> On Wed, 2019-03-20 at 09:59 +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio
> > ---
> > server/main-dispatcher.c | 13 ++---
> > 1 file changed, 2 insertions(+), 11 deletions(-)
> >
> > diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
> > index 82b25e6e..0dcfb2d4 100644
> > --- a/server/main-dispatcher.c
> > +++ b/server/main-dispatcher.c
> > @@ -149,22 +149,13 @@ typedef struct
> > MainDispatcherClientDisconnectMessage {
> > } MainDispatcherClientDisconnectMessage;
> >
> > /* channel_event - calls core->channel_event, must be done in main
> > thread */
> > -static void main_dispatcher_self_handle_channel_event(MainDispatcher
> > *self,
> > - int event,
> > - SpiceChannelEv
> > entInfo *info)
> > -{
> > -reds_handle_channel_event(self->priv->reds, event, info);
> > -}
> > -
> > static void main_dispatcher_handle_channel_event(void *opaque,
> > void *payload)
> > {
> > MainDispatcher *self = opaque;
> > MainDispatcherChannelEventMessage *channel_event = payload;
> >
> > -main_dispatcher_self_handle_channel_event(self,
> > - channel_event->event,
> > - channel_event->info);
> > +reds_handle_channel_event(self->priv->reds, channel_event-
> > >event, channel_event->info);
> > }
> >
> > void main_dispatcher_channel_event(MainDispatcher *self, int event,
> > SpiceChannelEventInfo *info)
> > @@ -172,7 +163,7 @@ void main_dispatcher_channel_event(MainDispatcher
> > *self, int event, SpiceChannel
> > MainDispatcherChannelEventMessage msg = {0,};
> >
> > if (pthread_self() ==
> > dispatcher_get_thread_id(DISPATCHER(self))) {
> > -main_dispatcher_self_handle_channel_event(self, event,
> > info);
> > +reds_handle_channel_event(self->priv->reds, event, info);
> > return;
> > }
> > msg.event = event;
>
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 05/10] main-dispatcher: Use dispatcher_send_message_generic
There should be some justification in the commit log about why this is
an improvement.
Jonathon
On Wed, 2019-03-20 at 09:59 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio
> ---
> server/main-dispatcher.c | 38 --
> 1 file changed, 8 insertions(+), 30 deletions(-)
>
> diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
> index 839e7242..904579a8 100644
> --- a/server/main-dispatcher.c
> +++ b/server/main-dispatcher.c
> @@ -121,15 +121,6 @@ main_dispatcher_init(MainDispatcher *self)
> self->priv = main_dispatcher_get_instance_private(self);
> }
>
> -enum {
> -MAIN_DISPATCHER_CHANNEL_EVENT = 0,
> -MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
> -MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
> -MAIN_DISPATCHER_CLIENT_DISCONNECT,
> -
> -MAIN_DISPATCHER_NUM_MESSAGES
> -};
> -
> typedef struct MainDispatcherChannelEventMessage {
> int event;
> SpiceChannelEventInfo *info;
> @@ -168,8 +159,8 @@ void main_dispatcher_channel_event(MainDispatcher
> *self, int event, SpiceChannel
> }
> msg.event = event;
> msg.info = info;
> -dispatcher_send_message(DISPATCHER(self),
> MAIN_DISPATCHER_CHANNEL_EVENT,
> -);
> +dispatcher_send_message_generic(DISPATCHER(self),
> main_dispatcher_handle_channel_event,
> +, sizeof(msg), false);
> }
>
>
> @@ -214,8 +205,8 @@ void
> main_dispatcher_seamless_migrate_dst_complete(MainDispatcher *self,
> }
>
> msg.client = g_object_ref(client);
> -dispatcher_send_message(DISPATCHER(self),
> MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
> -);
> +dispatcher_send_message_generic(DISPATCHER(self),
> main_dispatcher_handle_migrate_complete,
> +, sizeof(msg), false);
> }
>
> void main_dispatcher_set_mm_time_latency(MainDispatcher *self,
> RedClient *client, uint32_t latency)
> @@ -229,8 +220,8 @@ void
> main_dispatcher_set_mm_time_latency(MainDispatcher *self, RedClient
> *client
>
> msg.client = g_object_ref(client);
> msg.latency = latency;
> -dispatcher_send_message(DISPATCHER(self),
> MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
> -);
> +dispatcher_send_message_generic(DISPATCHER(self),
> main_dispatcher_handle_mm_time_latency,
> +, sizeof(msg), false);
> }
>
> void main_dispatcher_client_disconnect(MainDispatcher *self,
> RedClient *client)
> @@ -240,8 +231,8 @@ void
> main_dispatcher_client_disconnect(MainDispatcher *self, RedClient
> *client)
> if (!red_client_is_disconnecting(client)) {
> spice_debug("client %p", client);
> msg.client = g_object_ref(client);
> -dispatcher_send_message(DISPATCHER(self),
> MAIN_DISPATCHER_CLIENT_DISCONNECT,
> -);
> +dispatcher_send_message_generic(DISPATCHER(self),
> main_dispatcher_handle_client_disconnect,
> +, sizeof(msg), false);
> } else {
> spice_debug("client %p already during disconnection",
> client);
> }
> @@ -263,7 +254,6 @@ MainDispatcher* main_dispatcher_new(RedsState
> *reds)
> {
> MainDispatcher *self = g_object_new(TYPE_MAIN_DISPATCHER,
> "spice-server", reds,
> -"max-message-type",
> MAIN_DISPATCHER_NUM_MESSAGES,
> NULL);
> return self;
> }
> @@ -280,18 +270,6 @@ void main_dispatcher_constructed(GObject
> *object)
> dispatcher_get_recv_fd(DISPATCHER(self))
> ,
> SPICE_WATCH_EVENT_READ,
> dispatcher_handle_read,
> DISPATCHER(self));
> -dispatcher_register_handler(DISPATCHER(self),
> MAIN_DISPATCHER_CHANNEL_EVENT,
> -main_dispatcher_handle_channel_event
> ,
> -sizeof(MainDispatcherChannelEventMes
> sage), false);
> -dispatcher_register_handler(DISPATCHER(self),
> MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
> -main_dispatcher_handle_migrate_compl
> ete,
> -sizeof(MainDispatcherMigrateSeamless
> DstCompleteMessage), false);
> -dispatcher_register_handler(DISPATCHER(self),
> MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
> -main_dispatcher_handle_mm_time_laten
> cy,
> -sizeof(MainDispatcherMmTimeLatencyMe
> ssage), false);
> -dispatcher_register_handler(DISPATCHER(self),
> MAIN_DISPATCHER_CLIENT_DISCONNECT,
> -main_dispatcher_handle_client_discon
> nect,
> -sizeof(MainDispatcherClientDisconnec
> tMessage), false);
> }
>
> static void main_dispatcher_finalize(GObject *object)
Re: [Spice-devel] [PATCH spice-server 02/10] dispatcher: Allows to manage message without registering them
>
> On Wed, 2019-03-20 at 09:59 +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio
> > ---
> > server/dispatcher.c | 71 ++-
> > --
> > server/dispatcher.h | 15 ++
> > 2 files changed, 62 insertions(+), 24 deletions(-)
> >
> > diff --git a/server/dispatcher.c b/server/dispatcher.c
> > index 5f839ec4..0b18b32d 100644
> > --- a/server/dispatcher.c
> > +++ b/server/dispatcher.c
> > @@ -38,10 +38,13 @@
> > static void setup_dummy_signal_handler(void);
> > #endif
> >
> > +#define DISPATCHER_GENERIC_TYPE 0x7fffu
>
> I don't know if 'generic' is the right term here. The message itself is
> not really generic, it is just not pre-registered as a message type.
>
> Some suggestions for an alternative term (though I don't think any of
> them are perfect):
>
> "ad hoc"
> "unregistered"
> "custom"
>
Custom would be fine.
>
> > +
> > typedef struct DispatcherMessage {
> > -size_t size;
> > -bool ack;
> > dispatcher_handle_message handler;
> > +uint32_t size;
> > +uint32_t type:31;
> > +uint32_t ack:1;
>
> IMO, this bitfield seems a little like unnecessary optimization. Sure,
> it saves some data being sent through the dispatcher, but se're already
> sending significantly more 'header' data through the dispatcher, so an
> extra byte or so won't make that much difference, will it?
>
Actually are 8 bytes more, that is 50% more.
Beside small optimization the other reason is also Valgrind like checks,
using bool would leave hole which can be detected by memory debuggers
as "use of unitialised data".
> (e.g. previously we just sent a uin32_t 'type' and looked up the
> DispatcherMessage struct from memory. Now we're sending the entire
> DispatcherMessage struct).
>
> If we really wanted to avoid writing extra data, we could change the
> dispatcher protocol to something like:
>
> write message_type
> if (message_type == GENERIC)
> write DispatcherMessage struct
> write payload
>
> Then the DispatcherMessage struct would only get sent for the new
> GENERIC message type, and would still get looked up from memory for the
> old registered message types...
>
It depends on how extensively you want to use these custom/generic
messages in the future I suppose. Using 3 writes instead of 2 and more
logic seems more complicated to me.
I would personally also use writev/readv to avoid extra system calls
or packets (we use sockets here) or even rewrite using eventfd/memory
sharing... but that's maybe more paranoia I suppose.
> > } DispatcherMessage;
> >
> > struct DispatcherPrivate {
> > @@ -249,12 +252,11 @@ static int write_safe(int fd, uint8_t *buf,
> > size_t size)
> > static int dispatcher_handle_single_read(Dispatcher *dispatcher)
> > {
> > int ret;
> > -uint32_t type;
> > -DispatcherMessage *msg = NULL;
> > -uint8_t *payload = dispatcher->priv->payload;
> > +DispatcherMessage msg[1];
> > +uint8_t *payload;
> > uint32_t ack = ACK;
> >
> > -if ((ret = read_safe(dispatcher->priv->recv_fd, (uint8_t*),
> > sizeof(type), 0)) == -1) {
> > +if ((ret = read_safe(dispatcher->priv->recv_fd, (uint8_t*)msg,
> > sizeof(msg), 0)) == -1) {
> > g_warning("error reading from dispatcher: %d", errno);
> > return 0;
> > }
> > @@ -262,28 +264,28 @@ static int
> > dispatcher_handle_single_read(Dispatcher *dispatcher)
> > /* no message */
> > return 0;
> > }
> > -if (type >= dispatcher->priv->max_message_type) {
> > -spice_error("Invalid message type for this dispatcher: %u",
> > type);
> > -return 0;
> > +if (SPICE_UNLIKELY(msg->size > dispatcher->priv->payload_size))
> > {
>
> I think we've decided to move away from using spice types that are
> already implemented in glib. So I'd suggest G_UNLIKELY here.
>
I'll do it.
> > +dispatcher->priv->payload = g_realloc(dispatcher->priv-
> > >payload, msg->size);
> > +dispatcher->priv->payload_size = msg->size;
> > }
> > -msg = >priv->messages[type];
> > +payload = dispatcher->priv->payload;
> > if (read_safe(dispatcher->priv->recv_fd, payload, msg->size, 1)
> > == -1) {
> > g_warning("error reading from dispatcher: %d", errno);
> > /* TODO: close socketpair? */
> > return 0;
> > }
> > -if (dispatcher->priv->any_handler) {
> > -dispatcher->priv->any_handler(dispatcher->priv->opaque,
> > type, payload);
> > +if (dispatcher->priv->any_handler && msg->type !=
> > DISPATCHER_GENERIC_TYPE) {
> > +dispatcher->priv->any_handler(dispatcher->priv->opaque, msg-
> > >type, payload);
> > }
> > if (msg->handler) {
> > msg->handler(dispatcher->priv->opaque, payload);
> > } else {
> > -g_warning("error: no handler for message type %d", type);
> > +g_warning("error: no handler for message type %d", msg-
> > >type);
> > }
> > if (msg->ack) {
> > if
Re: [Spice-devel] [PATCH spice-server 04/10] main-dispatcher: Use reds as opaque for dispatcher
Acked-by: Jonathon Jongsma
On Wed, 2019-03-20 at 09:59 +, Frediano Ziglio wrote:
> No reason to pass through MainDispatcher, the purpose of
> MainDispatcher is to call reds functions from the right thread.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/main-dispatcher.c | 18 +-
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
> index 0dcfb2d4..839e7242 100644
> --- a/server/main-dispatcher.c
> +++ b/server/main-dispatcher.c
> @@ -152,10 +152,10 @@ typedef struct
> MainDispatcherClientDisconnectMessage {
> static void main_dispatcher_handle_channel_event(void *opaque,
> void *payload)
> {
> -MainDispatcher *self = opaque;
> +RedsState *reds = opaque;
> MainDispatcherChannelEventMessage *channel_event = payload;
>
> -reds_handle_channel_event(self->priv->reds, channel_event-
> >event, channel_event->info);
> +reds_handle_channel_event(reds, channel_event->event,
> channel_event->info);
> }
>
> void main_dispatcher_channel_event(MainDispatcher *self, int event,
> SpiceChannelEventInfo *info)
> @@ -176,30 +176,30 @@ void
> main_dispatcher_channel_event(MainDispatcher *self, int event,
> SpiceChannel
> static void main_dispatcher_handle_migrate_complete(void *opaque,
> void *payload)
> {
> -MainDispatcher *self = opaque;
> +RedsState *reds = opaque;
> MainDispatcherMigrateSeamlessDstCompleteMessage *mig_complete =
> payload;
>
> -reds_on_client_seamless_migrate_complete(self->priv->reds,
> mig_complete->client);
> +reds_on_client_seamless_migrate_complete(reds, mig_complete-
> >client);
> g_object_unref(mig_complete->client);
> }
>
> static void main_dispatcher_handle_mm_time_latency(void *opaque,
> void *payload)
> {
> -MainDispatcher *self = opaque;
> +RedsState *reds = opaque;
> MainDispatcherMmTimeLatencyMessage *msg = payload;
> -reds_set_client_mm_time_latency(self->priv->reds, msg->client,
> msg->latency);
> +reds_set_client_mm_time_latency(reds, msg->client, msg-
> >latency);
> g_object_unref(msg->client);
> }
>
> static void main_dispatcher_handle_client_disconnect(void *opaque,
> void *payload)
> {
> -MainDispatcher *self = opaque;
> +RedsState *reds = opaque;
> MainDispatcherClientDisconnectMessage *msg = payload;
>
> spice_debug("client=%p", msg->client);
> -reds_client_disconnect(self->priv->reds, msg->client);
> +reds_client_disconnect(reds, msg->client);
> g_object_unref(msg->client);
> }
>
> @@ -273,7 +273,7 @@ void main_dispatcher_constructed(GObject *object)
> MainDispatcher *self = MAIN_DISPATCHER(object);
>
> G_OBJECT_CLASS(main_dispatcher_parent_class)-
> >constructed(object);
> -dispatcher_set_opaque(DISPATCHER(self), self);
> +dispatcher_set_opaque(DISPATCHER(self), self->priv->reds);
>
> self->priv->watch =
> reds_core_watch_add(self->priv->reds,
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 03/10] main-dispatcher: Remove main_dispatcher_self_handle_channel_event
That's a weird function, but seems rather unrelated to the current
series.
Acked-by: Jonathon Jongsma
On Wed, 2019-03-20 at 09:59 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio
> ---
> server/main-dispatcher.c | 13 ++---
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
> index 82b25e6e..0dcfb2d4 100644
> --- a/server/main-dispatcher.c
> +++ b/server/main-dispatcher.c
> @@ -149,22 +149,13 @@ typedef struct
> MainDispatcherClientDisconnectMessage {
> } MainDispatcherClientDisconnectMessage;
>
> /* channel_event - calls core->channel_event, must be done in main
> thread */
> -static void main_dispatcher_self_handle_channel_event(MainDispatcher
> *self,
> - int event,
> - SpiceChannelEv
> entInfo *info)
> -{
> -reds_handle_channel_event(self->priv->reds, event, info);
> -}
> -
> static void main_dispatcher_handle_channel_event(void *opaque,
> void *payload)
> {
> MainDispatcher *self = opaque;
> MainDispatcherChannelEventMessage *channel_event = payload;
>
> -main_dispatcher_self_handle_channel_event(self,
> - channel_event->event,
> - channel_event->info);
> +reds_handle_channel_event(self->priv->reds, channel_event-
> >event, channel_event->info);
> }
>
> void main_dispatcher_channel_event(MainDispatcher *self, int event,
> SpiceChannelEventInfo *info)
> @@ -172,7 +163,7 @@ void main_dispatcher_channel_event(MainDispatcher
> *self, int event, SpiceChannel
> MainDispatcherChannelEventMessage msg = {0,};
>
> if (pthread_self() ==
> dispatcher_get_thread_id(DISPATCHER(self))) {
> -main_dispatcher_self_handle_channel_event(self, event,
> info);
> +reds_handle_channel_event(self->priv->reds, event, info);
> return;
> }
> msg.event = event;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 02/10] dispatcher: Allows to manage message without registering them
On Wed, 2019-03-20 at 09:59 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio
> ---
> server/dispatcher.c | 71 ++-
> --
> server/dispatcher.h | 15 ++
> 2 files changed, 62 insertions(+), 24 deletions(-)
>
> diff --git a/server/dispatcher.c b/server/dispatcher.c
> index 5f839ec4..0b18b32d 100644
> --- a/server/dispatcher.c
> +++ b/server/dispatcher.c
> @@ -38,10 +38,13 @@
> static void setup_dummy_signal_handler(void);
> #endif
>
> +#define DISPATCHER_GENERIC_TYPE 0x7fffu
I don't know if 'generic' is the right term here. The message itself is
not really generic, it is just not pre-registered as a message type.
Some suggestions for an alternative term (though I don't think any of
them are perfect):
"ad hoc"
"unregistered"
"custom"
> +
> typedef struct DispatcherMessage {
> -size_t size;
> -bool ack;
> dispatcher_handle_message handler;
> +uint32_t size;
> +uint32_t type:31;
> +uint32_t ack:1;
IMO, this bitfield seems a little like unnecessary optimization. Sure,
it saves some data being sent through the dispatcher, but se're already
sending significantly more 'header' data through the dispatcher, so an
extra byte or so won't make that much difference, will it?
(e.g. previously we just sent a uin32_t 'type' and looked up the
DispatcherMessage struct from memory. Now we're sending the entire
DispatcherMessage struct).
If we really wanted to avoid writing extra data, we could change the
dispatcher protocol to something like:
write message_type
if (message_type == GENERIC)
write DispatcherMessage struct
write payload
Then the DispatcherMessage struct would only get sent for the new
GENERIC message type, and would still get looked up from memory for the
old registered message types...
> } DispatcherMessage;
>
> struct DispatcherPrivate {
> @@ -249,12 +252,11 @@ static int write_safe(int fd, uint8_t *buf,
> size_t size)
> static int dispatcher_handle_single_read(Dispatcher *dispatcher)
> {
> int ret;
> -uint32_t type;
> -DispatcherMessage *msg = NULL;
> -uint8_t *payload = dispatcher->priv->payload;
> +DispatcherMessage msg[1];
> +uint8_t *payload;
> uint32_t ack = ACK;
>
> -if ((ret = read_safe(dispatcher->priv->recv_fd, (uint8_t*),
> sizeof(type), 0)) == -1) {
> +if ((ret = read_safe(dispatcher->priv->recv_fd, (uint8_t*)msg,
> sizeof(msg), 0)) == -1) {
> g_warning("error reading from dispatcher: %d", errno);
> return 0;
> }
> @@ -262,28 +264,28 @@ static int
> dispatcher_handle_single_read(Dispatcher *dispatcher)
> /* no message */
> return 0;
> }
> -if (type >= dispatcher->priv->max_message_type) {
> -spice_error("Invalid message type for this dispatcher: %u",
> type);
> -return 0;
> +if (SPICE_UNLIKELY(msg->size > dispatcher->priv->payload_size))
> {
I think we've decided to move away from using spice types that are
already implemented in glib. So I'd suggest G_UNLIKELY here.
> +dispatcher->priv->payload = g_realloc(dispatcher->priv-
> >payload, msg->size);
> +dispatcher->priv->payload_size = msg->size;
> }
> -msg = >priv->messages[type];
> +payload = dispatcher->priv->payload;
> if (read_safe(dispatcher->priv->recv_fd, payload, msg->size, 1)
> == -1) {
> g_warning("error reading from dispatcher: %d", errno);
> /* TODO: close socketpair? */
> return 0;
> }
> -if (dispatcher->priv->any_handler) {
> -dispatcher->priv->any_handler(dispatcher->priv->opaque,
> type, payload);
> +if (dispatcher->priv->any_handler && msg->type !=
> DISPATCHER_GENERIC_TYPE) {
> +dispatcher->priv->any_handler(dispatcher->priv->opaque, msg-
> >type, payload);
> }
> if (msg->handler) {
> msg->handler(dispatcher->priv->opaque, payload);
> } else {
> -g_warning("error: no handler for message type %d", type);
> +g_warning("error: no handler for message type %d", msg-
> >type);
> }
> if (msg->ack) {
> if (write_safe(dispatcher->priv->recv_fd,
> (uint8_t*), sizeof(ack)) == -1) {
> -g_warning("error writing ack for message %d", type);
> +g_warning("error writing ack for message %d", msg-
> >type);
> /* TODO: close socketpair? */
> }
> }
> @@ -300,25 +302,22 @@ void dispatcher_handle_recv_read(Dispatcher
> *dispatcher)
> }
> }
>
> -void dispatcher_send_message(Dispatcher *dispatcher, uint32_t
> message_type,
> - void *payload)
> +static void
> +dispatcher_send_message_internal(Dispatcher *dispatcher, const
> DispatcherMessage*msg,
> + void *payload)
> {
> -DispatcherMessage *msg;
> uint32_t ack;
> int send_fd = dispatcher->priv->send_fd;
>
> -assert(dispatcher->priv->max_message_type > message_type);
> -
[Spice-devel] [PATCH spice-streaming-agent] Add support log logging statistics from plugins
Allows the plugins to add information to the log.
Signed-off-by: Frediano Ziglio
---
Not entirely happy to export a kind of C function, any suggestion
welcome
---
include/spice-streaming-agent/plugin.hpp | 5 +
src/concrete-agent.cpp | 16 ++--
src/concrete-agent.hpp | 6 ++
src/frame-log.cpp| 9 +
src/frame-log.hpp| 2 ++
src/spice-streaming-agent.cpp| 8 ++--
6 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/include/spice-streaming-agent/plugin.hpp
b/include/spice-streaming-agent/plugin.hpp
index 3b265d9..a51f060 100644
--- a/include/spice-streaming-agent/plugin.hpp
+++ b/include/spice-streaming-agent/plugin.hpp
@@ -116,6 +116,11 @@ public:
* \todo passing options to entry point instead?
*/
virtual const ConfigureOption* Options() const = 0;
+/*!
+ * Write something in the log.
+ */
+__attribute__ ((format (printf, 2, 3)))
+virtual void LogStat(const char* format, ...) = 0;
};
typedef bool PluginInitFunc(spice::streaming_agent::Agent* agent);
diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index f94aead..d279656 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -5,13 +5,15 @@
*/
#include
+#include "concrete-agent.hpp"
+#include "frame-log.hpp"
+
#include
#include
#include
#include
#include
-
-#include "concrete-agent.hpp"
+#include
using namespace spice::streaming_agent;
@@ -145,3 +147,13 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture(const
std::setlog_statv(format, ap);
+va_end(ap);
+}
+}
diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
index 99dcf54..aa4d6aa 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -14,6 +14,8 @@
namespace spice {
namespace streaming_agent {
+class FrameLog;
+
struct ConcreteConfigureOption: ConfigureOption
{
ConcreteConfigureOption(const char *name, const char *value)
@@ -33,11 +35,15 @@ public:
// pointer must remain valid
void AddOption(const char *name, const char *value);
FrameCapture *GetBestFrameCapture(const std::set&
codecs);
+void SetFrameLog(FrameLog *logger) { this->logger = logger; }
+__attribute__ ((format (printf, 2, 3)))
+void LogStat(const char* format, ...) override;
private:
bool PluginVersionIsCompatible(unsigned pluginVersion) const;
void LoadPlugin(const std::string _filename);
std::vector> plugins;
std::vector options;
+FrameLog *logger = nullptr;
};
}} // namespace spice::streaming_agent
diff --git a/src/frame-log.cpp b/src/frame-log.cpp
index 62fffc3..db6b652 100644
--- a/src/frame-log.cpp
+++ b/src/frame-log.cpp
@@ -52,6 +52,15 @@ void FrameLog::log_stat(const char* format, ...)
}
}
+void FrameLog::log_statv(const char* format, va_list ap)
+{
+if (log_file) {
+fprintf(log_file, "%" PRIu64 ": ", get_time());
+vfprintf(log_file, format, ap);
+fputc('\n', log_file);
+}
+}
+
void FrameLog::log_frame(const void* buffer, size_t buffer_size)
{
if (log_file) {
diff --git a/src/frame-log.hpp b/src/frame-log.hpp
index 8503345..a104723 100644
--- a/src/frame-log.hpp
+++ b/src/frame-log.hpp
@@ -24,6 +24,8 @@ public:
__attribute__ ((format (printf, 2, 3)))
void log_stat(const char* format, ...);
+__attribute__ ((format (printf, 2, 0)))
+void log_statv(const char* format, va_list ap);
void log_frame(const void* buffer, size_t buffer_size);
static uint64_t get_time();
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 9507a54..f262118 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -401,13 +401,15 @@ int main(int argc, char* argv[])
register_interrupts();
try {
+FrameLog frame_log(log_filename, log_binary, log_frames);
+
+agent.SetFrameLog(_log);
+
// register built-in plugins
MjpegPlugin::Register();
agent.LoadPlugins(pluginsdir);
-FrameLog frame_log(log_filename, log_binary, log_frames);
-
for (const std::string& arg: old_args) {
frame_log.log_stat("Args: %s", arg.c_str());
}
@@ -419,8 +421,10 @@ int main(int argc, char* argv[])
cursor_updater.detach();
do_capture(stream_port, frame_log);
+agent.SetFrameLog(nullptr);
}
catch (std::exception ) {
+agent.SetFrameLog(nullptr);
syslog(LOG_ERR, "%s", err.what());
return EXIT_FAILURE;
}
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server v2 2/2] worker: Fix potential sprintf overflow
From: Christophe Fergeau If worker->qxl->id is bigger than 0x7ff (in other words, it's a negative signed int) then printf(worker_str, "display[%d]", worker->qxl->id); will need: "display[]" -> 9 bytes %d -> 11 bytes The trailing \0 will thus overflow our 20 bytes destination. As QXLInstance::id should be an unsigned int, this commit changes the format string to use %u. This also switches to snprintf. --- server/red-worker.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/red-worker.c b/server/red-worker.c index 8051d1e4..50612aca 100644 --- a/server/red-worker.c +++ b/server/red-worker.c @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, worker->zlib_glz_state = reds_get_zlib_glz_state(reds); worker->driver_cap_monitors_config = 0; char worker_str[SPICE_STAT_NODE_NAME_MAX]; -sprintf(worker_str, "display[%d]", worker->qxl->id); +snprintf(worker_str, sizeof(worker_str), "display[%u]", (unsigned int)worker->qxl->id); stat_init_node(>stat, reds, NULL, worker_str, TRUE); stat_init_counter(>wakeup_counter, reds, >stat, "wakeups", TRUE); stat_init_counter(>command_counter, reds, >stat, "commands", TRUE); -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server v2 1/2] red-worker: Use mnemonic for statistic buffer
Signed-off-by: Frediano Ziglio --- I was wondering where that "20" came. --- 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 3cb12b9c..8051d1e4 100644 --- a/server/red-worker.c +++ b/server/red-worker.c @@ -32,6 +32,7 @@ #include #include +#include #include #include #include @@ -1289,7 +1290,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, worker->jpeg_state = reds_get_jpeg_state(reds); worker->zlib_glz_state = reds_get_zlib_glz_state(reds); worker->driver_cap_monitors_config = 0; -char worker_str[20]; +char worker_str[SPICE_STAT_NODE_NAME_MAX]; sprintf(worker_str, "display[%d]", worker->qxl->id); stat_init_node(>stat, reds, NULL, worker_str, TRUE); stat_init_counter(>wakeup_counter, reds, >stat, "wakeups", TRUE); -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 01/10] dispatcher: Use NULL for pointer check
Acked-by: Jonathon Jongsma On Wed, 2019-03-20 at 09:59 +, Frediano Ziglio wrote: > handler is a pointer, check for NULL, not 0. > > Signed-off-by: Frediano Ziglio > --- > server/dispatcher.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/server/dispatcher.c b/server/dispatcher.c > index ce487f63..5f839ec4 100644 > --- a/server/dispatcher.c > +++ b/server/dispatcher.c > @@ -341,7 +341,7 @@ void dispatcher_register_handler(Dispatcher > *dispatcher, uint32_t message_type, > DispatcherMessage *msg; > > assert(message_type < dispatcher->priv->max_message_type); > -assert(dispatcher->priv->messages[message_type].handler == 0); > +assert(dispatcher->priv->messages[message_type].handler == > NULL); > msg = >priv->messages[message_type]; > msg->handler = handler; > msg->size = size; ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 2/2] red-worker: Avoid possible string formatting warning
On Wed, Mar 20, 2019 at 11:20:20AM -0400, Frediano Ziglio wrote: > > > > On Wed, Mar 20, 2019 at 02:51:29PM +, Frediano Ziglio wrote: > > > Although id is not supposed to be big prevent possible > > > warning/overflow. > > > > > > Signed-off-by: Frediano Ziglio > > > --- > > > server/red-worker.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > This was signaled by Christophe Fergeau > > > > > > diff --git a/server/red-worker.c b/server/red-worker.c > > > index 8051d1e4..a25a0cd8 100644 > > > --- a/server/red-worker.c > > > +++ b/server/red-worker.c > > > @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, > > > worker->zlib_glz_state = reds_get_zlib_glz_state(reds); > > > worker->driver_cap_monitors_config = 0; > > > char worker_str[SPICE_STAT_NODE_NAME_MAX]; > > > -sprintf(worker_str, "display[%d]", worker->qxl->id); > > > +snprintf(worker_str, sizeof(worker_str), "display[%d]", > > > worker->qxl->id); > > > > You pointed out that in the protocol, the id is 8 bits, so I'd change to > > worker->qxl->id & 0xff while at it. > > > > Note that with SPICE_STAT_NODE_NAME_MAX (which is 20), you can > > still get snprintf to misbehave: > > "display[]" is 9 bytes > > %d may need 11 bytes to be printed (if id is less than (unsigned > > int)-40) > > so we'd be need 20 bytes in the buffer plus the trailing \0. > > > > Christophe > > > > Your patch was better and it has a better explanation. > > The number you brought as example is weird! It does not fit into > a 32 bit so cannot be worker->qxl->id. Yep, sorry, unsigned/signed mix up in my head ;) It should have been -20 > As we currently support only > architectures where int is a 32 bit the (unsigned int)whatever will > fit into 10 characters however formatting with %d will be converted > to signed (so int) which won't fit in 10 characters (with the > additional sign it requires 11 characters). Yep, this is what I meant, with a wrong example. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 2/2] red-worker: Avoid possible string formatting warning
> > On Wed, Mar 20, 2019 at 02:51:29PM +, Frediano Ziglio wrote: > > Although id is not supposed to be big prevent possible > > warning/overflow. > > > > Signed-off-by: Frediano Ziglio > > --- > > server/red-worker.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > This was signaled by Christophe Fergeau > > > > diff --git a/server/red-worker.c b/server/red-worker.c > > index 8051d1e4..a25a0cd8 100644 > > --- a/server/red-worker.c > > +++ b/server/red-worker.c > > @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, > > worker->zlib_glz_state = reds_get_zlib_glz_state(reds); > > worker->driver_cap_monitors_config = 0; > > char worker_str[SPICE_STAT_NODE_NAME_MAX]; > > -sprintf(worker_str, "display[%d]", worker->qxl->id); > > +snprintf(worker_str, sizeof(worker_str), "display[%d]", > > worker->qxl->id); > > You pointed out that in the protocol, the id is 8 bits, so I'd change to > worker->qxl->id & 0xff while at it. > > Note that with SPICE_STAT_NODE_NAME_MAX (which is 20), you can > still get snprintf to misbehave: > "display[]" is 9 bytes > %d may need 11 bytes to be printed (if id is less than (unsigned > int)-40) > so we'd be need 20 bytes in the buffer plus the trailing \0. > > Christophe > Your patch was better and it has a better explanation. The number you brought as example is weird! It does not fit into a 32 bit so cannot be worker->qxl->id. As we currently support only architectures where int is a 32 bit the (unsigned int)whatever will fit into 10 characters however formatting with %d will be converted to signed (so int) which won't fit in 10 characters (with the additional sign it requires 11 characters). Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 2/2] red-worker: Avoid possible string formatting warning
On Wed, Mar 20, 2019 at 02:51:29PM +, Frediano Ziglio wrote: > Although id is not supposed to be big prevent possible > warning/overflow. > > Signed-off-by: Frediano Ziglio > --- > server/red-worker.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > This was signaled by Christophe Fergeau > > diff --git a/server/red-worker.c b/server/red-worker.c > index 8051d1e4..a25a0cd8 100644 > --- a/server/red-worker.c > +++ b/server/red-worker.c > @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, > worker->zlib_glz_state = reds_get_zlib_glz_state(reds); > worker->driver_cap_monitors_config = 0; > char worker_str[SPICE_STAT_NODE_NAME_MAX]; > -sprintf(worker_str, "display[%d]", worker->qxl->id); > +snprintf(worker_str, sizeof(worker_str), "display[%d]", worker->qxl->id); You pointed out that in the protocol, the id is 8 bits, so I'd change to worker->qxl->id & 0xff while at it. Note that with SPICE_STAT_NODE_NAME_MAX (which is 20), you can still get snprintf to misbehave: "display[]" is 9 bytes %d may need 11 bytes to be printed (if id is less than (unsigned int)-40) so we'd be need 20 bytes in the buffer plus the trailing \0. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 1/2] red-worker: Use mnemonic for statistic buffer
Signed-off-by: Frediano Ziglio --- I was wondering where that "20" came. --- 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 3cb12b9c..8051d1e4 100644 --- a/server/red-worker.c +++ b/server/red-worker.c @@ -32,6 +32,7 @@ #include #include +#include #include #include #include @@ -1289,7 +1290,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, worker->jpeg_state = reds_get_jpeg_state(reds); worker->zlib_glz_state = reds_get_zlib_glz_state(reds); worker->driver_cap_monitors_config = 0; -char worker_str[20]; +char worker_str[SPICE_STAT_NODE_NAME_MAX]; sprintf(worker_str, "display[%d]", worker->qxl->id); stat_init_node(>stat, reds, NULL, worker_str, TRUE); stat_init_counter(>wakeup_counter, reds, >stat, "wakeups", TRUE); -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 2/2] red-worker: Avoid possible string formatting warning
Although id is not supposed to be big prevent possible warning/overflow. Signed-off-by: Frediano Ziglio --- server/red-worker.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) This was signaled by Christophe Fergeau diff --git a/server/red-worker.c b/server/red-worker.c index 8051d1e4..a25a0cd8 100644 --- a/server/red-worker.c +++ b/server/red-worker.c @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, worker->zlib_glz_state = reds_get_zlib_glz_state(reds); worker->driver_cap_monitors_config = 0; char worker_str[SPICE_STAT_NODE_NAME_MAX]; -sprintf(worker_str, "display[%d]", worker->qxl->id); +snprintf(worker_str, sizeof(worker_str), "display[%d]", worker->qxl->id); stat_init_node(>stat, reds, NULL, worker_str, TRUE); stat_init_counter(>wakeup_counter, reds, >stat, "wakeups", TRUE); stat_init_counter(>command_counter, reds, >stat, "commands", TRUE); -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 10/10] Make some function static
Now that stuff are a bit more on their correct place some
function can be static.
Signed-off-by: Frediano Ziglio
---
server/cursor-channel.c | 11 ---
server/cursor-channel.h | 10 --
server/display-channel.c | 12 +---
server/display-channel.h | 5 -
4 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index d1294322..c88e5cd1 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -335,9 +335,14 @@ void cursor_channel_set_mouse_mode(CursorChannel *cursor,
uint32_t mode)
cursor->mouse_mode = mode;
}
-void cursor_channel_connect(CursorChannel *cursor, RedClient *client,
RedStream *stream,
-int migrate,
-RedChannelCapabilities *caps)
+/**
+ * Connect a new client to CursorChannel.
+ * This is the equivalent of RedChannel client connect callback.
+ * See comment on cursor_channel_new.
+ */
+static void
+cursor_channel_connect(CursorChannel *cursor, RedClient *client, RedStream
*stream,
+ int migrate, RedChannelCapabilities *caps)
{
CursorChannelClient *ccc;
diff --git a/server/cursor-channel.h b/server/cursor-channel.h
index 767325ea..ce1b92cc 100644
--- a/server/cursor-channel.h
+++ b/server/cursor-channel.h
@@ -66,16 +66,6 @@ void cursor_channel_do_init
(CursorChannel *cursor);
void cursor_channel_process_cmd (CursorChannel *cursor,
RedCursorCmd *cursor_cmd);
void cursor_channel_set_mouse_mode(CursorChannel *cursor,
uint32_t mode);
-/**
- * Connect a new client to CursorChannel.
- * This is the equivalent of RedChannel client connect callback.
- * See comment on cursor_channel_new.
- */
-void cursor_channel_connect (CursorChannel *cursor,
RedClient *client,
- RedStream *stream,
- int migrate,
- RedChannelCapabilities *caps);
-
/**
* Migrate a client channel from a CursorChannel.
* This is the equivalent of RedChannel client migrate callback.
diff --git a/server/display-channel.c b/server/display-channel.c
index fde4d5c3..b797159f 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -26,6 +26,12 @@
G_DEFINE_TYPE(DisplayChannel, display_channel, TYPE_COMMON_GRAPHICS_CHANNEL)
+static void display_channel_connect(RedChannel *channel, RedClient *client,
+RedStream *stream, int migration,
+RedChannelCapabilities *caps);
+static void display_channel_disconnect(RedChannelClient *rcc);
+static void display_channel_migrate(RedChannelClient *rcc);
+
enum {
PROP0,
PROP_N_SURFACES,
@@ -2602,7 +2608,7 @@ void display_channel_set_running(DisplayChannel *display,
bool running)
}
}
-void
+static void
display_channel_connect(RedChannel *channel, RedClient *client,
RedStream *stream, int migration,
RedChannelCapabilities *caps)
@@ -2625,7 +2631,7 @@ display_channel_connect(RedChannel *channel, RedClient
*client,
dcc_start(dcc);
}
-void display_channel_disconnect(RedChannelClient *rcc)
+static void display_channel_disconnect(RedChannelClient *rcc)
{
DisplayChannel *display =
DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
@@ -2652,7 +2658,7 @@ static void red_migrate_display(DisplayChannel *display,
RedChannelClient *rcc)
}
}
-void display_channel_migrate(RedChannelClient *rcc)
+static void display_channel_migrate(RedChannelClient *rcc)
{
DisplayChannel *display =
DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
red_migrate_display(display, rcc);
diff --git a/server/display-channel.h b/server/display-channel.h
index 989bf52b..bdf2f059 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -159,11 +159,6 @@ void display_channel_reset_image_cache(DisplayChannel
*self);
void display_channel_debug_oom(DisplayChannel *display, const char *msg);
void display_channel_set_running(DisplayChannel *display, bool running);
-void display_channel_connect(RedChannel *channel, RedClient *client,
- RedStream *stream, int migration,
- RedChannelCapabilities *caps);
-void display_channel_disconnect(RedChannelClient *rcc);
-void display_channel_migrate(RedChannelClient *rcc);
G_END_DECLS
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 06/10] Move thread/dispatching handling to RedChannel
Currently channel threading/handling is spread between RedQxl,
RedWorker and RedChannel.
Move more to RedChannel simplify RedQxl and RedWorker.
Signed-off-by: Frediano Ziglio
---
server/cursor-channel.c| 4 +-
server/cursor-channel.h| 4 +-
server/display-channel.c | 2 +
server/display-channel.h | 1 +
server/red-channel.c | 111 +--
server/red-qxl.c | 110 +-
server/red-replay-qxl.c| 3 -
server/red-stream-device.c | 2 +-
server/red-worker.c| 133 ++---
server/red-worker.h| 46 ++---
10 files changed, 160 insertions(+), 256 deletions(-)
diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 4220084f..e8af01b0 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -228,7 +228,8 @@ static void cursor_channel_send_item(RedChannelClient *rcc,
RedPipeItem *pipe_it
}
CursorChannel* cursor_channel_new(RedsState *server, int id,
- const SpiceCoreInterfaceInternal *core)
+ const SpiceCoreInterfaceInternal *core,
+ Dispatcher *dispatcher)
{
spice_debug("create cursor channel");
return g_object_new(TYPE_CURSOR_CHANNEL,
@@ -238,6 +239,7 @@ CursorChannel* cursor_channel_new(RedsState *server, int id,
"id", id,
"migration-flags", 0,
"handle-acks", TRUE,
+"dispatcher", dispatcher,
NULL);
}
diff --git a/server/cursor-channel.h b/server/cursor-channel.h
index 603c2c0a..767325ea 100644
--- a/server/cursor-channel.h
+++ b/server/cursor-channel.h
@@ -21,6 +21,7 @@
#include "common-graphics-channel.h"
#include "red-parse-qxl.h"
+#include "dispatcher.h"
G_BEGIN_DECLS
@@ -57,7 +58,8 @@ GType cursor_channel_get_type(void) G_GNUC_CONST;
* CursorChannel thread.
*/
CursorChannel* cursor_channel_new(RedsState *server, int id,
- const SpiceCoreInterfaceInternal *core);
+ const SpiceCoreInterfaceInternal *core,
+ Dispatcher *dispatcher);
void cursor_channel_reset (CursorChannel *cursor);
void cursor_channel_do_init (CursorChannel *cursor);
diff --git a/server/display-channel.c b/server/display-channel.c
index e179abfd..cb052bfc 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -2227,6 +2227,7 @@ static SpiceCanvas *image_surfaces_get(SpiceImageSurfaces
*surfaces, uint32_t su
DisplayChannel* display_channel_new(RedsState *reds,
QXLInstance *qxl,
const SpiceCoreInterfaceInternal *core,
+Dispatcher *dispatcher,
int migrate, int stream_video,
GArray *video_codecs,
uint32_t n_surfaces)
@@ -2246,6 +2247,7 @@ DisplayChannel* display_channel_new(RedsState *reds,
"n-surfaces", n_surfaces,
"video-codecs", video_codecs,
"handle-acks", TRUE,
+ "dispatcher", dispatcher,
NULL);
if (display) {
display_channel_set_stream_video(display, stream_video);
diff --git a/server/display-channel.h b/server/display-channel.h
index 948018cf..f64da0bd 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -99,6 +99,7 @@ struct Drawable {
DisplayChannel*display_channel_new
(RedsState *reds,
QXLInstance *qxl,
const
SpiceCoreInterfaceInternal *core,
+
Dispatcher *dispatcher,
int
migrate,
int
stream_video,
GArray
*video_codecs,
diff --git a/server/red-channel.c b/server/red-channel.c
index 1d88739e..8d05c971 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -70,6 +70,9 @@ struct RedChannelPrivate
uint32_t type;
uint32_t id;
+/* "core" interface to register events.
+ * Can be thread specific.
+ */
SpiceCoreInterfaceInternal *core;
gboolean handle_acks;
@@ -90,12 +93,29 @@ struct RedChannelPrivate
// TODO: when different channel_clients are in different threads
// from Channel -> need to protect!
pthread_t thread_id;
+Dispatcher *dispatcher;
RedsState
[Spice-devel] [PATCH spice-server 01/10] dispatcher: Use NULL for pointer check
handler is a pointer, check for NULL, not 0. Signed-off-by: Frediano Ziglio --- server/dispatcher.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/dispatcher.c b/server/dispatcher.c index ce487f63..5f839ec4 100644 --- a/server/dispatcher.c +++ b/server/dispatcher.c @@ -341,7 +341,7 @@ void dispatcher_register_handler(Dispatcher *dispatcher, uint32_t message_type, DispatcherMessage *msg; assert(message_type < dispatcher->priv->max_message_type); -assert(dispatcher->priv->messages[message_type].handler == 0); +assert(dispatcher->priv->messages[message_type].handler == NULL); msg = >priv->messages[message_type]; msg->handler = handler; msg->size = size; -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 07/10] Propagate running property from RedWorker to DisplayChannel
This is a preparatory patch to allows DisplayChannel to check
if device is running.
Signed-off-by: Frediano Ziglio
---
server/display-channel-private.h | 1 +
server/display-channel.c | 8
server/display-channel.h | 2 ++
server/red-worker.c | 2 ++
4 files changed, 13 insertions(+)
diff --git a/server/display-channel-private.h b/server/display-channel-private.h
index 58179531..067c6418 100644
--- a/server/display-channel-private.h
+++ b/server/display-channel-private.h
@@ -89,6 +89,7 @@ struct DisplayChannelPrivate
uint32_t renderer;
int enable_jpeg;
int enable_zlib_glz_wrap;
+bool running;
/* A ring of pending drawables for this DisplayChannel, regardless of which
* surface they're associated with. This list is mainly used to flush older
diff --git a/server/display-channel.c b/server/display-channel.c
index cb052bfc..49a2bd48 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -2540,3 +2540,11 @@ void display_channel_debug_oom(DisplayChannel *display,
const char *msg)
ring_get_length(>priv->current_list),
red_channel_sum_pipes_size(channel));
}
+
+void display_channel_set_running(DisplayChannel *display, bool running)
+{
+if (running == display->priv->running) {
+return;
+}
+display->priv->running = running;
+}
diff --git a/server/display-channel.h b/server/display-channel.h
index f64da0bd..bdf2f059 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -158,6 +158,8 @@ void display_channel_reset_image_cache(DisplayChannel
*self);
void display_channel_debug_oom(DisplayChannel *display, const char *msg);
+void display_channel_set_running(DisplayChannel *display, bool running);
+
G_END_DECLS
#endif /* DISPLAY_CHANNEL_H_ */
diff --git a/server/red-worker.c b/server/red-worker.c
index 07329b17..c9c47133 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -588,6 +588,7 @@ static void handle_dev_stop(void *opaque, void *payload)
spice_assert(worker->running);
worker->running = FALSE;
+display_channel_set_running(worker->display_channel, false);
display_channel_free_glz_drawables(worker->display_channel);
display_channel_flush_all_surfaces(worker->display_channel);
@@ -616,6 +617,7 @@ static void handle_dev_start(void *opaque, void *payload)
display_channel_wait_for_migrate_data(worker->display_channel);
}
worker->running = TRUE;
+display_channel_set_running(worker->display_channel, true);
worker->event_timeout = 0;
guest_set_client_capabilities(worker);
}
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 09/10] Make channel client callbacks virtual functions
From: Jonathon Jongsma
Rather than having an API to register client callbacks for each channel
type, make them vfuncs.
Since the client callbacks are registered identically for each channel
of the same type, it doesn't make sense for to require these to be
registered separately for each object. It's cleaner to have these be
per-class properties, so they've been converted to virtual functions.
---
server/cursor-channel.c | 4
server/display-channel.c| 5 +
server/inputs-channel.c | 9
server/main-channel.c | 7 +++
server/red-channel.c| 42 ++---
server/red-channel.h| 19 +++--
server/red-stream-device.c | 5 -
server/red-worker.c | 12 ---
server/smartcard.c | 6 ++
server/sound.c | 18 +++-
server/spicevmc.c | 7 +++
server/stream-channel.c | 5 +
server/tests/test-channel.c | 13 +---
13 files changed, 55 insertions(+), 97 deletions(-)
diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index e8af01b0..d1294322 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -392,6 +392,10 @@ cursor_channel_class_init(CursorChannelClass *klass)
channel_class->handle_message = red_channel_client_handle_message;
channel_class->send_item = cursor_channel_send_item;
+
+// client callbacks
+channel_class->connect = (channel_client_connect_proc)
cursor_channel_connect;
+channel_class->migrate = cursor_channel_client_migrate;
}
static void
diff --git a/server/display-channel.c b/server/display-channel.c
index a322045e..fde4d5c3 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -2500,6 +2500,11 @@ display_channel_class_init(DisplayChannelClass *klass)
channel_class->handle_migrate_data = handle_migrate_data;
channel_class->handle_migrate_data_get_serial =
handle_migrate_data_get_serial;
+// client callbacks
+channel_class->connect = display_channel_connect;
+channel_class->disconnect = display_channel_disconnect;
+channel_class->migrate = display_channel_migrate;
+
g_object_class_install_property(object_class,
PROP_N_SURFACES,
g_param_spec_uint("n-surfaces",
diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index a7df62e2..eff5421a 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -542,17 +542,12 @@ InputsChannel* inputs_channel_new(RedsState *reds)
static void
inputs_channel_constructed(GObject *object)
{
-ClientCbs client_cbs = { NULL, };
InputsChannel *self = INPUTS_CHANNEL(object);
RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
SpiceCoreInterfaceInternal *core =
red_channel_get_core_interface(RED_CHANNEL(self));
G_OBJECT_CLASS(inputs_channel_parent_class)->constructed(object);
-client_cbs.connect = inputs_connect;
-client_cbs.migrate = inputs_migrate;
-red_channel_register_client_cbs(RED_CHANNEL(self), _cbs);
-
red_channel_set_cap(RED_CHANNEL(self), SPICE_INPUTS_CAP_KEY_SCANCODE);
reds_register_channel(reds, RED_CHANNEL(self));
@@ -596,6 +591,10 @@ inputs_channel_class_init(InputsChannelClass *klass)
channel_class->send_item = inputs_channel_send_item;
channel_class->handle_migrate_data = inputs_channel_handle_migrate_data;
channel_class->handle_migrate_flush_mark =
inputs_channel_handle_migrate_flush_mark;
+
+// client callbacks
+channel_class->connect = inputs_connect;
+channel_class->migrate = inputs_migrate;
}
static SpiceKbdInstance* inputs_channel_get_keyboard(InputsChannel *inputs)
diff --git a/server/main-channel.c b/server/main-channel.c
index f866fb4a..a48e74be 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -264,15 +264,11 @@ static void
main_channel_constructed(GObject *object)
{
MainChannel *self = MAIN_CHANNEL(object);
-ClientCbs client_cbs = { NULL, };
G_OBJECT_CLASS(main_channel_parent_class)->constructed(object);
red_channel_set_cap(RED_CHANNEL(self),
SPICE_MAIN_CAP_SEMI_SEAMLESS_MIGRATE);
red_channel_set_cap(RED_CHANNEL(self), SPICE_MAIN_CAP_SEAMLESS_MIGRATE);
-
-client_cbs.migrate = main_channel_client_migrate;
-red_channel_register_client_cbs(RED_CHANNEL(self), _cbs);
}
static void
@@ -295,6 +291,9 @@ main_channel_class_init(MainChannelClass *klass)
channel_class->send_item = main_channel_client_send_item;
channel_class->handle_migrate_flush_mark =
main_channel_handle_migrate_flush_mark;
channel_class->handle_migrate_data = main_channel_handle_migrate_data;
+
+// client callbacks
+channel_class->migrate = main_channel_client_migrate;
}
static int main_channel_connect_semi_seamless(MainChannel *main_channel)
diff --git a/server/red-channel.c b/server/red-channel.c
index 8d05c971..3c0d126e 100644
[Spice-devel] [PATCH spice-server 08/10] Move DisplayChannel callbacks from RedWorker to DisplayChannel
Signed-off-by: Frediano Ziglio
---
server/display-channel.c | 104 +++
server/display-channel.h | 5 ++
server/red-worker.c | 113 ++-
3 files changed, 112 insertions(+), 110 deletions(-)
diff --git a/server/display-channel.c b/server/display-channel.c
index 49a2bd48..a322045e 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -2541,10 +2541,114 @@ void display_channel_debug_oom(DisplayChannel
*display, const char *msg)
red_channel_sum_pipes_size(channel));
}
+static void guest_set_client_capabilities(DisplayChannel *display)
+{
+int i;
+RedChannelClient *rcc;
+uint8_t caps[SPICE_CAPABILITIES_SIZE] = { 0 };
+int caps_available[] = {
+SPICE_DISPLAY_CAP_SIZED_STREAM,
+SPICE_DISPLAY_CAP_MONITORS_CONFIG,
+SPICE_DISPLAY_CAP_COMPOSITE,
+SPICE_DISPLAY_CAP_A8_SURFACE,
+};
+QXLInterface *qif = qxl_get_interface(display->priv->qxl);
+
+if (!red_qxl_check_qxl_version(display->priv->qxl, 3, 2)) {
+return;
+}
+if (!qif->set_client_capabilities) {
+return;
+}
+#define SET_CAP(a,c)\
+((a)[(c) / 8] |= (1 << ((c) % 8)))
+
+#define CLEAR_CAP(a,c) \
+((a)[(c) / 8] &= ~(1 << ((c) % 8)))
+
+if (!display->priv->running) {
+return;
+}
+if ((red_channel_get_n_clients(RED_CHANNEL(display)) == 0)) {
+red_qxl_set_client_capabilities(display->priv->qxl, FALSE, caps);
+} else {
+// Take least common denominator
+for (i = 0 ; i < SPICE_N_ELEMENTS(caps_available); ++i) {
+SET_CAP(caps, caps_available[i]);
+}
+FOREACH_CLIENT(display, rcc) {
+for (i = 0 ; i < SPICE_N_ELEMENTS(caps_available); ++i) {
+if (!red_channel_client_test_remote_cap(rcc,
caps_available[i]))
+CLEAR_CAP(caps, caps_available[i]);
+}
+}
+red_qxl_set_client_capabilities(display->priv->qxl, TRUE, caps);
+}
+}
+
void display_channel_set_running(DisplayChannel *display, bool running)
{
if (running == display->priv->running) {
return;
}
display->priv->running = running;
+if (running) {
+guest_set_client_capabilities(display);
+}
+}
+
+void
+display_channel_connect(RedChannel *channel, RedClient *client,
+RedStream *stream, int migration,
+RedChannelCapabilities *caps)
+{
+DisplayChannel *display = DISPLAY_CHANNEL(channel);
+DisplayChannelClient *dcc;
+
+spice_debug("connect new client");
+
+// FIXME not sure how safe is reading directly from reds
+SpiceServer *reds = red_channel_get_server(channel);
+dcc = dcc_new(display, client, stream, migration, caps,
+ spice_server_get_image_compression(reds),
reds_get_jpeg_state(reds),
+ reds_get_zlib_glz_state(reds));
+if (!dcc) {
+return;
+}
+display_channel_update_compression(display, dcc);
+guest_set_client_capabilities(display);
+dcc_start(dcc);
+}
+
+void display_channel_disconnect(RedChannelClient *rcc)
+{
+DisplayChannel *display =
DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
+
+guest_set_client_capabilities(display);
+
+red_channel_client_disconnect(rcc);
+}
+
+static void red_migrate_display(DisplayChannel *display, RedChannelClient *rcc)
+{
+/* We need to stop the streams, and to send upgrade_items to the client.
+ * Otherwise, (1) the client might display lossy regions that we don't
track
+ * (streams are not part of the migration data) (2) streams_timeout may
occur
+ * after the MIGRATE message has been sent. This can result in messages
+ * being sent to the client after MSG_MIGRATE and before MSG_MIGRATE_DATA
(e.g.,
+ * STREAM_CLIP, STREAM_DESTROY, DRAW_COPY)
+ * No message besides MSG_MIGRATE_DATA should be sent after MSG_MIGRATE.
+ * Notice that detach_and_stop_streams won't lead to any dev ram changes,
since
+ * handle_dev_stop already took care of releasing all the dev ram
resources.
+ */
+video_stream_detach_and_stop(display);
+if (red_channel_client_is_connected(rcc)) {
+red_channel_client_default_migrate(rcc);
+}
+}
+
+void display_channel_migrate(RedChannelClient *rcc)
+{
+DisplayChannel *display =
DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
+red_migrate_display(display, rcc);
}
diff --git a/server/display-channel.h b/server/display-channel.h
index bdf2f059..989bf52b 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -159,6 +159,11 @@ void display_channel_reset_image_cache(DisplayChannel
*self);
void display_channel_debug_oom(DisplayChannel *display, const char *msg);
void display_channel_set_running(DisplayChannel *display, bool
[Spice-devel] [PATCH spice-server 02/10] dispatcher: Allows to manage message without registering them
Signed-off-by: Frediano Ziglio
---
server/dispatcher.c | 71 ++---
server/dispatcher.h | 15 ++
2 files changed, 62 insertions(+), 24 deletions(-)
diff --git a/server/dispatcher.c b/server/dispatcher.c
index 5f839ec4..0b18b32d 100644
--- a/server/dispatcher.c
+++ b/server/dispatcher.c
@@ -38,10 +38,13 @@
static void setup_dummy_signal_handler(void);
#endif
+#define DISPATCHER_GENERIC_TYPE 0x7fffu
+
typedef struct DispatcherMessage {
-size_t size;
-bool ack;
dispatcher_handle_message handler;
+uint32_t size;
+uint32_t type:31;
+uint32_t ack:1;
} DispatcherMessage;
struct DispatcherPrivate {
@@ -249,12 +252,11 @@ static int write_safe(int fd, uint8_t *buf, size_t size)
static int dispatcher_handle_single_read(Dispatcher *dispatcher)
{
int ret;
-uint32_t type;
-DispatcherMessage *msg = NULL;
-uint8_t *payload = dispatcher->priv->payload;
+DispatcherMessage msg[1];
+uint8_t *payload;
uint32_t ack = ACK;
-if ((ret = read_safe(dispatcher->priv->recv_fd, (uint8_t*),
sizeof(type), 0)) == -1) {
+if ((ret = read_safe(dispatcher->priv->recv_fd, (uint8_t*)msg,
sizeof(msg), 0)) == -1) {
g_warning("error reading from dispatcher: %d", errno);
return 0;
}
@@ -262,28 +264,28 @@ static int dispatcher_handle_single_read(Dispatcher
*dispatcher)
/* no message */
return 0;
}
-if (type >= dispatcher->priv->max_message_type) {
-spice_error("Invalid message type for this dispatcher: %u", type);
-return 0;
+if (SPICE_UNLIKELY(msg->size > dispatcher->priv->payload_size)) {
+dispatcher->priv->payload = g_realloc(dispatcher->priv->payload,
msg->size);
+dispatcher->priv->payload_size = msg->size;
}
-msg = >priv->messages[type];
+payload = dispatcher->priv->payload;
if (read_safe(dispatcher->priv->recv_fd, payload, msg->size, 1) == -1) {
g_warning("error reading from dispatcher: %d", errno);
/* TODO: close socketpair? */
return 0;
}
-if (dispatcher->priv->any_handler) {
-dispatcher->priv->any_handler(dispatcher->priv->opaque, type, payload);
+if (dispatcher->priv->any_handler && msg->type != DISPATCHER_GENERIC_TYPE)
{
+dispatcher->priv->any_handler(dispatcher->priv->opaque, msg->type,
payload);
}
if (msg->handler) {
msg->handler(dispatcher->priv->opaque, payload);
} else {
-g_warning("error: no handler for message type %d", type);
+g_warning("error: no handler for message type %d", msg->type);
}
if (msg->ack) {
if (write_safe(dispatcher->priv->recv_fd,
(uint8_t*), sizeof(ack)) == -1) {
-g_warning("error writing ack for message %d", type);
+g_warning("error writing ack for message %d", msg->type);
/* TODO: close socketpair? */
}
}
@@ -300,25 +302,22 @@ void dispatcher_handle_recv_read(Dispatcher *dispatcher)
}
}
-void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
- void *payload)
+static void
+dispatcher_send_message_internal(Dispatcher *dispatcher, const
DispatcherMessage*msg,
+ void *payload)
{
-DispatcherMessage *msg;
uint32_t ack;
int send_fd = dispatcher->priv->send_fd;
-assert(dispatcher->priv->max_message_type > message_type);
-assert(dispatcher->priv->messages[message_type].handler);
-msg = >priv->messages[message_type];
pthread_mutex_lock(>priv->lock);
-if (write_safe(send_fd, (uint8_t*)_type, sizeof(message_type)) ==
-1) {
+if (write_safe(send_fd, (uint8_t*)msg, sizeof(*msg)) == -1) {
g_warning("error: failed to send message type for message %d",
- message_type);
+ msg->type);
goto unlock;
}
if (write_safe(send_fd, payload, msg->size) == -1) {
g_warning("error: failed to send message body for message %d",
- message_type);
+ msg->type);
goto unlock;
}
if (msg->ack) {
@@ -326,7 +325,7 @@ void dispatcher_send_message(Dispatcher *dispatcher,
uint32_t message_type,
g_warning("error: failed to read ack");
} else if (ack != ACK) {
g_warning("error: got wrong ack value in dispatcher "
- "for message %d\n", message_type);
+ "for message %d\n", msg->type);
/* TODO handling error? */
}
}
@@ -334,6 +333,29 @@ unlock:
pthread_mutex_unlock(>priv->lock);
}
+void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
+ void *payload)
+{
+DispatcherMessage *msg;
+
+assert(dispatcher->priv->max_message_type > message_type);
+assert(dispatcher->priv->messages[message_type].handler);
+msg =
[Spice-devel] [PATCH spice-server 05/10] main-dispatcher: Use dispatcher_send_message_generic
Signed-off-by: Frediano Ziglio
---
server/main-dispatcher.c | 38 --
1 file changed, 8 insertions(+), 30 deletions(-)
diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
index 839e7242..904579a8 100644
--- a/server/main-dispatcher.c
+++ b/server/main-dispatcher.c
@@ -121,15 +121,6 @@ main_dispatcher_init(MainDispatcher *self)
self->priv = main_dispatcher_get_instance_private(self);
}
-enum {
-MAIN_DISPATCHER_CHANNEL_EVENT = 0,
-MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
-MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
-MAIN_DISPATCHER_CLIENT_DISCONNECT,
-
-MAIN_DISPATCHER_NUM_MESSAGES
-};
-
typedef struct MainDispatcherChannelEventMessage {
int event;
SpiceChannelEventInfo *info;
@@ -168,8 +159,8 @@ void main_dispatcher_channel_event(MainDispatcher *self,
int event, SpiceChannel
}
msg.event = event;
msg.info = info;
-dispatcher_send_message(DISPATCHER(self), MAIN_DISPATCHER_CHANNEL_EVENT,
-);
+dispatcher_send_message_generic(DISPATCHER(self),
main_dispatcher_handle_channel_event,
+, sizeof(msg), false);
}
@@ -214,8 +205,8 @@ void
main_dispatcher_seamless_migrate_dst_complete(MainDispatcher *self,
}
msg.client = g_object_ref(client);
-dispatcher_send_message(DISPATCHER(self),
MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
-);
+dispatcher_send_message_generic(DISPATCHER(self),
main_dispatcher_handle_migrate_complete,
+, sizeof(msg), false);
}
void main_dispatcher_set_mm_time_latency(MainDispatcher *self, RedClient
*client, uint32_t latency)
@@ -229,8 +220,8 @@ void main_dispatcher_set_mm_time_latency(MainDispatcher
*self, RedClient *client
msg.client = g_object_ref(client);
msg.latency = latency;
-dispatcher_send_message(DISPATCHER(self),
MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
-);
+dispatcher_send_message_generic(DISPATCHER(self),
main_dispatcher_handle_mm_time_latency,
+, sizeof(msg), false);
}
void main_dispatcher_client_disconnect(MainDispatcher *self, RedClient *client)
@@ -240,8 +231,8 @@ void main_dispatcher_client_disconnect(MainDispatcher
*self, RedClient *client)
if (!red_client_is_disconnecting(client)) {
spice_debug("client %p", client);
msg.client = g_object_ref(client);
-dispatcher_send_message(DISPATCHER(self),
MAIN_DISPATCHER_CLIENT_DISCONNECT,
-);
+dispatcher_send_message_generic(DISPATCHER(self),
main_dispatcher_handle_client_disconnect,
+, sizeof(msg), false);
} else {
spice_debug("client %p already during disconnection", client);
}
@@ -263,7 +254,6 @@ MainDispatcher* main_dispatcher_new(RedsState *reds)
{
MainDispatcher *self = g_object_new(TYPE_MAIN_DISPATCHER,
"spice-server", reds,
-"max-message-type",
MAIN_DISPATCHER_NUM_MESSAGES,
NULL);
return self;
}
@@ -280,18 +270,6 @@ void main_dispatcher_constructed(GObject *object)
dispatcher_get_recv_fd(DISPATCHER(self)),
SPICE_WATCH_EVENT_READ, dispatcher_handle_read,
DISPATCHER(self));
-dispatcher_register_handler(DISPATCHER(self),
MAIN_DISPATCHER_CHANNEL_EVENT,
-main_dispatcher_handle_channel_event,
-sizeof(MainDispatcherChannelEventMessage),
false);
-dispatcher_register_handler(DISPATCHER(self),
MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
-main_dispatcher_handle_migrate_complete,
-
sizeof(MainDispatcherMigrateSeamlessDstCompleteMessage), false);
-dispatcher_register_handler(DISPATCHER(self),
MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
-main_dispatcher_handle_mm_time_latency,
-sizeof(MainDispatcherMmTimeLatencyMessage),
false);
-dispatcher_register_handler(DISPATCHER(self),
MAIN_DISPATCHER_CLIENT_DISCONNECT,
-main_dispatcher_handle_client_disconnect,
-sizeof(MainDispatcherClientDisconnectMessage),
false);
}
static void main_dispatcher_finalize(GObject *object)
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 00/10] RedChannel vfunc follow up
Yesterday evening me and Jonathon discussed about his patches for RedChannel vfuncs. We agreed that the function should be vfuncs however the patches didn't seem really great to me so we discussed different reasons. Came out with this series which is working pretty well. Probably I should add some comments/documentation in the code too, some changes: - RedChannel allows to be associated with a Dispatcher that will be used if the channel is run in different thread; - Dispatcher allows to manage not registered events (used to avoid allocating multiple dispatchers for a thread and reuse for instance RedWorker ones in RedChannel); - Jonathon original patches get rebased into the series, first patch went away, second got integrated encapsulating better DisplayChannel. Some patches are style/preparation. Currently Gitlab CI is experiencing some issues but yesterday executions were fine and my tests seems to work correctly. Frediano Ziglio (9): dispatcher: Use NULL for pointer check dispatcher: Allows to manage message without registering them main-dispatcher: Remove main_dispatcher_self_handle_channel_event main-dispatcher: Use reds as opaque for dispatcher main-dispatcher: Use dispatcher_send_message_generic Move thread/dispatching handling to RedChannel Propagate running property from RedWorker to DisplayChannel Move DisplayChannel callbacks from RedWorker to DisplayChannel Make some function static Jonathon Jongsma (1): Make channel client callbacks virtual functions server/cursor-channel.c | 19 ++- server/cursor-channel.h | 14 +-- server/dispatcher.c | 73 server/dispatcher.h | 15 +++ server/display-channel-private.h | 1 + server/display-channel.c | 125 server/display-channel.h | 3 + server/inputs-channel.c | 9 +- server/main-channel.c| 7 +- server/main-dispatcher.c | 67 +++ server/red-channel.c | 139 ++ server/red-channel.h | 19 ++- server/red-qxl.c | 110 +- server/red-replay-qxl.c | 3 - server/red-stream-device.c | 7 +- server/red-worker.c | 192 +-- server/red-worker.h | 46 ++-- server/smartcard.c | 6 +- server/sound.c | 18 ++- server/spicevmc.c| 7 +- server/stream-channel.c | 5 +- server/tests/test-channel.c | 13 +-- 22 files changed, 389 insertions(+), 509 deletions(-) -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 03/10] main-dispatcher: Remove main_dispatcher_self_handle_channel_event
Signed-off-by: Frediano Ziglio
---
server/main-dispatcher.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
index 82b25e6e..0dcfb2d4 100644
--- a/server/main-dispatcher.c
+++ b/server/main-dispatcher.c
@@ -149,22 +149,13 @@ typedef struct MainDispatcherClientDisconnectMessage {
} MainDispatcherClientDisconnectMessage;
/* channel_event - calls core->channel_event, must be done in main thread */
-static void main_dispatcher_self_handle_channel_event(MainDispatcher *self,
- int event,
- SpiceChannelEventInfo
*info)
-{
-reds_handle_channel_event(self->priv->reds, event, info);
-}
-
static void main_dispatcher_handle_channel_event(void *opaque,
void *payload)
{
MainDispatcher *self = opaque;
MainDispatcherChannelEventMessage *channel_event = payload;
-main_dispatcher_self_handle_channel_event(self,
- channel_event->event,
- channel_event->info);
+reds_handle_channel_event(self->priv->reds, channel_event->event,
channel_event->info);
}
void main_dispatcher_channel_event(MainDispatcher *self, int event,
SpiceChannelEventInfo *info)
@@ -172,7 +163,7 @@ void main_dispatcher_channel_event(MainDispatcher *self,
int event, SpiceChannel
MainDispatcherChannelEventMessage msg = {0,};
if (pthread_self() == dispatcher_get_thread_id(DISPATCHER(self))) {
-main_dispatcher_self_handle_channel_event(self, event, info);
+reds_handle_channel_event(self->priv->reds, event, info);
return;
}
msg.event = event;
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 04/10] main-dispatcher: Use reds as opaque for dispatcher
No reason to pass through MainDispatcher, the purpose of
MainDispatcher is to call reds functions from the right thread.
Signed-off-by: Frediano Ziglio
---
server/main-dispatcher.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
index 0dcfb2d4..839e7242 100644
--- a/server/main-dispatcher.c
+++ b/server/main-dispatcher.c
@@ -152,10 +152,10 @@ typedef struct MainDispatcherClientDisconnectMessage {
static void main_dispatcher_handle_channel_event(void *opaque,
void *payload)
{
-MainDispatcher *self = opaque;
+RedsState *reds = opaque;
MainDispatcherChannelEventMessage *channel_event = payload;
-reds_handle_channel_event(self->priv->reds, channel_event->event,
channel_event->info);
+reds_handle_channel_event(reds, channel_event->event, channel_event->info);
}
void main_dispatcher_channel_event(MainDispatcher *self, int event,
SpiceChannelEventInfo *info)
@@ -176,30 +176,30 @@ void main_dispatcher_channel_event(MainDispatcher *self,
int event, SpiceChannel
static void main_dispatcher_handle_migrate_complete(void *opaque,
void *payload)
{
-MainDispatcher *self = opaque;
+RedsState *reds = opaque;
MainDispatcherMigrateSeamlessDstCompleteMessage *mig_complete = payload;
-reds_on_client_seamless_migrate_complete(self->priv->reds,
mig_complete->client);
+reds_on_client_seamless_migrate_complete(reds, mig_complete->client);
g_object_unref(mig_complete->client);
}
static void main_dispatcher_handle_mm_time_latency(void *opaque,
void *payload)
{
-MainDispatcher *self = opaque;
+RedsState *reds = opaque;
MainDispatcherMmTimeLatencyMessage *msg = payload;
-reds_set_client_mm_time_latency(self->priv->reds, msg->client,
msg->latency);
+reds_set_client_mm_time_latency(reds, msg->client, msg->latency);
g_object_unref(msg->client);
}
static void main_dispatcher_handle_client_disconnect(void *opaque,
void *payload)
{
-MainDispatcher *self = opaque;
+RedsState *reds = opaque;
MainDispatcherClientDisconnectMessage *msg = payload;
spice_debug("client=%p", msg->client);
-reds_client_disconnect(self->priv->reds, msg->client);
+reds_client_disconnect(reds, msg->client);
g_object_unref(msg->client);
}
@@ -273,7 +273,7 @@ void main_dispatcher_constructed(GObject *object)
MainDispatcher *self = MAIN_DISPATCHER(object);
G_OBJECT_CLASS(main_dispatcher_parent_class)->constructed(object);
-dispatcher_set_opaque(DISPATCHER(self), self);
+dispatcher_set_opaque(DISPATCHER(self), self->priv->reds);
self->priv->watch =
reds_core_watch_add(self->priv->reds,
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 2/2] Make channel client callbacks virtual functions
>
> Rather than having an API to register client callbacks for each channel
> type, make them vfuncs.
>
> Since the client callbacks are registered identically for each channel
> of the same type, it doesn't make sense for to require these to be
> registered separately for each object. It's cleaner to have these be
> per-class properties, so they've been converted to virtual functions.
>
Why they are vfunc of RedChannel but accepts RedChannelClient?
I think this is just a bad inheritance where everything was in a single
file/class.
> Note that In order to avoid dependencies from the display and cursor
> channels to the worker (necessary to perform the message dispatching
> required), the worker itself subclasses these channels to provide
> dispatching client callbacks.
> ---
> server/cursor-channel.c | 4 +
> server/display-channel.c| 29
> server/display-channel.h| 7 -
> server/inputs-channel.c | 9 +-
> server/main-channel.c | 7 +-
> server/red-channel.c| 35 ++---
> server/red-channel.h| 19 ++-
> server/red-qxl.c| 111 +-
> server/red-stream-device.c | 5 -
> server/red-worker.c | 280 ++--
> server/red-worker.h | 4 +-
> server/smartcard.c | 6 +-
> server/sound.c | 18 ++-
> server/spicevmc.c | 7 +-
> server/stream-channel.c | 5 +-
> server/tests/test-channel.c | 13 +-
> 16 files changed, 316 insertions(+), 243 deletions(-)
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index d751211af..749c1c6b8 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -383,6 +383,10 @@ cursor_channel_class_init(CursorChannelClass *klass)
> channel_class->handle_message = red_channel_client_handle_message;
>
> channel_class->send_item = cursor_channel_send_item;
> +
> +// client callbacks
> +channel_class->connect = (channel_client_connect_proc)
> cursor_channel_connect;
> +channel_class->migrate = cursor_channel_client_migrate;
> }
>
> static void
> diff --git a/server/display-channel.c b/server/display-channel.c
> index e179abfd3..71912c1b9 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2224,35 +2224,6 @@ static SpiceCanvas
> *image_surfaces_get(SpiceImageSurfaces *surfaces, uint32_t su
> return p->surfaces[surface_id].context.canvas;
> }
>
> -DisplayChannel* display_channel_new(RedsState *reds,
> -QXLInstance *qxl,
> -const SpiceCoreInterfaceInternal *core,
> -int migrate, int stream_video,
> -GArray *video_codecs,
> -uint32_t n_surfaces)
> -{
> -DisplayChannel *display;
> -
> -/* FIXME: migrate is not used...? */
> -spice_debug("create display channel");
> -display = g_object_new(TYPE_DISPLAY_CHANNEL,
> - "spice-server", reds,
> - "core-interface", core,
> - "channel-type", SPICE_CHANNEL_DISPLAY,
> - "id", qxl->id,
> - "migration-flags",
> - (SPICE_MIGRATE_NEED_FLUSH |
> SPICE_MIGRATE_NEED_DATA_TRANSFER),
> - "qxl", qxl,
> - "n-surfaces", n_surfaces,
> - "video-codecs", video_codecs,
> - "handle-acks", TRUE,
> - NULL);
> -if (display) {
> -display_channel_set_stream_video(display, stream_video);
> -}
> -return display;
> -}
> -
> static SpiceCanvas *image_surfaces_get(SpiceImageSurfaces *surfaces,
> uint32_t surface_id);
> static void drawables_init(DisplayChannel *display);
> static void
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 948018cf3..83727e17a 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -96,13 +96,6 @@ struct Drawable {
> DisplayChannel *display;
> };
>
> -DisplayChannel*display_channel_new
> (RedsState *reds,
> -
> QXLInstance
> *qxl,
> - const
> SpiceCoreInterfaceInternal *core,
> - int
> migrate,
> - int
> stream_video,
> - GArray
> *video_codecs,
> -
> uint32_t
> n_surfaces);
> void display_channel_create_surface
> (DisplayChannel *display, uint32_t surface_id,
>
> uint32_t
>width,
>
Re: [Spice-devel] [PATCH spice-server v2] reds: Check we don't register a channel twice in reds_register_channel
Ack
On 3/19/19 4:11 PM, Frediano Ziglio wrote:
To avoid potential regressions, check it only if extra checks are
enabled.
This allows to check previous "Move channel registration to constructed
vfunc" commit.
Signed-off-by: Frediano Ziglio
---
server/reds.c | 8
1 file changed, 8 insertions(+)
Changes since v1:
- add a warning in normal case
diff --git a/server/reds.c b/server/reds.c
index bc043764..14e5728b 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -380,6 +380,14 @@ void stat_remove_counter(SpiceServer *reds, RedStatCounter
*counter)
void reds_register_channel(RedsState *reds, RedChannel *channel)
{
spice_assert(reds);
+
+uint32_t this_type, this_id;
+g_object_get(channel, "channel-type", _type, "id", _id, NULL);
+if (spice_extra_checks) {
+g_assert(reds_find_channel(reds, this_type, this_id) == NULL);
+} else {
+g_warn_if_fail(reds_find_channel(reds, this_type, this_id) == NULL);
+}
reds->channels = g_list_prepend(reds->channels, channel);
// create new channel in the client if possible
main_channel_registered_new_channel(reds->main_channel, channel);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
