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