>
> On Wed, 2019-03-20 at 09:59 +0000, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> > 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 0x7fffffffu
>
> 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*)&type,
> > 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 = &dispatcher->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*)&ack, 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 = &dispatcher->priv->messages[message_type];
> > pthread_mutex_lock(&dispatcher->priv->lock);
> > - if (write_safe(send_fd, (uint8_t*)&message_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(&dispatcher->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 = &dispatcher->priv->messages[message_type];
> > + dispatcher_send_message_internal(dispatcher, msg, payload);
> > +}
> > +
> > +void dispatcher_send_message_generic(Dispatcher *dispatcher,
> > dispatcher_handle_message handler,
> > + void *payload, uint32_t
> > payload_size, bool ack)
> > +{
> > + DispatcherMessage msg = {
> > + .handler = handler,
> > + .size = payload_size,
> > + .type = DISPATCHER_GENERIC_TYPE,
> > + .ack = ack,
> > + };
> > + dispatcher_send_message_internal(dispatcher, &msg, payload);
> > +}
> > +
> > void dispatcher_register_handler(Dispatcher *dispatcher, uint32_t
> > message_type,
> > dispatcher_handle_message handler,
> > size_t size, bool ack)
> > @@ -345,6 +367,7 @@ void dispatcher_register_handler(Dispatcher
> > *dispatcher, uint32_t message_type,
> > msg = &dispatcher->priv->messages[message_type];
> > msg->handler = handler;
> > msg->size = size;
> > + msg->type = message_type;
> > msg->ack = ack;
> > if (msg->size > dispatcher->priv->payload_size) {
> > dispatcher->priv->payload = g_realloc(dispatcher->priv-
> > >payload, msg->size);
> > diff --git a/server/dispatcher.h b/server/dispatcher.h
> > index bb968e56..b8339c06 100644
> > --- a/server/dispatcher.h
> > +++ b/server/dispatcher.h
> > @@ -99,6 +99,21 @@ typedef void (*dispatcher_handle_any_message)(void
> > *opaque,
> > void dispatcher_send_message(Dispatcher *dispatcher, uint32_t
> > message_type,
> > void *payload);
> >
> > +/* dispatcher_send_message_generic
> > + *
> > + * Sends a message to the receiving thread.
> > + *
> > + * If the sent message is a message type requires an ACK, this
> > function will
> > + * block until it receives an ACK from the receiving thread.
> > + *
> > + * @handler: callback to handle message
> > + * @payload: payload
> > + * @payload_size: size of payload
> > + * @ack: acknowledge required. Make message synchronous
> > + */
> > +void dispatcher_send_message_generic(Dispatcher *dispatcher,
> > dispatcher_handle_message handler,
> > + void *payload, uint32_t
> > payload_size, bool ack);
> > +
> > /* dispatcher_register_handler
> > *
> > * This function registers a message type with the dispatcher, and
> > registers
>
>
> Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
>
>
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel