gdbus branch review

[Alexander Larsson <alexl redhat com>]

I took a look at the gdbus branch. I didn't really try the code, as I
assume its been tested. I did notice some issues in the code (below).
Also, I think once these are fixed we should rebase the branch and merge
the changes so that its a clean set of changes that don't e.g. add stuff
and then remove it, or unnecessary split up changes because you found a
bug later.

------------------------------------------------------------------------

* monitor/proxy/dbus-interfaces.xml:

This uses CamelCase for the arg names, this looks weird in C code and
causes some unnecessary C code changes in the diffs, i.e:
-      if (g_strcmp0 (owner, sender) == 0 && g_strcmp0 (id, cancellation_id) == 0)
+      if (g_strcmp0 (owner, sender) == 0 && g_strcmp0 (id, CancellationId) == 0)
or
-      if (strcmp (drive_id, id) == 0)
+      if (strcmp (drive_id, Id) == 0)

Not a huge problem, but imho unnecessary as it clutters up e.g. the
change history in git blame.


* gvfsproxyvolumemonitor.c:

on_name_acquired/lost:
 Should drop the g_warning spew
 
handle_is_supported:
 Only call ensure_name_owner_changed_for_unique_name () if monitor !=
 NULL.

All the locks seem unnecessary since we're not using threads anymore for
the skeleton.
 
-static void
-append_mount (GMount *mount, DBusMessageIter *iter_array)
+static GVariant *
+append_mount (GMount *mount)

"append_*" seems like a strange name now :)


ask_password_cb () (and ask_question_cb, show_processes_cb, aborted_cb,

  connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error);
  if (connection == NULL)

This all should just call g_dbus_interface_skeleton_get_connection
(monitor_daemon) instead. There is no way this will fail
as then you would not have gotten the calls.


* GIO implementation in proxy (gproxydrive.c, gproxyvolumemonitor.c,
 etc):

  proxy = g_proxy_volume_monitor_get_dbus_proxy (data->drive->volume_monitor);
  g_proxy_volume_monitor_lock_for_timeout ();
  g_dbus_proxy_set_default_timeout (G_DBUS_PROXY (proxy), G_PROXY_VOLUME_MONITOR_DBUS_TIMEOUT);  /* 30 minute timeout */

These locks don't work, because lock is not always taken. The cases
where the lock is not taken expect to be using the -1 timeout, but may
not due to races with another call that uses a 30 sec timeout. Otoh, are
we really using threads here anymore? I think dropping threads in
GVfsProxyVolumeDaemon fixed that.

* metadata (client/gdaemonvfs.c client/gdaemonfile.c):

The proxy for the metadata daemon is recreated a lot, it should be
constructed once then then kept around in the GVfs for future use. This
is especially problematic since constructing the proxy does sync i/o,
and this is done in g_daemon_vfs_local_file_moved/removed which is
called for every local file remove, making them sync even if they are
async.

Furthermore, for the g_daemon_vfs_local_file_moved/removed case the
actual method call is sync too. This further adds sync i/o on a possibly
async codepath. This used to be a send + flush + ignore result
(send_message_oneway), and need to still be.