After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 749877 - [review] add vpn plugin support to libnm [th/libnm-vpn-plugin-bgo749877]
[review] add vpn plugin support to libnm [th/libnm-vpn-plugin-bgo749877]
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks: 749365 749686 749951
 
 
Reported: 2015-05-26 09:17 UTC by Thomas Haller
Modified: 2015-08-01 10:17 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Thomas Haller 2015-05-26 09:17:03 UTC
As mentioned in https://bugzilla.gnome.org/show_bug.cgi?id=749365#c4, I added support to libnm to load VPN plugins.

The point is to have one place (libnm) that does the loading of plugins. The knowledge about the name files is encapsulated there. And the loading of the NMVpnEditorPlugin instance.
Comment 1 Beniamino Galvani 2015-05-28 13:59:22 UTC
> libnm: add NMVpnPluginInfo class

+typedef struct {
+       char *filename;
+       NMVpnPluginInfo *plugin_info;
+       struct stat stat;
+} LoadDirInfo;

It seems that the stat member is used but never set.

+       /* we reqire at least a "name" */
require

libnm-core/nm-vpn-plugin-info.c should be added to po/POTFILES.in.
Comment 2 Thomas Haller 2015-05-28 14:28:57 UTC
(In reply to Beniamino Galvani from comment #1)
> > libnm: add NMVpnPluginInfo class
> 
> +typedef struct {
> +       char *filename;
> +       NMVpnPluginInfo *plugin_info;
> +       struct stat stat;
> +} LoadDirInfo;
> 
> It seems that the stat member is used but never set.
> 
> +       /* we reqire at least a "name" */
> require
> 
> libnm-core/nm-vpn-plugin-info.c should be added to po/POTFILES.in.

both fixed with a fixup commit.


Rebased on master and repushed.



I know, code doc and NM_AVAILABLE_IN_1_2 is missing... I add that after we agree on the API.
Comment 3 Dan Williams 2015-05-28 15:15:33 UTC
> libnm: add NMUtilsStrStrDictKey utility

Might be nice to have some #defines for the type flags, like:

#define V1_NULL 0x01
#define V2_NULL 0x02
#define BOTH_NULL 0x03


> libnm: add NMVPNPluginInfo class

nm_vpn_plugin_info_list_add() checks for a duplicate name and then continues, so if you (for whatever reason) add two NMVpnPluginInfo objects to the list outside of libnm, the function will only detect and replace the last one.  Maybe we don't care since the user would have to manually add the plugin to the list though.

I'd get rid of the 'check owner' argument to nm_vpn_plugin_info_check_file() too, since that's only used in one place (nm-vpn-service.c) and it's always 0 (root).  The VPN .name files should never be owned by anything else; if we need this for testcases then lets do that through private API and not public API.

Also, g_path_is_absolute() could be used instead of filename[0] != '/'.

Also, when would the do_validate_filename and check_absolute arguments *not* be set to TRUE?  Would the applet use this function at all and set the arguments to FALSE?  If the applet wouldn't use it or wouldn't use FALSE, I say we should just hardcode the values.

Also, is nm_vpn_plugin_info_check_file() supposed to be private, since it's not in libnm.ver?  If so, why does the prototype exist in the public headers?

I might even just get rid of nm_vpn_plugin_info_validate_filename() since it's only used in vpn_dir_changed().  If the file name is invalid, then nm_vpn_plugin_info_check_file() will fail anyway, and if it is valid, then we want to use the new NMVpnPluginInfo object that gets returned from that.  I don't see a great reason to have this as a separate function.

Why does nm_vpn_plugin_info_load_dir() care about the owner?  This is .name files we're reading here, and they should never be owned by anybody but root since they control how NM loads things.

Also, 'filename' is part of the NMVpnPluginInfo already, so there's no need to cache it in the LoadDirInfo struct.  You can just do this and skip all the alloc/dealloc stuff for the filename:

    gs_free char *filename = g_build_filename (dirname, fn, NULL),
    LoadDirInfo info = { 0 };

    if (nm_vpn_plugin_info_check_file (filename,

and then also use "nm_vpn_plugin_info_get_filename (a->plugin_info)" in _sort_files() just above.

You could also use gs_unref_array here too.

Shouldn't the nm_vpn_plugin_info_list_find_*() functions g_return_val_if_fail() if they are given NULL as an argument?  That's programmer error so we should warn there.

init_sync() should also require a 'service' property, not just name.  A VPN without a 'service' is pretty useless as it doesn't tell us anything about the VPN at all.  We should also require a 'program' line too, since NM can't do anything without that, and if NM can't do anything with it, it's pretty useless to clients too.

> libnm: add load method to NMVpnPluginInfo

In nm_vpn_plugin_info_load_editor_plugin(), why not use nm_vpn_plugin_info_get_service() instead of nm_vpn_plugin_info_lookup_property()?

Also, again about the check_owner...  why would an editor plugin ever be owned by something other than root?  If its for testcases, lets do some private functions for that.

Also, what would force_retry be used for?

Next, maybe STRLEN(LIBDIR)?  It's a #defined string so we might as well save some cycles.

Maybe rename "error_1" to "local" to stay consistent with other places in the code.

Also I think we can use g_strcmp0() for comparing the editor plugin name to the NMVpnPluginInfo and skip the !plug_name.  Also, why would the NMVpnPluginInfo's 'service' property ever be NULL?  The "if (service && g_strcmp0(plug_service, service)" is pretty odd.  The NMVpnPluginInfo is invalid if it doesn't have a 'service' property so it should never get created.  NM won't ever load anything without a 'service' property anyway (since it would be useless), nor should the clients.
Comment 4 Thomas Haller 2015-05-28 16:52:07 UTC
(In reply to Dan Williams from comment #3)
> > libnm: add NMUtilsStrStrDictKey utility
> 
> Might be nice to have some #defines for the type flags, like:
> 
> #define V1_NULL 0x01
> #define V2_NULL 0x02
> #define BOTH_NULL 0x03

ack.


> > libnm: add NMVPNPluginInfo class
> 
> nm_vpn_plugin_info_list_add() checks for a duplicate name and then
> continues, so if you (for whatever reason) add two NMVpnPluginInfo objects
> to the list outside of libnm, the function will only detect and replace the
> last one.  Maybe we don't care since the user would have to manually add the
> plugin to the list though.

I was thinking about this place too... maybe it would be simpler to just error out in case of duplicate names. Just like it does for other places. A user who really wants can workaround that. Replacing the instance unexpectedly seems error prone.


> I'd get rid of the 'check owner' argument to nm_vpn_plugin_info_check_file()
> too, since that's only used in one place (nm-vpn-service.c) and it's always
> 0 (root).  The VPN .name files should never be owned by anything else; if we
> need this for testcases then lets do that through private API and not public
> API.

I think check_owner is important. Only NM-core wants to check against 0 (root). Any other user application probably should allow 0 and getuid() -- so that it can load plugins installed with user-permissions (./configure --prefix=~/opt/).


> Also, g_path_is_absolute() could be used instead of filename[0] != '/'.

g_path_is_absolute() is really the same as "filename[0] != '/'. Anyway, I changed it.




> Also, when would the do_validate_filename and check_absolute arguments *not*
> be set to TRUE? Would the applet use this function at all and set the
> arguments to FALSE?  If the applet wouldn't use it or wouldn't use FALSE, I
> say we should just hardcode the values.

probably nm-applet should set both to TRUE, exactly like vpn_get_plugins() in clients/tui/vpn-helpers.c.

I don't know when somebody would want to set it to FALSE. The aim is to make the public API useful and flexible. And this conflicts with ~having only one way~ to do it.

> Also, is nm_vpn_plugin_info_check_file() supposed to be private, since it's
> not in libnm.ver?  If so, why does the prototype exist in the public headers?

The public API is to be discussed here and not yet decided. But I was thinking of having nm_vpn_plugin_info_check_file() public.

The idea is that nm_vpn_plugin_info_new_from_file() does not perform any checks on fils/permissions. This gives a user the flexibility to load ~any~ file.

In the usual case (99%), the user just calls nm_vpn_plugin_info_load_dir(), specifying which permission checks to perform.

By having nm_vpn_plugin_info_new_from_file() public API, a user could easily re-implement loading from separate files, eg. from a
  "NM_VPN_PLUGINS=/path/to/plug1:/path/to/plug2"
environment variable, without having to reimplement the permission checks.




> I might even just get rid of nm_vpn_plugin_info_validate_filename() since
> it's only used in vpn_dir_changed().  If the file name is invalid, then
> nm_vpn_plugin_info_check_file() will fail anyway, and if it is valid, then
> we want to use the new NMVpnPluginInfo object that gets returned from that. 
> I don't see a great reason to have this as a separate function.

The idea of having it public API gives a user the ability to reimplement parts of this -- but still using the same filename-validation as other NM files.


> Why does nm_vpn_plugin_info_load_dir() care about the owner?  This is .name
> files we're reading here, and they should never be owned by anybody but root
> since they control how NM loads things.

You could install the plugins as user (./configure --prefix). Checking against root would break that.


> Also, 'filename' is part of the NMVpnPluginInfo already, so there's no need
> to cache it in the LoadDirInfo struct.  You can just do this and skip all
> the alloc/dealloc stuff for the filename:
> 
>     gs_free char *filename = g_build_filename (dirname, fn, NULL),
>     LoadDirInfo info = { 0 };
> 
>     if (nm_vpn_plugin_info_check_file (filename,
> 
> and then also use "nm_vpn_plugin_info_get_filename (a->plugin_info)" in
> _sort_files() just above.

changed.


> You could also use gs_unref_array here too.

changed.


> Shouldn't the nm_vpn_plugin_info_list_find_*() functions
> g_return_val_if_fail() if they are given NULL as an argument?  That's
> programmer error so we should warn there.

changed.



> init_sync() should also require a 'service' property, not just name.  A VPN
> without a 'service' is pretty useless as it doesn't tell us anything about
> the VPN at all.  We should also require a 'program' line too, since NM can't
> do anything without that, and if NM can't do anything with it, it's pretty
> useless to clients too.

maybe in the future we could have a config file that only specifies a "plugin" that is useful to libnm (but not to NM-core). In fact, "service" and "program" is unused by anybody except NM-core.


> > libnm: add load method to NMVpnPluginInfo
> 
> In nm_vpn_plugin_info_load_editor_plugin(), why not use
> nm_vpn_plugin_info_get_service() instead of
> nm_vpn_plugin_info_lookup_property()?

fixed.


> Also, again about the check_owner...  why would an editor plugin ever be
> owned by something other than root?  If its for testcases, lets do some
> private functions for that.

dito.


> Also, what would force_retry be used for?

I think if a load fails once, any later attempts to reload the plugin should fail straight ways -- unless the user really wants to try a second time (by setting force_retry).


> Next, maybe STRLEN(LIBDIR)?  It's a #defined string so we might as well save
> some cycles.

Changed.

The compiler can anyway optimize strlen() for a constant. The use of STRLEN() is that is has compile time checks against using it with a non-const-string, and that it can be used where the compiler needs a constant expression, like:
   char buf[STRLEN("prefix=") + 50];


> Maybe rename "error_1" to "local" to stay consistent with other places in
> the code.
> 
> Also I think we can use g_strcmp0() for comparing the editor plugin name to
> the NMVpnPluginInfo and skip the !plug_name.

changed

> Also, why would the
> NMVpnPluginInfo's 'service' property ever be NULL?  The "if (service &&
> g_strcmp0(plug_service, service)" is pretty odd.  The NMVpnPluginInfo is
> invalid if it doesn't have a 'service' property so it should never get
> created. 

As said before, I could image usecases.

> NM won't ever load anything without a 'service' property anyway
> (since it would be useless), nor should the clients.

"NM won't ever load", but clients might want to load such plugins.



I'm not saying the current version has perfect API. Maybe really simplify it? 
How exactly? Maybe I squash those fixup commits first :)
Comment 5 Thomas Haller 2015-05-28 21:12:24 UTC
I did some major refactoring ... does it look better now?

(It was too complicated to do it with fixup! commits... it's all squashed together. The previous version is still there at th/libnm-vpn-plugin-bgo749877-1)
Comment 6 Jiri Klimes 2015-05-29 14:10:57 UTC
> libnm: add NMVpnPluginInfo class
#define NM_VPN_PLUGIN_INFO_KF_GROUP_GNOME        "GNOME"
should be changed to
#define NM_VPN_PLUGIN_INFO_KF_GROUP_LIBNM        "libnm"
"libnm" is what is used in src/connection-editor/vpn-helpers.c and it is more logical than "GNOME".

All the API functions should declare "Since: 1.2" and be moved to the 1.2 secrion in libnm.ver.

> libnm: move NMVpnEditorPlugin to libnm-core/
in commit message: "pringing" . You mean "bringing"?

> libnm-core/nm-vpn-editor-plugin.h
Any reason for using __NM_VPN_PLUGIN_H__ instead of __NM_VPN_EDITOR_PLUGIN_H__?
Comment 7 Thomas Haller 2015-05-29 14:39:34 UTC
(In reply to Jiri Klimes from comment #6)
> > libnm: add NMVpnPluginInfo class
> #define NM_VPN_PLUGIN_INFO_KF_GROUP_GNOME        "GNOME"
> should be changed to
> #define NM_VPN_PLUGIN_INFO_KF_GROUP_LIBNM        "libnm"
> "libnm" is what is used in src/connection-editor/vpn-helpers.c and it is
> more logical than "GNOME".

these 2 defines are names for the keyfile (top-level) sections which are
  [VPN Connection]
  name=...
  [GNOME]
  properties=...
Hence the name "KF_GROUP"...




The path to the libnm plugin I intended as:
1)
  [VPN Connection]
  plugin=

contrary to src/connection-editor/vpn-helpers.c -- in which danw added:
2)
  [libnm]
  properties=...


But thinking about it, I prefer now:
3)
  [libnm]
  plugin=...



  
Opinions? 1, 2, or 3?



> All the API functions should declare "Since: 1.2"

Before adding documentation/Since, I wait for a few positive reviews. Still TODO.

> and be moved to the 1.2
> secrion in libnm.ver.

It is in the right section already.



> > libnm: move NMVpnEditorPlugin to libnm-core/
> in commit message: "pringing" . You mean "bringing"?

fixed.

> > libnm-core/nm-vpn-editor-plugin.h
> Any reason for using __NM_VPN_PLUGIN_H__ instead of
> __NM_VPN_EDITOR_PLUGIN_H__?

I renamed the file later... forgot to adjust. Fixed now.


Repushed.
Comment 8 Thomas Haller 2015-06-01 18:00:54 UTC
Repushed with gtk-doc and Since: annotations.
Comment 9 Thomas Haller 2015-06-02 09:47:03 UTC
Repushed with new patches.

now contains the patch:
  "libnm: NMVpnPluginOld fixes", originally from bug 749686, branch jk/libnm-nm-vpn-plugin-old-fixes.


also contains 4 patches from bug 749951 that copy NMVpnPluginOld to a new NMVpnServicePlugin class.
Comment 10 Beniamino Galvani 2015-06-04 07:48:15 UTC
> libnm/vpn: add new NMVpnServicePlugin class

libnm/nm-vpn-service-plugin.c must be added to POTFILES.in.

> libnm: add nm_vpn_editor_plugin_load_from_file() function

+       if (g_module_symbol (module, "nm_vpn_editor_plugin_factory", (gpointer) &factory)) {
+               gs_free_error GError *factory_error = NULL;
+               gboolean success = FALSE;
+
+

Extra blank line.
Comment 11 Thomas Haller 2015-06-04 12:19:30 UTC
(In reply to Beniamino Galvani from comment #10)
> > libnm/vpn: add new NMVpnServicePlugin class
> 
> libnm/nm-vpn-service-plugin.c must be added to POTFILES.in.
> 
> 
> Extra blank line.

Thanks. Both fixed and repushed.
Comment 12 Thomas Haller 2015-06-08 16:45:55 UTC
Added fixup commit with support for /usr/lib/NetworkManager/VPN directory.
Comment 13 Dan Williams 2015-06-11 22:08:42 UTC
(In reply to Thomas Haller from comment #4)
> (In reply to Dan Williams from comment #3)
> > > libnm: add NMVPNPluginInfo class
> > 
> > nm_vpn_plugin_info_list_add() checks for a duplicate name and then
> > continues, so if you (for whatever reason) add two NMVpnPluginInfo objects
> > to the list outside of libnm, the function will only detect and replace the
> > last one.  Maybe we don't care since the user would have to manually add the
> > plugin to the list though.
> 
> I was thinking about this place too... maybe it would be simpler to just
> error out in case of duplicate names. Just like it does for other places. A
> user who really wants can workaround that. Replacing the instance
> unexpectedly seems error prone.

Yeah, that sounds fine.

> > I'd get rid of the 'check owner' argument to nm_vpn_plugin_info_check_file()
> > too, since that's only used in one place (nm-vpn-service.c) and it's always
> > 0 (root).  The VPN .name files should never be owned by anything else; if we
> > need this for testcases then lets do that through private API and not public
> > API.
> 
> I think check_owner is important. Only NM-core wants to check against 0
> (root). Any other user application probably should allow 0 and getuid() --
> so that it can load plugins installed with user-permissions (./configure
> --prefix=~/opt/).

But why would there be any user-permissions plugins?  NetworkManager will never load those because they aren't trusted.  This API is to be used only by NetworkManager to load VPN plugins that it will then run internally, which means we must ensure they are root-only, and second by clients that want to find out what VPN plugins NM knows about (which means root-only again) and load the UI plugins for them.  I just don't see a use case for non-root permissions plugins...  If we can't think of a good one then I don't think we should add API without a clear relevant use-case.

> I don't know when somebody would want to set it to FALSE. The aim is to make
> the public API useful and flexible. And this conflicts with ~having only one
> way~ to do it.
> 
> > Also, is nm_vpn_plugin_info_check_file() supposed to be private, since it's
> > not in libnm.ver?  If so, why does the prototype exist in the public headers?
> 
> The public API is to be discussed here and not yet decided. But I was
> thinking of having nm_vpn_plugin_info_check_file() public.
> 
> The idea is that nm_vpn_plugin_info_new_from_file() does not perform any
> checks on fils/permissions. This gives a user the flexibility to load ~any~
> file.

But again, these are VPN plugin service files.  Why would we ever load VPN service files that aren't installed in /etc/NetworkManager/VPN/?  If a client loads them from somewhere else, then NM doesn't know about them, and NM can't start that VPN connection.

The only use-case I can think of is if some administrator wants to create VPN connections without having the specific plugin active on that specific system, but that's a very tenuous use-case that I don't think supports having this all be public API forever...

> In the usual case (99%), the user just calls nm_vpn_plugin_info_load_dir(),
> specifying which permission checks to perform.
> 
> By having nm_vpn_plugin_info_new_from_file() public API, a user could easily
> re-implement loading from separate files, eg. from a
>   "NM_VPN_PLUGINS=/path/to/plug1:/path/to/plug2"
> environment variable, without having to reimplement the permission checks.

Right, but why would any client of this API do that?

> > I might even just get rid of nm_vpn_plugin_info_validate_filename() since
> > it's only used in vpn_dir_changed().  If the file name is invalid, then
> > nm_vpn_plugin_info_check_file() will fail anyway, and if it is valid, then
> > we want to use the new NMVpnPluginInfo object that gets returned from that. 
> > I don't see a great reason to have this as a separate function.
> 
> The idea of having it public API gives a user the ability to reimplement
> parts of this -- but still using the same filename-validation as other NM
> files.
> 
> 
> > Why does nm_vpn_plugin_info_load_dir() care about the owner?  This is .name
> > files we're reading here, and they should never be owned by anybody but root
> > since they control how NM loads things.
> 
> You could install the plugins as user (./configure --prefix). Checking
> against root would break that.

This plugin would then be non-root, and NM would never be able to run it due to the security issues.  Which means it's kinda useless.

> > init_sync() should also require a 'service' property, not just name.  A VPN
> > without a 'service' is pretty useless as it doesn't tell us anything about
> > the VPN at all.  We should also require a 'program' line too, since NM can't
> > do anything without that, and if NM can't do anything with it, it's pretty
> > useless to clients too.
> 
> maybe in the future we could have a config file that only specifies a
> "plugin" that is useful to libnm (but not to NM-core). In fact, "service"
> and "program" is unused by anybody except NM-core.

Yes, these are only used by NM itself, except that 'service' is the unique key that identifies this specific VPN plugin.  Right now the service daemon is tied to the various UIs that know how to write config that the service-daemon can use.

'service' is also the only thing that a client can use to know what UI to load to edit a connection.

> > Also, what would force_retry be used for?
> 
> I think if a load fails once, any later attempts to reload the plugin should
> fail straight ways -- unless the user really wants to try a second time (by
> setting force_retry).

What's the use-case here though?  How would the thing that passes force_retry=TRUE know that it should retry?

> > Also, why would the
> > NMVpnPluginInfo's 'service' property ever be NULL?  The "if (service &&
> > g_strcmp0(plug_service, service)" is pretty odd.  The NMVpnPluginInfo is
> > invalid if it doesn't have a 'service' property so it should never get
> > created. 
> 
> As said before, I could image usecases.

I'd like to hear them :)

> > NM won't ever load anything without a 'service' property anyway
> > (since it would be useless), nor should the clients.
> 
> "NM won't ever load", but clients might want to load such plugins.

Why would clients do that?

-----

I think I'm getting confused because I'm thinking of the [VPN Connection] and other sections together.  Basically, the [VPN Connection] stuff always needs to be in a secure, root-owned location, because NM is loading them and that needs to be protected.

But the other stuff [GNOME, etc] doesn't.  So if you're thinking that might be split up in the future, OK, because NM doesn't care where that stuff is.  But then we'll need to adjust the API further.  Is this what you're thinking?  Or something else?

If you could just talk about some of the use-cases you're thinking of for non-root/non-etc plugin config files that would help...
Comment 14 Thomas Haller 2015-06-12 11:36:00 UTC
(In reply to Dan Williams from comment #13)
> (In reply to Thomas Haller from comment #4)
> > (In reply to Dan Williams from comment #3)

> > > init_sync() should also require a 'service' property, not just name.  A VPN
> > > without a 'service' is pretty useless as it doesn't tell us anything about
> > > the VPN at all.  We should also require a 'program' line too, since NM can't
> > > do anything without that, and if NM can't do anything with it, it's pretty
> > > useless to clients too.
>
> > maybe in the future we could have a config file that only specifies a
> > "plugin" that is useful to libnm (but not to NM-core). In fact, "service"
> > and "program" is unused by anybody except NM-core.
> 
> Yes, these are only used by NM itself, except that 'service' is the unique
> key that identifies this specific VPN plugin.  Right now the service daemon
> is tied to the various UIs that know how to write config that the
> service-daemon can use.
> 
> 'service' is also the only thing that a client can use to know what UI to
> load to edit a connection.

Very good point, that the connection has not the "name" but NMSettingVpn:service-type.
I fixed it so that service is now mandatory (too).



> > > Also, what would force_retry be used for?
> > 
> > I think if a load fails once, any later attempts to reload the plugin should
> > fail straight ways -- unless the user really wants to try a second time (by
> > setting force_retry).
> 
> What's the use-case here though?  How would the thing that passes
> force_retry=TRUE know that it should retry?

Passing force_retry on initial load has no effect (because no retry is happening). I'd expect that users don't want to retry, they should attempt to load once, if it fails bail out with error and that's it.

Force-retry is for the case where you actually want to retry.

Actually, I added the argument as making it kinda obvious to the caller that there is this behavior. A user should think: what means "force_retry", and which behavior do I actually want?


Note, that if the caller doesn't care, he anyway should just use
  nm_vpn_plugin_info_load_editor_plugin()
vs.
  nm_vpn_plugin_info_load_editor_plugin_full()
with the _full() function has this argument.

Anyway, I can drop it. Which sole behavior should load_full() have?


> I think I'm getting confused because I'm thinking of the [VPN Connection]
> and other sections together.  Basically, the [VPN Connection] stuff always
> needs to be in a secure, root-owned location, because NM is loading them and
> that needs to be protected.

Most of our other discussion points are about:

  - why would you ever load the plugin-info from a non-system-directory? 
    (g_getenv ("NM_VPN_PLUGIN_DIR")), i.e. from a directory different then
    what NM daemon sees.
  - why would you load non-root owned plugins (if NM daemon does not)?
  - why would you load a plugin-info without "program" (if it's useless for 
    NM daemon)?


> But the other stuff [GNOME, etc] doesn't.  So if you're thinking that might
> be split up in the future, OK, because NM doesn't care where that stuff is. 
> But then we'll need to adjust the API further.  Is this what you're
> thinking?  Or something else?

exactly.


> If you could just talk about some of the use-cases you're thinking of for
> non-root/non-etc plugin config files that would help...

nm-applet aleady loads the "GNOME.properties" shared library. With 1.2, libnm also loads "libnm.plugin", which serves the same purpose.
In the future, "libnm.plugin" could provide even more functionality beyond "NMVpnEditor"/import/export.
  - adding VPN support for nmcli/nmtui.
  - expose VPN functions via GIO introspection

The sky is the limit :)

IMO it'd be useful for a client to load a different plugin (user-owned).
E.g. a user could compile a patched version of the VPN plugin that fixes a bug in the client library. Then he could start nmcli/nm-applet and make use of the fix.
Or user could not have root-permissions to install the plugin system-wide. It still can be useful to load the plugin in the client (libnm).

He can then:

   export NM_VPN_PLUGIN_DIR=~/opt/var/lib/NetworkManager/VPN
   nm-applet

For debugging/development it's also great. I don't see a reason to restrict a program run by user 1000 to load a library owned by the same user.
Comment 15 Dan Williams 2015-07-02 20:45:23 UTC
I don't know...  I'm just not convinced that this is useful for anything other than debugging.  If the user wants to patch things, then they can do that and install the RPM on their own system...  what do others think?

I don't want to block this too long on that though, so maybe it's best to remove the API changes, merge it, and then talk about whether the non-root GUI plugin thing is useful?
Comment 16 Thomas Haller 2015-07-08 10:56:01 UTC
(In reply to Dan Williams from comment #15)
> I don't know...  I'm just not convinced that this is useful for anything
> other than debugging.  If the user wants to patch things, then they can do
> that and install the RPM on their own system...  what do others think?

debugging: yeay, 1+

Also: it is common that a user without root-permissions can compile and install an application in his $HOME/bin path. With the files owned by the user, not by root.

The "libnm.plugin" libraries will be used by nm-applet and nmcli (i.e. user applications). Why can a user not load a library that he owns?


> I don't want to block this too long on that though, so maybe it's best to
> remove the API changes, merge it, and then talk about whether the non-root
> GUI plugin thing is useful?

Which API changes do you mean? Everything is new API.

- nm-applet loads "GNOME.properties" plugin. It does not check the file 
  permission/owner at all!
- libnm will load "libnm.plugin" plugin. Although having a similar use, this is 
  a different plugin with new API. I added permission/owner check, because
  I think it is useful (maybe not strictly speaking necessary, but I think it's 
  better to be extra cautious security wise). I already make more strict checks 
  then nm-applet does, but you seem to require them even stricter.


Note that a user (nm-applet?) can already skip owner checks with the new API by doing:

  - instead of using nm_vpn_plugin_info_load_dir(), it is simple to reimplement
    it based on nm_vpn_plugin_info_load_dir_full()
  - instead of nm_vpn_plugin_info_load_editor_plugin(), it can readily use 
    nm_vpn_plugin_info_load_editor_plugin_full().

Owner check is only done by the simple API nm_vpn_plugin_info_load_dir() and nm_vpn_plugin_info_load_editor_plugin().
Comment 17 Beniamino Galvani 2015-07-10 08:02:51 UTC
> libnm: add NMVpnPluginInfo class

+/**
+ * nm_vpn_plugin_info_check_file:
+ [...]
+ * @check_file: pass a callback to do your own validation.
+ * @user_data: user data for @check_file.

Add the @error parameter.

+NMVpnPluginInfo *
+nm_vpn_plugin_info_list_find_by_name (GSList *list, const char *name)
+{
+       GSList *iter;
+
+       if (!name)
+               g_return_val_if_reached (NULL);

Is there any difference with the usual g_return_val_if_fail (name, NULL) ?



> libnm: add nm_vpn_editor_plugin_load_from_file() function

+/**
+ * nm_vpn_editor_plugin_load_from_file:
+ [...]
+ * @check_service: if not-null, check that the loaded plugin advertizes

advertises

+       if (g_module_symbol (module, "nm_vpn_editor_plugin_factory", (gpointer) &factory)) {
+               gs_free_error GError *factory_error = NULL;
+               gboolean success = FALSE;
+
+

extra empty line



> vpn: refactor vpn-manager to use NMVpnPluginInfo

 nm_vpn_manager_init (NMVpnManager *self)
 {

+       if (priv->monitor_lib) {
+               priv->monitor_id_lib = g_signal_connect (priv->monitor_lib, "changed",
                                                     G_CALLBACK (vpn_dir_changed), self);
        }

+       g_object_unref (file);
+       if (priv->monitor_etc) {
+               priv->monitor_id_etc = g_signal_connect (priv->monitor_etc, "changed",
+                                                    G_CALLBACK (vpn_dir_changed), self);

indentation
Comment 18 Thomas Haller 2015-07-10 09:48:46 UTC
(In reply to Beniamino Galvani from comment #17)
> > libnm: add NMVpnPluginInfo class
> 
> +NMVpnPluginInfo *
> +nm_vpn_plugin_info_list_find_by_name (GSList *list, const char *name)
> +{
> +       GSList *iter;
> +
> +       if (!name)
> +               g_return_val_if_reached (NULL);
> 
> Is there any difference with the usual g_return_val_if_fail (name, NULL) ?

yes there is -- if complie with G_DISABLE_CHECKS.

g_return_val_if_fail() can be completely removed by compiling with "#ifdef G_DISABLE_CHECKS"

   g_return_val_if_fail (cond, value);

is equal to

   #ifndef G_DISABLE_CHECKS
   if (!(cond)) {
       g_critical ("assert failed");
       return (value);
   }
   #endif

But g_return_*if_reached() will always return.

   if (!(cond))
       g_return_val_if_reached (value);

is equal to

   if (!(cond)) {
   #ifndef G_DISABLE_CHECKS
       g_critical ("assert failed");
   #endif
       return (value);
   }





fixed the rest and repushed.
Comment 19 Beniamino Galvani 2015-07-10 12:04:56 UTC
(In reply to Thomas Haller from comment #18)
> > Is there any difference with the usual g_return_val_if_fail (name, NULL) ?
> 
> yes there is -- if complie with G_DISABLE_CHECKS.
> 
> g_return_val_if_fail() can be completely removed by compiling with "#ifdef
> G_DISABLE_CHECKS"

Ah, then it's ok.
Comment 20 Thomas Haller 2015-07-14 17:49:41 UTC
Pushed also branch th/vpn-plugin-info-bgo749877 to nm-applet, which makes use of the new API.

Currently it cannot yet load any plugins, because no plugins exist yet.
Comment 21 Dan Williams 2015-07-14 17:56:22 UTC
> libnm: add NMVpnPluginInfo class

Shouldn't nm_vpn_plugin_info_load_dir_full() use nm_vpn_plugin_info_list_add()?

Also, let's keep the 'dir[]' array in nm_vpn_plugin_info_load_dir() sorted by index?

Also, is there no way to return an error from nm_vpn_plugin_info_new_with_data() at all?  How about calling g_initable_init() there and returning the error, if any?  It seems like from_file() and from_data() should have the same arguments and do the same thing.  Also, there's nothing about this function that would take a long time, so I think initializing immediately is the right thing to do.

Also, I'm a bit skeptical about the file validation stuff still; why would there ever be different extensions?  There does need to be some file extension so that we recognize these files specifically (and ignore random stuff, tmpfiles, renames etc).  NM will also only ever look for .name...

> libnm: add load method to NMVpnPluginInfo

Why do we need nm_vpn_plugin_info_set_editor_plugin()?  I'd would drop this until we figure out better use-cases for user plugins.

> vpn: refactor vpn-manager to use NMVpnPluginInfo

In try_add_service(), doesn't nm_vpn_plugin_info_list_add() already check for duplicates?
Comment 22 Thomas Haller 2015-07-15 09:44:54 UTC
(In reply to Dan Williams from comment #21)
> > libnm: add NMVpnPluginInfo class
> 
> Shouldn't nm_vpn_plugin_info_load_dir_full() use
> nm_vpn_plugin_info_list_add()?

Well, that is up to definition. But IMO no:

If you call nm_vpn_plugin_info_load_dir_full() it does not sort out duplicates (and hence does not use nm_vpn_plugin_info_list_add()).

This allows the caller to handle duplicates however he wants.


If the caller wants to get a certain default-behavior, then it should use nm_vpn_plugin_info_load_dir() instead, which has a certain default-policy of rejecting duplicates -- and which considers not only one, but 3 directories (user, lib, etc).



Maybe the names function names nm_vpn_plugin_info_load_dir() vs. nm_vpn_plugin_info_load_dir_full() are misleading, because the former is more then just calling _full() with certain default parameters.

I pushed a fixup renaming the functions:
  load_dir() -> load_dir_default()
  load_dir_full() -> load_dir()




> Also, let's keep the 'dir[]' array in nm_vpn_plugin_info_load_dir() sorted
> by index?

Oh, that was a bug. Removed the explicit indexing and added code comment.


> Also, is there no way to return an error from
> nm_vpn_plugin_info_new_with_data() at all?  How about calling
> g_initable_init() there and returning the error, if any?  It seems like
> from_file() and from_data() should have the same arguments and do the same
> thing.  Also, there's nothing about this function that would take a long
> time, so I think initializing immediately is the right thing to do.

Added an @error argument. It was already initialized immediately.


> Also, I'm a bit skeptical about the file validation stuff still; why would
> there ever be different extensions?  There does need to be some file
> extension so that we recognize these files specifically (and ignore random
> stuff, tmpfiles, renames etc).  NM will also only ever look for .name...

Well, I'm not saying, it could not be different. But the idea is to have an API that has right behavior by default, but also expose the more powerful methods.

E.g., most users don't care, they call nm_vpn_plugin_info_load_dir_default() and that does the right thing.
OTOH,

  - users who care, can load all files from a directory, with stronger 
    influencing which files to load (nm_vpn_plugin_info_load_dir())
    (including without enforcing a ".name" extension).
  - users who really care, can reimplement nm_vpn_plugin_info_load_dir() based
    on the building blocks nm_vpn_plugin_info_check_file_full() and 
    nm_vpn_plugin_info_new_from_file().
  - users who really really care, can create whatever plugin infos they want by 
    calling nm_vpn_plugin_info_new_with_data().

Basically, all the powerful methods are there anyway because they are used by nm_vpn_plugin_info_load_dir_default(). I think it's useful to expose them as API too, so that users don't think: nm_vpn_plugin_info_load_dir_default() is not flexible enough, so I reimplement the whole loading (badly).

The API should give a best-practice, but give access to the knobs if need be.


Arguably, the question is: whether there is any need :) 

Usually, I think it's better to be conservative in what to expose as public API. But these functions are mostly independent. If they are not useful, deprecating them doesn't cause much pain. It's not that they affect other components.
But I would hope that this API is flexible enough to be used by nmcli, nmtui, nm-applet, gnome-control-center and get it done right-right once.


> > libnm: add load method to NMVpnPluginInfo
> 
> Why do we need nm_vpn_plugin_info_set_editor_plugin()?  I'd would drop this
> until we figure out better use-cases for user plugins.

Same as above. It allows the user to load a NMVpnEditorPlugin however he wants and then inject it to NMVpnPluginInfo.

 
> > vpn: refactor vpn-manager to use NMVpnPluginInfo
> 
> In try_add_service(), doesn't nm_vpn_plugin_info_list_add() already check
> for duplicates?

fixed.




Repushed with fixup commits.
Comment 23 Lubomir Rintel 2015-07-16 09:51:54 UTC
I'd like the API to be a lot smaller initially; it's definitely a lot easier to extend it in future than to deprecate it.

Could we probably hide the calls that expose implementation details and ones that do modifications (*_set, *_add)?

> nm_vpn_plugin_info_get_name(), nm_vpn_plugin_info_get_service(), 
> nm_vpn_plugin_info_list_find_by_service(), 
> nm_vpn_plugin_info_get_editor_plugin(), nm_vpn_plugin_info_load_editor_plugin()
These ones seem to be the only ones that are absolutely needed.

> nm_vpn_plugin_info_load_dir()
Maybe this one could be invoked automatically on the first call to the above?

> nm_vpn_plugin_info_get_filename(), nm_vpn_plugin_info_get_plugin(), 
> nm_vpn_plugin_info_get_program()
This might be useful if the daemon uses the API, but can be private until then?

> nm_vpn_plugin_info_lookup_property(), nm_vpn_plugin_info_validate_filename(), 
> nm_vpn_plugin_info_check_file()
What would be the use of these in the public API?

> nm_vpn_plugin_info_get_default_dir_etc(), 
> nm_vpn_plugin_info_get_default_dir_lib(), 
> nm_vpn_plugin_info_get_default_dir_user()
Why expose this on the API? It sounds more like exposing implementation details.

> nm_vpn_plugin_info_load_dir_full(), nm_vpn_plugin_info_load_editor_plugin_full()
Can be private for now I think?

> nm_vpn_plugin_info_list_add(), nm_vpn_plugin_info_list_remove()
I don't think it's useful for the API users to manipulate the list now.  Specifically the making _add() method public will make it impossible to add mandatory properties in future.

> nm_vpn_plugin_info_list_find_by_name()
What's the use of this? nmcli?

> nm_vpn_plugin_info_list_find_by_filename()
I can't think of why would the API user need this.

> nm_vpn_plugin_info_set_editor_plugin()
This doesn't seem useful to the API user.
Comment 24 Thomas Haller 2015-07-16 10:44:15 UTC
(In reply to Lubomir Rintel from comment #23)
> I'd like the API to be a lot smaller initially; it's definitely a lot easier
> to extend it in future than to deprecate it.


> Could we probably hide the calls that expose implementation details and ones
> that do modifications (*_set, *_add)?

There are no *_set() functions. And NMVpnPluginInfo is immutable (except of load_editor_plugin() and set_editor_plugin()).


But there two other functions:

nm_vpn_plugin_info_list_remove()
nm_vpn_plugin_info_list_add()

which are very simple convenience functions to manipulate a GSList. See below.



> > nm_vpn_plugin_info_get_name(), nm_vpn_plugin_info_get_service(), 
> > nm_vpn_plugin_info_list_find_by_service(), 
> > nm_vpn_plugin_info_get_editor_plugin(), nm_vpn_plugin_info_load_editor_plugin()
> These ones seem to be the only ones that are absolutely needed.
> 
> > nm_vpn_plugin_info_load_dir()
> Maybe this one could be invoked automatically on the first call to the above?

that would imply that there is some static internal list of plugins. That would be possible, but it requires the user of the API to use the internally tracked instances of NMVpnPluginInfos.
That might not what the user want -- e.g. nm-applet doesn't want the list provided from nm_vpn_plugin_info_load_dir() but resorts them and remove those without loadable plugins. Also core, doesn't want to load plugins from the user directory, and it wants to reject different plugins.

Adding a more comprehensive API for managing the internal list seems more heavy.
The API for handling the list of available plugins is really just the following methods. They operate on a GSList, owned by the caller:

  nm_vpn_plugin_info_load_dir()
  nm_vpn_plugin_info_load_dir_full()
  nm_vpn_plugin_info_list_add()
  nm_vpn_plugin_info_list_remove()
  nm_vpn_plugin_info_list_find*()



> > nm_vpn_plugin_info_get_filename(), nm_vpn_plugin_info_get_plugin(), 
> > nm_vpn_plugin_info_get_program()
> This might be useful if the daemon uses the API, but can be private until
> then?

The daemon already uses the API.

nm_vpn_plugin_info_get_filename() returns the filename from where the keyfile was loaded, i.e. what you pass to nm_vpn_editor_plugin_load_from_file().

The others are accessors to properties of the keyfile. "get_plugin" for "libnm.plugin" property and get_program() for "VPN Connection.program". It's really just for convenience. Instead of handling keyfile directly.


> > nm_vpn_plugin_info_lookup_property(), nm_vpn_plugin_info_validate_filename(), 
> > nm_vpn_plugin_info_check_file()
> What would be the use of these in the public API?

lookup_property() gives access to all value of the keyfile. While there is a get_plugin(), there is not an accessor for every property. You cannot drop lookup_property() *and* get_plugin().




> > nm_vpn_plugin_info_get_default_dir_etc(), 
> > nm_vpn_plugin_info_get_default_dir_lib(), 
> > nm_vpn_plugin_info_get_default_dir_user()
> Why expose this on the API? It sounds more like exposing implementation
> details.

As there is no internal manager, it is up to the user to load the files from a directory. These functions tell the user which are the default directories.


> > nm_vpn_plugin_info_load_dir_full(), nm_vpn_plugin_info_load_editor_plugin_full()
> Can be private for now I think?




 
> > nm_vpn_plugin_info_list_add(), nm_vpn_plugin_info_list_remove()
> I don't think it's useful for the API users to manipulate the list now. 
> Specifically the making _add() method public will make it impossible to add
> mandatory properties in future.

This doesn't "add"/"remove" properties to the NMVpnPluginInfo. The properties are immutable. It basically adds the instance to the GSList. It's to manager a list of instances.


> > nm_vpn_plugin_info_list_find_by_name()
> What's the use of this? nmcli?
>
> > nm_vpn_plugin_info_list_find_by_filename()
> I can't think of why would the API user need this.

They are convenience method to iterate the list of plugins.


> > nm_vpn_plugin_info_set_editor_plugin()
> This doesn't seem useful to the API user.
Comment 25 Dan Williams 2015-07-17 22:05:27 UTC
Hmm, did it change from info_list -> infos?  I liked info_list better...

One thought; what if VPNPluginInfo was a GObject?  Then the user could attach whatever editor plugin they wanted to the info, however they wanted it.  We'd keep load_editor_plugin(), but ditch set_editor_plugin() and such.  NM doesn't care about the editor plugins at all obviously, so these bits are useless for it.

Also, as it is right now, priv->editor_plugin_loaded doesn't actually do anything.  There are notes in load_editor_plugin() about returning an error if the plugin previously failed, but I don't see anywhere that priv->editor_plugin_loaded gets set to TRUE when priv->editor_plugin != NULL.
Comment 26 Thomas Haller 2015-07-19 13:17:24 UTC
(In reply to Dan Williams from comment #25)
> Hmm, did it change from info_list -> infos?  I liked info_list better...

I did, because the nm_vpn_plugin_info_list*() functions don't have a @self argument as first argument. Hence the name-prefix was a bit misleading. Anyway, I renamed back.


> One thought; what if VPNPluginInfo was a GObject?  

NMVpnPluginInfo is a GObject. Do you mean NMVpnEditorPlugin from nm_vpn_plugin_info_*_editor_plugin()?

> Then the user could
> attach whatever editor plugin they wanted to the info, however they wanted
> it.  We'd keep load_editor_plugin(), but ditch set_editor_plugin() and such.
> NM doesn't care about the editor plugins at all obviously, so these bits are
> useless for it.

Note that NMVpnPluginInfo already assumes that priv->editor_plugin is a GObject. I does g_object_unref to free it.

Same for nm_vpn_editor_plugin_load_from_file() which does:

»···»···editor_plugin = factory (&factory_error);
»···»···g_assert (!editor_plugin || G_IS_OBJECT (editor_plugin));



Do you want to change:

NMVpnEditorPlugin *nm_vpn_plugin_info_get_editor_plugin  (NMVpnPluginInfo *plugin_info);
void               nm_vpn_plugin_info_set_editor_plugin  (NMVpnPluginInfo *self, NMVpnEditorPlugin *plugin);
NMVpnEditorPlugin *nm_vpn_plugin_info_load_editor_plugin (NMVpnPluginInfo *plugin_info,
                                                          GError **error);

to:

GObject *nm_vpn_plugin_info_get_editor_plugin  (NMVpnPluginInfo *plugin_info);
void     nm_vpn_plugin_info_set_editor_plugin  (NMVpnPluginInfo *self,
                                                GObject *plugin);
GObject *nm_vpn_plugin_info_load_editor_plugin (NMVpnPluginInfo *plugin_info,
                                                GError **error);


Via nm_vpn_plugin_info_set_editor_plugin() you can already attach *any* plugin that satisfies the NMVpnEditorPlugin interface. Why would you want to attach an object that doesn't implement that interface? How can you even use the object then (if it doesn't have an interface that it satisfies.


Also: "Then the user could attach whatever editor plugin they wanted to the info, however they wanted it.  We'd keep load_editor_plugin(), but ditch set_editor_plugin() and such."
How does "attach whatever they want" go together with "ditch set_editor_plugin()". How would the user attach anything if there is no load-method?
You mean, attach with g_object_set_data()? They already can do that (yeay).



> Also, as it is right now, priv->editor_plugin_loaded doesn't actually do
> anything.  There are notes in load_editor_plugin() about returning an error
> if the plugin previously failed, but I don't see anywhere that
> priv->editor_plugin_loaded gets set to TRUE when priv->editor_plugin != NULL.

editor_plugin_loaded gets loaded, whenever you call nm_vpn_plugin_info_load_editor_plugin().

The idea is, that if you call nm_vpn_plugin_info_load_editor_plugin() and it fails (e.g. the plugin could not be found), then the plugin is marked as already loaded. If you call nm_vpn_plugin_info_load_editor_plugin() again, it fails right away, without trying anew to load it.




Repushed with renaming.
Comment 27 Lubomir Rintel 2015-07-25 08:56:02 UTC
Pushed some fixups.
Tested to work fine.
Comment 28 Thomas Haller 2015-07-26 09:18:43 UTC
rebased on master
Comment 30 Dan Williams 2015-08-01 03:38:10 UTC
I think the applet side of things looks fine to me too, though I think you're right about the last commit being probably wrong.  It might have been that way before, but I don't think we need ot keep the sorting exactly the same.  I think we should go with NM_VPN_EDITOR_PLUGIN_NAME (eg, drop that TODO commit).

Unfortunately, I didn't realize there was an applet branch for this and while testing out Lubo's VPN plugin changes I pushed two commits to make things work that will conflict with th/vpn-plugin-info-bgo749877 (applet).  Feel free to either rebase on top of git master, or to revert those two changes to git master and merge your branch here.  Which ever is easier.
Comment 31 Thomas Haller 2015-08-01 10:17:35 UTC
now also merge the change to nm-applet to master:

https://git.gnome.org/browse/network-manager-applet/commit/?id=2fa478eab14093203937d69d24d563bfd88dac0d