[Spice-devel] [PATCH spice 2/2] reds: Refactor agent message forwarding

[Pavel Grunt]

On Mon, 2016-09-12 at 08:38 -0400, Frediano Ziglio wrote:
> I think the rationale of this is to remove the error return
> parameter and
> make vdi_port_read_buf_process easier/shorter.
Yes, also I have some patches which will make use of the return value
> 
> > 
> > ---
> >  server/reds.c | 55 ++++++++++++++++++++------------------------
> > -----------
> >  1 file changed, 20 insertions(+), 35 deletions(-)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 800107b..a13a14a 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -763,34 +763,18 @@ static void
> > vdi_port_read_buf_release(uint8_t *data,
> > void *opaque)
> >      red_pipe_item_unref((RedPipeItem *)opaque);
> >  }
> >  
> > -/* returns TRUE if the buffer can be forwarded */
> > -static gboolean vdi_port_read_buf_process(RedCharDeviceVDIPort
> > *dev,
> > -                                          RedVDIReadBuf *buf,
> > gboolean
> > *error)
> > +/* returns AGENT_MSG_FILTER_OK if the buffer can be forwarded,
> > +           AGENT_MSG_FILTER_PROTO_ERROR on error */
> 
> 
> Why AGENT_MSG_FILTER_DISCARD is not documented ?
Sorry, fixed locally
> 
> > +static int vdi_port_read_buf_process(RedCharDeviceVDIPort *dev,
> > RedVDIReadBuf *buf)
> 
> 
> Don't we have an enum for this return instead of using int ?
Sure, I will change it - I got inspired by
"agent_msg_filter_process_data" which also returns int
> 
> enum {
>     AGENT_MSG_FILTER_OK,
>     AGENT_MSG_FILTER_DISCARD,
>     AGENT_MSG_FILTER_PROTO_ERROR,
>     AGENT_MSG_FILTER_MONITORS_CONFIG,
>     AGENT_MSG_FILTER_END
> };      
> 
> mumble... weird enum. Beside AGENT_MSG_FILTER_END which is
> not used there is OK but also a AGENT_MSG_FILTER_MONITORS_CONFIG
> which I suppose it's OK too... but this is not related to this
> patch.
> 
> >  {
> > -    int res;
> > -
> > -    *error = FALSE;
> > -
> >      switch (dev->priv->vdi_chunk_header.port) {
> > -    case VDP_CLIENT_PORT: {
> > -        res = agent_msg_filter_process_data(&dev->priv-
> > >read_filter,
> > -                                            buf->data, buf->len);
> > -        switch (res) {
> > -        case AGENT_MSG_FILTER_OK:
> > -            return TRUE;
> > -        case AGENT_MSG_FILTER_DISCARD:
> > -            return FALSE;
> > -        case AGENT_MSG_FILTER_PROTO_ERROR:
> > -            *error = TRUE;
> > -            return FALSE;
> > -        }
> > -    }
> > +    case VDP_CLIENT_PORT:
> > +        return agent_msg_filter_process_data(&dev->priv-
> > >read_filter,
> > buf->data, buf->len);
> >      case VDP_SERVER_PORT:
> > -        return FALSE;
> > +        return AGENT_MSG_FILTER_DISCARD;
> >      default:
> >          spice_warning("invalid port");
> > -        *error = TRUE;
> > -        return FALSE;
> > +        return AGENT_MSG_FILTER_PROTO_ERROR;
> >      }
> >  }
> >  
> > @@ -881,7 +865,6 @@ static RedPipeItem
> > *vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
> >              dev->priv->read_state =
> > VDI_PORT_READ_STATE_READ_DATA;
> >          }
> >          case VDI_PORT_READ_STATE_READ_DATA: {
> > -            gboolean error = FALSE;
> >              n = sif->read(reds->vdagent, dev->priv->receive_pos,
> >              dev->priv->receive_len);
> >              if (!n) {
> >                  return NULL;
> > @@ -900,12 +883,13 @@ static RedPipeItem
> > *vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
> >              } else {
> >                  dev->priv->read_state =
> > VDI_PORT_READ_STATE_GET_BUFF;
> >              }
> > -            if (vdi_port_read_buf_process(reds->agent_dev,
> > dispatch_buf,
> > &error)) {
> > +            switch (vdi_port_read_buf_process(reds->agent_dev,
> > dispatch_buf)) {
> > +            case AGENT_MSG_FILTER_OK:
> >                  return &dispatch_buf->base;
> > -            } else {
> > -                if (error) {
> > -                    reds_agent_remove(reds);
> > -                }
> > +            default:
> > +            case AGENT_MSG_FILTER_PROTO_ERROR:
> > +                reds_agent_remove(reds);
> 
> 
> I would add a fall through comment here, at least to make some
> automatic tools more happy.

Do you mean some comment which is recognized by the automatic tools ?
Can you give an example ?

> 
> > +            case AGENT_MSG_FILTER_DISCARD:
> >                  red_pipe_item_unref(&dispatch_buf->base);
> >              }
> >          }
> > @@ -1275,22 +1259,23 @@ void
> > reds_on_main_channel_migrate(RedsState *reds,
> > MainChannelClient *mcc)
> >      if (agent_dev->priv->read_filter.msg_data_to_read ||
> >          read_data_len > sizeof(VDAgentMessage)) { /* msg header
> > has been
> >          read */
> >          RedVDIReadBuf *read_buf = agent_dev->priv-
> > >current_read_buf;
> > -        gboolean error = FALSE;
> >  
> >          spice_debug("push partial read %u (msg first chunk? %d)",
> >          read_data_len,
> >                      !agent_dev->priv-
> > >read_filter.msg_data_to_read);
> >  
> >          read_buf->len = read_data_len;
> > -        if (vdi_port_read_buf_process(reds->agent_dev, read_buf,
> > &error)) {
> > +        switch (vdi_port_read_buf_process(reds->agent_dev,
> > read_buf)) {
> > +        case AGENT_MSG_FILTER_OK:
> >              main_channel_client_push_agent_data(mcc,
> >                                                  read_buf->data,
> >                                                  read_buf->len,
> >                                                  vdi_port_read_buf
> > _release,
> >                                                  read_buf);
> > -        } else {
> > -            if (error) {
> > -               reds_agent_remove(reds);
> > -            }
> > +            break;
> > +        default:
> > +        case AGENT_MSG_FILTER_PROTO_ERROR:
> > +            reds_agent_remove(reds);
> > +        case AGENT_MSG_FILTER_DISCARD:
> >              red_pipe_item_unref(&read_buf->base);
> >          }
> >  
> 
> 
> Looks like copy&paste
I can use some macro to avoid it

>  but this is not a regression, caller code of
> vdi_port_read_buf_process were very similar even before.
yes
> 
> Frediano

Thanks,
Pavel