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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel