connman.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Daniel Wagner <wagi@monom.org>
To: Jussi Laakkonen <jussi.laakkonen@jolla.com>
Cc: connman@lists.linux.dev
Subject: Re: [PATCH 1/2] vpn-provider: Implement connmand online state checking
Date: Tue, 17 Aug 2021 18:55:28 +0200	[thread overview]
Message-ID: <20210817165528.amfp4fyf22ljaehz@carbon.lan> (raw)
In-Reply-To: <20210817151443.32305-2-jussi.laakkonen@jolla.com>

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

  reply	other threads:[~2021-08-17 16:55 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-17 15:14 [PATCH 0/2] Implement connmand state monitoring to vpnd Jussi Laakkonen
2021-08-17 15:14 ` [PATCH 1/2] vpn-provider: Implement connmand online state checking Jussi Laakkonen
2021-08-17 16:55   ` Daniel Wagner [this message]
2021-08-18  9:21     ` Jussi Laakkonen
2021-08-18 10:31   ` [PATCH v2 " Jussi Laakkonen
2021-08-19 12:11     ` Jussi Laakkonen
2021-08-19 12:30       ` Daniel Wagner
2021-08-19 12:38         ` Santtu Lakkala
2021-08-19 13:06           ` Daniel Wagner
2021-08-19 12:40         ` Jussi Laakkonen
2021-08-29 18:27           ` Daniel Wagner
2021-08-30  6:59             ` Jussi Laakkonen
2021-08-30  8:11               ` Daniel Wagner
2021-08-20 13:05   ` [PATCH v3 " Jussi Laakkonen
2021-08-17 15:14 ` [PATCH 2/2] vpn: Handle ENOLINK error in connect callback Jussi Laakkonen
2021-08-20 13:06   ` [PATCH v2 2/2] vpn: Refactor connect_reply() and handle NoCarrier -> ENOLINK error Jussi Laakkonen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210817165528.amfp4fyf22ljaehz@carbon.lan \
    --to=wagi@monom.org \
    --cc=connman@lists.linux.dev \
    --cc=jussi.laakkonen@jolla.com \
    --subject='Re: [PATCH 1/2] vpn-provider: Implement connmand online state checking' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).