Re: [Tracker] REVIEW: external-crawler branch, part1

[Martyn Russell <martyn lanedo com>]

On 08/08/14 08:58, Philip Van Hoof wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> I'll review in my slack time. So I'll do this chunk per chunk. Let's
> start with the first few commits ;-)
>
> e5b5280b5f336b80b00d9c9671a72391ab1f0a9b
>
> +               data->dbus_path = g_strdup_printf ("/%s", dbus_name);
> +
> +               p = data->dbus_path;
> +               while ((p = strchr (p, '.')) != NULL) {
> +                       *p++ = '/';
> +               }
> +
> +               data->dbus_name = l->data;
>
>
> D-Bus service names and D-Bus paths shouldn't be correlated like this.

Do you have a better approach in mind?
I admit this is quick and dirty, but if there is a proper API somewhere, 
I will use it.

> It should be possible to have a different D-Bus service name than the
> object's path. Translating the name to a path by search and replacing
> . with / doesn't sound right to me.

Without a .desktop file as we currently use, is there a way to do this?

> f985f643b849a61fe7a10a3fd9c4f7f4a8083b13
>
> In finalize:
>
> +       if (priv->roots_to_notify) {
> +               g_hash_table_unref (priv->roots_to_notify);
> +       }
>
> vs.
>
> notify_roots_finished:
>
> +       if (check_queues && g_hash_table_size (fs->priv->roots_to_notify) < 2) {
>
>
> So if you check priv->roots_to_notify for not being NULL in finalize,
> then isn't it possible that in notify_roots_finished the same member
> can also be NULL? Or is notify_roots_finished always disconnected in a
> timely matter?

I will check. Thanks for noticing.

> c2863b39b4e8f178c4d87ceee5d96ad0c3647585
>
>         if (fs->priv->item_queue_blocker) {
>                 trace_eq ("   cancelled: item queue blocked waiting for file '%s'",
> - -                       g_file_get_path (fs->priv->item_queue_blocker));
> +                         g_file_get_uri (fs->priv->item_queue_blocker));
>                 return;
>
> "  a string containing the GFile's URI. The returned string should be
> freed with g_free() when no longer needed."
>
> Afaik this was and is in both cases a memory leak. You need to
> g_free() the result of g_file_get_uri. You probably just search and
> replaced here, but it found a leak ;-)

You're right, but so does _get_path() both leak. The trace_*() functions 
are usually disabled, making this a moot point, it depends on how 
particular you want to be. This existed before I got here, I think we 
can blame Aleksander :)

--
Regards,
Martyn

Founder & Director @ Lanedo GmbH.
http://www.linkedin.com/in/martynrussell