Re: [PATCH v3 3/7] src:(pacrunner-manager): Object for interaction with PacRunner

[Beniamino Galvani <bgalvani redhat com>]

On Tue, Aug 02, 2016 at 02:01:07AM +0530, Atul Anand wrote:
> A new object NMPacRunnerManager has been added to manage and interact
> PacRunner. It invokes both DBus methods on PacRunner DBus interface.
> It stores the returned object path from CreateProxyConfiguration()
> to feed as parameter to DestroyProxyCofiguration() when network goes down.

[...]

> +static void
> +pacrunner_proxy_cb (GObject *source, GAsyncResult *res, gpointer user_data)
> +{
> +     NMPacRunnerManager *self = NULL;
> +     NMPacRunnerManagerPrivate *priv;
> +     gs_free_error GError *error = NULL;
> +     GDBusProxy *proxy;
> +
> +     self = NM_PACRUNNER_MANAGER (user_data);
> +     priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
> +
> +     proxy = g_dbus_proxy_new_for_bus_finish (res, &error);
> +     if (!proxy && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
> +             /* Mark PacRunner unavailable on DBus */
> +             priv->started = FALSE;
> +             _LOGD ("failed to connect to pacrunner via DBus: %s", error->message);
> +             return;
> +     }

We should handle all errors, not only CANCELLED. For example:

    if (!proxy) {
        if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
            _LOGW ("failed to connect to pacrunner via D-Bus: %s", error->message);
        g_clear_error (&error);
        return;
    }

> +static void
> +start_pacrunner (NMPacRunnerManager *self)
> +{
> +     NMPacRunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
> +
> +     if (priv->started)
> +             return;
> +
> +     priv->pacrunner_cancellable = g_cancellable_new ();
> +
> +     g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM,
> +                               G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START,

At least on Fedora, pacrunner seems to be D-Bus activated and so the
method invocation fails if the service is not running. Maybe we should
remove the G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START flag, so that it's
started when needed?

> +                               NULL,
> +                               PACRUNNER_DBUS_SERVICE,
> +                               PACRUNNER_DBUS_PATH,
> +                               PACRUNNER_DBUS_INTERFACE,
> +                               priv->pacrunner_cancellable,
> +                              (GAsyncReadyCallback) pacrunner_proxy_cb,
> +                               self);
> +     priv->started = TRUE;
> +}

> +static void
> +pacrunner_remove_done (GObject *source, GAsyncResult *res, gpointer user_data)
> +{
> +     NMPacRunnerManager *self = NM_PACRUNNER_MANAGER (user_data);
> +     NMPacRunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
> +     gs_free_error GError *error = NULL;
> +     gs_unref_variant GVariant *ret = NULL;
> +
> +     ret = g_dbus_proxy_call_finish (priv->pacrunner, res, &error);
> +     if (!ret || g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))

I think the second condition after || can never be true because @error
is always NULL when @ret is non-NULL.


> +             _LOGD ("Couldn't remove proxy config from pacrunner");
> +     else
> +             _LOGD ("Sucessfully removed proxy config from pacrunner");
> +}
> +
> +/**
> + * nm_pacrunner_manager_remove():
> + * @self: the #NMPacRunnerManager
> + * @iface: the iface for the connection to be removed
> + * from PacRunner
> + */
> +void
> +nm_pacrunner_manager_remove (NMPacRunnerManager *self, const char *iface)
> +{
> +     NMPacRunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
> +     GList *list;
> +
> +     for (list = g_list_first(priv->remove); list; list = g_list_next(list)) {
> +             struct remove_data *data = list->data;
> +             if (g_strcmp0 (data->iface, iface) == 0) {
> +                     if ((priv->pacrunner) && (data->path))

You can remove the inner parenthesis.

Beniamino

Attachment: signature.asc
Description: PGP signature