[Spice-devel] [PATCH 05/15] Remove dependency of vdi_port_read_buf_process on RedsState

[Christophe Fergeau]

On Thu, Mar 10, 2016 at 12:17:19PM -0500, Frediano Ziglio wrote:
> > 
> > From: Christophe Fergeau <cfergeau at redhat.com>
> > 
> > This makes it easier to move the VDIPort API to a different file, and
> > make it as self-contained as possible.
> > ---
> >  server/reds.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 736dca6..08913e8 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -632,14 +632,16 @@ static void vdi_port_read_buf_release(uint8_t *data,
> > void *opaque)
> >  }
> >  
> >  /* returns TRUE if the buffer can be forwarded */
> > -static int vdi_port_read_buf_process(RedsState *reds, VDIReadBuf *buf)
> > +static gboolean vdi_port_read_buf_process(VDIPortState *state, VDIReadBuf
> > *buf, gboolean *error)
> >  {
> > -    VDIPortState *state = &reds->agent_state;
> >      int res;
> >  
> > +    *error = FALSE;
> > +
> >      switch (state->vdi_chunk_header.port) {
> >      case VDP_CLIENT_PORT: {
> > -        res = agent_msg_filter_process_data(&state->read_filter, reds,
> > +        res = agent_msg_filter_process_data(&state->read_filter,
> > +
> > spice_char_device_get_server(state->base),
> 
> Actually you can get RedsState (and you are using it) from state->base.

Not really, the patch I wrote did not have anything related to
spice_char_device_get_server(state->base),
agent_msg_filter_process_data() did not need a RedsState argument.
See
https://cgit.freedesktop.org/~teuf/spice/commit/?h=replay-rebase&id=11d842991eb48643965169a77588a5514c0c1776
 for the initial version.

The RedsState argument to agent_msg_filter_process_data() was added in
a1e62fa5ae98 in order to fix a regression, but in my opinion this is a
step in the wrong direction. We should try to use such a global object
as little as possible, and in particular not introduce it in small
self-contained code.

> So why this complication of having to pass back a flag to say that
> we need to call reds_agent_remove instead of just replacing reds
> argument with state but continue to call reds_agent_remove here?

I'd like to have some layering, the reasoning was probably that the code
managing the vdibuf data had no business in calling the higher level
reds_agent_remove(). It should instead report errors, and let higher
layers do whatever cleanup they need (note, this is just general talk, I
haven't dug back in this code to check if all of this applies here, but
I guess it does).

I agree this patch is odd as it is, but the way of making it better is
not to stop reporting errors back to the caller, but rather to stop
passing a RedsState to agent_msg_filter_process_data().

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