Created attachment 213442 [details] [review]
gfile: Plug memory leak in g_file_make_directory_with_parents()

Review of attachment 213442 [details] [review]:

Patch looks good to me, just some nitpicks below

::: gio/gfile.c
@@ +3401,3 @@
 {
   gboolean result;
+  GFile *work_file = NULL;

this initialization is not really needed

@@ +3404,3 @@
   GList *list = NULL, *l;
   GError *my_error = NULL;
 

Not strictly related to the patch, but while at it we should probablly add

  g_return_val_if_fail (G_IS_FILE (file), FALSE);

@@ -3424,1 +3426,1 @@
       parent_file = g_file_get_parent (work_file);

I propose to move "unref(work_file); work_file = NULL" here right after its last use and then assign work_file to parent only in the "if"

(In reply to comment #2)
> Review of attachment 213442 [details] [review]:
> 
> Patch looks good to me, just some nitpicks below
> 
> ::: gio/gfile.c
> @@ +3401,3 @@
>  {
>    gboolean result;
> +  GFile *work_file = NULL;
> 
> this initialization is not really needed

I prefer to initialize all variables, because at some gcc optimization levels (namely -O0), you don't get warnings about used-but-not-initialized, and it's just safer.

> Not strictly related to the patch, but while at it we should probablly add
> 
>   g_return_val_if_fail (G_IS_FILE (file), FALSE);

Done.
 
> @@ -3424,1 +3426,1 @@
>        parent_file = g_file_get_parent (work_file);
> 
> I propose to move "unref(work_file); work_file = NULL" here right after its
> last use and then assign work_file to parent only in the "if"

Hmm... the logic here makes my head hurt...Why do you want to do this?  Can you provide a patch on top of mine so I can look at what you're suggesting concretely?

(In reply to comment #3)
> (In reply to comment #2)
> > Review of attachment 213442 [details] [review] [details]:
> > 
> > Patch looks good to me, just some nitpicks below
> > 
> > ::: gio/gfile.c
> > @@ +3401,3 @@
> >  {
> >    gboolean result;
> > +  GFile *work_file = NULL;
> > 
> > this initialization is not really needed
> 
> I prefer to initialize all variables, because at some gcc optimization levels
> (namely -O0), you don't get warnings about used-but-not-initialized, and it's
> just safer.
> 

Dunno, it is not consistent with the glib/gtk practice and usually I prefer to see the warning than to have to debug an unexpected NULL value. Anyway, just phylosophy, do not consider this blocking by any means

> > Not strictly related to the patch, but while at it we should probablly add
> > 
> >   g_return_val_if_fail (G_IS_FILE (file), FALSE);
> 
> Done.
> 
> > @@ -3424,1 +3426,1 @@
> >        parent_file = g_file_get_parent (work_file);
> > 
> > I propose to move "unref(work_file); work_file = NULL" here right after its
> > last use and then assign work_file to parent only in the "if"
> 
> Hmm... the logic here makes my head hurt...Why do you want to do this?  Can you
> provide a patch on top of mine so I can look at what you're suggesting
> concretely?

The logic here is indeed a bit twisted and when reviewing the patch I tried to think how to make the code shorter and slightly simpler, so I think we agree on the goal :)

I guess I'll give it a try and see if it works, but once again do not consider this blocking by any means and go ahead and commit as far as I am concerned

This is the way I was proposing to do it (I can provide it as a patch, but I think showing the full code makes it more clear)

..
  work_file = g_object_ref (file);

  while (work_file) 
    {
      GFile *parent_file;

      g_clear_error (&my_error);

      parent_file = g_file_get_parent (work_file);
      g_object_unref (work_file);
      work_file = NULL;

      if (parent_file == NULL)
        break;

      result = g_file_make_directory (parent_file, cancellable, &my_error);

      if (!result && my_error->code == G_IO_ERROR_NOT_FOUND)
        {
          list = g_list_prepend (list, parent_file);  /* Transfer ownership of ref */
          work_file = g_object_ref (parent_file);
        }
      else
        g_object_unref (parent_file);
    }

...

Created attachment 213823 [details] [review]
patch

here is the patch on top of yours in case you prefer it.

Note: not tested.


Speaking of which, this function could use an unit test in tests/live-g-file.c

Review of attachment 213823 [details] [review]:

::: gio/gfile.c
@@ +3418,3 @@
   work_file = g_object_ref (file);
+
+  while (work_file)

This change completely breaks the logic.  Note for example, "work_file" isn't changed in the non-error case.

Since I've tested my original patch in a few conditions, let's go with it.  I've fixed the trailing whitespace.

Attachment 213442 [details] pushed as 5a57144 - gfile: Plug memory leak in g_file_make_directory_with_parents()

(In reply to comment #7)
> Review of attachment 213823 [details] [review]:
> 
> ::: gio/gfile.c
> @@ +3418,3 @@
>    work_file = g_object_ref (file);
> +
> +  while (work_file)
> 
> This change completely breaks the logic.  Note for example, "work_file" isn't
> changed in the non-error case.
> 

I do not see what you mean: it is set to NULL and you exit the loop and since the if condition is the same as the loop condition it does not change things...

> Since I've tested my original patch in a few conditions, let's go with it. 
> I've fixed the trailing whitespace.

Anyway sure, no need to further fiddle with the function