On Thu, Mar 10, 2016 at 12:17:19PM -0500, Frediano Ziglio wrote:
> >
> > From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> >
> > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel