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

[Jonathon Jongsma]

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

>   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)

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.

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