> all: use gresources
    
    It makes removes some overhead and makes debugging easier without the
       ^^^^^^


 plugin_sources = \
+    properties/resources.c \
+    properties/resources.h \
     properties/nm-openvpn-editor-plugin.c \
     properties/nm-openvpn-editor-plugin.h \

I think the resources belong to "editor_sources"



> build: bump requirement to NetworkManager 1.8

+    PKG_CHECK_MODULES(LIBNMA, libnma >= 1.8.0)

>= 1.7.0, not 1.8.0.
Arguably, at this point it doesn't matter anymore as 1.7.0 is history. But you could compile 1.8.0 against 1.7.0 development branch -- and that may or may not work, but that is correct, 1.7.0 is devel, it might have the API you need or it might not. If you do that, it's up to you but the configure should shouldn't forbid that.

E.g. building the VPN plugin against 1.7.92 is perfectly fine and should be allowed.







+#ifdef NM_VPN_OLD
+#define nm_utils_is_valid_iface_name(n,e) nm_utils_iface_valid_name(n)
+#endif

the @e argument must not be ignored.

+#ifdef NM_VPN_OLD
+#define _nm_utils_is_valid_iface_name(n) nm_utils_iface_valid_name(n)
+#else
+#define _nm_utils_is_valid_iface_name(n) nm_utils_is_valid_iface_name(n, NULL)
+#endif


I think it was a mistake to mark nm_utils_iface_valid_name() as deprecated. There is really nothing wrong with using it. And it's not that we would ever drop that symbol. It should be un-deprecated -- of course, at this point, that doesn't help either.


Also, such compatibility wrappers should be in shared/nm-default.h



> properties: switch to using the NMACertChooser



 #ifdef NM_VPN_OLD
 #define nm_utils_is_valid_iface_name(n,e) nm_utils_iface_valid_name(n)
+#include <nm-cert-chooser.h>
+#else
+#include <nma-cert-chooser.h>
 #endif

such includes, should be done by "shared/nm-default.h"

+
+    g_free (this_cert);
+    g_free (other_cert);
+    g_free (this_key);
+    g_free (other_key);

gs_free?




+    this_cert = nma_cert_chooser_get_cert (this, &scheme);
+    other_cert = nma_cert_chooser_get_cert (other, &scheme);
+    this_key = nma_cert_chooser_get_key (this, &scheme);
+    other_key = nma_cert_chooser_get_key (other, &scheme);

you ignore @scheme, but for nma_cert_chooser_get_key() it a mandatory argument. Either, the argument should not be mandatory (allow-none), or more likely: using the string without considering the scheme is a bug, and you must use it.
E.g.

-    if (is_pkcs12 (this_cert)) {
+    if (   scheme == NM_SETTING_802_1X_CK_SCHEME_PATH
+        && is_pkcs12 (this_cert)) {

(In reply to Thomas Haller from comment #1)
> > all: use gresources
>     
>     It makes removes some overhead and makes debugging easier without the
>        ^^^^^^
> 
> 
>  plugin_sources = \
> +    properties/resources.c \
> +    properties/resources.h \
>      properties/nm-openvpn-editor-plugin.c \
>      properties/nm-openvpn-editor-plugin.h \
> 
> I think the resources belong to "editor_sources"

Yes. Fixed.


> > build: bump requirement to NetworkManager 1.8
> 
> +    PKG_CHECK_MODULES(LIBNMA, libnma >= 1.8.0)
> 
> >= 1.7.0, not 1.8.0.
> Arguably, at this point it doesn't matter anymore as 1.7.0 is history. But
> you could compile 1.8.0 against 1.7.0 development branch -- and that may or
> may not work, but that is correct, 1.7.0 is devel, it might have the API you
> need or it might not. If you do that, it's up to you but the configure
> should shouldn't forbid that.
> 
> E.g. building the VPN plugin against 1.7.92 is perfectly fine and should be
> allowed.

Well,  there's really not much point in this...

> +#ifdef NM_VPN_OLD
> +#define nm_utils_is_valid_iface_name(n,e) nm_utils_iface_valid_name(n)
> +#endif
> 
> the @e argument must not be ignored.

Well, it's called in precisely one single place and e is NULL there, so I'm rather confident it's quite okay to ignore it.

> 
> +#ifdef NM_VPN_OLD
> +#define _nm_utils_is_valid_iface_name(n) nm_utils_iface_valid_name(n)
> +#else
> +#define _nm_utils_is_valid_iface_name(n) nm_utils_is_valid_iface_name(n,
> NULL)
> +#endif

> Also, such compatibility wrappers should be in shared/nm-default.h

Why? That doesn't sound like a very useful thing. It is not -- and should not be -- a general purpose compatibility layer or anything like that. It merely adapts one precise use of a function for older code base with the intent of not harming readability for the newer one.

Decorating it with an extra underscore harms readability and so does moving it away from the single location where it's used.

> > properties: switch to using the NMACertChooser
> 
> 
> 
>  #ifdef NM_VPN_OLD
>  #define nm_utils_is_valid_iface_name(n,e) nm_utils_iface_valid_name(n)
> +#include <nm-cert-chooser.h>
> +#else
> +#include <nma-cert-chooser.h>
>  #endif
> 
> such includes, should be done by "shared/nm-default.h"

Why? Including a GUI-specific header that's used in precisely one source file from all compilation units doesn't sound like a very good idea to me.

> +    g_free (this_cert);
> +    g_free (other_cert);
> +    g_free (this_key);
> +    g_free (other_key);
> 
> gs_free?

Why? This seems to work and look all right to me.

> +    this_cert = nma_cert_chooser_get_cert (this, &scheme);
> +    other_cert = nma_cert_chooser_get_cert (other, &scheme);
> +    this_key = nma_cert_chooser_get_key (this, &scheme);
> +    other_key = nma_cert_chooser_get_key (other, &scheme);
> 
> you ignore @scheme, but for nma_cert_chooser_get_key() it a mandatory
> argument. Either, the argument should not be mandatory (allow-none), or more
> likely: using the string without considering the scheme is a bug, and you
> must use it.
> E.g.
> 
> -    if (is_pkcs12 (this_cert)) {
> +    if (   scheme == NM_SETTING_802_1X_CK_SCHEME_PATH
> +        && is_pkcs12 (this_cert)) {

Fixed.

Pushed an updated branch.

Merged in 59e86e72a44b430efddcc16fc8f56024f6e91e0e