Re: gdbus branch review
- From: Tomas Bzatek <tbzatek redhat com>
- To: Alexander Larsson <alexl redhat com>
- Cc: gvfs-list gnome org
- Subject: Re: gdbus branch review
- Date: Thu, 31 May 2012 15:13:33 +0200
On Thu, 2012-05-24 at 10:12 +0200, Alexander Larsson wrote:
> Really? Why not create it in the GVfs initializer? That should be
> protected by a lock already. Or if you want to do it lazily, use a
> g_once_init_* thing?
I would still like to retain the on-demand behaviour. Since the metadata
proxy is only used for three calls, I don't want to create it every time
GDaemonVfs or GDaemonFile is used.
Using g_bus_watch_name_on_connection() and its handlers for proxy
creation and destruction would be a good way unless we need it
immediately. There might not be a chance for proxy creation between
g_daemon_vfs_class_init() and the call demanding the proxy as long as
callbacks are async.
> I worry a bit about the GPrivate use with
> g_bus_watch_name_on_connection(). On which thread is the
> metadata_daemon_vanished callback called? Are we guaranteed that it'll
> be on the thread that had the GPrivate? That seems unlikely as it might
> not even run a mainloop...
Ok, GPrivate turned to be hard manageable with the current tools if you
want to be able to clean it by multiple events. Same thing we're facing
in bug 676825.
So I went for the dumb-most solution and used a static proxy instance
surrounded by locks. Haven't committed the changes yet, attaching for
review as I have a feeling I'll rework the patch once more.
> If you don't flush, then a commandline invocation like gvfs-copy will
> exit with the dbus outgoing message queue non-empty, and we will thus
> lose the metadata change. I agree that a flush might be unnecessarily
> costly though. Not sure what the best approach is here. Maybe GDBus
> could do a flush in an atexit handler? Or maybe the gvfs commandline
> utils should ensure flushing on exit?
I don't really like adding workarounds to client apps due to our
imperfection. Looking at the impact as a whole, these two affected
functions are called from GLocalFile side only and there don't
necessarily need to be other traffic generated by other GIO calls. I've
added a sync flush to be on the safe side:
http://git.gnome.org/browse/gvfs/commit/?h=gdbus&id=a541205d83d7a66cb37d2c797aa78a0e258fc4e6
Btw. searching through the code, I couldn't find moved()/removed()
counterparts for GVfs mounts. Since we're storing metadata even for GVfs
mounts, there should be a mechanism to update the db for these
operations. How's that done?
--
Tomas Bzatek <tbzatek redhat com>
diff --git a/client/gdaemonfile.c b/client/gdaemonfile.c
index 5c4f76b..5ab98d6 100644
--- a/client/gdaemonfile.c
+++ b/client/gdaemonfile.c
@@ -29,6 +29,7 @@
#include <sys/socket.h>
#include "gdaemonfile.h"
+#include "gdaemonvfs.h"
#include "gvfsdaemondbus.h"
#include "gdaemonmount.h"
#include <gvfsdaemonprotocol.h>
@@ -55,8 +56,6 @@ G_DEFINE_TYPE_WITH_CODE (GDaemonFile, g_daemon_file, G_TYPE_OBJECT,
G_IMPLEMENT_INTERFACE (G_TYPE_FILE,
g_daemon_file_file_iface_init))
-static GPrivate metadata_proxy = G_PRIVATE_INIT (g_object_unref);
-
static void
g_daemon_file_finalize (GObject *object)
{
@@ -2209,52 +2208,6 @@ g_daemon_file_query_writable_namespaces (GFile *file,
return list;
}
-static void
-metadata_daemon_vanished (GDBusConnection *connection,
- const gchar *name,
- gpointer user_data)
-{
- guint *watcher_id = user_data;
-
- g_private_replace (&metadata_proxy, NULL);
- if (*watcher_id > 0)
- g_bus_unwatch_name (*watcher_id);
-}
-
-static GVfsMetadata *
-get_metadata_proxy (GCancellable *cancellable, GError **error)
-{
- GVfsMetadata *proxy;
- guint *watcher_id;
-
- proxy = g_private_get (&metadata_proxy);
- if (proxy == NULL)
- {
- proxy = gvfs_metadata_proxy_new_for_bus_sync (G_BUS_TYPE_SESSION,
- G_DBUS_PROXY_FLAGS_NONE,
- G_VFS_DBUS_METADATA_NAME,
- G_VFS_DBUS_METADATA_PATH,
- cancellable,
- error);
- g_private_replace (&metadata_proxy, proxy);
-
- if (proxy == NULL)
- return NULL;
-
- /* a place in memory to store the returned ID in */
- watcher_id = g_malloc0 (sizeof (guint));
- *watcher_id = g_bus_watch_name_on_connection (g_dbus_proxy_get_connection (G_DBUS_PROXY (proxy)),
- G_VFS_DBUS_METADATA_NAME,
- G_BUS_NAME_WATCHER_FLAGS_AUTO_START,
- NULL,
- metadata_daemon_vanished,
- watcher_id,
- g_free);
- }
-
- return proxy;
-}
-
static gboolean
set_metadata_attribute (GFile *file,
const char *attribute,
@@ -2279,7 +2232,7 @@ set_metadata_attribute (GFile *file,
g_free (treename);
res = FALSE;
- proxy = get_metadata_proxy (cancellable, error);
+ proxy = _g_daemon_vfs_get_metadata_proxy (cancellable, error);
if (proxy)
{
@@ -2313,6 +2266,7 @@ set_metadata_attribute (GFile *file,
res = FALSE;
g_variant_builder_unref (builder);
+ g_object_unref (proxy);
}
meta_tree_unref (tree);
diff --git a/client/gdaemonvfs.c b/client/gdaemonvfs.c
index 33c218a..6a29539 100644
--- a/client/gdaemonvfs.c
+++ b/client/gdaemonvfs.c
@@ -41,7 +41,6 @@
#include "gvfsiconloadable.h"
#include <glib/gi18n-lib.h>
#include <glib/gstdio.h>
-#include <metadata-dbus.h>
typedef struct {
char *type;
@@ -77,7 +76,9 @@ struct _GDaemonVfsClass
G_DEFINE_DYNAMIC_TYPE (GDaemonVfs, g_daemon_vfs, G_TYPE_VFS)
static GDaemonVfs *the_vfs = NULL;
-static GPrivate metadata_proxy = G_PRIVATE_INIT (g_object_unref);
+
+G_LOCK_DEFINE_STATIC (metadata_proxy);
+static GVfsMetadata *metadata_proxy = NULL;
G_LOCK_DEFINE_STATIC(mount_cache);
@@ -1289,41 +1290,51 @@ metadata_daemon_vanished (GDBusConnection *connection,
{
guint *watcher_id = user_data;
- g_private_replace (&metadata_proxy, NULL);
+ G_LOCK (metadata_proxy);
+ g_clear_object (&metadata_proxy);
+ G_UNLOCK (metadata_proxy);
+
if (*watcher_id > 0)
g_bus_unwatch_name (*watcher_id);
}
-static GVfsMetadata *
-get_metadata_proxy (GError **error)
+GVfsMetadata *
+_g_daemon_vfs_get_metadata_proxy (GCancellable *cancellable, GError **error)
{
GVfsMetadata *proxy;
guint *watcher_id;
- proxy = g_private_get (&metadata_proxy);
- if (proxy == NULL)
+ G_LOCK (metadata_proxy);
+
+ proxy = NULL;
+ if (metadata_proxy == NULL)
{
- proxy = gvfs_metadata_proxy_new_for_bus_sync (G_BUS_TYPE_SESSION,
- G_DBUS_PROXY_FLAGS_NONE,
- G_VFS_DBUS_METADATA_NAME,
- G_VFS_DBUS_METADATA_PATH,
- NULL,
- error);
- g_private_replace (&metadata_proxy, proxy);
-
- if (proxy == NULL)
- return NULL;
-
- /* a place in memory to store the returned ID in */
- watcher_id = g_malloc0 (sizeof (guint));
- *watcher_id = g_bus_watch_name_on_connection (g_dbus_proxy_get_connection (G_DBUS_PROXY (proxy)),
- G_VFS_DBUS_METADATA_NAME,
- G_BUS_NAME_WATCHER_FLAGS_AUTO_START,
- NULL,
- metadata_daemon_vanished,
- watcher_id,
- g_free);
+ metadata_proxy = gvfs_metadata_proxy_new_for_bus_sync (G_BUS_TYPE_SESSION,
+ G_DBUS_PROXY_FLAGS_NONE,
+ G_VFS_DBUS_METADATA_NAME,
+ G_VFS_DBUS_METADATA_PATH,
+ cancellable,
+ error);
+
+ if (proxy != NULL)
+ {
+ /* a place in memory to store the returned ID in */
+ watcher_id = g_malloc0 (sizeof (guint));
+ *watcher_id = g_bus_watch_name_on_connection (g_dbus_proxy_get_connection (G_DBUS_PROXY (proxy)),
+ G_VFS_DBUS_METADATA_NAME,
+ G_BUS_NAME_WATCHER_FLAGS_AUTO_START,
+ NULL,
+ metadata_daemon_vanished,
+ watcher_id,
+ g_free);
+ }
}
+
+ if (metadata_proxy != NULL)
+ /* take the reference so that we don't need to protect returned object against racy metadata_daemon_vanished() */
+ proxy = g_object_ref (metadata_proxy);
+
+ G_UNLOCK (metadata_proxy);
return proxy;
}
@@ -1380,7 +1391,7 @@ g_daemon_vfs_local_file_set_attributes (GVfs *vfs,
FALSE,
&tree_path);
- proxy = get_metadata_proxy (error);
+ proxy = _g_daemon_vfs_get_metadata_proxy (NULL, error);
if (proxy == NULL)
{
res = FALSE;
@@ -1442,6 +1453,7 @@ g_daemon_vfs_local_file_set_attributes (GVfs *vfs,
meta_lookup_cache_free (cache);
meta_tree_unref (tree);
g_free (tree_path);
+ g_object_unref (proxy);
}
}
@@ -1469,7 +1481,7 @@ g_daemon_vfs_local_file_removed (GVfs *vfs,
&tree_path);
if (tree)
{
- proxy = get_metadata_proxy (NULL);
+ proxy = _g_daemon_vfs_get_metadata_proxy (NULL, NULL);
if (proxy)
{
metatreefile = meta_tree_get_filename (tree);
@@ -1483,6 +1495,7 @@ g_daemon_vfs_local_file_removed (GVfs *vfs,
/* flush the call with the expense of sending all queued messages on the connection */
g_dbus_connection_flush_sync (g_dbus_proxy_get_connection (G_DBUS_PROXY (proxy)),
NULL, NULL);
+ g_object_unref (proxy);
}
meta_tree_unref (tree);
@@ -1516,7 +1529,7 @@ g_daemon_vfs_local_file_moved (GVfs *vfs,
&tree_path2);
if (tree1 && tree2 && tree1 == tree2)
{
- proxy = get_metadata_proxy (NULL);
+ proxy = _g_daemon_vfs_get_metadata_proxy (NULL, NULL);
if (proxy)
{
metatreefile = meta_tree_get_filename (tree1);
@@ -1531,6 +1544,7 @@ g_daemon_vfs_local_file_moved (GVfs *vfs,
/* flush the call with the expense of sending all queued messages on the connection */
g_dbus_connection_flush_sync (g_dbus_proxy_get_connection (G_DBUS_PROXY (proxy)),
NULL, NULL);
+ g_object_unref (proxy);
}
}
diff --git a/client/gdaemonvfs.h b/client/gdaemonvfs.h
index 5b1b72e..f96ab89 100644
--- a/client/gdaemonvfs.h
+++ b/client/gdaemonvfs.h
@@ -29,6 +29,7 @@
#include "gmounttracker.h"
#include "gvfsuriutils.h"
#include <metatree.h>
+#include <metadata-dbus.h>
G_BEGIN_DECLS
@@ -78,6 +79,9 @@ int _g_daemon_vfs_append_metadata_for_set (GVariantBuilder *builder
GFileAttributeType type,
gpointer value);
+GVfsMetadata * _g_daemon_vfs_get_metadata_proxy (GCancellable *cancellable,
+ GError **error);
+
G_END_DECLS
[
Date Prev][Date Next] [
Thread Prev][Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
