Re: [Tracker] couple of fixes

["Samuel Cormier-Iijima" <sciyoshi gmail com>]

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/