Re: gdbus branch review

[Tomas Bzatek <tbzatek redhat com>]

On Mon, 2012-04-23 at 15:39 +0200, Alexander Larsson wrote:
> 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.

Thanks for the review, Alex! I've fixed most of the things though there
are still some open questions - please see below.

> ------------------------------------------------------------------------
> 
> * 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.

Done, changed args to meet current gdbus-codegen style.

> 
> * gvfsproxyvolumemonitor.c:
> 
> on_name_acquired/lost:
>  Should drop the g_warning spew
>  

Done, the rest are print_debug() calls enabled through the DEBUG_ENABLED
define, my plan is to disable it before a stable release.

> handle_is_supported:
>  Only call ensure_name_owner_changed_for_unique_name () if monitor !=
>  NULL.

Right, fixed.

> All the locks seem unnecessary since we're not using threads anymore for
> the skeleton.

Removed, volume monitor daemons should be on a safe side.

> -static void
> -append_mount (GMount *mount, DBusMessageIter *iter_array)
> +static GVariant *
> +append_mount (GMount *mount)
> 
> "append_*" seems like a strange name now :)

Got rid of that, renamed.
You know how hard is to come with a decent function name or a commit
message? :-)

> 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.

Fixed, good idea.

> * 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.

Found this little note: 
  "GVolumeMonitor is not thread-default-context aware, and so should not
   be used other than from the main thread, with no
   thread-default-context active."

Keeping that in mind, I've removed my silly workarounds.


> * 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.

Oh, I think I've been facing the excessive proxy creation before, on
Midori offline cache cleaning. Anyway, I've pushed the metadata proxy in
a GPrivate instance, so we have separate proxies for every thread. While
GDBusProxy should be thread-safe, its creation and check for NULL would
have to be surrounded by locks.

Now we only need to hope a series of operations would take place in the
same thread to benefit from the thread-local storage.

For the sync call in async ops thing, I don't see any use of the
affected methods in async calls, all seem to be called from GIO sync
ones. Since they're having "local_file_" prefix I don't expect them to
be called from somewhere else.

Async variants of the calls use g_simple_async_result_run_in_thread() to
make a sync operation async. So I think we're on safe side here.


> 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.

Got your point, waiting for answer is not necessary, we're not expecting
any value back. Changed to async calls, the only risk here is a missing
flush call. If we use g_dbus_connection_flush_sync() to ensure the
message got actually out, we might face accidental flush of tons of
messages from other places and threads as long as session bus connection
is shared in this case. Not sure which way is better.

-- 
Tomas Bzatek <tbzatek redhat com>