[Spice-devel] [spice-server 07/10] Remove IncomingHandlerInterface

[Christophe Fergeau]

On Thu, Feb 09, 2017 at 11:33:41AM -0600, Jonathon Jongsma wrote:
> 
> Same comment as for the OutgoingHandler struct: removing these callback
> funcs means that the IncomingHandler name is no longer a very good fit.
> It's not so much a 'handler' as it is a collection of memory buffers
> and state variables for the incoming message. Perhaps a rename would
> increase understandability?

It's renamed in v2 (in a follow-up patch).

> > +static int red_channel_client_parse(RedChannelClient *client,
> > uint8_t *msg,
> > +                                    uint32_t msg_size, uint16_t
> > msg_type)
> > +{
> > +    RedChannel *channel = red_channel_client_get_channel(client);
> > +    RedChannelClass *klass = RED_CHANNEL_GET_CLASS(channel);
> > +    int ret_handle;
> > +
> > +    if (klass->parser) {
> > +        uint8_t *parsed;
> > +        size_t parsed_size;
> > +        message_destructor_t parsed_free;
> > +
> > +        parsed = klass->parser(msg, msg + msg_size, msg_type,
> > +                               SPICE_VERSION_MINOR, &parsed_size,
> > &parsed_free);
> > +        if (parsed == NULL) {
> > +            spice_printerr("failed to parse message type %d",
> > msg_type);
> > +            red_channel_client_release_msg_buf(client, msg_type,
> > msg_size, msg);
> > +            /* FIXME: handle disconnection in caller */
> > +            red_channel_client_disconnect(client);
> > +            return -1;
> 
> I'm not sure about this. Our handle_parsed()/handle_message vfuncs
> technically have an 'int' return value, but we treat them as booleans
> (most vfunc implementations return TRUE or FALSE). Failures in those
> functions are represented by a 0 (FALSE). This new function will
> sometimes return those values directly, but sometimes return a special
> -1. So we have the following possibilities: -1 (failure), 0 (failures,
> or 1 (success). It works, but I find it a little odd. And it confused
> me a bit when reviewing the code below (see comment below).

Very good point, I did not realize this was returning a boolean
when working on the series. I've reworked this in v2,
red_channel_client_parse() is only going to call ->parser() and return a
boolean, and we'll keep checking for TRUE/FALSE in _handle_incoming().
My feeling is that it might be better to hide the call to ->parser() in
the ->handle_parsed()/->handle_message() implementation of subclasses.
Maybe I'll look into that as a followup too.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170214/c73a71ef/attachment.sig>