[Spice-devel] [spice-gtk v4 10/24] file-xfer: improve helper function to queue agent message

[Victor Toso]

Hi,

On Fri, Jun 24, 2016 at 03:04:03PM -0500, Jonathon Jongsma wrote:
> On Thu, 2016-06-23 at 19:37 +0200, Victor Toso wrote:
> > file_xfer_queue() function belongs to channel-main so it should not
> > access SpiceFileTransferTask private struct (self->buffer).
> > 
> > This patch changes:
> > * rename function: file_xfer_queue -> file_xfer_queue_msg_to_agent
> >   As it makes more clear what this helper function does;
> > * rename variabale: self -> xfer_task
>
> variable

Fixed

> 
> >   As this is not a SpiceFileTransferTask' function.
> > * Use buffer provided by spice_file_transfer_task_read_finish()
> >   instead of accessing SpiceFileTransferTask's private structure
> > 
> > This change is related to split SpiceFileTransferTask from
> > channel-main.
> > ---
> >  src/channel-main.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index fef72cd..e57ee73 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -1935,19 +1935,19 @@ static void file_xfer_data_flushed_cb(GObject
> > *source_object,
> >      spice_file_transfer_task_read_async(self, file_xfer_read_async_cb, NULL);
> >  }
> >  
> > -static void file_xfer_queue(SpiceFileTransferTask *self, int data_size)
> > +static void file_xfer_queue_msg_to_agent(SpiceFileTransferTask *xfer_task, 
> > gchar *buffer, int data_size)
> >  {
> >      VDAgentFileXferDataMessage msg;
> >      SpiceMainChannel *channel;
> >  
> > -    channel = spice_file_transfer_task_get_channel(self);
> > +    channel = spice_file_transfer_task_get_channel(xfer_task);
> >      g_return_if_fail(channel != NULL);
> >  
> > -    msg.id = spice_file_transfer_task_get_id(self);
> > +    msg.id = spice_file_transfer_task_get_id(xfer_task);
> >      msg.size = data_size;
> >      agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_DATA,
> >                           &msg, sizeof(msg),
> > -                         self->buffer, data_size, NULL);
> > +                         buffer, data_size, NULL);
> >      spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE);
> >  }
> >  
> > @@ -1973,7 +1973,7 @@ static void file_xfer_read_async_cb(GObject
> > *source_object,
> >          return;
> >      }
> >  
> > -    file_xfer_queue(xfer_task, count);
> > +    file_xfer_queue_msg_to_agent(xfer_task, buffer, count);
> >      if (count == 0)
> >          /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent */
> >          return;
>
> Looks fine, but personally I'm not sure that we really even need a
> separate function for this. It's only called from a single place, and
> it's a very small function. If we just moved the code into the calling
> function, we'd probably end up with smaller and more-readable code and
> wouldn't need to worry about the function name, etc.  
>
> Another alternative is something like:
>
> static void file_xfer_queue_msg_to_agent(SpiceMainChannel*, guint32 task_id,
> gchar *buffer, int data_size)

Agreed!

> It feels a bit awkward to me to pass the SpiceFileTransferTask to this
> function and then just extract a couple pieces of data from it.
>
> But even so, this is an improvement over the current code, so if you
> want to keep it this way, I won't argue.

I'll change it!

As I mentioned before, I think we can think about overall improvements
on these file_xfer_* functions but I tried to avoid for now to avoid
introducing more possibilities to (track) errors.

>
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>

Thanks!

>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel