Hi,
Here's the summary of today's IRC meeting.
---
COMMUNITY MEETING
Place: #openvpn-meeting on irc.freenode.net
Date: Wednesday 7th Dec 2016
Time: 20:00 CET (19:00 UTC)
Planned meeting topics for this meeting were here:
<https://community.openvpn.net/openvpn/wiki/Topics-2016-12-07>
The next meeting has been scheduled to a week from now (Wed 14th
December), at the same time as today.
Your local meeting time is easy to check from services such as
<http://www.timeanddate.com/worldclock>
SUMMARY
cron, dazo, janjust, krzee, mattock, selvanair and syzzer participated
in this meeting.
---
Discussed patchwork briefly:
<http://jk.ozlabs.org/projects/patchwork/>
Noted that patchwork has not yet been installed, and that will not
change before 2.4.0 is out.
---
Discussed the OpenVPN 2.4_rc2 release:
<https://community.openvpn.net/openvpn/wiki/StatusOfOpenvpn24>
Two problems have been found that need to be fixed for 2.4_rc2:
- systemd + --chroot is an unhappy combination
- the --mtu-disc option is broken on server side
The first issue has a pending patch, and work on the second one has started.
---
Discussed the --no-iv option is OpenVPN. It was agreed that the
performance gains (~1.5% with packet size of 1500 bytes) are not big
enough to warrant keeping this (insecure) option around.
Syzzer sent a "deprecate --no-iv" patch to the list during the meeting.
---
Discussed OpenVPN 2.4 security audits by OSTIF.org and PIA:
<https://ostif.org/>
<https://www.privateinternetaccess.com/>
These two organizations have so far been working on roughly the same
goal without knowledge of each other. It was agreed that we should
ensure that they remain in sync so that their work does not overlap too
much.
---
Discussed the "great code reformatting" planned for OpenVPN 2.4_rc2. The
reformatted code will follow the Allman style:
<https://community.openvpn.net/openvpn/wiki/CodeStyle>
All tweaks to the coding style were documented on the above page.
Agreed to use the reformatting approach suggested by syzzer:
(1) define the exact tool, version and flags we use for the formatting,
(2) get an ACK on that on the list
(3) push a commit with just running that on the codebase
(4) get some public ACKs on the commit SHA to confirm this is exactly
running the tool
(5) send style fixup patches following the default procedure
Also agreed to dazo's suggestion of adding a tools/script directory
which has a simple script running the indenting properly, and which can
then be used by patch contributors as well.
Also agreed to create a reformatting branch in Git.
--
Discussed removing user choices from the Windows installer:
<https://github.com/OpenVPN/openvpn-build/issues/59>
<https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13390.html>
It was agreed that refactoring the installer should be taken care of
after currently open PRs have been merged. The goal is to get a
refactored installer in OpenVPN 2.4_rc2 (due 15th Dec).
---
Full chatlog has been attached to this email.
--
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc
irc freenode net: mattock
(21:01:09) mattock: meeting time
(21:01:31) ***dazo brb
(21:03:18) cron2: burbs
(21:04:04) krzee: gday
(21:04:08) mattock: let me know when you've burbed enough :P
(21:04:11) mattock: hi krzee!
(21:04:16) ***cron2 wants beer
(21:04:42) ***dazo is here
(21:04:50) syzzer: evening
(21:04:59) mattock: https://community.openvpn.net/openvpn/wiki/Topics-2016-12-07
(21:05:01) vpnHelper: Title: Topics-2016-12-07 – OpenVPN Community (at
community.openvpn.net)
(21:05:32) mattock: hi all!
(21:05:34) cron2: 2. has been answered on the list already "nothing happened,
everyone too busy, later!" - check
(21:05:47) krzee: !beer
(21:05:55) mattock: yes, patchwork
(21:06:14) mattock: no progress there
(21:06:18) cron2: (as long as it's not *forgotten*...)
(21:06:20) mattock: 2.4 next?
(21:06:29) dazo: yes
(21:07:12) dazo: we have two issues located .... systemd + --chroot is an
unhappy combination, patch resolving it for 2.4_rc2/0 is on the list ... and
then --mtu-disc is broken on server side
(21:07:31) dazo: (which we were discussing on #openvpn-devel right before this
meeting)
(21:07:38) cron2: and AES-OFB ASSERT()s on reneg
(21:07:43) dazo: right!
(21:07:49) cron2: (patch on the list as well)
(21:08:04) syzzer: *-OFB/CFB, actually
(21:08:09) syzzer: but yeah
(21:09:25) dazo: anyone want to review syzzer patch? Or should I add that to
my TODO list as well?
(21:10:02) cron2: I have no idea what CO_PACKET_ID_LONG_FORM does or why it
breaks AES-OFB/CFB, but besides that, it looks logical...
(21:11:11) dazo: I'm planning to work another 3-4 hours today ... and I'd like
to get all those issues for rc2 closed as soon as possible and I have a chance
to have a look at it today
(21:11:18) cron2: ah, I should just read the lines above
(21:11:52) cron2: syzzer: so why does it ASSERT? Or was that "it gets confused
and that might lead to all the most exciting things"
(21:12:12) syzzer: cron2: it gets confused
(21:12:15) cron2: dazo: as a side note, we're not releasing rc2 tomorrow
(21:12:30) mattock: next week
(21:12:47) mattock: according to the original schedule
(21:12:50) mattock: https://community.openvpn.net/openvpn/wiki/StatusOfOpenvpn24
(21:12:52) vpnHelper: Title: StatusOfOpenvpn24 – OpenVPN Community (at
community.openvpn.net)
(21:12:54) syzzer: because in in the cfb/ofb case, it basically sets teh flags
to 0 (or *_LONG_FORM)
(21:13:00) cron2: (so no need to pull an all-nighter tonight, better get enough
sleep)
(21:13:02) dazo: cron2: I know ... but I'd like to get a "git master" spin out
on at least one or two of my VPN servers ... to have some extra testing time in
production
(21:13:20) syzzer: which borkes if the CFB.OFB code does ASSERT(flags & USE_IV)
(21:13:28) cron2: well, that is comparably easy :)
(21:13:33) cron2: ic. makes sense
(21:14:02) cron2: (and it would also explode if someone pushes AES-GCM with
--no-iv set before)
(21:14:09) syzzer: exactly
(21:14:28) syzzer: more nails on the coffin of --no-iv!
(21:15:03) syzzer: that sh*t merely complicates things
(21:15:32) cron2: I'll fix a few typos in the commit message on the fly, and
change the trac bit to "Trac: #784" ("new format as ordered by chief dazo")
(21:15:46) syzzer: sure
(21:16:22) dazo: cron2: the "Trac:" formatting will also be caught by
git-ackam/git-regen-ackmsg too
(21:16:23) krzee: do people actively use --no-iv?
(21:16:34) syzzer: I hope not...
(21:16:45) krzee: thats what i was thinking :D
(21:16:54) krzee: like, isnt there a reason for is!?
(21:17:29) syzzer: the openvpn protocol is relatively robust against IV reuse,
but it is still a Bad Idea
(21:17:39) cron2: test running
(21:18:27) syzzer: so, what do you think of my --no-iv "bugfix"? simple enough
to include into 2.4?
(21:18:53) cron2: syzer: "20:17 < cron2> test running"
(21:19:37) janjust [~janjust@openvpn/community/support/janjust] è entrato nella
stanza.
(21:19:38) syzzer: ah, good, that already includes the deprecation patch?
(21:19:45) krzee: jjk!
(21:19:51) janjust: Evening all, thought I'd join you for a change ;)
(21:19:59) cron2: no, that's just trac 784
(21:19:59) mattock: hi janjust!
(21:20:04) dazo: janjust: welcome!
(21:20:06) cron2: did not see anything else yet
(21:20:09) mattock: so this --no-iv option is odd
(21:20:09) janjust: thx all
(21:20:12) syzzer: not, h,
(21:20:27) mattock: I wonder if the performance benefit for using it is
actually significant...
(21:20:38) syzzer: gah, typo in address. will resend
(21:21:18) cron2: I can't even properly test OFB here... as my test server gets
rebuilt twice a week with mbedTLS which doesn't do that
(21:21:53) janjust: performance? I'm listening
(21:22:33) syzzer: mattock: not using the IV saves you 16 bytes overhead for CBC
(21:22:38) dazo: janjust: just a side-track discussion if why we have --no-iv
(21:22:40) syzzer: *AES-CBC
(21:23:03) dazo: s/if/of/
(21:23:06) mattock: janjust: have you tested the performance impact of using
--no-iv?
(21:23:26) mattock: syzzer: for each packet?
(21:23:30) janjust: yeah I tested that some years ago
(21:23:32) syzzer: mattock: yes
(21:23:58) janjust: same as using no auth - it depends on your MTU size; for
networks with small MTU sizes any bytes saved have a larger impact
(21:24:31) janjust: for default = 1500 bytes you gain on the order of 16 / 1400
bytes = ~1.5%
(21:24:53) mattock: yeah, so significant, but not incredible improvement
(21:25:16) syzzer: those that want performance should just switch to AES-GCM :)
(21:25:17) janjust: exactly, EXCEPT when you're on older satellite links with
crooked MTUs of 800 bytes
(21:25:30) dazo: --no-iv have been around since the beginning of our git
history ... so I think this is just old code we've inherited ... not convinced
we should have it though
(21:25:43) cron2: syzzer: https://paste.fedoraproject.org/501199/48113873/ - ok?
(21:25:53) mattock: as cron2 tends to say: "we have too many options"
(21:25:58) janjust: --no-iv is a nice debugging feature
(21:26:12) syzzer: janjust: why is that useful for debugging/
(21:26:25) dazo: (syzzer beat me to it)
(21:26:36) syzzer: cron2: ack
(21:26:44) janjust: if you do it right your traffic will be encrypted exactly
the same every tyime
(21:26:48) cron2: syzzer: new version, even better I think :-) -
https://paste.fedoraproject.org/501202/48113879/
(21:27:07) syzzer: janjust: that's why we have --cipher none :)
(21:27:32) janjust: syzzer: yes but sometime you want "predictable encryption"
(21:27:32) cron2: I'm sure --cipher none will crash if you set --no-iv at the
same time. Cipher none *loves* to crash on us :-)
(21:27:35) syzzer: cron2: fine too
(21:27:59) syzzer: janjust: for that, just rig the prng ;)
(21:28:48) janjust: true, and I admit that it's an exotic case
(21:29:07) janjust: but the openvpn code path for cipher none is quite
different compared to having a cipher
(21:29:17) dazo: I can see the academic use of --no-iv and rigged prngs ... but
I agree with cron2, we have too many options ... and users adding --no-iv into
production are (hopefulle) close to none, and if they really do need it (MTU <
800) we will have 2.3 for a bit longer
(21:29:38) ***cron2 ACKs the "deprecate --no-iv" patch
(21:29:40) janjust: OTOH we could also achieve it by specifying an OpenSSL NULL
cipher
(21:29:43) cron2: (that *just* hit the list)
(21:30:52) dazo: feature-ACK on syzzer deprecate --no-iv patch ... need to
fully test for a full ACK
(21:31:35) syzzer: o/
(21:31:38) syzzer: ar
(21:31:40) syzzer: \o/
(21:31:51) cron2: one-armed crypto bandit?
(21:32:01) dazo: janjust: just a complete side track .... in addition to OSTIF
funding a security audit of OpenVPN v2.4, PIA have announced they've engaged
Matt Green to do a security audit on v2.4 as well
(21:32:15) cron2: Wed Dec 7 20:32:05 2016 WARNING: --no-iv is deprecated and
will be removed in 2.5
(21:32:18) cron2: tested
(21:32:25) janjust: dazo, nice!
(21:32:41) mattock: dazo: and why is PIA working on a separate security audit?
(21:33:00) cron2: syzzer: for 2.5, we should move the "disabling NCP mode
(--ncp-disable) because not in P2MP ..." line somewhat "down the line" - it's
printed before anything else usually, which is a bit weird (before the version
line)
(21:33:15) cron2: I'm not sure who is to blame for that, most likely me
(21:33:32) mattock: Derek from OSTIF gave me some numbers of how much they've
raised, and the sums are quite substantial already
(21:33:35) dazo: mattock: they were not aware of our OSTIF discussions and had
approached him before we had our announcement .... Matt Green will have a much
more crypto focused review though
(21:34:03) mattock: dazo: in any case PIA should work with OSTIF so that the
reviews don't overlap
(21:34:10) mattock: unnecessarily much at least
(21:34:22) syzzer: yeah, we should just ask OSTIF to focus on the software
security more than on crypto
(21:34:23) janjust: I agree with mattock; the last thing we'd want is two
different outcomes
(21:34:37) ***cron2 needs to be afk for ~15 minutes to do christmas cookie
chocolate covering which needs to be right *now* (while the temperature is just
right)
(21:34:43) syzzer: as the crypto wonk will focus on the crypto ;)
(21:34:43) dazo: mattock: that's what I've been discussing with caleb1 (on
#openvpn-devel) and Kelticfox (on PM) today already
(21:34:49) mattock: cron2: have fun!
(21:35:10) mattock: dazo: those guys are from PIA?
(21:35:11) ***janjust fully expects cron2 to fax us all a box of chocolate Xmas
cookies
(21:35:14) dazo: mattock: yes
(21:35:16) mattock: ok
(21:35:20) mattock: good to know
(21:35:32) cron2: you could agree on the mechanics of the reformatting in the
mean time - how do we get this reviewed properly, without sending "full openvpn
source" (because the diff will be larger than the original files in most cases)
across the list...
(21:35:51) syzzer: yeah, I thought a bit about that
(21:36:00) dazo: I think it is good that we have more audits on critical code
paths ... but the more trivial code paths does not need to be that extensive
(21:37:21) dazo: on the reformatting ... I think we need to do that without ML,
tbh ... we can push it out to a review branch ... where we can pull it down and
do git diff -w (--ignore-white-space) to see better the changes
(21:37:56) dazo: the review won't be trivial ... so here we need to put some
faith into those pushing out these changes and merging them into master
(21:38:14) syzzer: what about (1) define the exact tool, version and flags we
use for the formatting, (2) get an ACK on that on the list, (3) push a commit
with just running that on the codebase, (4) get some public ACKs on the commit
SHA to confirm this is exactly running the tool (5) send style fixup patches
following the default procedure
(21:38:16) janjust: as long as we don't adopt GNU C coding standards (e.g.
"if true {" on the same line) then I'll be ok
(21:38:55) syzzer: janjust: we are currently using the GNU style :p
(21:38:58) dazo: janjust: allman style is decided ... spaces (no tabs), width 4
... always curly braces on if() stuff
(21:39:05) syzzer: and want to switch to allman
(21:39:20) syzzer: https://community.openvpn.net/openvpn/wiki/CodeStyle
(21:39:21) vpnHelper: Title: CodeStyle – OpenVPN Community (at
community.openvpn.net)
(21:39:36) janjust: actually we're inconsisten in style
(21:39:42) dazo: yep!
(21:39:46) syzzer: janjust: yes, but mostly gnu :p
(21:39:49) janjust: +t somewhere; but I really really dislike gnu
(21:39:55) janjust: I remember discussing this in Delft
(21:41:03) dazo: for tool ... I personally found astyle fairly easy to use
(21:41:31) syzzer: dazo: before switching to the tool - what do you think of my
proposed procedure above?
(21:42:53) dazo: syzzer: sounds reasonable ... step 3) I pressume is 'git push'
(21:43:13) syzzer: sort of 'reproducible reformatting'
(21:43:20) dazo: yes, I like that
(21:43:25) syzzer: yeah, commit & push
(21:43:32) dazo: makes sense
(21:44:16) dazo: should we add a tools/script directory where we have a simple
script running the indenting properly ... which can be used by patch
contributors as well?
(21:44:27) janjust: dazo: +!
(21:44:30) janjust: +1
(21:45:53) dazo: :)
(21:46:07) syzzer: yes, something like that. I was thinking of just putting
the reformatting flags on the CodeStyle page
(21:46:10) cron2: syzzer: I like that approach (and the "document tool
invocation in tree")
(21:46:34) cron2: I've started on the CodeStyle page with optiosn for gnu
indent (does not work really well for me) and uncrustify, but I'm open to use
astyle if that works
(21:47:12) dazo: I've looked at uncrustify a long time ago ... it is a powerful
tool ... but it got the same issue OpenVPN have .... too many options
(21:47:33) dazo: (*that* could be an argument why we should use uncrustify
though ... we're consistent in loving options!)
(21:48:00) dazo: I'm open to use both astyle and uncrustify
(21:48:01) janjust: before you know it cron2 will submit patches to uncrustify
to remove all sorts of options ;)
(21:48:13) dazo: hehehe
(21:49:09) syzzer: I heven't tried either yet, but as long as it does the job,
I'm fine with it
(21:49:18) selvanair [~sn...@www.silvermet.ca] è entrato nella stanza.
(21:49:45) janjust: ooo Selva's in Canada.. I always wondered
(21:51:11) selvanair: janjust: logged in from someone else's server.. but
canada part is right
(21:52:00) janjust: and hi selvanair , of course
(21:52:24) mattock: hi selvanair!
(21:52:28) selvanair: hi guys, looks like I missed the meeting..
(21:52:39) mattock: not really, we still have stuff to discuss
(21:52:52) janjust: and cron2's busy making xmas cookies
(21:53:01) mattock: anyways, cron2 could hide quite a few "remote some rarely
used options" in midst of "great reformatting"
(21:53:03) mattock: :P
(21:53:06) mattock: remove
(21:53:19) janjust: hehe
(21:53:23) dazo: so ... how do we end up with a decision on astyle vs
uncrustify?
(21:53:35) selvanair: sounds good -- so the Grand Reformattng is on!
(21:53:47) janjust: because we were discussing the procedure and which tool to
use with what options
(21:53:55) dazo: One part would be that we should have as few clean-ups after
running the first automated clean-up
(21:54:22) cron2: dazo: since you brought up astyle, you get the job of testing
it vs. tun.c to see which options are needed to produce a conforming *and*
useful output :-)
(21:54:52) cron2: tun.c has functions with excessive long argument lists,
excessive #ifdef orgys, excessively long variable names and declaration lists,
and all that...
(21:55:03) dazo: fair enough! that's a good test base
(21:55:28) ***syzzer was just playing a bit with astyle
(21:55:47) cron2: my uncrustify options in the wiki page seem to work, at least
I did not spot anything totally broken anymore. Ident insisted on reformatting
stuff that ended up being just ugly afterwards
(21:56:02) cron2: as a side note, my uncrustify suggested options *keep* the
old function declaration style
(21:56:05) cron2: static void
(21:56:11) cron2: foo(int bar);
(21:56:13) cron2: instead of going for
(21:56:20) cron2: static void foo(int bar)
(21:56:33) cron2: because - especially in tun.c - that caused massive ugliness
(21:56:46) cron2: (remove the ";" up there, declaration, not prototyle)
(21:56:51) syzzer: I usually do prefer the one-line approach, but I can also
see how that becomes quite ugly with long names...
(21:57:29) selvanair: Looking at the "style manual": isn't it better to break
lines before operators rather than after it (the so-calle Knuth style becuase
of the way displayed equations are formatted) -- ignore if its too late to
discuss style..
(21:57:54) syzzer: selvanair: no it's not too late
(21:57:57) cron2: init_tun() becomes REALLY ugly if you shift everything to the
right by 15 characters
(21:58:10) syzzer: I just wrote up some stuff, and it's most definitely open
for discussion
(21:58:20) selvanair: good to know
(21:58:48) cron2: selvanair: no strong opinion on that. We should decide
something and write it up.
(22:00:07) dazo: I generally prefer all on one line as well, but if it makes
the result unreadable .... readability trumps one line
(22:00:22) selvanair: I think savings = pay + bribe + kickback - rent - beer
looks better when broken with continuing lines started as '+ kickback' than
leaving the + in the prvious line..
(22:01:11) selvanair: dazo: thought we are imposing 80 char per line max..
(22:01:33) dazo: yeah .... I'd prefer 100, tbh
(22:01:53) selvanair: me too
(22:01:58) dazo: (who does development on a text terminal fixed at 80 chars
these days?)
(22:02:03) ***cron2
(22:02:16) cron2: lines longer than 80 characters are usually a sign of some
confusion
(22:02:17) dazo: but you don't have to be fixed at 80?
(22:02:18) mattock: is the terminal green?
(22:02:23) dazo: lol
(22:02:25) selvanair: Not even me who runs vi as init :)
(22:02:27) syzzer: dazo: well, I like the linux-argument - too much
indentation? just add functions.
(22:02:28) janjust: you've still got a teletype terminal, cron2?
(22:02:31) cron2: mattock: you know how my terminals look like :)
(22:02:43) cron2: janjust: I have an original VT220 in my basement, yes.
(22:02:54) dazo: syzzer: I agree ... but they even have tab-with 8
(22:02:59) janjust: no I meant teletype: the one where you type a line and it
is printed on paper instead of on screen
(22:03:14) janjust: the original VT220 was/is awesome :)
(22:03:15) cron2: never had one of those... I'm one of the young ones!
(22:03:21) dazo: :-P
(22:03:22) syzzer: dazo: so with our indent with of 4, we are quite mild ;)
(22:03:45) ***janjust is old but not gray/grey
(22:03:50) mattock: so do we have cases where lines go way past 80 characters?
(22:03:50) dazo: yeah, but still we have so much code which do have a bit too
much nesting already ....
(22:04:09) syzzer: mostly function call with lots of arguments, or debug strings
(22:04:16) cron2: we do, but everything syzzer has touched has been wrappified
(22:04:22) dazo: I have a few patches recently which I had to reformat ....
they went up to 90-ish
(22:05:10) ***syzzer has been slapping people with the 80 char limit, hehe
(22:05:27) mattock: so would a limit of 100 characters simplify the Great
Reformatting significantly?
(22:05:43) syzzer: hm, I don't think so actually
(22:05:55) cron2: no... most of it is braces, proper indentation, and breaking
150+ lines
(22:06:41) syzzer: (I do like Selva's suggestion wrt the Knuth style btw)
(22:06:49) cron2: write it up :)
(22:07:01) syzzer: selvanair: will you do the honours?
(22:07:08) selvanair: syzzer: I edited the code-style page -- please revert if
you dont like it
(22:07:13) cron2: that was quick
(22:07:26) syzzer: hehe, amazing
(22:07:26) selvanair: he he -- a bot working in parallel
(22:07:56) mattock: do we have git pre-push hooks or something that warn about
wrong style (e.g. tabs)?
(22:08:27) syzzer: selvanair: works for me!
(22:08:28) cron2: not yet
(22:08:50) syzzer: does github support hooks?
(22:09:16) syzzer: or you want local hooks?
(22:09:20) dazo: the pre-push stuff needs to be installed locally ... and can't
easily be done "automatically" upon git clone
(22:09:40) cron2: I would need local hooks, otherwise it's too late if github
refuses something while gitlab and sf take it
(22:09:43) dazo: otherwise we need something on the git remotes
(22:09:51) dazo: yes
(22:10:21) syzzer: ah, ok, mostly for the core devs then
(22:10:39) dazo: yeah
(22:10:42) ***syzzer is admin at work, and likes writing server-side hooks to
slap colleagues ;)
(22:11:03) dazo: should we have padding around operators?
(22:11:10) dazo: a=bar((b-c)*a,d--);
(22:11:11) dazo: vs
(22:11:15) dazo: a = bar((b - c) * a, d--);
(22:11:37) syzzer: after a , for sure
(22:11:49) dazo: agreed
(22:11:55) cron2: "sort of, but not enforced" - for long operands, I like
spaces, but for "b+3" I find it hard to read
(22:12:31) cron2: around " = ", yes, in all cases
(22:12:35) dazo: if we want to have some kind of automated stuff, we need to
have some consistency
(22:12:40) cron2: after a ", " - in most cases, maybe even "all cases"
(22:12:57) selvanair: and around " > "
(22:13:00) cron2: dazo: uncrustify can be told to leave things alone
(22:13:10) syzzer: yeah, this is also one case where I'd say "it depends"
(22:13:22) cron2: "add, remove, ignore, warn" is typically the value you can
set an "insert_space_for_xyz" to
(22:13:45) dazo: well, "it depends" is quite vague for a styling tool :)
(22:14:07) syzzer: if it is one or the other, I'd choose spaces
(22:14:34) dazo: we can also do "don't touch, let it be"
(22:14:35) syzzer: not even sure if I prefer 'spaces' or 'do not touch'
(22:14:47) cron2: I find the example *with* spaces not exactly clear either...
and might want to write a = bar( (b-c) * a, d-- ) instead
(22:15:21) cron2: so I'd go for "we document our preferences in the CodingStyle
page, and tell the tool to leave it alone"
(22:15:30) cron2: so, next round of cookie massacre...
(22:15:31) syzzer: fine bt me
(22:15:57) mattock: done with code style?
(22:16:05) dazo: char &foo1;
(22:16:09) syzzer: mattock: never!
(22:16:10) dazo: char& foo1;
(22:16:16) syzzer: char &foo
(22:16:22) syzzer: just like char *foo
(22:16:24) dazo: agreed :)
(22:16:29) janjust: char& foo1 reminds me of c++ too much
(22:17:03) cron2: +1
(22:17:12) cron2: (cookie massacre needs more cooling down first)
(22:17:30) ***janjust wants a cookie
(22:18:07) cron2: no 3D scanner here yet :(
(22:19:56) syzzer: example updated with "const char *"
(22:20:38) cron2: no blank before the opening bracket in functions?
(22:20:50) cron2: something_equals(dummy) vs. something_equals (dummy)
(22:21:00) dazo: Heres another one ....
(22:21:01) dazo: --------------
(22:21:02) syzzer: I'd say not
(22:21:02) janjust: or something_equals( dummy )
(22:21:03) dazo: isFoo = true;
(22:21:04) dazo: if (isFoo) {
(22:21:04) dazo: bar();
(22:21:04) dazo: } else {
(22:21:04) dazo: anotherBar();
(22:21:05) dazo: }
(22:21:07) dazo: isBar = false;
(22:21:09) dazo: ------------------
(22:21:11) dazo: vs
(22:21:13) dazo: ---------------
(22:21:20) dazo: isFoo = true;
(22:21:25) dazo: // new line
(22:21:26) cron2: dazo: that's easy: braces get their own line, always
(22:21:30) dazo: if (isFoo) {
(22:21:30) dazo: bar();
(22:21:30) dazo: } else {
(22:21:30) dazo: anotherBar();
(22:21:31) dazo: }
(22:21:32) cron2: and no C++ comments
(22:21:35) dazo: // new line
(22:21:35) mattock: (and it goes on) :P
(22:21:38) dazo: isBar = false;
(22:21:41) dazo: ---------------
(22:21:57) cron2: covered by the myfunction() example :)
(22:21:58) dazo: so should it be compact, or have a blank line before/after an
if() block?
(22:22:20) dazo: so compact
(22:22:21) janjust: depends on the context/code,dazo
(22:22:22) cron2: your if() block is not right
(22:22:29) cron2: :)
(22:22:33) syzzer: yeah, compact
(22:22:33) janjust: mostly compact , but sometimes a newline "just makes sense"
(22:22:37) dazo: well ... irc messed it up
(22:22:41) syzzer: exactly, like prose ;)
(22:22:52) cron2: no, you use the } else { thing we already decided against
(22:23:06) selvanair: no blank after functions is ok, but after if its needed
isn't it: if(x) is ugly -- I'd prefer use "if (x)"
(22:23:12) dazo: ahh, ignore that ... that was not the crux here :)
(22:23:12) cron2: and "what janjust says", this is a leave-alone thing
(22:23:35) cron2: if ( and for ( etc. needs the blank, yes
(22:23:52) syzzer: selvanair++
(22:24:01) cron2: very short if (...) blocks do not warrant an empty line, more
complex stuff does
(22:24:04) syzzer: (as does the example)
(22:24:12) dazo: https://paste.fedoraproject.org/501248/14811422/
(22:24:27) dazo: tun.c ... with astyle --style=allman --indent=spaces=4
--convert-tabs --max-code-length=80 --align-reference=name --lineend=linux
--indent-switches -j
(22:24:54) cron2: we do not want --convert-tabs
(22:25:12) syzzer: cron2: we don't?
(22:25:19) dazo: oh yes we do
(22:25:32) dazo: we don't want a mixture of tab/spaces when we've decided for
spaces only
(22:25:39) cron2: I'm fairly sure we agreed that we keep tabs in front of the
lines, with tabstop=8
(22:25:50) dazo: nope, only spaces is what I recall
(22:25:59) cron2: you recall wishful thinking :-)
(22:26:14) syzzer: I recall that too
(22:26:18) janjust: mmm looks pretty good, dazo, but what about function
declarations with multiple parms? check out check_subnet_conflict vs
ifconfig_options_string
(22:26:26) dazo: tabs vs. spaces: "not mixed"
(22:26:27) dazo: majority for "only spaces, no tabs"
(22:26:27) dazo: this is what we change to: all spaces
(22:26:31) dazo: http://community.openvpn.net/openvpn/wiki/MunichHackathon2014
(22:26:32) vpnHelper: Title: MunichHackathon2014 – OpenVPN Community (at
community.openvpn.net)
(22:26:38) selvanair: me too -- spaces only please
(22:26:45) janjust: +1
(22:26:45) mattock: damn, he found documentation
(22:26:46) janjust: +
(22:26:49) janjust: +1
(22:26:49) selvanair: ++1
(22:26:54) janjust: 1++
(22:26:56) selvanair: ++2
(22:27:04) syzzer: -
(22:27:06) cron2: I was looking at the page, but couldn't find the paragraph
yet, so, well, since we've agreed...
(22:27:29) cron2: btw, it has some say about line length...
(22:27:55) dazo: right ... so syzzer tricked us to 80 charsd!
(22:28:05) dazo: s/sd$/s/
(22:28:12) syzzer: well, "what we have now" is 80, just not enforced :p
(22:28:20) dazo: hehe :)
(22:28:32) cron2: looking at how we spent the last 60 minutes, I think we
should focus on "reindent, tabs-to-spaces, add braces to single-line if/else
terms, and do not bikeshed the conversion to death"
(22:28:42) janjust: Repeat: what about function declarations with multiple
parms? check out check_subnet_conflict vs ifconfig_options_string
(22:28:47) mattock: "syzzer voluntereers to make the non-consistent files
consistent "
(22:29:02) syzzer: oh, damn :')
(22:29:22) dazo: #if 0 /* too many false positives */
(22:29:24) mattock: +1 on no more bikeshedding
(22:29:25) cron2: janjust: I'd go for "wrap depending on total length and try
to make it non-ugly"
(22:29:40) janjust: hehe cron2, and how do we teach the tool to do that?
(22:29:42) syzzer: cron2++
(22:29:48) cron2: janjust: we don't
(22:30:06) dazo: my pastebin ... tries to do some 80 chars limitations
(22:30:06) cron2: "every option gets its own line" can be about as ugly as "all
crammed in a single large line"
(22:30:20) cron2: half the annoyance of tun.c is argv_...() calls with "every
argument on a single line"
(22:30:24) dazo: ack ... that's a case-by-case too
(22:30:40) selvanair: As for a conversion of exisitng code, a reasonable
output is good enough -- we can impose style more strongly for new code.
(22:30:41) dazo: (but astyle shouldn't touch that with my args)
(22:30:42) cron2: and then 3 of those, 20 lines of code for 3 function calls
and 2 if()s
(22:30:50) cron2: selvanair: +1
(22:31:31) cron2: dazo: the indentation of the function prototypes looks ugly -
they should be aligned with the first argument, not "with something arbitrary",
no?
(22:31:41) cron2: (line 66-73)
(22:31:42) dazo: which line?
(22:31:56) dazo: it's aligned isn't it?
(22:32:11) selvanair: Thngs like msg(M_INFO, "loong text....") is going to be
hard to get automatically wrapped into 80 cahrs, but that's ok isn't it?
(22:32:18) dazo: oh you mean the argument names?
(22:32:20) cron2: well, in the pastebin, it's not, but that might be variable
font length
(22:32:45) cron2: I see the first set as aligned to the "i" of
"netsh_ifconfig", and the second set to the "d" of "netsh_set_dns6_servers",
which is different from each other, and makes no sense
(22:32:55) syzzer: variable font length? verbatim here
(22:33:13) cron2: syzzer: where are the parameters aligned to for you?
(22:33:17) syzzer: yes
(22:33:28) dazo: selvanair: yes, agreed on long string lines
(22:33:29) syzzer: right under the ( +1
(22:33:31) dazo: cron2: http://imgur.com/a/LdQlB
(22:33:32) vpnHelper: Title: Imgur: The most awesome images on the Internet (at
imgur.com)
(22:33:42) cron2: *sigh*
(22:33:50) syzzer: yes, that
(22:33:58) cron2: chrome stinks. firefox shows them nicely aligned
(22:34:07) dazo: :)
(22:34:13) selvanair: lines 66-73 look ok on my browser..
(22:34:21) selvanair: firefox++
(22:34:22) cron2: the internet is so full on broken software
(22:34:29) dazo: lol :)
(22:34:50) cron2: generally speaking, firefox sucks more often than chrome,
though (like in sucking up more memory than EMACS ever did...)
(22:35:35) dazo: that's true .... firefox is getting where netscape once was
... where firefox was a relief and incredibly fast compared to netscape
(22:35:45) cron2: do we have agreement on the indent level for switch/case?
(22:36:05) cron2: I tend to indent only *two* for the "case" labels, and two
more for the code block (so code is indented +4, not +8)?
(22:36:09) cron2: line 273
(22:36:13) syzzer: dazo: I still see ifs without braces int he paste, btw
(22:36:26) dazo: oh ... which line?
(22:36:39) syzzer: 380
(22:36:41) cron2: syzzer: interesting. The ones I saw have been converted
(which are the ones I remember from my own tests)
(22:36:51) dazo: eek
(22:36:56) cron2: indeed
(22:37:13) selvanair: and more at line 986
(22:37:24) dazo: -j should have taken care of that, I though ... need to double
check
(22:37:34) syzzer: re switch case: either 4-4 or 0-4
(22:37:48) syzzer: 2-2 would be confusing I think, given that we do 4 for
indents
(22:38:27) selvanair: 4-4
(22:38:27) dazo: syzzer++
(22:38:39) syzzer: I'd say 4-4 too
(22:38:42) cron2: well, with 2-2, you end up with +4 for the code level, and
the label gets a "relative -2" so is easy to spot
(22:38:54) cron2: 4-4 gives you +8 for the code level, which quickly adds up
(22:39:19) cron2: (interesting enough, uncrustify got that one wrong with my
sample options and gave me 2-4, which looks weird)
(22:39:37) dazo: I don't see an issue with that ... as if that gets too much,
the code should be refactored
(22:39:40) cron2: but anyway, if everyone wants 4-4, so be it
(22:39:51) syzzer: cron2: that will be tricky to teach to editors though...
(22:40:24) cron2: syzzer: what, too lazy to do your indents by hand? You have
no sense for beauty!
(22:40:45) syzzer: cron2: exactly!
(22:41:02) janjust: hmm if this indentation topic is continuing until 23:00
then we're going to see a Godwin ...
(22:41:03) selvanair: +1 for indent by hand and mess it up camp..
(22:41:05) cron2: dazo: well, yes. There's 71876 lines of .c that need
refactoring in src/openvpn/... but that's not a strong argument here an dnow
(22:41:27) cron2: selvanair: of course not "mess it up". Do it properly, with
loving care! :-)
(22:41:46) selvanair: more out of pride than love...
(22:42:09) cron2: anyway, the pastebin from dazo needs fixing the if(), but
otherwise it looks like something far better than today
(22:42:38) cron2: so if it learns to add the missing braces, we're at
"worksforme" there
(22:42:44) syzzer: If we have a consistent code style, we code even consider
adding the vim auto formatting thingies into our c files
(22:42:46) dazo: absolutely ... if we can't get astyle doing if() properly,
then it's most likely uncrustify we'll end up with
(22:43:03) cron2: stuff like line 468 will have to be done manually when the
great tun.c refactoring starts
(22:43:07) mattock: janjust: I suspended a while back already :P
(22:43:24) selvanair: quite impressive actually -- far from the old days of
running indent
(22:43:49) mattock: so topic #3 anyone?
(22:43:58) cron2: dazo: but why is it adding if()s to the stuff in the
beginning?!
(22:44:06) cron2: well, braces to the if()s...
(22:44:14) dazo: I dunno ... trying to figure that out
(22:44:29) cron2: mattock: what selva suggests makes sense :)
(22:44:44) mattock: cron2: on GitHub?
(22:44:54) cron2: iservice needs to be installed if [X] gui is checked, so
maybe make that "[X] GUI and interactive service"
(22:45:13) cron2: mattock: can't remember if that was on the list or on github,
but where the discussion was had
(22:45:47) syzzer: on the list, and I agree
(22:46:06) selvanair: I'm proposing to install iservice always with the core.
Then GUI and TAP and possibly openvpnserv2 are all independent add ons
(22:46:10) syzzer: at least the discussion I saw
(22:46:32) mattock: the GitHub link contains a lot more refactorings
(22:47:12) mattock: we already have a plan, but the question is/was: who large
changes are we willing to accept to the Windows installer beyond 2.4_rc?
(22:47:12) dazo: selvanair++
(22:47:17) cron2: mmmh, yeah - people *might* want to script openvpn with
iservice
(22:47:35) selvanair: cron2: exactly
(22:47:38) cron2: mattock: "anything that makes sense", and "not having
iservice if not installing openvpnserv2" doesn't, so "bugfix"
(22:47:44) selvanair: Also reduces dependencies.
(22:48:00) selvanair: Comamndline install still will have the full enchilada of
options
(22:48:25) dazo: which makes sense ... proper advanced users will use the
command line primarily and not a gui wizard
(22:48:59) selvanair: And be responsible choosing right options
(22:50:08) selvanair: mattock: as long as we are not taking anything out of the
default installation, there are no big changes.
(22:50:10) dazo: yes
(22:51:31) mattock: ok, fine by me
(22:52:01) mattock: we just need to refactor the installer carefully in order
not to introduce bugs in installers that users would not expect to change much
(22:52:19) mattock: fortunately the refactoring should make things quite a bit
simpler
(22:52:34) mattock: anything else for today?
(22:53:10) cron2: timeline for reformatting up to rc2
(22:53:20) cron2: and who does it
(22:53:42) syzzer: well, you or dazo, since you are the ones allowed to push to
master
(22:53:58) syzzer: that is the 'run the script, push to master' bit
(22:54:08) selvanair: mattock: sure: we do not change any sections, but just
hide some sections and make it harder to unselect, always install openvpn,
iservice and dlls -- no point in making dlls un-selectable.
(22:54:22) dazo: mattock: as long as we have the sane defaults _we_ recommend
users to use, and don't add too many whistles and bells (only those options
which are truly needed, not any "nice to have"-option) ... then I think we're
on a good path, and the possbilities to fail are less
(22:54:29) cron2: syzzer: you have point there :-)
(22:54:32) syzzer: I'll make some time to scavenge through files afterwards,
looking for ugliness to fix
(22:54:42) dazo: great
(22:55:38) syzzer: maybe push to a branch first, so people can verify the
'reproducible reformatting'
(22:55:42) dazo: cron2: regarding the if() braces ... for some reason it
currently doesn't add {} to if(), if it is some kind of if() nesting ... not
sure if it's a bug or feature ... looking through options once again
(22:56:00) cron2: ok. dazo said he has some time tonight - so if we could get
a mail to -devel with "this is the tool we intend to use, you have one day to
scream!" by tomorrow that should get us started
(22:56:02) dazo: syzzer: that's my thought as well ... a reformat branch
(22:56:07) mattock: dazo, selvanair: agreed
(22:56:25) mattock: just wanted a consensus on this, because changes in the
Windows installer affect tons of users
(22:56:40) cron2: then "run the script with the options suggested" and push to
"reformat", people ACK that this is really only "what the tool does", and then
merge that
(22:57:02) dazo: mattock: which is why simplicity is key ... most users windows
don't understand lots of these options anyhow
(22:57:15) dazo: cron2++
(22:57:44) syzzer: agreed
(22:58:03) selvanair: mattock: assuming most of those tons of users all install
using defaults, we can make sure they are not affected.
(22:59:25) dazo: selvanair: mattock: we will do some mistakes when reducing
options and providing, in our view, saner options .... so we will most likely
need to run a few installers and tweak the defaults for the GUI install ....
but that shouldn't stop us from trying
(23:00:39) mattock: selvanair: we can be pretty sure 99%+ are using the
defaults - as you pointed out, SecService handles setting up config and log dirs
(23:00:51) mattock: and nobody (afaik) has complained about "where is my log
directory?"
(23:00:58) selvanair: yes, tests are still needed. I test on a couple of
machines before submitting any patches and mattock is also very heavily
invested in testing.. I think we can finalize it by early next week.
(23:01:00) mattock: =nobody checks of the Service section
(23:01:20) mattock: it would be good to get the installer changes into rc2
(23:01:25) mattock: rather than in 2.4.0
(23:02:00) mattock: let's merge what we have available now and apply more
fixes/refactoring on top of that
(23:02:15) selvanair: mattock: first figure out what to do with active-setup (I
think its good now :) and net40. Then we can do this quick.
(23:02:25) mattock: +1
(23:02:25) selvanair: mattock: agreed
(23:02:55) mattock: I will work on this stuff tomorrow and on Friday
(23:03:33) mattock: everything done for today, then?
(23:03:40) mattock: getting rather late here
(23:03:52) selvanair: 4pm here
(23:04:09) janjust: toronto?
(23:04:12) selvanair: yep
(23:04:23) syzzer: I think we're good for today, yes
(23:04:25) janjust: I saw some nice vids about the snow&ice in toronto
(23:04:34) krzee: i liked toronto
(23:04:39) plaisthos: toronto is nice
(23:04:48) janjust: next openvpn meeting is in toronto!
(23:04:53) krzee: woooo
(23:04:58) mattock: that would be something!
(23:05:01) krzee: i can actually make it easier!
(23:05:06) selvanair: hehe -- welcome..
(23:05:16) cron2: did I send the mail?
(23:05:58) janjust: how, krzee ?
(23:06:08) janjust: ow never mind, I understand your remark
(23:06:10) janjust: it IS getting late
(23:06:11) krzee: janjust: much easier for me to take it to toronto
(23:06:22) krzee: it wont make anything easier for anybody else tho :D
(23:06:31) krzee: make it to toronto*
(23:06:42) ***janjust has never been to toronto. Vancouver is nice tho
(23:07:06) ***syzzer neither, but would like to :)
(23:07:13) janjust: we can all get on the OpenVPN Inc corporate jet and fly
straight to Toronto
(23:07:23) mattock: I'm sure :P
(23:07:27) cron2: I'm in!
(23:07:39) plaisthos: :)
(23:08:02) mattock: and here ends the official meeting chatlog :)
------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
