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@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel