Hi,
Here's the summary of today's IRC meeting.
---
COMMUNITY MEETING
Place: #openvpn-devel on irc.freenode.net
List-Post: openvpn-devel@lists.sourceforge.net
Date: Monday 10th Aug 2015
Time: 20:00 CEST (18:00 UTC)
Planned meeting topics for this meeting were here:
<https://community.openvpn.net/openvpn/wiki/Topics-2015-08-10>
The next meeting has been scheduled to two weeks from this meeting:
<https://community.openvpn.net/openvpn/wiki/Topics-2015-08-24>
Your local meeting time is easy to check from services such as
<http://www.timeanddate.com/worldclock>
SUMMARY
cron2, dazo, mattock and plaisthos participated in this meeting.
---
Discussed the "Add TFTP and WPAD DHCP options" patch:
<http://article.gmane.org/gmane.network.openvpn.devel/9929>
The patch got a feature-ACK, but it had some code and merging issues.
Cron2 notified janjust of the issues.
--
Discussed the "Reworking the interface for querying users" patchset:
<http://thread.gmane.org/gmane.network.openvpn.devel/9657>
Patch 1/4 will need some modifications. Patch 4/4 talks about --icon,
which does not exist. It has also been reported that the patchset can
cause build failures in certain cases:
<http://thread.gmane.org/gmane.network.openvpn.devel/9657/focus=9661>
Dazo will investigate and make all the necessary modifications.
--
Full chatlog has been attached to this email.
--
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc
irc freenode net: mattock
(20:58:37) mattock: I'll be a few minutes late
(21:03:05) ***cron2 is sort of here
(21:04:34) ***dazo is also sort of here
(21:06:12) ***plaisthos too
(21:09:12) cron2: dazo: I thought you're on vacation?
(21:09:20) dazo: back today :)
(21:09:25) cron2: ah!
(21:09:42) cron2: so we can put Tim's patches back on the agenda
(21:09:44) dazo: so it's the "conquer the mailbox" day today
(21:09:49) cron2: haha :)
(21:10:42) dazo: I haven't had time to poke at his patches ... and brain
capacity is getting low right now ... but I'll try to follow the other topics
(21:11:29) cron2: we're not exactly organized today... :-) - there's a few
patch sets that could use review...
(21:11:50) dazo: :)
(21:12:00) cron2: verify-client-cert sounds like "syzzer" (or maybe
plaisthos)... what about the DHCP option patch?
(21:13:17) cron2: plaisthos: anything on your agenda, when you have the chance?
(21:13:31) cron2: or shall we declare "it's too hot, come back in two weeks"?
:-)
(21:15:11) dazo: I'm looking at the DHCP patch ... I understand the cool things
about TFTP and such ... I just don't see how that is useful in an OpenVPN
context ... but that's seeing things from a PXE boot perspective ... the WPAD
part of it can easily get a feature-ACK from me
(21:15:49) cron2: someone on the -users list specifically asked for the TFTP -
they have an application that they want to use via OpenVPN and it needs it...
(21:15:58) cron2: the code is good enough, I think
(21:16:14) dazo: however the TFTP part isn't really complicated, from an ovpn
perspective ...
(21:17:42) mattock: hi
(21:17:47) cron2: ho :)
(21:17:56) ***cron2 has one for mattock1... T-Shirts!
(21:18:39) mattock: oh no
(21:18:43) mattock: no T-shirts, please
(21:18:46) mattock: it was a nightmare
(21:19:10) cron2: but now you know how it works
(21:19:24) ***cron2 wants an "OpenVPN Hackathon Delft 2015" T-Shirt... :-)
(21:19:37) dazo: cron2++
(21:19:43) mattock: the most convenient way is to buy them myself and keep them
of my bookkeeping
(21:19:57) mattock: then bill Francis with imaginary hours
(21:19:59) mattock: :P
(21:20:10) cron2: oh, so the refunding was the nightmarish part? VAT, and
stuff?
(21:20:23) mattock: everything related to moving things from one country to
other
(21:20:52) mattock: anyways, I could probably manage with some trickery
(21:21:21) mattock: anyways, to the patches
(21:21:28) ***cron2 plays small kitten and looks at mattock1 with laaarge eyes
(21:21:31) cron2: :)
(21:21:37) dazo: :)
(21:21:49) cron2: anyway... I think the DHCP patch is good, though jjk needs to
learn our formatting rules :-)
(21:21:51) mattock: so now it's one T-shirt per gettogether, right? :P
(21:22:19) cron2: nah
(21:22:20) dazo: nah, just once per year! :-P
(21:22:26) cron2: *g*
(21:25:09) mattock: so DHCP patch: feature-ACK, code-NACK due to formatting
issues
(21:25:27) cron2: + char str[256];
(21:25:27) cron2: + strncpy( str, o->wpad_url, 255 );
(21:25:27) cron2: + strcat( str, "\r" );
(21:25:29) mattock: how about "verify-client-cert" patch?
(21:25:30) cron2: hrmph
(21:25:52) cron2: this is *not* safe coding style
(21:26:46) Crashdemon ha abbandonato la stanza (quit: Ping timeout: 244
seconds).
(21:26:47) cron2: if wpad_url is longer than 255 characters, this will a) in
every case overrun by 1 byte, and b) usually just crash in interesting ways, as
str[255] isn't guaranteed to be 0
(21:26:50) dazo: agreed ... what about snprintf(str, 255, "%s\r", o->wpad_url);
?
(21:26:56) dazo: sorru
(21:27:13) dazo: snprintf(str, 255, "%.254s\r", o->wpad_url);
(21:27:36) cron2: in that case you wouldn't even need snprintf :) - but yes,
something along these lines
(21:27:53) ***cron2 just inserts dazo's code
(21:28:03) cron2: and reformats the whitespace on the fly
(21:28:14) dazo: snprintf() will remove some compiler warnings which sprintf()
can cause on some systems
(21:28:50) cron2: well, this code is actually not correct either :)
(21:29:09) cron2: 254 bytes from wpad_url, + "\n" + 0 = 256, so snprintf()
would truncate-off the "\n"
(21:29:09) dazo: I didn't promise perfect code today :)
(21:29:23) ***cron2 puts sizeof(str) there
(21:29:35) dazo: that'll work better!
(21:29:58) dazo: I just wonder ... WPAD options ... that's Windows centric?
(21:30:12) dazo: (at least that's only where I've seen them used)
(21:30:12) cron2: WPAD is the "here's your proxy" magic
(21:30:40) cron2: no idea whether the browsers have picked this up elsewhere...
most likely not, because there's no communication path between dhcp-client and
browser
(21:30:42) dazo: okay, fair enough ... if it also works on non-Windows, I won't
comment on the #else block
(21:31:09) dazo: well, NetworkManager can in theory pick it up and configure
the networking layer
(21:31:12) cron2: (now *that* is something where stuff like systemd/dbus really
improves on the traditional unix way)
(21:31:20) cron2: yeah
(21:35:41) dazo: I don't see any other issues, but don't trust my judgment today
(21:35:54) cron2: this patch does *so* not apply with "git am"...
(21:35:59) cron2: --- openvpn-2.3.7/src/openvpn/tun.c 2015-06-08
08:16:35.000000000 +0200
(21:36:02) cron2: +++ /tmp/build-x86_64/openvpn-2.3.7/src/openvpn/tun.c
2015-07-16 12:10:59.5385
(21:36:11) dazo: it doesn't even apply with patch
(21:36:27) dazo: JJK needs to learn 'git format-patch' :)
(21:36:42) cron2: indeed :)
(21:36:54) cron2: but I'm good at patch mangling...
(21:37:13) ***dazo didn't have patience enough today :)
(21:38:10) cron2: what
(21:38:18) cron2: there is a chunk in there that is so totally unrelated
(21:38:26) cron2: + struct tuntap_options *o =
&options->tuntap_options;
(21:38:27) cron2: +
(21:39:11) dazo: dhcp_option_address_parse ("GW", p[1], o->gw, &o->gw_len,
msglevel); .... not needed?
(21:39:38) cron2: no, that was inside a {} somewhere else
(21:39:39) dazo: I thought that was needed for the TFTP stuff
(21:39:55) cron2: went right out of scope two lines later
(21:40:00) cron2: 5354
(21:40:19) cron2: ok, enough is enough
(21:43:50) dazo: got enough brain power to start looking at my bigger patch-set?
(21:49:27) cron2: oh
(21:49:35) cron2: you looked at a different version of the v3 patch
(21:49:44) ***cron2 rolls eyes
(21:49:50) Crashdemon
[~jir...@dslb-094-220-067-153.094.220.pools.vodafone-ip.de] è entrato nella
stanza.
(21:50:03) dazo: cron2: nope, I looked at the rev3 patch
(21:50:18) dazo:
http://thread.gmane.org/gmane.network.openvpn.devel/9864/focus=9908
(21:50:20) vpnHelper: Title: Gmane Loom (at thread.gmane.org)
(21:50:28) cron2: there's a v3 patch in 55a3dbf8.5010...@nikhef.nl (Subject:
PATCH v2), and *another* v3 patch in 55a786e3.7010...@nikhef.nl
(21:50:34) cron2: with subject PATCH v3
(21:50:43) dazo: meh
(21:51:07) cron2: "yours" has the dhcp_option_address_parse ("GW"...) bit,
while mine only has the "struct tuntap_options *o = ..."
(21:51:27) dazo: ehm? http://article.gmane.org/gmane.network.openvpn.devel/9908
(21:51:29) vpnHelper: Title: Gmane -- Re: PATCH v2 Add TFTP and WPAD DHCP
options (at article.gmane.org)
(21:51:42) cron2: that is "v2", as the subject says :)
(21:51:52) dazo: that's what mid.gmane.org gave me
(21:51:59) dazo: duh!
(21:52:04) cron2: http://article.gmane.org/gmane.network.openvpn.devel/9929
(21:52:07) vpnHelper: Title: Gmane -- PATCH v3 Add TFTP and WPAD DHCP options
(at article.gmane.org)
(21:52:14) cron2: this is "v3 with subject v3"
(21:52:29) dazo: Looking at the right stuff now :)
(21:52:37) dazo: agreed ... now it's not needed
(21:52:41) cron2: the only difference seems to be the "GW" line
(21:53:45) dazo: yeah
(21:56:25) cron2: as for the patch set...
(21:56:39) ***cron2 looks at plaisthos... "he's the one with brains, I'm the
good-looking one!" :-)
(21:57:10) dazo: the v3 patch actually applied fairly well with patch -p1
(21:57:30) dazo: (two rejects, but that's not too bad)
(21:57:37) cron2: that is way too bad
(21:57:55) cron2: a patch to areas that have not been touched by other folks in
ages should not have rejects
(21:57:56) dazo: one of them is the not needed struct tuntap_options
(21:58:13) cron2: the other one is the new dhcp options - whitespace change in
the line before that
(21:58:15) dazo: fair enough
(21:58:36) cron2: I could have fixed that as well, but at that point I didn't
feel like it should be my job
(21:58:46) cron2: coming to your patchset :)
(21:58:52) dazo: wohoo!
(21:59:15) cron2: dwdm2 sent a comment about your autoconf magic failing if
libsystemd.pc and systemd.pc both exist...
(21:59:19) mattock: http://thread.gmane.org/gmane.network.openvpn.devel/9657 I
assume
(21:59:21) vpnHelper: Title: Gmane Loom (at thread.gmane.org)
(22:00:55) dazo: oh fun ... both libsystemd.pc and systemd.pc installed at once
(22:01:00) ***dazo wonder which distro he uses
(22:01:44) cron2: 1/4 has managed to confuse git a bit... the patch looks way
more complicated than it actually is, I think
(22:03:08) cron2: the console.h patch seems to have funny characters in it, in
the comments
(22:03:18) dazo: that's right, it's not too complicated ... but as it moves
around on some functions, patches gets odd
(22:03:20) cron2: /**# Local pointer to the gc_arena...
(22:05:10) cron2: I would not use QUERY_USER_FOREACH() - it's trivial enough
that actually hiding it in a macro makes it harder to understand what is
happening - so you have to add a comment to explain what it does
(22:05:40) dazo: The /**< syntax is Doxygen stuff ... to "tag" these comments
to the right place in the HTML
(22:06:00) cron2: ok, good, didn't know that and it just looked weird in the
web view
(22:06:54) cron2: I'm not sure you should use function names that start with
"_" - as far as I remember, these are reserved for "library internal use", and
we're not a library...
(22:07:54) plaisthos: sorry was away, catching up now
(22:08:01) cron2: I'd just leave them their existing name - and maybe even
leave them in console.c, as not much seems to be won by moving them around and
introducing a new .c file
(22:08:20) cron2: console.c has 242 lines, that's not *so* big that we really
need to split out stuff...
(22:08:34) dazo: The QUERY_USER_FOREACH() macro is something I've picked up
from the kernel sources ... such things are used there often, to simplify
reading the code ... but I have no strong feelings, in my head it makes it
easier to read the code - but I can understand this can be confusing when it's
not been seen so often
(22:09:20) dazo: cron2: the '_' prefix ... still in patch 1?
(22:09:21) cron2: since we're just walking lists elsewhere in the code, I'd
stick to that
(22:09:24) cron2: yes
(22:12:27) cron2: the new invocation interface (get_console_input() ->
query_user_init(), query_user_add(), query_user_exec()) is much longer to
achieve the same thing... maybe introduce a wrapper that will do the same
thing, but doesn't make the 3 places where it's called so much longer?
(22:13:21) dazo: just a sec ... catching up while fixing the patch-set on the
fly
(22:15:31) cron2: comment 4/4 talks about --icon, but the code does not have it
(22:16:18) cron2: besides that, 2/4, 3/4 and 4/4 look good to me (modulo
dwdm2's double-.pc remark)
(22:16:51) cron2: but I find 1/4 is overengineering to achieve the desired
result
(22:17:57) Crashdemon ha abbandonato la stanza (quit: Ping timeout: 255
seconds).
(22:17:58) dazo: I'm not quite sure I follow the "maybe introduce a wrapper
that will do the same thing...."
(22:18:44) cron2: misc.c, pkcs11.c - a single function call to
get_console_input() expands to about a screenfull of query_user_...() calls
(22:19:09) cron2: can we have a query_user_dothething() call that will init(),
add() and exec() for these cases, in one go?
(22:20:29) dazo: hmm ... the idea is that this API can provide a possibility to
query for PKCS#11 pin code, --key password in the same go as username/password
(22:20:38) cron2: the old API does that :-)
(22:21:01) dazo: no, it does it several places ... and I haven't come that far
with further patches to reach that goal
(22:21:36) dazo: the idea is that there is one call to the "external side"
which is told "I need this and this and this" ... and provides all the
information back in one go
(22:22:05) dazo: how it is now ... it is "I need this" ... "Oh, and I need this
too" ... "I also need this" .... which is three interactions
(22:22:24) cron2: I understand, but I see no reason why there should not be a
wrapper that callse query_user_init(), adds arguments with query_user_add(),
and then executes the thing with query_user_exe(), without the callers having
to do all this
(22:23:03) cron2: the wrapper function of course would have "more" arguments,
some of them being flags and others possibly NULL for unwanted functionality -
just like get_console_input() does today
(22:23:13) cron2: magic, with few lines of code
(22:23:30) ***dazo looks closer at the old and new code
(22:23:31) cron2: the internal abstraction with _init, _add, _exec is fine
(22:24:10) cron2: but the change in misc.c and pkcs11.c is making a long and
complicated function longer and more complicated, and I'm wondering if this
cannot be done more elegantly
(22:25:49) dazo: look at the username/password section ... around line 1114
(22:26:43) cron2: which branch, which file?
(22:26:50) dazo: misc.c
(22:27:01) dazo: and I'm on the branch with the patch
(22:27:13) cron2: the get_console_input() block inside #ifdef ENABLE_CLIENT_CR?
(22:27:17) dazo: what could be done more elegant is to use stdarg on the
query_user_add() .... the code gets somewhat longer due to that 2 prompts needs
to be setup, which has a variable prefix
(22:27:48) dazo: http://fpaste.org/253587/39234866/
(22:28:20) cron2: no, the thing is: if there is a single call to
query_user_add(), just stuff it all in a single function that does _init() and
_exec() as well. Don't put all the *qucfg mess into the caller
(22:28:38) cron2: query_user_single_execution_add_and_go()
(22:28:42) dazo: ahh, just those places where there is a single interaction
(22:29:24) dazo: I see ... well, the benefit won't last too long when I get
further to grab all this in one go ... then the query_user_init() and
query_user_exec() will be moved elsewhere
(22:29:25) cron2: if there's more to be stuffed in, one need to look more
closely, of course
(22:29:50) dazo: but sure, I can add that for the time being and we'll remove
it again once it is no longer needed
(22:29:57) cron2: in that case, I stop objecting, but *this* is just too
complicated for my brain :-)
(22:30:38) dazo: I'm not done improving the query user interface with these
patches ... this was just a good first milestone :)
(22:30:42) cron2: one could argue whether query_user_init() shouldn't just
msg(M_FATAL, ...) itself upon failing - this should be a "canthappen" anyway...
(22:31:34) cron2: 3 lines saved per invocation...
(22:31:46) dazo: I can make query_user_init() become a void function .... and
do the msg(M_FATAL, ...) inside that function
(22:32:13) dazo: it is unlikely to happen ... but not checking for malloc()
failures is a bigger fail :)
(22:32:25) cron2: this :) (yes, it would be nice to know *which* call failed,
but arguably, this would have to fail on a malloc() of so few bytes that it's
very unlikely to be useful knowledge)
(22:32:39) cron2: the memory saved in not having 3 different error messages
makes up for the memory needed :)
(22:32:45) dazo: fair enough! I'll fix that!
(22:33:50) cron2: so, now my brain is spent... need some chocolate...
(22:42:13) mattock: cron2: spent enough for today, or until you get the
chocolate? :P
(22:43:14) cron2: I think we did quite a bit of work, and I'm out now :-) -
feel free to go on, though!
(22:45:49) dazo: cron2: I'll have an updated patch of the first one out by
tomorrow
(22:46:04) mattock: ok so "the patchset needs some modifications which dazo
will provide"?
(22:47:11) dazo: mattock1: yeah ... and cron2 said: besides that, 2/4, 3/4 and
4/4 look good to me (modulo dwdm2's double-.pc remark)
(22:47:25) mattock: yeah, 1/4 gets most modifications
(22:47:34) dazo: the double-.pc remark is an odd one - but I'll investigate
that one too
(22:57:51) mattock: ok, summary ready, will send
(22:58:06) mattock: meeting concluded apparently :)
