>    core: add function and testcase to parse vpnc stdout/stderr

utils_handle_output() is a really confusing function... I don't have any specific suggestions though. You should definitely document its calling conventions though, so people hacking on this later don't have to reverse-engineer it.

In test-vpnc-output.c, you should put a "{" after each use of TEST_HEADER and a "}" before each use of TEST_FOOTER, so people's editors will keep the "body" indented the way you have it.



>    core: parse vpnc stdin/stdout and request secrets asynchronously

This adds a hard dependency on new vpnc, but doesn't check for it and will just bomb completely on old vpnc...

>+		status = g_io_channel_read_chars (pipe->channel,
>+				                          buf,

indent


>+	} while (/* status == G_IO_STATUS_AGAIN || */ bytes_read);

Remove the commented-out part, since you definitely don't want that.

(In reply to comment #1)
> >    core: add function and testcase to parse vpnc stdout/stderr
> 
> utils_handle_output() is a really confusing function... I don't have any
> specific suggestions though. You should definitely document its calling
> conventions though, so people hacking on this later don't have to
> reverse-engineer it.
> 

Yeah, I guess it's quite tricky to parse the stuff. I didn't spot any issue on reading the code, but I didn't run/test that.

(In reply to comment #1)
> >    core: add function and testcase to parse vpnc stdout/stderr
> 
> utils_handle_output() is a really confusing function... I don't have any
> specific suggestions though. You should definitely document its calling
> conventions though, so people hacking on this later don't have to
> reverse-engineer it.

Ok.

> In test-vpnc-output.c, you should put a "{" after each use of TEST_HEADER and a
> "}" before each use of TEST_FOOTER, so people's editors will keep the "body"
> indented the way you have it.
> 
> 
> 
> >    core: parse vpnc stdin/stdout and request secrets asynchronously
> 
> This adds a hard dependency on new vpnc, but doesn't check for it and will just
> bomb completely on old vpnc...

Yeah, not sure how to do that.  vpnc prints it's version # to stdout, but that's long after we need to know whether to use the old code or the new code.  Upstream suggested using the SVN revision somehow (???) but that's not really workable.  I suppose we could run "vpnc --long-help" and grep for "interactive-stdin", but upstream also asked if there wasn't some way to autodetect that stdin was a tty or whatever.  So some of this is going to be pending on the final format of the upstream patches, but the mechanics of handling stdin won't change.

> >+		status = g_io_channel_read_chars (pipe->channel,
> >+				                          buf,
> 
> indent
> 
> 
> >+	} while (/* status == G_IO_STATUS_AGAIN || */ bytes_read);
> 
> Remove the commented-out part, since you definitely don't want that.

Looks good.

In my opinion a drop off is not strictly a programmer error but an external condition. Are the g_return_if_fail() just last resort checks or do you expect them to actually occur? In my opinion an attempt to talk to a disconnected agent is either a programming error that should be fixed (if the caller knows that in time) or it's internally a valid condition that deserves at least one real error message but no [soft] assertions afterwards.

(In reply to comment #4)
> Looks good.
> 
> In my opinion a drop off is not strictly a programmer error but an external
> condition. Are the g_return_if_fail() just last resort checks or do you expect
> them to actually occur? In my opinion an attempt to talk to a disconnected
> agent is either a programming error that should be fixed (if the caller knows
> that in time) or it's internally a valid condition that deserves at least one
> real error message but no [soft] assertions afterwards.

Please ignore, wrong bug report.

Btw, dcbw/need-secrets doesn't exist, is it merged already or renamed?

(In reply to comment #5)
> (In reply to comment #4)
> > Looks good.
> > 
> > In my opinion a drop off is not strictly a programmer error but an external
> > condition. Are the g_return_if_fail() just last resort checks or do you expect
> > them to actually occur? In my opinion an attempt to talk to a disconnected
> > agent is either a programming error that should be fixed (if the caller knows
> > that in time) or it's internally a valid condition that deserves at least one
> > real error message but no [soft] assertions afterwards.
> 
> Please ignore, wrong bug report.
> 
> Btw, dcbw/need-secrets doesn't exist, is it merged already or renamed?

It's a branch of network-manager-vpnc in gnome git (bug is filed against the "VPNC" component of NM) though I shoudl have specified the git repo in the initial comment.

danw's comments addressed; branch repushed.

Renamed the NeedSecrets signal to SecretsRequested.

(In reply to comment #8)
> Renamed the NeedSecrets signal to SecretsRequested.

Nevermind; this is for NM not vpnc.

this is sort of blocked until we figure out exactly what changes are going to be made to vpnc

Re-pushed with fixes for ConnectAsyncAllowed().

the newly-commented utils_handle_output() is a lot better

(random minor thing: you say "semicolon" instead of "colon" in one comment)

>+	/* Note: if vpnc sent a server message ending with ':' but we
>+	 * happened to only read up to the ':' but not the EOL, we'll
>+	 * confuse the server message with an input prompt.  vpnc is not
>+	 * helpful here.
>+	 */

or if there happens to be a ':' at byte 512. Changing data_available() to have two loops (first read in a loop until bytes_read==0, then utils_handle_output() in a loop until consumed==0) would make both cases less likely.

vpnc_check_version() will reject 0.6.2 on the grounds that 2 < 4. You need to break out early (with use_interactive = TRUE) if major > 0 or minor > 5.

It's also weird that vpnc_check_version() both returns a value and sets a global variable...


Otherwise, looks good, assuming the vpnc patches go in. I don't think we should commit this before that though.

*** Bug 584690 has been marked as a duplicate of this bug. ***

Rebased, repushed, and fixed up for most recent upstream vpnc changes.  Upstream requested that CTRL-D be used instead of CTRL-Z, and I fixed a bug or two as well.

basically looks good


> core: add function and testcase to parse vpnc stdout/stderr

>+		if (g_ascii_isspace (output->str[0]) || (i == 1 && IS_EOL (output->str[i])))
>+			    *server_message_done = TRUE;

Hm... why is that "i == 1" rather than "i == 0" ?

>+	 * ...  vpnc is not
>+	 * helpful here.
>+	 */

so I should have thought of this before, but if we have to patch vpnc anyway, couldn't we make it be more helpful in --interactive-stdin mode?


> core: parse vpnc stdin/stdout and request secrets asynchronously

>+	char end[] = { CEOT };

I think 0x04 would be more obvious... I don't think most people know the ASCII control character names, and if you don't know that, then it's not obvious that that's even what that is...

(I think it's also slightly weird to use ^D in this context given that it's NOT end-of-file... but I guess that's up to upstream.)

>+	lines = g_strsplit_set (output, "\n", -1);
...
>+			versions = g_strsplit_set (p, ".", -1);

g_strsplit() would be more idiomatic there. (g_strsplit_set() means "split on any of the provided characters", so it's weird to use it and pass only one character.)

(In reply to comment #15)
> > core: add function and testcase to parse vpnc stdout/stderr
> 
> >+		if (g_ascii_isspace (output->str[0]) || (i == 1 && IS_EOL (output->str[i])))
> >+			    *server_message_done = TRUE;
> 
> Hm... why is that "i == 1" rather than "i == 0" ?

I think it was meant to capture the case where the previous for() loop had exited after parsing 1 character, but it's easier to just do:

    if (g_ascii_isspace (output->str[0]) || IS_EOL (output->str[0]))

I think, and that still passes the testcases.  The intent with this if() is to terminate reading the server message if a blank line is found.

> >+	 * ...  vpnc is not
> >+	 * helpful here.
> >+	 */
> 
> so I should have thought of this before, but if we have to patch vpnc anyway,
> couldn't we make it be more helpful in --interactive-stdin mode?

It turns out that upstream fixed things so that there is no --interactive-stdin option anymore, it Just Works (more or less) now.  So while we could patch vpnc, upstream is already 100% ready for this already.

> > core: parse vpnc stdin/stdout and request secrets asynchronously
> 
> >+	char end[] = { CEOT };
> 
> I think 0x04 would be more obvious... I don't think most people know the ASCII
> control character names, and if you don't know that, then it's not obvious that
> that's even what that is...

Fixed.

> (I think it's also slightly weird to use ^D in this context given that it's NOT
> end-of-file... but I guess that's up to upstream.)

I'd originally used ^Z but upstream had this to say during patch review:

---------
Ctrl-Z is a DOS/Windows convension.
Ctrl-D as first char of a line is more suitable in Unix world
http://en.wikipedia.org/wiki/End-of-file
I do not have specific preference, but comments are welcome

Would be good to have a note about this new behavior in man-page.

Instead of using magic number, consider including
  sys/ttydefaults.h
and use the macros CEOF for Ctrl-D or CSUPS for Ctrl-Z.
--------

CEOF actually equals CEOT in sys/ttydefaults.h so I think I went with CEOT because it was the base identifier. 

> >+	lines = g_strsplit_set (output, "\n", -1);
> ...
> >+			versions = g_strsplit_set (p, ".", -1);
> 
> g_strsplit() would be more idiomatic there. (g_strsplit_set() means "split on
> any of the provided characters", so it's weird to use it and pass only one
> character.)

Done.

I've also rebased on git master and fixed everything up for the final implementation of the patches that upstream pushed to the SVN repo.

> core: parse vpnc stdin/stdout and request secrets asynchronously

> This requires vpnc SVN 550 or later.

The code and comments say "0.5.4 or later"... is that still accurate?

>	fprintf (pipe->error ? stderr : stdout, "%.*s", (int) bytes_read, buf);

Everywhere you check pipe->error, you do "pipe->error ? stderr : stdout". You should just have a FILE* member of Pipe rather than "gboolean error".

>+	if (kill (priv->pid, SIGTERM) == 0)
>+		g_timeout_add (2000, ensure_killed, GINT_TO_POINTER (priv->pid));
>+	else
>+		kill (priv->pid, SIGKILL);

according to the man page, kill() can only fail if the signal number is invalid (which we know isn't the case here), the pid is invalid, or you don't have permissions. So sending SIGKILL if the first kill() fails isn't going to help.

>+	priv->infd = -1;
>+	vpnc_cleanup (plugin, FALSE);

doesn't this leak infd? (if it wasn't already -1)

>+		char *joined = g_strjoinv (",", (char **) hints);
>+		g_message ("Requesting new secrets: '%s' (%s)", prompt, joined);

kind of overkill to call g_strjoinv since you know there's only one element in the array.

>+data_available (GIOChannel *source,

I think this will get stuck in an infinite loop on EOF. (It will read, get back G_IO_STATUS_EOF, and return TRUE, and then get called again in the next main loop iteration.)

>+	char *pwhelper_args = NULL;

>+	g_free (pwhelper_args);

not used any more

>+vpnc_check_version (void)

+		num = strtol (versions[0], NULL, 10);
+		if (errno || num < 0 || num > 1000)
+			break;
+
+		/* Fail if minor version < 5 */

You need to set interactive_available=TRUE and break if major > 0. And likewise, if major == 0 and minor > 5.

>+		/* Fail if micro version < 4 */
>+		errno = 0;
>+		num = strtol (versions[2], NULL, 10);
>+		if (errno || num < 3 || num > 1000)

"3" should be "4" in the last line

(In reply to comment #17)
> > core: parse vpnc stdin/stdout and request secrets asynchronously
> 
> > This requires vpnc SVN 550 or later.
> 
> The code and comments say "0.5.4 or later"... is that still accurate?

0.5.4 would be the first version that has the fixes, but upstream moves slowly so the version is still 0.5.3.  So it's still accurate.

> >	fprintf (pipe->error ? stderr : stdout, "%.*s", (int) bytes_read, buf);
> 
> Everywhere you check pipe->error, you do "pipe->error ? stderr : stdout". You
> should just have a FILE* member of Pipe rather than "gboolean error".

Done.

> >+	if (kill (priv->pid, SIGTERM) == 0)
> >+		g_timeout_add (2000, ensure_killed, GINT_TO_POINTER (priv->pid));
> >+	else
> >+		kill (priv->pid, SIGKILL);
> 
> according to the man page, kill() can only fail if the signal number is invalid
> (which we know isn't the case here), the pid is invalid, or you don't have
> permissions. So sending SIGKILL if the first kill() fails isn't going to help.

Fixed.

> >+	priv->infd = -1;
> >+	vpnc_cleanup (plugin, FALSE);
> 
> doesn't this leak infd? (if it wasn't already -1)

I can't recall why that was there.  Removed.

> >+		char *joined = g_strjoinv (",", (char **) hints);
> >+		g_message ("Requesting new secrets: '%s' (%s)", prompt, joined);
> 
> kind of overkill to call g_strjoinv since you know there's only one element in
> the array.

True, fixed.

> >+data_available (GIOChannel *source,
> 
> I think this will get stuck in an infinite loop on EOF. (It will read, get back
> G_IO_STATUS_EOF, and return TRUE, and then get called again in the next main
> loop iteration.)

I fixed this up too, now the returns FALSE from the GIOFunc.

> >+	char *pwhelper_args = NULL;
> 
> >+	g_free (pwhelper_args);
> 
> not used any more

Fixed.

> >+vpnc_check_version (void)
> 
> +        num = strtol (versions[0], NULL, 10);
> +        if (errno || num < 0 || num > 1000)
> +            break;
> +
> +        /* Fail if minor version < 5 */
> 
> You need to set interactive_available=TRUE and break if major > 0. And
> likewise, if major == 0 and minor > 5.
> 
> >+		/* Fail if micro version < 4 */
> >+		errno = 0;
> >+		num = strtol (versions[2], NULL, 10);
> >+		if (errno || num < 3 || num > 1000)
> 
> "3" should be "4" in the last line

I found a better way, which is looking for "--password-helper" which was added the same time as all the interactive fixes, so I fixed up the code to just look for that instead of trying to parse the version and stuff.

Repushed.

>+		if (strstr (output, "--password-helper <executable>"))

I'd check for just the "--password-helper" part, in case the "<executable>" part gets rewritten or localized later.

Everything else looks good.

Fixed the password-helper bit and merged to master.