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*)&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) {
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*)&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.
> +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 > mes
[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*)&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)) {
+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(dispa
