Re: PATCH: anti memleak part 1

[Emmanuel <e allaud wanadoo fr>]

On 2001.10.18 16:55 Ali Akcaagac wrote:
> jo, subject says everything. please REVIEW the patch before

> diff -ruN balsa-cvs/src/address-book-config.c
> balsa/src/address-book-config.c
> --- balsa-cvs/src/address-book-config.c	Thu Oct 18 16:38:15 2001
> +++ balsa/src/address-book-config.c	Thu Oct 18 16:51:17 2001
> @@ -94,7 +94,10 @@
>      GtkWidget *bbox;
>      GtkWidget *button;
>      GtkWidget *page;
> -    gchar *name;
> +    gchar *name = NULL;
> +    gchar *path = NULL;
> +    gchar *host_name = NULL;
> +    gchar *base_dn = NULL;
>      gint num;
>  
>      abc = g_new0(AddressBookConfig, 1);
> @@ -184,27 +187,25 @@
>      name = gtk_entry_get_text(GTK_ENTRY(abc->name_entry));
>      if (address_book == NULL) {
>  	if (abc->create_type == LIBBALSA_TYPE_ADDRESS_BOOK_VCARD) {
> -	    gchar *path =
> -		gnome_file_entry_get_full_path(GNOME_FILE_ENTRY
> -					       (abc->ab_specific.vcard.path),
> FALSE);
> +	    path = gnome_file_entry_get_full_path(GNOME_FILE_ENTRY
> +						  (abc->ab_specific.vcard.path),
> +						  FALSE);
>  	    if (path != NULL) 
>  		address_book = libbalsa_address_book_vcard_new(name,
> path);
>  	} else if (abc->create_type == LIBBALSA_TYPE_ADDRESS_BOOK_LDIF)
> {
> -	    gchar *path =
> -		gnome_file_entry_get_full_path(GNOME_FILE_ENTRY
> -					       (abc->ab_specific.ldif.path),
> FALSE);
> +	    path = gnome_file_entry_get_full_path(GNOME_FILE_ENTRY
> +						  (abc->ab_specific.ldif.path),
> +						  FALSE);
>  	    if (path != NULL)
>  		address_book = libbalsa_address_book_ldif_new(name,
> path);
>  #ifdef ENABLE_LDAP
>  	} else if (abc->create_type == LIBBALSA_TYPE_ADDRESS_BOOK_LDAP)
> {
> -	    gchar *host_name =
> -		gtk_entry_get_text(GTK_ENTRY
> -				   (abc->ab_specific.ldap.host_name));
> -	    gchar *base_dn =
> -		gtk_entry_get_text(GTK_ENTRY
> -				   (abc->ab_specific.ldap.base_dn));
> -	    address_book =
> -		libbalsa_address_book_ldap_new(name, host_name,
> base_dn);
> +	    host_name = gtk_entry_get_text(GTK_ENTRY
> +					   (abc->ab_specific.ldap.host_name));
> +	    base_dn = gtk_entry_get_text(GTK_ENTRY
> +					 (abc->ab_specific.ldap.base_dn));
> +	    address_book = libbalsa_address_book_ldap_new(name,
> host_name,
> +			   				  base_dn);
>  #endif
>  	} else
>  	    g_assert_not_reached();
> @@ -219,9 +220,9 @@
>  
>  	if (LIBBALSA_IS_ADDRESS_BOOK_VCARD(address_book)) {
>  	    LibBalsaAddressBookVcard *vcard;
> -	    gchar *path =
> -		gnome_file_entry_get_full_path(GNOME_FILE_ENTRY
> -					       (abc->ab_specific.vcard.path),
> FALSE);
> +	    path = gnome_file_entry_get_full_path(GNOME_FILE_ENTRY
> +						  (abc->ab_specific.vcard.path),
> +						  FALSE);
>  
>  	    vcard = LIBBALSA_ADDRESS_BOOK_VCARD(address_book);
>  	    if (path) {
> @@ -230,10 +231,9 @@
>  	    }
>  	} else if (LIBBALSA_IS_ADDRESS_BOOK_LDIF(address_book)) {
>  	    LibBalsaAddressBookLdif *ldif;
> -	    gchar *path =
> -		gnome_file_entry_get_full_path(GNOME_FILE_ENTRY
> -					       (abc->ab_specific.ldif.path),
> 
> -					       FALSE);
> +	    path = gnome_file_entry_get_full_path(GNOME_FILE_ENTRY
> +						  (abc->ab_specific.ldif.path),
> +						  FALSE);
>  
>  	    ldif = LIBBALSA_ADDRESS_BOOK_LDIF(address_book);
>  	    if (path) {
> @@ -243,12 +243,10 @@
>  #ifdef ENABLE_LDAP
>  	} else if (LIBBALSA_IS_ADDRESS_BOOK_LDAP(address_book)) {
>  	    LibBalsaAddressBookLdap *ldap;
> -	    gchar *host_name =
> -		gtk_entry_get_text(GTK_ENTRY
> -				   (abc->ab_specific.ldap.host_name));
> -	    gchar *base_dn =
> -		gtk_entry_get_text(GTK_ENTRY
> -				   (abc->ab_specific.ldap.base_dn));
> +	    host_name =	gtk_entry_get_text(GTK_ENTRY
> +					   (abc->ab_specific.ldap.host_name));
> +	    base_dn = gtk_entry_get_text(GTK_ENTRY
> +					 (abc->ab_specific.ldap.base_dn));
>  
>  	    ldap = LIBBALSA_ADDRESS_BOOK_LDAP(address_book);
>  
> @@ -265,7 +263,22 @@
>  					 (abc->expand_aliases_button));
>      }
>      gtk_widget_destroy(abc->window);
> -    g_free(abc);
> +
> +    if (abc != NULL)
> +	g_free(abc);
> +
> +    if (name != NULL)
> +	g_free(name);
> +
> +    if (path != NULL)
> +	g_free(path);
> +
> +    if (host_name != NULL)
> +	g_free(host_name);
> +
> +    if (base_dn != NULL)
> +	g_free(base_dn);
> +
>      return address_book;
>  }

OK first of all you don't need to test if a pointer is !=NULL before
applying g_free() on it, indeed g_free is just a no-op on NULL-pointer.
Second you assign to eg name different pointer that are result of different
functions then you g_free it. I think you should g_free it each time you've
finished with what you've assigned to it (in case the result of the
function is a newly allocated buffer, that is not allways the case).

So the rule is : I assign a newly allocated buffer to a pointer (could be
result of a func call), I do what I want to do with it, when I'm done I
g_free it before I need the pointer for another allocation.

Other comments coming soon.
Bye
Manu