On Wed, 2009-02-11 at 17:55 +0000, Martyn Russell wrote: > Philip Van Hoof wrote: > > This is a new patch but with the indentation right for the two new files > > this time > > Hi Philip, > > Some initial comments. I have not had a chance to try the new code yet. > > > #1, I don't have src/trackerd/tracker-push.[ch] to patch, so the patch > fails to apply. This also means I have some more reviewing to do once I > have those files.
Attached You can also just use git on this (branch is called push): http://git.codethink.co.uk/?p=tracker;a=shortlog;h=push > #2, Before we go on, can you explain ALL the libraries and what they do. > From the names I don't really have any concept of what they are > supposed to be doing. As far as I can see, we have: > > - libtracker_evolution_indexer.la (now renamed) > - libtracker-evolution-daemon-module.la > - libtracker-evolution-indexer-module.la > - ... *-indexer-module.la is an 'indexer' module which runs in tracker-indexer. It will pump the data actually into the database. *-daemon-module.la is a 'deamon' module which runs in trackerd. It will receive the data from the external application (presumably over DBus) and it will 'pass it on' to the *-indexer-module.la. > #3, The naming convention here should be a bit more thought out I think. > Something like libtracker-push-evolution-{daemon|index}.la. Either that > or we follow the convention we have for the indexer, i.e. > libtracker-module-evolution-{daemon|indexer}.la. They will be in this > directory anyway: $libdir/tracker/push-modules/. > > +pushd_modules_LTLIBRARIES = libtracker-evolution-daemon-module.la > +pushi_modules_LTLIBRARIES = libtracker-evolution-indexer-module.la Added -module to the filename. > #4, Will tracker-push.[ch] apply to JUST evolution or all modules which > push data to Tracker? If this is just for evolution I would rather that > was reflected in the file name. tracker-push.[ch] will apply to all modules which push data to Tracker. > #5, Can we keep names consistent here please, i.e. keep to _init() - we > use that everywhere in the Tracker source. If we don't we use _new(). > > -tracker_evolution_storer_init (TrackerConfig *config, > - TrackerIndexer *indexer) > +tracker_push_module_create (TrackerConfig *config, > + TrackerIndexer *indexer) Renamed > #6, Why did you remove this? > -G_DEFINE_TYPE (TrackerEvolutionRegistrar, tracker_evolution_registrar, > G_TYPE_OBJECT) It's not removed, it's at line 68 in tracker-evolution-registrar.c > #7, Do we really have to use goto statements here? Can't we just > propagate the error up and return? > > + /* Creation of the registrar */ > + if (!org_freedesktop_DBus_request_name (dbus_proxy, > + > TRACKER_EVOLUTION_REGISTRAR_SERVICE, > + DBUS_NAME_FLAG_DO_NOT_QUEUE, > + &result, &nerror)) { > + > + g_critical ("Could not setup DBus, %s in use\n", > + TRACKER_EVOLUTION_REGISTRAR_SERVICE); > + > + goto handle_error; > + } > + > + if (nerror) > + goto handle_error; Fixed > #8, Remember to use void in functions with no argument signature: > > +void > +tracker_push_shutdown () Fixed > #9, I presume this is where Carlos suggested we use a GInterface: > > +typedef struct { > + TrackerPushRegistrar* (*create) (void); > + void (*shutdown) (TrackerPushRegistrar *registrar); > + TrackerPushRegistrar *registrar; > + GModule *module; > +} PushModule; > + > > Since we do this in both tracker-push.[ch] for the trackerd and > tracker-indexer. It feels like we are duplicating a lot of stuff here to > the other tracker-push.c file. Also, with the above code block, it looks > like you are defining the functions again in the struct, doesn't it make > more sense to define that struct in a header and be used by both > trackerd and tracker-indexer? No it doesn't, the structures are not the same, they don't have the same purpose. And the init code is also different for the daemon and the indexer modules. So these two can't be shared. > #10, Why do you wait until the end of the load_modules() function > (tracker-indexer/tracker-push.c) to handle the error? Isn't it just > easier to print the message, handle the error then return there instead > of adding a goto? > > handle_error: > > if (error) { > g_debug ("%s\n", error->message); > g_error_free (error); > } Fixed > #11, Can we not use while() for list iterations. It is less maintainable > and error prone: > > + while (copy && !found) { > + PushModule *p_module = copy->data; > + TrackerPushRegistrar *registrar = p_module->registrar; > + > ... > > It means, if someone puts in a continue; in there, the whole block is > broken because they forget to add: > > copy = g_list_next (copy); > > It is easier for people to understand the rules of the iteration if they > are all listed at the top of the code block: > > for (; copy && !found; copy = g_list_next (copy)) { > ... > } Fixed > > #12, I see you have more places where you use goto on an error just to > print something out. To avoid jumping around the code to find out what > is actually happening, it is more maintainable for places with errors to > just be handled there and then and for return to be called (unless there > are a lot of places in the function that would have to do the same > operation at the end - like cleaning up memory). Can we do that and > avoid goto unless we need to use it. It makes it harder to understand > the flow of the functions. Sure. For errors I often use the concept of 'try, except, finally', but with C not having support for exceptions and such try-except blocks I often simulate it by using a goto. Which is probably one of the few situations where goto isn't necessarily a bad idea. But I'll replace them where possible. > #13, For the cases where we reference another object here, we need to > make sure we reference the new object first. We do this to protect > ourselves from unreferencing the same object we are about to reference, > (in the unlikely event that we set the object to the same object we > already have in priv->object): > > +void > +tracker_push_registrar_set_object (TrackerPushRegistrar *registrar, > + GObject *object) > +{ > + TrackerPushRegistrarPrivate *priv; > + > + priv = TRACKER_PUSH_REGISTRAR_GET_PRIVATE (registrar); > + > + if (priv->object) > + g_object_unref (priv->object); > + > + if (object) > + priv->object = g_object_ref (object); > + else > + priv->object = NULL; > +} > + So, what is the problem here exactly? > #14, For all the tracker_push_registrar_() calls which are properties, > we need to add g_object_notify() calls when there is a change. > Done > #15, Spacing: > > + if (TRACKER_PUSH_REGISTRAR_GET_CLASS (registrar)->enable) { > + > + TRACKER_PUSH_REGISTRAR_GET_CLASS (registrar)->enable (registrar, > + Fixed > #16, Just wondering if this makes more sense to have a boolean in the > ->enable method and no ->disable method. > > +struct TrackerPushRegistrarClass { > + GObjectClass parent_class; > + > + void (*enable) (TrackerPushRegistrar *registrar, > + DBusGConnection *connection, > + DBusGProxy *dbus_proxy, > + GError **error); > + > + void (*disable) (TrackerPushRegistrar *registrar); > +}; I think it's better that way because the module developer's enable and disable are really distinctive kinds of functions. These are btw called when the DBus service disappears and appears. > #17, Since we no longer use this: > > -#ifdef HAVE_EVOLUTION_PLUGIN > > Do we actually have an option to build the push modules at this point? > Currently they are disabled by default. We should do the same here for a > while until things stabilise. Added: --enable-evolution-push-module --enable-rss-push-module --enable-kmail-push-module > Other than that, thanks for the patch and sorry for the delayed response > to reviewing it. I would like to get this committed some time this week > if possible, does that sound plausible to you Philip? :) Yeah, I can commit this today. I think it's best if you just use the git at http://git.codethink.co.uk/?p=tracker;a=shortlog;h=push -- Philip Van Hoof, freelance software developer home: me at pvanhoof dot be gnome: pvanhoof at gnome dot org http://pvanhoof.be/blog http://codeminded.be
/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ /* * Copyright (C) 2008, Nokia * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public * License as published by the Free Software Foundation; either * version 2 of the License, or (at your option) any later version. * * This library is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * General Public License for more details. * * You should have received a copy of the GNU General Public * License along with this library; if not, write to the * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, * Boston, MA 02110-1301, USA. * * Authors: * Philip Van Hoof <[email protected]> */ #include "config.h" #include <string.h> #include <time.h> #include <dbus/dbus-glib-lowlevel.h> #include "tracker-push.h" #include "tracker-push-registrar.h" typedef struct { TrackerConfig *config; DBusGConnection *connection; DBusGProxy *dbus_proxy; GList *modules; } PushSupportPrivate; typedef struct { TrackerPushRegistrar* (*create) (void); void (*shutdown) (TrackerPushRegistrar *registrar); TrackerPushRegistrar *registrar; GModule *module; } PushModule; static GStaticPrivate private_key = G_STATIC_PRIVATE_INIT; static void unload_modules (PushSupportPrivate *private) { GList *copy = private->modules; while (copy) { PushModule *p_module = copy->data; p_module->shutdown (p_module->registrar); g_object_unref (p_module->registrar); g_module_close (p_module->module); g_slice_free (PushModule, p_module); copy = g_list_next (copy); } g_list_free (private->modules); private->modules = NULL; } static void load_modules (PushSupportPrivate *private) { GError *error = NULL; GDir *dir = g_dir_open (PUSH_MODULES_DIR, 0, &error); const gchar *name; if (error) goto handle_error; while ((name = g_dir_read_name (dir)) != NULL) { if (g_str_has_suffix (name, G_MODULE_SUFFIX)) { gchar *path = g_build_filename (PUSH_MODULES_DIR, name, NULL); PushModule *p_module = g_slice_new (PushModule); p_module->module = g_module_open (path, G_MODULE_BIND_LOCAL); if (!g_module_symbol (p_module->module, "tracker_push_module_shutdown", (gpointer *) &p_module->shutdown) || !g_module_symbol (p_module->module, "tracker_push_module_create", (gpointer *) &p_module->create)) { g_warning ("Could not load module symbols for '%s': %s", path, g_module_error ()); g_module_close (p_module->module); g_slice_free (PushModule, p_module); } else { g_module_make_resident (p_module->module); p_module->registrar = p_module->create (); private->modules = g_list_prepend (private->modules, p_module); } g_free (path); } } handle_error: if (error) { g_debug ("%s\n", error->message); g_error_free (error); } } static void name_owner_changed_cb (DBusGProxy *proxy, gchar *name, gchar *old_owner, gchar *new_owner, gpointer user_data) { GList *copy; PushSupportPrivate *private; gboolean found = FALSE; private = g_static_private_get (&private_key); g_return_if_fail (private != NULL); /* If we receive a NameOwnerChanged about the manager's service */ copy = private->modules; while (copy && !found) { PushModule *p_module = copy->data; TrackerPushRegistrar *registrar = p_module->registrar; const gchar *service = tracker_push_registrar_get_service (registrar); if (g_strcmp0 (name, service) == 0) { if (tracker_is_empty_string (new_owner) && !tracker_is_empty_string (old_owner)) { tracker_push_registrar_disable (registrar); } if (tracker_is_empty_string (old_owner) && !tracker_is_empty_string (new_owner)) { GError *error = NULL; tracker_push_registrar_enable (registrar, private->connection, private->dbus_proxy, &error); if (error) { g_debug ("%s\n", error->message); g_error_free (error); } } found = TRUE; } copy = g_list_next (copy); } if (!found) { copy = private->modules; /* If the manager's service is found, start the registrar */ while (copy) { PushModule *p_module = copy->data; TrackerPushRegistrar *registrar = p_module->registrar; const gchar *service = tracker_push_registrar_get_service (registrar); if (g_strcmp0 (name, service) == 0) { GError *error = NULL; tracker_push_registrar_enable (registrar, private->connection, private->dbus_proxy, &error); if (error) { g_debug ("%s\n", error->message); g_error_free (error); } break; } copy = g_list_next (copy); } } } static void list_names_reply_cb (DBusGProxy *proxy, DBusGProxyCall *call, gpointer user_data) { PushSupportPrivate *private; GError *error = NULL; GStrv names = NULL; guint i = 0; gboolean found = FALSE; private = g_static_private_get (&private_key); g_return_if_fail (private != NULL); dbus_g_proxy_end_call (proxy, call, &error, G_TYPE_STRV, &names, G_TYPE_INVALID); if (error) { g_warning ("%s", error->message); g_error_free (error); if (names) g_strfreev (names); return; } while (names[i] != NULL && !found) { GList *copy = private->modules; /* If the manager's service is found, start the registrar */ while (copy) { PushModule *p_module = copy->data; TrackerPushRegistrar *registrar = p_module->registrar; const gchar *service = tracker_push_registrar_get_service (registrar); if (g_strcmp0 (names[i], service) == 0) { GError *lerror = NULL; tracker_push_registrar_enable (registrar, private->connection, private->dbus_proxy, &lerror); if (lerror) { g_debug ("%s\n", lerror->message); g_error_free (lerror); } found = TRUE; break; } copy = g_list_next (copy); } i++; } g_strfreev (names); } static void free_private (PushSupportPrivate *private) { if (private->connection) dbus_g_connection_unref (private->connection); if (private->config) g_object_unref (private->config); g_free (private); } void tracker_push_init (TrackerConfig *config) { DBusGConnection *connection; GError *error = NULL; PushSupportPrivate *private; private = g_new0 (PushSupportPrivate, 1); g_static_private_set (&private_key, private, (GDestroyNotify) free_private); load_modules (private); connection = dbus_g_bus_get (DBUS_BUS_SESSION, &error); if (!error) { GList *copy; DBusError dbus_error; private->config = g_object_ref (config); private->connection = dbus_g_connection_ref (connection); private->dbus_proxy = dbus_g_proxy_new_for_name (private->connection, DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS); /* We listen for NameOwnerChanged to know when the manager's service * comes up and to know when it goes down */ dbus_g_proxy_add_signal (private->dbus_proxy, "NameOwnerChanged", G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_INVALID); dbus_g_proxy_connect_signal (private->dbus_proxy, "NameOwnerChanged", G_CALLBACK (name_owner_changed_cb), NULL, NULL); dbus_error_init (&dbus_error); copy = private->modules; while (copy) { PushModule *p_module = copy->data; TrackerPushRegistrar *registrar = p_module->registrar; const gchar *service = tracker_push_registrar_get_service (registrar); gchar *dbus_string = g_strdup_printf ("type='signal'," "sender='" DBUS_SERVICE_DBUS "',interface='" DBUS_INTERFACE_DBUS "',path='" DBUS_PATH_DBUS "',member='NameOwnerChanged'," "arg0='%s'", service); dbus_bus_add_match ((DBusConnection *) dbus_g_connection_get_connection (private->connection), dbus_string, &dbus_error); if (dbus_error_is_set (&dbus_error)) { g_warning ("%s for rule=%s\n", dbus_error.message, dbus_string); g_free (dbus_string); dbus_error_free (&dbus_error); break; } g_free (dbus_string); copy = g_list_next (copy); } /* If the manager service is up and running, then list_names_reply_cb * will execute activate_registrar, as it'll appear in the results of * the ListNames DBus function. If not then we will just wait for the * NameOwnerChanged to emit that the manager's service has came up. */ dbus_g_proxy_begin_call (private->dbus_proxy, "ListNames", list_names_reply_cb, NULL, NULL, G_TYPE_INVALID, G_TYPE_INVALID); } else { g_critical ("Could not setup DBus, %s\n", error->message); g_error_free (error); } } void tracker_push_shutdown (void) { PushSupportPrivate *private; private = g_static_private_get (&private_key); g_return_if_fail (private != NULL); if (private->dbus_proxy) { g_object_unref (private->dbus_proxy); private->dbus_proxy = NULL; } unload_modules (private); g_static_private_set (&private_key, NULL, NULL); }
/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ /* * Copyright (C) 2008, Nokia * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public * License as published by the Free Software Foundation; either * version 2 of the License, or (at your option) any later version. * * This library is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * General Public License for more details. * * You should have received a copy of the GNU General Public * License along with this library; if not, write to the * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, * Boston, MA 02110-1301, USA. * * Authors: * Philip Van Hoof <[email protected]> */ #ifndef __TRACKERD_PUSH_H__ #define __TRACKERD_PUSH_H__ #if !defined (TRACKER_ENABLE_INTERNALS) && !defined (TRACKER_COMPILATION) #error "TRACKER_ENABLE_INTERNALS not defined, this must be defined to use tracker's internal functions" #endif #include <glib.h> #include <gmodule.h> #include <glib-object.h> #include <dbus/dbus-glib-bindings.h> #include <libtracker-common/tracker-common.h> G_BEGIN_DECLS void tracker_push_init (TrackerConfig *config); void tracker_push_shutdown (void); G_END_DECLS #endif /* __TRACKERD_PUSH_H__ */
_______________________________________________ tracker-list mailing list [email protected] http://mail.gnome.org/mailman/listinfo/tracker-list
