[Libosinfo] [libosinfo PATCH] os: Don't leak scripts list

[Fabiano Fidêncio]

Hey!

On Thu, Nov 8, 2018 at 10:41 AM Christophe Fergeau <cfergeau at redhat.com> wrote:
>
> hey,
>
> On Wed, Nov 07, 2018 at 09:54:15PM +0100, Fabiano Fidêncio wrote:
> > osinfo_list_get_elements() calls g_hash_table_get_values() which returns
> > a GList that must be freed after used.
> >
> > For more info, please, refer to:
> > https://developer.gnome.org/glib/unstable/glib-Hash-Tables.html#g-hash-table-get-values
> >
> > Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> > ---
> >  osinfo/osinfo_os.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/osinfo/osinfo_os.c b/osinfo/osinfo_os.c
> > index 4f74331..f89861b 100644
> > --- a/osinfo/osinfo_os.c
> > +++ b/osinfo/osinfo_os.c
> > @@ -611,16 +611,21 @@ OsinfoInstallScript *osinfo_os_find_install_script(OsinfoOs *os, const gchar *pr
> >      g_return_val_if_fail(OSINFO_IS_OS(os), NULL);
> >      GList *scripts = osinfo_list_get_elements(OSINFO_LIST(os));
> >      GList *tmp = scripts;
> > +    OsinfoInstallScript *s = NULL;
> >
> >      while (tmp) {
> >          OsinfoInstallScript *script = tmp->data;
> > -        if (g_str_equal(profile, osinfo_install_script_get_profile(script)))
> > -            return script;
> > +        if (g_str_equal(profile, osinfo_install_script_get_profile(script))) {
> > +            s = script;
> > +            break;
> > +        }
>
> Do we really need both 's' and 'script'? I think you could move 'script'
> declaration out of the while() block, and achieve the same result?

Just for the record, I've talked with Christophe on IRC ...

Basically if we move script declaration out of the loop we'd have to
set it to NULL each iteration. The new patch would look like this:

--- a/osinfo/osinfo_os.c
+++ b/osinfo/osinfo_os.c
@@ -611,16 +611,20 @@ OsinfoInstallScript
*osinfo_os_find_install_script(OsinfoOs *os, const gchar *pr
     g_return_val_if_fail(OSINFO_IS_OS(os), NULL);
     GList *scripts = osinfo_list_get_elements(OSINFO_LIST(os));
     GList *tmp = scripts;
+    OsinfoInstallScript *script = NULL;

     while (tmp) {
-        OsinfoInstallScript *script = tmp->data;
+        script = tmp->data;
         if (g_str_equal(profile, osinfo_install_script_get_profile(script)))
-            return script;
+            break;

         tmp = tmp->next;
+        script = NULL;
     }

-    return NULL;
+    g_list_free(scripts);
+
+    return script;
 }

-- 

We also agreed to go for this version (the new one).

>
> Looks good apart from this.
>
> Christophe
>
> >
> >          tmp = tmp->next;
> >      }
> >
> > -    return NULL;
> > +    g_list_free(scripts);
> > +
> > +    return s;
> >  }
> >
> >
> > --
> > 2.19.1
> >
> > _______________________________________________
> > Libosinfo mailing list
> > Libosinfo at redhat.com
> > https://www.redhat.com/mailman/listinfo/libosinfo