LGTM

Thanks. Merged to master: https://git.gnome.org/browse/network-manager-vpnc/commit/?id=71b299034e33e5efbce24cfba09b9ca3c3b0d1d3

For libreswan:

https://git.gnome.org/browse/network-manager-libreswan/log/?h=th/vpn-plugin-debug-bgo766872

> beb2bf1 service: show better error message when /etc/ipsec.d/ does not exist

This sounds redundant to me; the error handling seems fine already. Why is
ENOENT a special case?

> 0329845 build: add configure option "--with-nm-ipsec-conf" and "--with-nm-ipsec-secrets-dir"
> b4350c5 shared: cleanup includes in "utils.c"
> 5a02aef properties: move nm_libreswan_config_read() to import_from_file()

Ok.

> 418ee41 properties: cache errno in local variable

Why is this a good idea?

> 722e9aa properties: fail import of files that have no "conn" section

There's a slight possibility of regressing here; but that probably is alright.

> ccb40c9 shared: refactor write_config_option() to treat trailing newline separate

Why? The new write_config_option_newline() seems significantly less legible
than the original code?

> 161d83e properties,service: check for errors in write_config_option()
> 9ac534f shared: refactor debug logging for nm_libreswan_config_write()
> 7388fd1 service: don't print PSK to stdout for logging
> edd8308 shared: move write_config_option() from header to source file

Well; okay. Previous commits indeed make the function too complicated to be
unsuitable for inlining, but I'm not entirely convinced it's necessary.

Not that inlining would be too important here...

> c817c2f service: namespace global variables by putting them into a struct

This looks pointless and a bit confusing to me.

> 011232f shared: add shared file "shared/nm-vpn/nm-vpn-plugin-macros.h"

> c702609 service: replace g_log() by new logging macros
> 6a2121c service: allow setting log_level via environment variable NM_VPN_LOG_LEVEL

Ok.

> 2fba1c8 shared: refactor arguments for nm_libreswan_config_write()

At this point it seems that nm_libreswan_config_write()'s prototype has become
really terrible. Perhaps a function that returns a string buffer instead of the
fd, debug callback and the newline flag might be a saner thing to do?

> 05b02a6 service: pass the log-level and logging-prefix to the helper

>                 if (strcmp (argv[1], "--bus-name") == 0) {
> ...
> -               ifupdown_script = g_strdup_printf ("\"%s --bus-name %s\"", NM_LIBRESWAN_HELPER_PATH, bus_name);

You removed the --bus-name argument from ifupdown but still are parsing it in the helper?

> 7456cc1 service: add @out_stdout argument to do_spawn()

Why? (The commit messge might need an improvement)

> 5e084cf service: log command line in do_spawn()
> 7709fd2 service/trivial: move code
> ffe2f2d service: always read stdout/stderr of ipsec command

Ok.

(In reply to Lubomir Rintel from comment #4)

Thank you for your comments.


> > beb2bf1 service: show better error message when /etc/ipsec.d/ does not exist
> 
> This sounds redundant to me; the error handling seems fine already. Why is
> ENOENT a special case?

ENOENT sounds like the file we want to create doesn't exists. But the failure is that the parent directory does not exists. That is not clear from the error message that you get.

I mean, sure. We can also do with less clear error messages...



> > 418ee41 properties: cache errno in local variable
> 
> Why is this a good idea?

we (should) do this usually. The idea is obviously that the errno doesn't get modified from the moment when we care about it until it is used/printed.

          errsv = errno;
          g_set_error (error, NMV_EDITOR_PLUGIN_ERROR, 0,
                       _("Can't open file '%s': %s"), path, g_strerror (errsv));

e.g. is _() evaluated first or g_strerror()? I think that depends on the compiler/arch.
Could _() fail and overwrite errno? `man 3 gettext` says "no" to that. The macro _() can also mean to call g_dgettext(), which may call setlocale(), textdomain(). from the latter it is quite unclear to me, whether they are guaranteed not to modify errno.

Instead of doing such reasoning over and over, I think copying errno is a good pattern.


> > 722e9aa properties: fail import of files that have no "conn" section
> 
> There's a slight possibility of regressing here; but that probably is
> alright.

I think that is ok. As long as we still import *valid* files (which I think we do). Note the previously, it would return a NMConnection without ID set, thus you end up seeing g_criticals about a connection that doesn't verify
(just try to import an empty file).


> > ccb40c9 shared: refactor write_config_option() to treat trailing newline separate
> 
> Why? The new write_config_option_newline() seems significantly less legible
> than the original code?

write_config_option*() is 30 LOC in total. IMO it is legible enough.

The reason is to call write_config_option() without a trailing newline. Because the passed message will also be printed by debug-logging... thereby it is important that those messages don't have a trailing newline.


On master, it is currently wrong because it does:

    if (debug)
         g_print ("Config: %s", string);

(note that there is one case, where @string has no trailing newline).
Later, we use logging macros that do not expect trailing newlines in their messages.




> 
> > 161d83e properties,service: check for errors in write_config_option()
> > 9ac534f shared: refactor debug logging for nm_libreswan_config_write()
> > 7388fd1 service: don't print PSK to stdout for logging
> > edd8308 shared: move write_config_option() from header to source file
> 
> Well; okay. Previous commits indeed make the function too complicated to be
> unsuitable for inlining, but I'm not entirely convinced it's necessary.
> 
> Not that inlining would be too important here...

I don't think this function was inlined, nor should it be.
I moved it away, because usually we don't have large functions in header files (valid exceptions to this do exist).


> > c817c2f service: namespace global variables by putting them into a struct
> 
> This looks pointless and a bit confusing to me.

Fair enough. I like to recognize global variables by their name (in this case, by their "gl." prefix).


> > 2fba1c8 shared: refactor arguments for nm_libreswan_config_write()
> 
> At this point it seems that nm_libreswan_config_write()'s prototype has
> become
> really terrible. Perhaps a function that returns a string buffer instead of
> the
> fd, debug callback and the newline flag might be a saner thing to do?

Probably, but that would mean a complete refactoring of how to write the configuration, which I'd rather not do that now.

It may be terribly, but not hard to understand.
There are exactly two callers of nm_libreswan_config_write(). Each argument to the function has a precise meaning and a name that kinda tells you what it does. If that doesn't suffice yet, then look at utils.c and see what the argument does (e.g. grep for @openswan, leads to exactly one place).

I do have problems understanding some code in "nm-libreswan-service.c" (e.g. connect_step()). OTOH, nm_libreswan_config_write() is easy to understand.


> > 05b02a6 service: pass the log-level and logging-prefix to the helper
> 
> >                 if (strcmp (argv[1], "--bus-name") == 0) {
> > ...
> > -               ifupdown_script = g_strdup_printf ("\"%s --bus-name %s\"", NM_LIBRESWAN_HELPER_PATH, bus_name);
> 
> You removed the --bus-name argument from ifupdown but still are parsing it
> in the helper?

If you update the package while being connected via an old libreswan plugin, then removing the --bus-name argument will bork your current connection.
We certainly don't support such a scenario (upgrading package without requiring to re-connect) in general.
So, it's not a strong reason. However, I'd rather not break it only to save 10 LOC.



> > 7456cc1 service: add @out_stdout argument to do_spawn()
> 
> Why? (The commit messge might need an improvement)

I added it because I thought I need it (which I didn't). I left it for consistency. Now, I dropped it.

> properties: fail import of files that have no "conn" section

+       if( !has_conn) {

spacing

> > > c817c2f service: namespace global variables by putting them into a struct
> >
> > This looks pointless and a bit confusing to me.
>
> Fair enough. I like to recognize global variables by their name (in this
> case, by their "gl." prefix).

I'm in favor of distinguishing global variables somehow.

LGTM.

(In reply to Beniamino Galvani from comment #6)

thanks. Fixed and re-pushed.

Looks good to merge to me now.

merged to master: https://git.gnome.org/browse/network-manager-libreswan/commit/?id=de704fcc060ef4f0dcced7668a2b15ebc3bf596c