Re: [Tracker] couple of fixes



On 9/21/06, Jamie McCracken <jamiemcc blueyonder co uk> wrote:
Jamie McCracken wrote:
> Samuel Cormier-Iijima wrote:
>> OK, I've split this up into two patches, and got rid of changing all
>> of the tracker_logs to g_messages. So now, tracker_log is just a macro
>> that expands to g_message. inotify.patch is the patch for numbers 1
>> and 3. Cheers,
>
> Okay the intoify patch is a bit tricky to review due to the complexity
> of the code. (I am a bit apprehensive about changing them as it can
> introduce new bugs and leaks)
>
> I think the delete_watch leaks res object in your patch
>
> I will look at it a bit more tomorrow but I may only accept part of your
> patch if thats okay with you.
>
> The inotify move stuff is especially  complex but well tested and so I
> want to keep that bit as is. I will accept the rest though. (I hope you
> understand the time taking to review *and* test for bugs and leaks can
> be excessive especially with time in short supply)

sorry for my rant - just been short on time lately.

I've now completed review of your entire inotify patch

I've accepted it all but have made the following changes:

1) I put back the if (str) g_free (str) stuff at the end of
process_inotify_events which your patch removed.

In general, its almost always better to have these then not to prevent a
sigabort from glibc shutting down tracker if we try and free stuff thats
not allocated.

In process_inotify_events monitor_name can be NULL so its definitely
needed here.

Actually, if str is NULL, g_free returns and does nothing:
http://developer.gnome.org/doc/API/2.0/glib/glib-Memory-Allocation.html#g-free

So therfore, if (str) g_free (str) and g_free (str) are equivalent,
and actually the first is also redundant and takes more code :-) But
anyways that's a small point :-)

Cheers,
Samuel Cormier-Iijima

2) Added tracker_db_free_result (res) to delete_watch to fix the leak in
your patch.


Have now added to cvs along with your logging patch - thanks for your
time on this.


--
Mr Jamie McCracken
http://jamiemcc.livejournal.com/




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]