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