Re: [PATCH 1/2] vpn-provider: Implement connmand online state checking

[Daniel Wagner <wagi@monom.org>]

Hi Jussi,

Just a bunch of small nits.

On Tue, Aug 17, 2021 at 06:14:42PM +0300, Jussi Laakkonen wrote:
> +static gboolean do_connect_timeout_function(gpointer data)
> +{
> +	DBG("");
> +
> +	/*
> +	 * Keep in main loop if no agents are present yet or if connman is not
> +	 * yet online
> +	 */
> +	if (!connman_agent_get_info(NULL, NULL, NULL) || !connman_online) {
> +		return G_SOURCE_CONTINUE;

Doesn't this get the mainloop busy looping?

> +	} else {

As there is a return above, no need to do a else statement. Makes also
the indention one less :)

> +		struct vpn_provider_connect_data *cdata = data;
> +		struct vpn_provider *provider = cdata->provider;
> +		int err;
> +
> +		provider->do_connect_timeout = 0;
> +		err = __vpn_provider_connect(provider, cdata->msg);
> +
> +		if (err < 0 && err != -EINPROGRESS) {
> +			g_dbus_send_message(cdata->conn,
> +				__connman_error_failed(cdata->msg, -err));
> +			cdata->msg = NULL;
> +		}
> +
> +		return G_SOURCE_REMOVE;
> +	}
> +}

[...]

> +static void do_connect_later(struct vpn_provider *provider,
> +					DBusConnection *conn, DBusMessage *msg)
> +{
> +	struct vpn_provider_connect_data *cdata =
> +		g_try_new0(struct vpn_provider_connect_data, 1);

Do a g_new0() instead of a g_try_new0().

> +
> +	cdata->conn = dbus_connection_ref(conn);
> +	cdata->msg = dbus_message_ref(msg);
> +	cdata->provider = provider;
> +
> +	if (provider->do_connect_timeout)
> +		g_source_remove(provider->do_connect_timeout);
> +
> +	provider->do_connect_timeout =
> +		g_timeout_add_seconds_full(G_PRIORITY_DEFAULT, 1,
> +					do_connect_timeout_function, cdata,
> +					do_connect_timeout_free);
> +}
> +

[...]

> +static gboolean run_get_connman_state(gpointer user_data)
> +{
> +	const char *path = "/";
> +	const char *method = "GetProperties";
> +	gboolean rval = FALSE;
> +
> +	DBusMessage *msg = NULL;
> +	DBusPendingCall *call = NULL;
> +
> +	DBG("");
> +
> +	msg = dbus_message_new_method_call(CONNMAN_SERVICE, path,
> +		CONNMAN_MANAGER_INTERFACE, method);
> +
> +	if (!msg)
> +		goto out;
> +
> +	if (!(rval = g_dbus_send_message_with_reply(connection, msg,
> +		&call, -1))) {

Move the 'rval = ...' outside the if condition.

> +		connman_error("Cannot call %s on %s",
> +			method, CONNMAN_MANAGER_INTERFACE);
> +		goto out;
> +	}
> +
> +	if (!call) {
> +		connman_error("set pending call failed");
> +		goto error;
> +	}
> +
> +	if (!dbus_pending_call_set_notify(call, get_connman_state_reply,
> +		NULL, NULL)) {
> +		connman_error("set notify to pending call failed");
> +		goto error;
> +	}
> +
> +out:
> +	if (msg)
> +		dbus_message_unref(msg);
> +
> +	/* In case sending was success, unset timeout function id */
> +	if (rval) {
> +		DBG("unsetting get_connman_state_timeout id");
> +		get_connman_state_timeout = 0;
> +	}
> +
> +	/* Return FALSE in case of success to remove from main event loop */
> +	return !rval;
> +
> +error:
> +	if (call)
> +		dbus_pending_call_unref(call);
> +
> +	rval = G_SOURCE_REMOVE;

This looks wrong. Shouldn't this be either TRUE or FALSE?

> +	goto out;

Please try to avoid jumping up again. Forward goto's are easy to read
but the backwards one tend to be hard to grasp (at least for me).

Daniel