[Bug 27400] review fix-segaults

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=27400


Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|0.9                         |git master




--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2010-03-31 10:24:57 PST ---
Short version: please apply the Principle of Least Astonishment whenever you
design APIs :-)

In the first patch it seems you're changing the semantics of
tpl_channel_text_clean_up_stale_tokens from "free the list and the contained
strings" to "free the list, don't free the contained strings". Are you sure
this isn't a memory leak?

In any case, I think this signature is extremely surprising - functions
shouldn't take ownership of (i.e. destroy) one of their arguments unless the
name of the function makes this extremely obvious. I'd prefer it like this:

/* make it iterate non-destructively */
void tpl_channel_text_clean_up_stale_tokens (..., const GList *);

/* at each use: */
tpl_channel_text_clean_up_stale_tokens (..., list);
g_list_foreach (list, (GFunc) g_free, NULL);    /* if needed */
g_list_free (list);

In the second patch I think the argument change is a necessary evil with the
API as currently designed, but it surprises me that a function called
tpl_channel_text_msg_token_exist_in_cache() has side-effects. (Also, it should
have been called ..._exists_... anyway.)

Please rename this function to tpl_channel_text_msg_token_remove_from_cache()
to make the side-effect obvious - there is considerable precedent for a
"remove" method returning a gboolean to say whether it actually removed
anything (e.g. g_hash_table_remove()).

> + * frees the list and its items (char *).

We use gchar * for strings allocated by the g_malloc/g_free family, and char *
for strings allocated by something else (e.g. malloc/free, xmlMalloc/xmlFree) -
g_malloc is not guaranteed to be compatible with malloc on all platforms.

>  * used by:

There's no need for this; please assume that future developers are capable of
operating grep correctly :-)

> +  if (g_list_length (l) > 1)
> +    CRITICAL ("found more than one log-id %s, it shouldn't be possible",
> +        tpl_message_token);

No. Please think about GList's underlying structure, and why this assertion
will fail.

Suppose we have this valid list (pretend for a moment that l->data was a simple
string and not some sort of struct):

["one", "two", "three"]

Because GList is a doubly-linked list, this is actually represented as:

l1 = { .data = "one", .prev = NULL , .next = l2 }
l2 = { .data = "two", .prev = l1 , .next = l3 }
l3 = { .data = "three", .prev = l2 , .next = NULL }

Now search for "one" or "two"; you'll get l1 or l2. The g_list_length() will be
3 or 2, respectively, and the assertion will fail.

(Even in situations where this would work, "g_list_length (foo) > 1" is just an
inefficient O(n) way to spell "foo != NULL && foo->next != NULL", which is
O(1).)

The rest looks OK.


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.