>+#define VPN_PLUGIN_ERROR (vpn_plugin_error_quark ())
>+...

You don't need this; nm-errors in libnm-core already defines and registers all the errors used in all public NM APIs.

>+static struct in6_addr
>+ip6_addr_from_variant (GVariant *v, struct in6_addr **new_addr)
...
>+			return * (struct in6_addr *) bytes;

bytes is not necessarily correctly aligned for a struct in6_addr, which might matter on non-x86?

The whole "sometimes the caller wants a const pointer and sometimes they want allocated memory so, hey, let's provide both in the same API" thing is weird anyway. How about just taking a "struct in6_addr *out_addr" and memcpy'ing into it, and do the allocation separately in the two places you need that? (See also below re ip6_external_gw.)

>-	if (val && G_VALUE_HOLDS_BOOLEAN (val) && g_value_get_boolean (val)) {
>+	if (g_variant_lookup (dict, NM_VPN_PLUGIN_CAN_PERSIST, "b", &b)) {

&& b

>+		ip6_addr_from_variant (v, &priv->ip6_external_gw);
>+		if (!IN6_IS_ADDR_UNSPECIFIED (priv->ip6_external_gw))
>+			success = TRUE;

This will crash if v is the wrong length (in which case priv->ip6_external_gw won't have been set).

>+} VpnIp4Route;

appears to be unused

>+	if (g_variant_lookup (dict, NM_VPN_PLUGIN_IP4_CONFIG_DNS, "@au", &v)) {
>+		u32_array = g_variant_get_fixed_array (v, &len, sizeof (guint32));
>+		for (i = 0; i < len; i++)
>+			nm_ip4_config_add_nameserver (config, u32_array[i]);
>+		g_variant_unref (v);
> 	}

Note that it would be simpler to use the non-"@" form here and elsewhere:

    if (g_variant_lookup (dict, NM_VPN_PLUGIN_IP4_CONFIG_DNS, "au", &iter)) {
        while (g_variant_iter_next (&iter, "u", &u32))
            nm_ip4_config_add_nameserver (config, u32);
    }

>+		nm_log_warn (LOGD_VPN, "VPN connection '%s' failed to connect: '%s'.",
>+			         nm_connection_get_id (NM_VPN_CONNECTION_GET_PRIVATE (self)->connection),

indentation

pushed two fixups

(In reply to Thomas Haller from comment #2)
> pushed two fixups

For the GCancellable one, my thought was that if we're cancelling one operation, we're going to cancel them all.  We only cancel anything if the VPN is getting disconnected or getting disposed, so at that point we don't care about any of the ongoing async operations.

The users of a cancellable are:

Connect()
ConnectInteractive()
proxy creation
NewSecrets()
NeedSecrets()

None of these operations overlap, and if any of them are cancelled they all should be.  So I don't think we need to track individual cancellables for every call...

(In reply to Dan Williams from comment #3)
> None of these operations overlap, and if any of them are cancelled they all
> should be.  So I don't think we need to track individual cancellables for
> every call...

how can you be sure that there are no overlapping invocations?

> priv->proxy receives "g-signal" with "SecretsRequired"
 > signal_cb()
  > plugin_interactive_secrets_required()
   > get_secrets() (set priv->secrets_id)
    > nm_settings_connection_get_secrets()
     |
     | (async)
     |
     > get_secrets_cb() (clear priv->secrets_id)
      > NewSecrets/NeedSecrets

Note that nm_settings_connection_cancel_secrets() does not cancel the pending NewSecrets/NeedSecrets.


It seems complicated to prove that there are no overlapping invocations. And what's the point? If you change anything, you have to do all the reasoning again.
OTOH, you can also add an assertion that might catch a race:

 static AsyncData *
 _create_async_user_data (NMVpnConnection *self)
 {
     NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE (self);
     AsyncData *async_data;
+    gboolean had_pending = !!priv->pending_async_data;
 
     async_data = g_slice_new (AsyncData);
     async_data->self = self;
     async_data->cancellable = g_cancellable_new ();
 
     priv->pending_async_data = g_slist_prepend (priv->pending_async_data, async_data);
+    g_return_val_if_fail (!had_pending, async_data);
     return async_data;
 }

I see what you mean with the reference to https://developer.gnome.org/gio/stable/GCancellable.html#g-cancellable-reset though, but the code only cancels the cancellable in dispose(), after which it will never be used.  So I don't think there's any possibility that the same cancellable will get cancelled twice, which is what your patch tries to deal with.

AFAIUI the issue with cancellables is that they should not be re-used after being cancelled, and I don't believe the code in nm-vpn-connection.c ever does that.  They are certainly intended to be passed to multiple operations, all of which can be cancelled at the same time with a single cancellable, which is what the code *does* try to do.

BTW, repushed with fixups for danw's comments...

(In reply to Dan Williams from comment #6)
> AFAIUI the issue with cancellables is that they should not be re-used after
> being cancelled, and I don't believe the code in nm-vpn-connection.c ever
> does that.  They are certainly intended to be passed to multiple operations,
> all of which can be cancelled at the same time with a single cancellable,
> which is what the code *does* try to do.

I was wrong, you are correct:

https://developer.gnome.org/gio/stable/GCancellable.html#g-cancellable-new

One GCancellable can be used in multiple consecutive operations or in multiple concurrent operations.


good then.

looks good, but remember to remove the GCancellable fixup before squashing

Dropped that commit.  Resulting in:

231b039 vpn: convert NMVpnConnection <-> VPN service communication to GDBus (bgo #745307)