wiki:ACKSystemReview

Version 16 (modified by Samuli Seppänen, 8 years ago) (diff)

--

Introduction

The purpose of this analysis is to estimate the usefulness of the ACK/NACK system as used in OpenVPN project's development. This study goes back two years, and the data is obtained from Git logs on a Git clone taken on 5th April 2012:

$ git log --since="2 years ago"

This data is cross-referenced with the emails stored in the openvpn-devel mailing list archive. As reviewing all patches and ACKs is not possible, a random sample is taken.

The hypothesis is that the ACK/NACK process is most suitable for reviewing individual patches or small patchsets sent to the openvpn-devel mailing list by developers not intimately familiar with OpenVPN. A less bureaucratic process might be more suitable for core developers, especially when less security-sensitive parts are being modified.

Current ACK/NACK code review system

Scope

Most patches have required an ACK before going into Git "master". There are a only few exceptions:

Evolution

The original ACK system required an ACK from one developer for the code to enter the Git repository. This process was later (add date?) split into two logical parts:

  • Feature-ACK: this should be the primary consideration when reviewing patches. Merging useless patches makes no sense whatsoever.
  • Code-ACK: if the feature makes sense, the next step would be to review that the code itself makes sense (e.g. has no obvious errors and follows established conventions)

Still later (add date?) a move towards lazy consensus was taken. So, as it stands now (Mar 2012), code can go to the Git repository without an ACK, but with a considerable delay.

Pros

The formal ACK/NACK system was imposed to increase OpenVPN code quality by decreasing the chances of bad code getting into the tree. The mandatory ACK forces peer review. In software engineering jargon, it fulfills the roles of

Limitations

There are limits to what the ACK process can do. For example:

  • Runtime errors may be very difficult to notice
  • Seeing the "big picture" may be difficult, especially with big patchsets
  • Whether a particular problems is noticed depends highly on the reviewer's skills

Cons

The ACK/NACK process has it's cons:

  • Requiring an ACK slows down flow of code from contributors to the "master" development tree. This problem gets worse as the patchset size increases.
  • Due to the above, it takes longer to get the code into wide circulation. This means bugs are reported later than if the code had been merged as soon as it's ready.

Raw data

This raw data is based on Git clone done on 5th Apr 2012:

Generic statistics

  • Commit count: 635
  • ACK number: 458
  • Average ACK count: .72

ACKs by person

  • Adriaan de Jong 20
  • Alon Bar-Lev 3
  • Davide Guerri 1
  • David Sommerseth 155
  • Eric F Crist 1
  • Gert Doering 107
  • Gilles Espinasse 1
  • Heiko Hund 3
  • James Yonan 116
  • Jan Just Keijser 2
  • Kazuyoshi Aizawa 2
  • krzee 3

Effects of the ACK system on patches

The following Git command was used to generate a list of all patches in Trac format:

$ git log --date=short --since="2 years ago" --pretty=format:"||%ci||%ad||%h||%s||-||%an||-||-||-||"

A random sample of 64 patches (out of 634) was then taken using shuf -n 64. A few notes:

  • Delay is calculated as "day when applied to git" - "day when sent to the ml". For example, 26th Mar - 24th Mar = 2 days.
  • Patchset refers to a major patchset with 10 or more patches.
  • Author date is the date the patch was introduced (e.g. sent to the mailing list)
  • Commit date is the date when the patch was commited to Git

Here's the list of patches that were studied:

Commit dateAuthor dateIdentifierPatch namePatchsetAuthorModifiedDelay (total)Delay (after ACK)
2012-03-302012-03-283144411Enable pedantic in windows compilationBuildsystemAlon Bar-LevNo20
2012-03-262012-03-24405f338build: windows: set vendor to openvpn project + cleanups-Alon Bar-LevNo20
2012-03-222012-02-29fbae7d2build: plugins: properly use CC, CFLAGS and LDFLAGS-Alon Bar-LevNo237
2012-03-222012-02-29553d95dcleanup: memcmp.c: remove unused source-Alon Bar-LevNo2316
2012-03-222012-02-2951bd56fbuild: autotools: first pass of trivial autotools changes-Alon Bar-LevNo237
2012-02-202012-02-184ebc587define access mode flag X_OK as 0 on Windows-Heiko HundUnknown2-
2012-03-302012-02-148e5613cMigrated x509_get_sha1_hash to use the garbage collector-Adriaan de JongYes453
2012-01-232012-01-2262c613dPlatform cleanup for FreeBSD-Gert DoeringNo10
2011-12-142011-11-038407991Fixed client issues with DHCP Router option extraction/deletion when using layer 2 with DHCP proxy:-James YonanNo41-
2011-11-212011-10-317ac7170Reordered functions to ensure warning-free Windows build-Adriaan de JongNo2111
2011-11-212011-10-3154628d1Moved prng_uninit out of crypto_uninit_lib-Adriaan de JongNo21?
2011-11-212011-10-311d90851Moved from strsep to strtok, for Windows compatibility-Adriaan de JongNo21?
2011-10-222011-10-16eaacf8dMoved to PolarSSL 1.0.0:-Adriaan de JongNo8?
2011-12-142011-10-11359adbfRaised D_PID_DEBUG_LOW from level 3 to 4 to reduce replay error verbosity at level 3.-James YonanNo34-
2011-09-212011-09-168ca19c0Platform cleanup for NetBSD-Gert DoeringNo50
2011-08-312011-08-31d90428dadd --mark option to set SO_MARK sockopt-Heiko HundNo00
2011-08-242011-08-192627335CC_PRINT character class now allows any 8-bit character value >= 32. This is done to allow UTF-8 and restrict the use of control characters in usernames, passwords, common names, etc.-James YonanNo5-
2012-02-212011-08-1714a382aremove wrapper code for Windows CryptoAPI function-Heiko HundNo40
2011-08-242011-08-16f0257abFor all accesses to "struct route_list 2 3 AUTHORS build, ... ,version.sh.in rl", check first that rl is non-NULL-Gert DoeringUnknown8?
2011-10-222011-07-28c94eff3Added back checks for ks->authenticated in verify_user_pass-Adriaan de JongNo86?
2011-08-192011-07-25576dc96Merge remote branch SVN 2.1 into the git tree-David Sommerseth---
2011-10-222011-07-05a9bf901Added an extra define to allow building without PKCS#11PolarSSLAdriaan de JongNo109?
2011-10-222011-07-05557624eHardening: periodically reset the PRNG's nonce valuePolarSSLAdriaan de JongNo109?
2011-10-222011-07-018c96419Fixed a missing include in ssl_backend.h-Adriaan de Jong--
2011-10-222011-06-303e44ea5Refactored tls-verify script code-Adriaan de Jong--
2011-07-012011-06-29d22a379Made domake-win builds to use easy-rsa/2.0/openssl-1.0.0.cnf-Samuli Seppänen--
2011-10-192011-06-27eab0cf2Refactored TLS_PRF to new hmac and md primitives-Adriaan de Jong--
2011-10-192011-06-23b01cb9eRefactored crypto initialisation functions-Adriaan de Jong--
2011-10-192011-06-234a5a603Refactored NTLM DES key generation-Adriaan de Jong--
2011-06-202011-06-2054c739eRevert "Add new openssl.cnf to easy-rsa/Windows"-David Sommerseth--
2011-11-252011-06-160f2bc0dDo some file/directory tests before really starting openvpn-David Sommerseth--
2011-08-252011-05-26739fa98Fix a Visual Studio 2008 build error in options.c-Samuli Seppanen--
2011-05-272011-05-2621fc2edDon't define ENABLE_PUSH_PEER_INFO if SSL is not available-David Sommerseth--
2011-04-272011-04-27b70d99fFix compile issues when using --enable-small and --disable-ssl/--disable-crypto-Gustavo Zacarias--
2011-04-262011-04-24d549726Added 'dir' flag to "crl-verify" (see man page for info).-James Yonan--
2011-04-262011-04-12e51935dFor Mac OSX, when DARWIN_USE_IPCONFIG is defined ,.. the ipconfig command sometimes fails if executed immediately after the tun device open.-James YonanNo14-
2011-04-122011-04-122a12831For Mac OSX, when DARWIN_USE_IPCONFIG is defined ... the ipconfig command sometimes fails if executed immediately after the tun device open.-James Yonan---
2011-04-142011-04-039ed122eFixed bug in port-share that could cause port share process to crash with output like this:-James YonanNo11-
2011-03-252011-03-25dc2ccc8Clarify --tmp-dir option-chantraNo00
2011-03-212011-03-211df945eVersion 2.1.3n-James Yonan---
2011-03-172011-03-17d02a86dRenamed branch to reflect that it is no longer beta.-James Yonan---
2011-03-212011-03-1558704eaUpdated INSTALL-win32.txt-Samuli SeppänenNo63
2011-03-252011-02-28a75c7ddFixed typo in plugin.h-Stefan HellermannNo25?
2011-02-272011-02-11a7f0fc3Added configure.h and version.m4 variable parsing to win/config.py-Samuli Seppänen?16?
2011-02-272011-02-114e4aa65Several modifications to win/make_dist.py to allow building the NSI installer-Samuli Seppänen?16?
2010-11-182010-11-157581c8fRemoved functions not being used anywhere-David SommersethNo3?
2010-11-182010-11-1533ee747Fixed potential misinterpretation of boolean logic-David SommersethNo3?
2010-11-132010-11-1322178d0Merge branch 'svn-BETA21' into bugfix2.1-David Sommerseth---
2010-11-122010-10-30f0eac1aMake "topology subnet" work on Solaris (ifconfig + route metric changes by Kazuyoshi Aizawa, adding of local "connected subnet" route by me)-Gert DoeringNo12?
2010-10-212010-10-2159afc4aFix problem with special case route targets ('remote_host')-Gert DoeringNo00
2010-11-122010-09-3058f8d94Add HTTP/1.1 Host header-Lars HupelNo4340
2010-07-272010-07-2775dfe3dAdded "net stop dnscache" and "net start dnscache" in front of existing --register-dns commands.-James Yonan--
2010-10-212010-07-164c91fc8Fixes openssl-1.0.0 compilation warning-chantra--
2010-10-212010-04-277d5e26cFix certificate serial number export-Davide Brini--
2010-10-212010-04-22892e64bFix missing /bin/bash -> /bin/sh-Davide Brini--
2010-10-212010-04-16add7fe0Updated the man page to reflect the behavioural change of create_temp_file()-David Sommerseth--
2010-10-212010-03-101e02046On TARGET_LINUX define _GNU_SOURCE if not defined-David Sommerseth--
2011-04-242010-03-0794d50a1when deleting a route on win32, also add gateway address (otherwise netsh.exe will succeed, but silently ignore request)-Gert Doering--
2010-10-212010-03-02d04b858Several updates to openvpn.8 (man page updates)-Karl O. Pinc--
2011-03-252010-02-20a698464* undo mroute.c changes related to ipv6 payload, nothing to do w/ipv6 transport afterall.-JuanJo? Ciarlante--
2010-10-212010-02-19adfe37fverb 5 logging wrongly reports received bytes-David Sommerseth--
2010-10-212010-02-18058f3d0[PATCH] Change verify-cn so cn is no longer hardcoded in openvpn's config file-Karl O. Pinc--
2011-03-252009-10-05e293510* TODO.ipv6 update-JuanJo? Ciarlante--
2011-03-252009-09-24e478770* make possible to x-compile openvpn/win32 in Linux-JuanJo? Ciarlante--

Analysis

Attachments (1)

  • gitlogparse.sh (1.8 KB) - added by Samuli Seppänen 8 years ago. A very ugly script used to generate statistics during ACK system review

Download all attachments as: .zip