[Spice-devel] [spice-gtk v1 10/10] file-transfer: fix leak on _task_get_filename

[Victor Toso]

Hi,

On Mon, Aug 01, 2016 at 12:37:00PM +0200, Pavel Grunt wrote:
> Hi Victor,
>
> On Sat, 2016-07-30 at 00:26 +0200, Victor Toso wrote:
> > This fixes the leak of filename. The documentation says that this is
> > transfer none so Spicy and Virt-Viewer are not trying to free this
> > string.
> >
> > The other option would be changing the documentation to transfer full
> > and fix the applications.
>
> It sounds like a better option to me

Okay, I'll change it

>
> >
> > This patch breaks API by using const* to return value and ABI as
> > client application that was free'ing will need to fix it.
> >
> > While on it, we use a lot of g_file_get_basename() in the code so I
> > kept the filename saved as it should not change during the file
> > transfer.
> Ok
>
> Thanks,
> Pavel

Cheers,
  toso

> > ---
> >  src/spice-file-transfer-task.c | 20 +++++++++-----------
> >  src/spice-file-transfer-task.h |  2 +-
> >  2 files changed, 10 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
> > index fb48b9b..70571f0 100644
> > --- a/src/spice-file-transfer-task.c
> > +++ b/src/spice-file-transfer-task.c
> > @@ -45,6 +45,7 @@ struct _SpiceFileTransferTask
> >      gboolean                       completed;
> >      gboolean                       pending;
> >      GFile                          *file;
> > +    gchar                          *filename;
> >      SpiceMainChannel               *channel;
> >      GFileInputStream               *file_stream;
> >      GFileCopyFlags                 flags;
> > @@ -212,11 +213,9 @@ static void
> > spice_file_transfer_task_read_stream_cb(GObject *source_object,
> >          gint64 now = g_get_monotonic_time();
> >  
> >          if (interval < now - self->last_update) {
> > -            gchar *basename = g_file_get_basename(self->file);
> >              self->last_update = now;
> >              SPICE_DEBUG("read %.2f%% of the file %s",
> > -                        100.0 * self->read_bytes / self->file_size,
> > basename);
> > -            g_free(basename);
> > +                        100.0 * self->read_bytes / self->file_size, self-
> > >filename);
> >          }
> >      }
> >  
> > @@ -246,16 +245,14 @@ static void
> > spice_file_transfer_task_close_stream_cb(GObject      *object,
> >  
> >      if (self->error == NULL && spice_util_get_debug()) {
> >          gint64 now = g_get_monotonic_time();
> > -        gchar *basename = g_file_get_basename(self->file);
> >          double seconds = (double) (now - self->start_time) /
> > G_TIME_SPAN_SECOND;
> >          gchar *file_size_str = g_format_size(self->file_size);
> >          gchar *transfer_speed_str = g_format_size(self->file_size / seconds);
> >  
> >          g_warn_if_fail(self->read_bytes == self->file_size);
> >          SPICE_DEBUG("transferred file %s of %s size in %.1f seconds (%s/s)",
> > -                    basename, file_size_str, seconds, transfer_speed_str);
> > +                    self->filename, file_size_str, seconds,
> > transfer_speed_str);
> >  
> > -        g_free(basename);
> >          g_free(file_size_str);
> >          g_free(transfer_speed_str);
> >      }
> > @@ -540,9 +537,9 @@ void spice_file_transfer_task_cancel(SpiceFileTransferTask
> > *self)
> >   *
> >   * Since: 0.31
> >   **/
> > -char* spice_file_transfer_task_get_filename(SpiceFileTransferTask *self)
> > +const char* spice_file_transfer_task_get_filename(SpiceFileTransferTask
> > *self)
> >  {
> > -    return g_file_get_basename(self->file);
> > +    return self->filename;
> >  }
> >  
> >  /****************************************************************************
> > ***
> > @@ -608,6 +605,7 @@ spice_file_transfer_task_dispose(GObject *object)
> >      g_clear_object(&self->file);
> >      g_clear_object(&self->file_stream);
> >      g_clear_error(&self->error);
> > +    g_clear_pointer(&self->filename, g_free);
> >  
> >      G_OBJECT_CLASS(spice_file_transfer_task_parent_class)->dispose(object);
> >  }
> > @@ -626,14 +624,14 @@ static void
> >  spice_file_transfer_task_constructed(GObject *object)
> >  {
> >      SpiceFileTransferTask *self = SPICE_FILE_TRANSFER_TASK(object);
> > +    self->filename = g_file_get_basename(self->file);
> >  
> >      if (spice_util_get_debug()) {
> > -        gchar *basename = g_file_get_basename(self->file);
> >          self->start_time = g_get_monotonic_time();
> >          self->last_update = self->start_time;
> >  
> > -        SPICE_DEBUG("transfer of file %s has started", basename);
> > -        g_free(basename);
> > +        SPICE_DEBUG("transfer of file %s has started", self->filename);
> > +        g_free(self->filename);
> >      }
> >  }
> >  
> > diff --git a/src/spice-file-transfer-task.h b/src/spice-file-transfer-task.h
> > index 4f179fb..43d1d86 100644
> > --- a/src/spice-file-transfer-task.h
> > +++ b/src/spice-file-transfer-task.h
> > @@ -41,7 +41,7 @@ typedef struct _SpiceFileTransferTaskClass
> > SpiceFileTransferTaskClass;
> >  
> >  GType spice_file_transfer_task_get_type(void) G_GNUC_CONST;
> >  
> > -char* spice_file_transfer_task_get_filename(SpiceFileTransferTask *self);
> > +const char* spice_file_transfer_task_get_filename(SpiceFileTransferTask
> > *self);
> >  void spice_file_transfer_task_cancel(SpiceFileTransferTask *self);
> >  double spice_file_transfer_task_get_progress(SpiceFileTransferTask *self);
> >  
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel