[Spice-devel] [spice-server 04/10] Remove IncomingHandlerInterface::{alloc, release}_msg_buf
Frediano Ziglio
fziglio at redhat.com
Fri Feb 10 13:17:47 UTC 2017
>
> On Tue, 2017-02-07 at 11:59 +0100, Christophe Fergeau wrote:
> > Similarly to the previous commits, this removes an indirection level,
> > IncomingHandlerInterface stores pointers to
> > alloc_recv_buf/release_recv_buf vfuncs, but these are always set to
> > RedChannel::alloc_recv_buf/RedChannel::release_recv_buf, which are
> > also
> > vfuncs which can be overridden if needed. This commit removes the
> > indirection and directly calls the relevant methods.
> >
> > Not sure whether the corresponding vfuncs belong in
> > RedChannel or if they should be moved to RedChannelClient.
>
> They do seem more appropriate to RedChannelClient, since the first
> argument is RedChannelClient*. If we do move them, it could be done in
> a subsequent commit, though. Maybe more of this stuff should be moved
> to the client?
>
For me it's quite obvious.
RedChannel with RedChannelClient handle a channel.
The separation allows to handle multiple clients or having a channel
with no clients attached (this last could be easily achieved without
RedChannelClient by the way).
RedChannelClient have the responsibility of dealing with a client
so for instance has a stream while RedChannel don't.
The RedChannelClient should deal with client state allowing the
channel to send different data if the client states are different
or filter messages based on client properties (for instance you
can think of an input channel connected to a client that is not
allowed to send input commands).
As these callbacks are dealing with raw data we use for reading
data from client stream these should be RedChannelClient
responsibility.
The fact that RedChannelClient only code calls these callback
and that the functions accepts a RedChannelClient* as argument
are just confirmation, an OO approach should start from
responsibility.
<OT>
On the implementation detail. Message structures returned by
the demarshaller code could contain pointers to this buffer.
If this buffer is allocated statically inside the RedChannelClient
and we store the structure message for later use we should not
free the RedChannelClient we get dandling pointers.
However I think this is not the case (the message is handled
straight away).
</OT>
> > ---
> > server/red-channel-client.c | 40
> > ++++++++++++++++++++++++++++++++++++----
> > server/red-channel.c | 4 ----
> > server/red-channel.h | 2 --
> > 3 files changed, 36 insertions(+), 10 deletions(-)
> >
> > diff --git a/server/red-channel-client.c b/server/red-channel-
> > client.c
> > index a5c8e9f..a617582 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -1000,6 +1000,33 @@ void
> > red_channel_client_shutdown(RedChannelClient *rcc)
> > }
> > }
> >
> > +static uint8_t *red_channel_client_alloc_msg_buf(RedChannelClient
> > *client,
> > + uint16_t type,
> > uint32_t size)
> > +{
> > + RedChannel *channel = red_channel_client_get_channel(client);
> > + RedChannelClass *klass = RED_CHANNEL_GET_CLASS(channel);
> > +
> > + if (klass->alloc_recv_buf) {
> > + return klass->alloc_recv_buf(client, type, size);
> > + }
On red_channel_constructed there's this test
spice_assert(klass->config_socket && klass->on_disconnect &&
klass->alloc_recv_buf && klass->release_recv_buf);
so the test is useless.
> > +
> > + g_return_val_if_reached(NULL);
> > +}
> > +
> > +static void red_channel_client_release_msg_buf(RedChannelClient
> > *client,
> > + uint16_t type,
> > uint32_t size,
> > + uint8_t *msg)
> > +{
> > + RedChannel *channel = red_channel_client_get_channel(client);
> > + RedChannelClass *klass = RED_CHANNEL_GET_CLASS(channel);
> > +
> > + if (klass->release_recv_buf) {
> > + klass->release_recv_buf(client, type, size, msg);
> > + }
> > +
> > + g_return_if_reached();
>
> It seems that this will trigger every time? Probably a copy-paste error
> from the alloc_ function?
>
> > +}
> > +
> > static void red_peer_handle_outgoing(RedsStream *stream,
> > OutgoingHandler *handler)
> > {
> > ssize_t n;
> > @@ -1127,7 +1154,7 @@ static void red_peer_handle_incoming(RedsStream
> > *stream, IncomingHandler *handle
> > msg_type = handler->header.get_msg_type(&handler->header);
> > if (handler->msg_pos < msg_size) {
> > if (!handler->msg) {
> > - handler->msg = handler->cb->alloc_msg_buf(handler-
> > >opaque, msg_type, msg_size);
> > + handler->msg =
> > red_channel_client_alloc_msg_buf(handler->opaque, msg_type,
> > msg_size);
> > if (handler->msg == NULL) {
> > spice_printerr("ERROR: channel refused to
> > allocate buffer.");
> > handler->cb->on_error(handler->opaque);
> > @@ -1139,7 +1166,8 @@ static void red_peer_handle_incoming(RedsStream
> > *stream, IncomingHandler *handle
> > handler->msg + handler-
> > >msg_pos,
> > msg_size - handler-
> > >msg_pos);
> > if (bytes_read == -1) {
> > - handler->cb->release_msg_buf(handler->opaque,
> > msg_type, msg_size, handler->msg);
> > + red_channel_client_release_msg_buf(handler->opaque,
> > msg_type, msg_size,
> > + handler->msg);
> > handler->cb->on_error(handler->opaque);
> > return;
> > }
> > @@ -1156,7 +1184,9 @@ static void red_peer_handle_incoming(RedsStream
> > *stream, IncomingHandler *handle
> > SPICE_VERSION_MINOR, &parsed_size, &parsed_free);
> > if (parsed == NULL) {
> > spice_printerr("failed to parse message type %d",
> > msg_type);
> > - handler->cb->release_msg_buf(handler->opaque,
> > msg_type, msg_size, handler->msg);
> > + red_channel_client_release_msg_buf(handler->opaque,
> > + msg_type,
> > msg_size,
> > + handler->msg);
> > handler->cb->on_error(handler->opaque);
> > return;
> > }
> > @@ -1168,7 +1198,9 @@ static void red_peer_handle_incoming(RedsStream
> > *stream, IncomingHandler *handle
> > handler->msg);
> > }
> > handler->msg_pos = 0;
> > - handler->cb->release_msg_buf(handler->opaque, msg_type,
> > msg_size, handler->msg);
> > + red_channel_client_release_msg_buf(handler->opaque,
> > + msg_type, msg_size,
> > + handler->msg);
> > handler->msg = NULL;
> > handler->header_pos = 0;
> >
> > diff --git a/server/red-channel.c b/server/red-channel.c
> > index 3abe9c7..9e2e527 100644
> > --- a/server/red-channel.c
> > +++ b/server/red-channel.c
> > @@ -224,10 +224,6 @@ red_channel_constructed(GObject *object)
> > spice_assert(klass->handle_migrate_data ||
> > !(self->priv->migration_flags &
> > SPICE_MIGRATE_NEED_DATA_TRANSFER));
> >
> > - self->priv->incoming_cb.alloc_msg_buf =
> > - (alloc_msg_recv_buf_proc)klass->alloc_recv_buf;
> > - self->priv->incoming_cb.release_msg_buf =
> > - (release_msg_recv_buf_proc)klass->release_recv_buf;
> > self->priv->incoming_cb.handle_message =
> > (handle_message_proc)klass->handle_message;
> > self->priv->incoming_cb.handle_parsed =
> > (handle_parsed_proc)klass->handle_parsed;
> > self->priv->incoming_cb.parser = klass->parser;
> > diff --git a/server/red-channel.h b/server/red-channel.h
> > index a24330a..38c192c 100644
> > --- a/server/red-channel.h
> > +++ b/server/red-channel.h
> > @@ -70,9 +70,7 @@ typedef void (*on_input_proc)(void *opaque, int n);
> >
> > typedef struct IncomingHandlerInterface {
> > handle_message_proc handle_message;
> > - alloc_msg_recv_buf_proc alloc_msg_buf;
> > on_incoming_error_proc on_error; // recv error or handle_message
> > error
> > - release_msg_recv_buf_proc release_msg_buf; // for errors
> > // The following is an optional alternative to handle_message,
> > used if not null
> > spice_parse_channel_func_t parser;
> > handle_parsed_proc handle_parsed;
Frediano
More information about the Spice-devel
mailing list