[Spice-devel] [PATCH 02/15] Change vdi_port_read_buf_process() to take RedsState arg

[Jonathon Jongsma]

On Tue, 2016-01-19 at 12:03 +0100, Pavel Grunt wrote:
> On Tue, 2016-01-19 at 05:12 -0500, Frediano Ziglio wrote:
> > > 
> > > Hi,
> > > 
> > > On Mon, 2016-01-18 at 16:37 +0000, Frediano Ziglio wrote:
> > > > From: Jonathon Jongsma <jjongsma at redhat.com>
> > > > 
> > > > ---
> > > >  server/reds.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/server/reds.c b/server/reds.c
> > > > index 25e9f90..b347d1d 100644
> > > > --- a/server/reds.c
> > > > +++ b/server/reds.c
> > > > @@ -662,7 +662,7 @@ 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(int port, VDIReadBuf *buf)
> > > > +static int vdi_port_read_buf_process(RedsState *reds, int port,
> > > > VDIReadBuf *buf)
> > > 
> > > I like when the first parameter corresponds with the function name.
> > > Moving reds to the last position?
> > > 
> > > Reviewed-by: Pavel Grunt <pgrunt at redhat.com>
> > > 
> > 
> > Usually I agree with you but usually these class of functions have
> > the same
> > first parameters. It's kind of this in C++ or self in python. They
> > specify
> and some comments say "More consistent with glib naming conventions"
> > the object they are working on. But in this case many vdi_port
> > functions don't
> > have a port parameter (actually only one) so
> Yeah, I noticed that in the following patches. I would prefer if we
> could follow some (naming) style.
> 
> > Acked-by: Frediano Ziglio <fziglio at redhat.com>
> > 
> > However as Fabiano say (and I agree) a Nack is stronger than an Ack.
> > 
> > Frediano
> 
> I didn't say nack, just asked for an opinion.

I just thought I should mention that this is an intermediate refactoring step.
In a future commit on this branch, Christophe removes the dependency of these
functions on RedsState and changes the first parameter to VdiPortState* (which
matches the naming convention more closely)


> 
> Pavel
> 
> 
> > 
> > 
> > > >  {
> > > >      VDIPortState *state = &reds->agent_state;
> > > >      int res;
> > > > @@ -787,7 +787,7 @@ static SpiceCharDeviceMsgToClient
> > > > *vdi_port_read_one_msg_from_device(SpiceCharDe
> > > >              } else {
> > > >                  state->read_state =
> > > > VDI_PORT_READ_STATE_GET_BUFF;
> > > >              }
> > > > -            if (vdi_port_read_buf_process(state-
> > > > > vdi_chunk_header.port, dispatch_buf)) {
> > > > +            if (vdi_port_read_buf_process(reds, state-
> > > > > vdi_chunk_header.port, dispatch_buf)) {
> > > >                  return dispatch_buf;
> > > >              } else {
> > > >                  vdi_port_read_buf_unref(dispatch_buf);
> > > > @@ -1156,7 +1156,7 @@ void
> > > > reds_on_main_channel_migrate(MainChannelClient *mcc)
> > > >                      !agent_state->read_filter.msg_data_to_read);
> > > >  
> > > >          read_buf->len = read_data_len;
> > > > -        if (vdi_port_read_buf_process(agent_state-
> > > > > vdi_chunk_header.port, read_buf)) {
> > > > +        if (vdi_port_read_buf_process(reds, agent_state-
> > > > > vdi_chunk_header.port, read_buf)) {
> > > >              main_channel_client_push_agent_data(mcc,
> > > >                                                  read_buf->data,
> > > >                                                  read_buf->len,
> > > 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel