Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 1 | HOW TO GET YOUR CODE ACCEPTED IN HAPROXY |
| 2 | READ THIS CAREFULLY BEFORE SUBMITTING CODE |
| 3 | |
| 4 | THIS DOCUMENT PROVIDES SOME RULES TO FOLLOW WHEN SENDING CONTRIBUTIONS. PATCHES |
| 5 | NOT FOLLOWING THESE RULES WILL SIMPLY BE REJECTED IN ORDER TO PROTECT ALL OTHER |
| 6 | RESPECTFUL CONTRIBUTORS' VALUABLE TIME. |
| 7 | |
| 8 | |
| 9 | Background |
| 10 | ---------- |
| 11 | |
| 12 | During the development cycle of version 1.6, much more time was spent reviewing |
| 13 | poor quality submissions, fixing them and troubleshooting the bugs they |
| 14 | introduced than doing any development work. This is not acceptable as it ends |
| 15 | up with people actually slowing down the project for the features they're the |
| 16 | only ones interested in. On the other end of the scale, there are people who |
| 17 | make the effort of polishing their work to contribute excellent quality work |
| 18 | which doesn't even require a review. Contrary to what newcomers may think, it's |
| 19 | very easy to reach that level of quality and get your changes accepted quickly, |
| 20 | even late in the development cycle. It only requires that you make your homework |
| 21 | and not rely on others to do it for you. The most important point is that |
| 22 | HAProxy is a community-driven project, all involved participants must respect |
| 23 | all other ones' time and work. |
| 24 | |
| 25 | |
| 26 | Preparation |
| 27 | ----------- |
| 28 | |
| 29 | It is possible that you'll want to add a specific feature to satisfy your needs |
| 30 | or one of your customers'. Contributions are welcome, however maintainers are |
| 31 | often very picky about changes. Patches that change massive parts of the code, |
| 32 | or that touch the core parts without any good reason will generally be rejected |
| 33 | if those changes have not been discussed first. |
| 34 | |
| 35 | The proper place to discuss your changes is the HAProxy Mailing List. There are |
| 36 | enough skilled readers to catch hazardous mistakes and to suggest improvements. |
| 37 | There is no other place where you'll find as many skilled people on the project, |
| 38 | and these people can help you get your code integrated quickly. You can |
| 39 | subscribe to it by sending an empty e-mail at the following address : |
| 40 | |
| 41 | haproxy+subscribe@formilux.org |
| 42 | |
| 43 | If you have an idea about something to implement, *please* discuss it on the |
| 44 | list first. It has already happened several times that two persons did the same |
| 45 | thing simultaneously. This is a waste of time for both of them. It's also very |
| 46 | common to see some changes rejected because they're done in a way that will |
| 47 | conflict with future evolutions, or that does not leave a good feeling. It's |
| 48 | always unpleasant for the person who did the work, and it is unpleasant in |
| 49 | general because value people's time and efforts are valuable and would be better |
| 50 | spent working on something else. That would not happen if these were discussed |
| 51 | first. There is no problem posting work in progress to the list, it happens |
| 52 | quite often in fact. Also, don't waste your time with the doc when submitting |
| 53 | patches for review, only add the doc with the patch you consider ready to merge. |
| 54 | |
| 55 | Another important point concerns code portability. Haproxy requires gcc as the |
| 56 | C compiler, and may or may not work with other compilers. However it's known to |
| 57 | build using gcc 2.95 or any later version. As such, it is important to keep in |
| 58 | mind that certain facilities offered by recent versions must not be used in the |
| 59 | code : |
| 60 | |
| 61 | - declarations mixed in the code (requires gcc >= 3.x and is a bad practice) |
| 62 | - GCC builtins without checking for their availability based on version and |
| 63 | architecture ; |
| 64 | - assembly code without any alternate portable form for other platforms |
| 65 | - use of stdbool.h, "bool", "false", "true" : simply use "int", "0", "1" |
| 66 | - in general, anything which requires C99 (such as declaring variables in |
| 67 | "for" statements) |
| 68 | |
| 69 | Since most of these restrictions are just a matter of coding style, it is |
| 70 | normally not a problem to comply. |
| 71 | |
Willy Tarreau | 9d84cd6 | 2017-07-18 06:56:40 +0200 | [diff] [blame] | 72 | When modifying some optional subsystem (SSL, Lua, compression, device detection |
| 73 | engines), please make sure the code continues to build (and to work) when these |
| 74 | features are disabled. Similarly, when modifying the SSL stack, please always |
| 75 | ensure that supported OpenSSL versions continue to build and to work, especially |
| 76 | if you modify support for alternate libraries. Clean support for the legacy |
| 77 | OpenSSL libraries is mandatory, support for its derivatives is a bonus and may |
| 78 | occasionally break eventhough a great care is taken. In other words, if you |
| 79 | provide a patch for OpenSSL you don't need to test its derivatives, but if you |
| 80 | provide a patch for a derivative you also need to test with OpenSSL. |
| 81 | |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 82 | If your work is very confidential and you can't publicly discuss it, you can |
| 83 | also mail willy@haproxy.org directly about it, but your mail may be waiting |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 84 | several days in the queue before you get a response, if you get a response at |
| 85 | all. Retransmit if you don't get a response by one week. Please note that |
| 86 | direct sent e-mails to this address for non-confidential subjects may simply |
| 87 | be forwarded to the list or be deleted without notification. |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 88 | |
| 89 | If you'd like a feature to be added but you think you don't have the skills to |
| 90 | implement it yourself, you should follow these steps : |
| 91 | |
| 92 | 1. discuss the feature on the mailing list. It is possible that someone |
| 93 | else has already implemented it, or that someone will tell you how to |
| 94 | proceed without it, or even why not to do it. It is also possible that |
| 95 | in fact it's quite easy to implement and people will guide you through |
| 96 | the process. That way you'll finally have YOUR patch merged, providing |
| 97 | the feature YOU need. |
| 98 | |
| 99 | 2. if you really can't code it yourself after discussing it, then you may |
| 100 | consider contacting someone to do the job for you. Some people on the |
| 101 | list might sometimes be OK with trying to do it. |
| 102 | |
| 103 | |
| 104 | Rules : the 12 laws of patch contribution |
| 105 | ----------------------------------------- |
| 106 | |
| 107 | People contributing patches must apply the following rules. That may sound heavy |
| 108 | at the beginning but it's common sense more than anything else and contributors |
| 109 | do not think about them anymore after a few patches. |
| 110 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 111 | 1) Comply with the license |
| 112 | |
| 113 | Before modifying some code, you have read the LICENSE file ("main license") |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 114 | coming with the sources, and all the files this file references. Certain |
| 115 | files may be covered by different licenses, in which case it will be |
| 116 | indicated in the files themselves. In any case, you agree to respect these |
| 117 | licenses and to contribute your changes under the same licenses. If you want |
| 118 | to create new files, they will be under the main license, or any license of |
| 119 | your choice that you have verified to be compatible with the main license, |
Tim Düsterhus | 4896c44 | 2016-11-29 02:15:19 +0100 | [diff] [blame] | 120 | and that will be explicitly mentioned in the affected files. The project's |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 121 | maintainers are free to reject contributions proposing license changes they |
| 122 | feel are not appropriate or could cause future trouble. |
| 123 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 124 | 2) Develop on development branch, not stable ones |
| 125 | |
| 126 | Your work may only be based on the latest development version. No development |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 127 | is made on a stable branch. If your work needs to be applied to a stable |
| 128 | branch, it will first be applied to the development branch and only then will |
| 129 | be backported to the stable branch. You are responsible for ensuring that |
| 130 | your work correctly applies to the development version. If at any moment you |
| 131 | are going to work on restructuring something important which may impact other |
| 132 | contributors, the rule that applies is that the first sent is the first |
| 133 | served. However it is considered good practice and politeness to warn others |
| 134 | in advance if you know you're going to make changes that may force them to |
| 135 | re-adapt their code, because they did probably not expect to have to spend |
| 136 | more time discovering your changes and rebasing their work. |
| 137 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 138 | 3) Read and respect the coding style |
| 139 | |
| 140 | You have read and understood "doc/coding-style.txt", and you're actively |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 141 | determined to respect it and to enforce it on your coworkers if you're going |
| 142 | to submit a team's work. We don't care what text editor you use, whether it's |
| 143 | an hex editor, cat, vi, emacs, Notepad, Word, or even Eclipse. The editor is |
| 144 | only the interface between you and the text file. What matters is what is in |
| 145 | the text file in the end. The editor is not an excuse for submitting poorly |
| 146 | indented code, which only proves that the person has no consideration for |
| 147 | quality and/or has done it in a hurry (probably worse). Please note that most |
| 148 | bugs were found in low-quality code. Reviewers know this and tend to be much |
| 149 | more reluctant to accept poorly formated code because by experience they |
| 150 | won't trust their author's ability to write correct code. It is also worth |
| 151 | noting that poor quality code is painful to read and may result in nobody |
| 152 | willing to waste their time even reviewing your work. |
| 153 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 154 | 4) Present clean work |
| 155 | |
| 156 | The time it takes for you to polish your code is always much smaller than the |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 157 | time it takes others to do it for you, because they always have to wonder if |
| 158 | what they see is intended (meaning they didn't understand something) or if it |
| 159 | is a mistake that needs to be fixed. And since there are less reviewers than |
| 160 | submitters, it is vital to spread the effort closer to where the code is |
| 161 | written and not closer to where it gets merged. For example if you have to |
| 162 | write a report for a customer that your boss wants to review before you send |
| 163 | it to the customer, will you throw on his desk a pile of paper with stains, |
| 164 | typos and copy-pastes everywhere ? Will you say "come on, OK I made a mistake |
| 165 | in the company's name but they will find it by themselves, it's obvious it |
| 166 | comes from us" ? No. When in doubt, simply ask for help on the mailing list. |
| 167 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 168 | 5) Documentation is very important |
| 169 | |
| 170 | There are four levels of importance of quality in the project : |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 171 | |
| 172 | - The most important one, and by far, is the quality of the user-facing |
| 173 | documentation. This is the first contact for most users and it immediately |
| 174 | gives them an accurate idea of how the project is maintained. Dirty docs |
| 175 | necessarily belong to a dirty project. Be careful to the way the text you |
| 176 | add is presented and indented. Be very careful about typos, usual mistakes |
| 177 | such as double consonants when only one is needed or "it's" instead of |
| 178 | "its", don't mix US english and UK english in the same paragraph, etc. |
| 179 | When in doubt, check in a dictionary. Fixes for existing typos in the doc |
| 180 | are always welcome and chasing them is a good way to become familiar with |
| 181 | the project and to get other participants' respect and consideration. |
| 182 | |
| 183 | - The second most important level is user-facing messages emitted by the |
| 184 | code. You must try to see all the messages your code produces to ensure |
| 185 | they are understandable outside of the context where you wrote them, |
| 186 | because the user often doesn't expect them. That's true for warnings, and |
| 187 | that's even more important for errors which prevent the program from |
| 188 | working and which require an immediate and well understood fix in the |
| 189 | configuration. It's much better to say "line 35: compression level must be |
| 190 | an integer between 1 and 9" than "invalid argument at line 35". In HAProxy, |
| 191 | error handling roughly represents half of the code, and that's about 3/4 of |
| 192 | the configuration parser. Take the time to do something you're proud of. A |
| 193 | good rule of thumb is to keep in mind that your code talks to a human and |
| 194 | tries to teach him/her how to proceed. It must then speak like a human. |
| 195 | |
| 196 | - The third most important level is the code and its accompanying comments, |
| 197 | including the commit message which is a complement to your code and |
| 198 | comments. It's important for all other contributors that the code is |
| 199 | readable, fluid, understandable and that the commit message describes what |
| 200 | was done, the choices made, the possible alternatives you thought about, |
| 201 | the reason for picking this one and its limits if any. Comments should be |
| 202 | written where it's easy to have a doubt or after some error cases have been |
| 203 | wiped out and you want to explain what possibilities remain. All functions |
| 204 | must have a comment indicating what they take on input and what they |
| 205 | provide on output. Please adjust the comments when you copy-paste a |
| 206 | function or change its prototype, this type of lazy mistake is too common |
| 207 | and very confusing when reading code later to debug an issue. Do not forget |
| 208 | that others will feel really angry at you when they have to dig into your |
| 209 | code for a bug that your code caused and they feel like this code is dirty |
| 210 | or confusing, that the commit message doesn't explain anything useful and |
| 211 | that the patch should never have been accepted in the first place. That |
| 212 | will strongly impact your reputation and will definitely affect your |
| 213 | chances to contribute again! |
| 214 | |
| 215 | - The fourth level of importance is in the technical documentation that you |
| 216 | may want to add with your code. Technical documentation is always welcome |
| 217 | as it helps others make the best use of your work and to go exactly in the |
| 218 | direction you thought about during the design. This is also what reduces |
| 219 | the risk that your design gets changed in the near future due to a misuse |
| 220 | and/or a poor understanding. All such documentation is actually considered |
| 221 | as a bonus. It is more important that this documentation exists than that |
| 222 | it looks clean. Sometimes just copy-pasting your draft notes in a file to |
| 223 | keep a record of design ideas is better than losing them. Please do your |
| 224 | best so that other ones can read your doc. If these docs require a special |
| 225 | tool such as a graphics utility, ensure that the file name makes it |
| 226 | unambiguous how to process it. So there are no rules here for the contents, |
| 227 | except one. Please write the date in your file. Design docs tend to stay |
| 228 | forever and to remain long after they become obsolete. At this point that |
| 229 | can cause harm more than it can help. Writing the date in the document |
| 230 | helps developers guess the degree of validity and/or compare them with the |
| 231 | date of certain commits touching the same area. |
| 232 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 233 | 6) US-ASCII only! |
| 234 | |
| 235 | All text files and commit messages are written using the US-ASCII charset. |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 236 | Please be careful that your contributions do not contain any character not |
| 237 | printable using this charset, as they will render differently in different |
| 238 | editors and/or terminals. Avoid latin1 and more importantly UTF-8 which some |
| 239 | editors tend to abuse to replace some US-ASCII characters with their |
| 240 | typographic equivalent which aren't readable anymore in other editors. The |
| 241 | only place where alternative charsets are tolerated is in your name in the |
| 242 | commit message, but it's at your own risk as it can be mangled during the |
| 243 | merge. Anyway if you have an e-mail address, you probably have a valid |
| 244 | US-ASCII representation for it as well. |
| 245 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 246 | 7) Comments |
| 247 | |
| 248 | Be careful about comments when you move code around. It's not acceptable that |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 249 | a block of code is moved to another place leaving irrelevant comments at the |
| 250 | old place, just like it's not acceptable that a function is duplicated without |
| 251 | the comments being adjusted. The example below started to become quite common |
| 252 | during the 1.6 cycle, it is not acceptable and wastes everyone's time : |
| 253 | |
| 254 | /* Parse switching <str> to build rule <rule>. Returns 0 on error. */ |
| 255 | int parse_switching_rule(const char *str, struct rule *rule) |
| 256 | { |
| 257 | ... |
| 258 | } |
| 259 | |
| 260 | /* Parse switching <str> to build rule <rule>. Returns 0 on error. */ |
| 261 | void execute_switching_rule(struct rule *rule) |
| 262 | { |
| 263 | ... |
| 264 | } |
| 265 | |
| 266 | This patch is not acceptable either (and it's unfortunately not that rare) : |
| 267 | |
| 268 | + if (!session || !arg || list_is_empty(&session->rules->head)) |
| 269 | + return 0; |
| 270 | + |
| 271 | /* Check if session->rules is valid before dereferencing it */ |
| 272 | if (!session->rules_allocated) |
| 273 | return 0; |
| 274 | |
| 275 | - if (!arg || list_is_empty(&session->rules->head)) |
| 276 | - return 0; |
| 277 | - |
| 278 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 279 | 8) Short, readable identifiers |
| 280 | |
| 281 | Limit the length of your identifiers in the code. When your identifiers start |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 282 | to sound like sentences, it's very hard for the reader to keep on track with |
| 283 | what operation they are observing. Also long names force expressions to fit |
| 284 | on several lines which also cause some difficulties to the reader. See the |
| 285 | example below : |
| 286 | |
| 287 | int file_name_len_including_global_path; |
| 288 | int file_name_len_without_global_path; |
| 289 | int global_path_len_or_zero_if_default; |
| 290 | |
| 291 | if (global_path) |
| 292 | global_path_len_or_zero_if_default = strlen(global_path); |
| 293 | else |
| 294 | global_path_len_or_zero_if_default = 0; |
| 295 | |
| 296 | file_name_len_without_global_path = strlen(file_name); |
| 297 | file_name_len_including_global_path = |
| 298 | file_name_len_without_global_path + 1 + /* for '/' */ |
| 299 | global_path_len_or_zero_if_default ? |
| 300 | global_path_len_or_zero_if_default : default_path_len; |
| 301 | |
| 302 | Compare it to this one : |
| 303 | |
| 304 | int f, p; |
| 305 | |
| 306 | p = global_path ? strlen(global_path) : default_path_len; |
| 307 | f = p + 1 + strlen(file_name); /* 1 for '/' */ |
| 308 | |
| 309 | A good rule of thumb is that if your identifiers start to contain more than |
| 310 | 3 words or more than 15 characters, they can become confusing. For function |
| 311 | names it's less important especially if these functions are rarely used or |
| 312 | are used in a complex context where it is important to differenciate between |
| 313 | their multiple variants. |
| 314 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 315 | 9) Unified diff only |
| 316 | |
| 317 | The best way to build your patches is to use "git format-patch". This means |
| 318 | that you have committed your patch to a local branch, with an appropriate |
| 319 | subject line and a useful commit message explaining what the patch attempts |
| 320 | to do. It is not strictly required to use git, but what is strictly required |
| 321 | is to have all these elements in the same mail, easily distinguishible, and |
| 322 | a patch in "diff -up" format (which is also the format used by Git). This |
| 323 | means the "unified" diff format must be used exclusively, and with the |
| 324 | function name printed in the diff header of each block. That significantly |
| 325 | helps during reviews. Keep in mind that most reviews are done on the patch |
| 326 | and not on the code after applying the patch. Your diff must keep some |
| 327 | context (3 lines above and 3 lines below) so that there's no doubt where the |
| 328 | code has to be applied. Don't change code outside of the context of your |
| 329 | patch (eg: take care of not adding/removing empty lines once you remove |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 330 | your debugging code). If you are using Git (which is strongly recommended), |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 331 | always use "git show" after doing a commit to ensure it looks good, and |
| 332 | enable syntax coloring that will automatically report in red the trailing |
| 333 | spaces or tabs that your patch added to the code and that must absolutely be |
| 334 | removed. These ones cause a real pain to apply patches later because they |
| 335 | mangle the context in an invisible way. Such patches with trailing spaces at |
| 336 | end of lines will be rejected. |
| 337 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 338 | 10) One patch per feature |
| 339 | |
| 340 | Please cut your work in series of patches that can be independently reviewed |
| 341 | and merged. Each patch must do something on its own that you can explain to |
| 342 | someone without being ashamed of what you did. For example, you must not say |
| 343 | "This is the patch that implements SSL, it was tricky". There's clearly |
| 344 | something wrong there, your patch will be huge, will definitely break things |
| 345 | and nobody will be able to figure what exactly introduced the bug. However |
| 346 | it's much better to say "I needed to add some fields in the session to store |
| 347 | the SSL context so this patch does this and doesn't touch anything else, so |
| 348 | it's safe". Also when dealing with series, you will sometimes fix a bug that |
| 349 | one of your patches introduced. Please do merge these fixes (eg: using git |
| 350 | rebase -i and squash or fixup), as it is not acceptable to see patches which |
| 351 | introduce known bugs even if they're fixed later. Another benefit of cleanly |
| 352 | splitting patches is that if some of your patches need to be reworked after |
| 353 | a review, the other ones can still be merged so that you don't need to care |
| 354 | about them anymore. When sending multiple patches for review, prefer to send |
| 355 | one e-mail per patch than all patches in a single e-mail. The reason is that |
| 356 | not everyone is skilled in all areas nor has the time to review everything |
| 357 | at once. With one patch per e-mail, it's easy to comment on a single patch |
| 358 | without giving an opinion on the other ones, especially if a long thread |
| 359 | starts about one specific patch on the mailing list. "git send-email" does |
| 360 | that for you though it requires a few trials before getting it right. |
| 361 | |
| 362 | If you can, please always put all the bug fixes at the beginning of the |
| 363 | series. This often makes it easier to backport them because they will not |
| 364 | depend on context that your other patches changed. |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 365 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 366 | 11) Real commit messages please! |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 367 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 368 | Please properly format your commit messages. To get an idea, just run |
| 369 | "git log" on the file you've just modified. Patches always have the format |
| 370 | of an e-mail made of a subject, a description and the actual patch. If you |
| 371 | are sending a patch as an e-mail formatted this way, it can quickly be |
| 372 | applied with limited effort so that's acceptable : |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 373 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 374 | - A subject line (may wrap to the next line, but please read below) |
| 375 | - an empty line (subject delimiter) |
| 376 | - a non-empty description (the body of the e-mail) |
| 377 | - the patch itself |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 378 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 379 | The subject describes the "What" of the change ; the description explains |
| 380 | the "why", the "how" and sometimes "what next". For example a commit message |
| 381 | looking like this will be rejected : |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 382 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 383 | | From: Mr Foobar <foobar@example.com> |
| 384 | | Subject: BUG: fix typo in ssl_sock |
| 385 | | |
| 386 | |
| 387 | This one as well (too long subject, not the right place for the details) : |
| 388 | |
| 389 | | From: Mr Foobar <foobar@example.com> |
| 390 | | Subject: BUG/MEDIUM: ssl: use an error flag to prevent ssl_read() from |
| 391 | | returning 0 when dealing with large buffers because that can cause |
| 392 | | an infinite loop |
| 393 | | |
| 394 | |
| 395 | This one ought to be used instead : |
| 396 | |
| 397 | | From: Mr Foobar <foobar@example.com> |
| 398 | | Subject: BUG/MEDIUM: ssl: fix risk of infinite loop in ssl_sock |
| 399 | | |
| 400 | | ssl_read() must not return 0 on error or the caller may loop forever. |
| 401 | | Instead we add a flag to the connection to notify about the error and |
| 402 | | check it at all call places. This situation can only happen with large |
| 403 | | buffers so a workaround is to limit buffer sizes. Another option would |
| 404 | | have been to return -1 but it required to use signed ints everywhere |
| 405 | | and would have made the patch larger and riskier. This fix should be |
| 406 | | backported to versions 1.2 and upper. |
| 407 | |
| 408 | It is important to understand that for any reader to guess the text above |
| 409 | when it's absent, it will take a huge amount of time. If you made the |
| 410 | analysis leading to your patch, you must explain it, including the ideas |
| 411 | you dropped if you had a good reason for this. |
| 412 | |
| 413 | While it's not strictly required to use Git, it is strongly recommended |
| 414 | because it helps you do the cleanest job with the least effort. But if you |
| 415 | are comfortable with writing clean e-mails and inserting your patches, you |
| 416 | don't need to use Git. |
| 417 | |
| 418 | But in any case, it is important that there is a clean description of what |
| 419 | the patch does, the motivation for what it does, why it's the best way to do |
| 420 | it, its impacts, and what it does not yet cover. Also, in HAProxy, like many |
| 421 | projects which take a great care of maintaining stable branches, patches are |
| 422 | reviewed later so that some of them can be backported to stable releases. |
| 423 | |
| 424 | While reviewing hundreds of patches can seem cumbersome, with a proper |
| 425 | formatting of the subject line it actually becomes very easy. For example, |
| 426 | here's how one can find patches that need to be reviewed for backports (bugs |
| 427 | and doc) between since commit ID 827752e : |
| 428 | |
| 429 | $ git log --oneline 827752e.. | grep 'BUG\|DOC' |
| 430 | 0d79cf6 DOC: fix function name |
| 431 | bc96534 DOC: ssl: missing LF |
| 432 | 10ec214 BUG/MEDIUM: lua: the lua fucntion Channel:close() causes a segf |
| 433 | bdc97a8 BUG/MEDIUM: lua: outgoing connection was broken since 1.6-dev2 |
| 434 | ba56d9c DOC: mention support for RFC 5077 TLS Ticket extension in start |
| 435 | f1650a8 DOC: clarify some points about SSL and the proxy protocol |
| 436 | b157d73 BUG/MAJOR: peers: fix current table pointer not re-initialized |
| 437 | e1ab808 BUG/MEDIUM: peers: fix wrong message id on stick table updates |
| 438 | cc79b00 BUG/MINOR: ssl: TLS Ticket Key rotation broken via socket comma |
| 439 | d8e42b6 DOC: add new file intro.txt |
| 440 | c7d7607 BUG/MEDIUM: lua: bad error processing |
| 441 | 386a127 DOC: match several lua configuration option names to those impl |
| 442 | 0f4eadd BUG/MEDIUM: counters: ensure that src_{inc,clr}_gpc0 creates a |
| 443 | |
| 444 | It is made possible by the fact that subject lines are properly formatted and |
| 445 | always respect the same principle : one part indicating the nature and |
| 446 | severity of the patch, another one to indicate which subsystem is affected, |
| 447 | and the last one is a succinct description of the change, with the important |
| 448 | part at the beginning so that it's obvious what it does even when lines are |
| 449 | truncated like above. The whole stable maintenance process relies on this. |
| 450 | For this reason, it is mandatory to respect some easy rules regarding the |
| 451 | way the subject is built. Please see the section below for more information |
| 452 | regarding this formatting. |
| 453 | |
Willy Tarreau | 9d84cd6 | 2017-07-18 06:56:40 +0200 | [diff] [blame] | 454 | As a rule of thumb, your patch MUST NEVER be made only of a subject line, |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 455 | it *must* contain a description. Even one or two lines, or indicating |
| 456 | whether a backport is desired or not. It turns out that single-line commits |
| 457 | are so rare in the Git world that they require special manual (hence |
| 458 | painful) handling when they are backported, and at least for this reason |
| 459 | it's important to keep this in mind. |
| 460 | |
Willy Tarreau | 9d84cd6 | 2017-07-18 06:56:40 +0200 | [diff] [blame] | 461 | Each patch fixing a bug MUST be tagged with "BUG", a severity level, an |
| 462 | indication of the affected subsystem and a brief description of the nature |
| 463 | of the issue in the subject line, and a detailed analysis in the message |
| 464 | body. The explanation of the user-visible impact and the need for |
| 465 | backporting to stable branches or not are MANDATORY. Bug fixes with no |
| 466 | indication will simply be rejected as they are very likely to cause more |
| 467 | harm when nobody is able to tell whether or not the patch needs to be |
| 468 | backported or can be reverted in case of regression. |
| 469 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 470 | 12) Discuss on the mailing list |
| 471 | |
| 472 | When submitting changes, please always CC the mailing list address so that |
| 473 | everyone gets a chance to spot any issue in your code. It will also serve |
| 474 | as an advertisement for your work, you'll get more testers quicker and |
| 475 | you'll feel better knowing that people really use your work. It's often |
| 476 | convenient to prepend "[PATCH]" in front of your mail's subject to mention |
| 477 | that this e-mail contains a patch (or a series of patches), because it will |
| 478 | easily catch reviewer's attention. It's automatically done by tools such as |
| 479 | "git format-patch" and "git send-email". If you don't want your patch to be |
| 480 | merged yet and prefer to show it for discussion, better tag it as "[RFC]" |
| 481 | (stands for "Request For Comments") and it will be reviewed but not merged |
| 482 | without your approval. It is also important to CC any author mentioned in |
| 483 | the file you change, or a subsystem maintainers whose address is mentioned |
| 484 | in a MAINTAINERS file. Not everyone reads the list on a daily basis so it's |
| 485 | very easy to miss some changes. Don't consider it as a failure when a |
| 486 | reviewer tells you you have to modify your patch, actually it's a success |
| 487 | because now you know what is missing for your work to get accepted. That's |
| 488 | why you should not hesitate to CC enough people. Don't copy people who have |
| 489 | no deal with your work area just because you found their address on the |
| 490 | list. That's the best way to appear careless about their time and make them |
| 491 | reject your changes in the future. |
| 492 | |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 493 | |
| 494 | Patch classifying rules |
| 495 | ----------------------- |
| 496 | |
| 497 | There are 3 criteria of particular importance in any patch : |
| 498 | - its nature (is it a fix for a bug, a new feature, an optimization, ...) |
| 499 | - its importance, which generally reflects the risk of merging/not merging it |
| 500 | - what area it applies to (eg: http, stats, startup, config, doc, ...) |
| 501 | |
| 502 | It's important to make these 3 criteria easy to spot in the patch's subject, |
| 503 | because it's the first (and sometimes the only) thing which is read when |
| 504 | reviewing patches to find which ones need to be backported to older versions. |
| 505 | It also helps when trying to find which patch is the most likely to have caused |
| 506 | a regression. |
| 507 | |
| 508 | Specifically, bugs must be clearly easy to spot so that they're never missed. |
| 509 | Any patch fixing a bug must have the "BUG" tag in its subject. Most common |
| 510 | patch types include : |
| 511 | |
| 512 | - BUG fix for a bug. The severity of the bug should also be indicated |
| 513 | when known. Similarly, if a backport is needed to older versions, |
| 514 | it should be indicated on the last line of the commit message. If |
| 515 | the bug has been identified as a regression brought by a specific |
| 516 | patch or version, this indication will be appreciated too. New |
| 517 | maintenance releases are generally emitted when a few of these |
| 518 | patches are merged. If the bug is a vulnerability for which a CVE |
| 519 | identifier was assigned before you publish the fix, you can mention |
| 520 | it in the commit message, it will help distro maintainers. |
| 521 | |
Tim Düsterhus | 4896c44 | 2016-11-29 02:15:19 +0100 | [diff] [blame] | 522 | - CLEANUP code cleanup, silence of warnings, etc... theoretically no impact. |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 523 | These patches will rarely be seen in stable branches, though they |
| 524 | may appear when they remove some annoyance or when they make |
| 525 | backporting easier. By nature, a cleanup is always of minor |
| 526 | importance and it's not needed to mention it. |
| 527 | |
| 528 | - DOC updates to any of the documentation files, including README. Many |
| 529 | documentation updates are backported since they don't impact the |
| 530 | product's stability and may help users avoid bugs. So please |
| 531 | indicate in the commit message if a backport is desired. When a |
| 532 | feature gets documented, it's preferred that the doc patch appears |
| 533 | in the same patch or after the feature patch, but not before, as it |
| 534 | becomes confusing when someone working on a code base including |
| 535 | only the doc patch won't understand why a documented feature does |
| 536 | not work as documented. |
| 537 | |
| 538 | - REORG code reorganization. Some blocks may be moved to other places, |
| 539 | some important checks might be swapped, etc... These changes |
| 540 | always present a risk of regression. For this reason, they should |
| 541 | never be mixed with any bug fix nor functional change. Code is |
| 542 | only moved as-is. Indicating the risk of breakage is highly |
| 543 | recommended. Minor breakage is tolerated in such patches if trying |
| 544 | to fix it at once makes the whole change even more confusing. That |
| 545 | may happen for example when some #ifdefs need to be propagated in |
| 546 | every file consecutive to the change. |
| 547 | |
| 548 | - BUILD updates or fixes for build issues. Changes to makefiles also fall |
| 549 | into this category. The risk of breakage should be indicated if |
| 550 | known. It is also appreciated to indicate what platforms and/or |
| 551 | configurations were tested after the change. |
| 552 | |
| 553 | - OPTIM some code was optimised. Sometimes if the regression risk is very |
| 554 | low and the gains significant, such patches may be merged in the |
| 555 | stable branch. Depending on the amount of code changed or replaced |
| 556 | and the level of trust the author has in the change, the risk of |
| 557 | regression should be indicated. |
| 558 | |
| 559 | - RELEASE release of a new version (development or stable). |
| 560 | |
| 561 | - LICENSE licensing updates (may impact distro packagers). |
| 562 | |
| 563 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 564 | When the patch cannot be categorized, it's best not to put any type tag, and to |
| 565 | only use a risk or complexity information only as below. This is commonly the |
| 566 | case for new features, which development versions are mostly made of. |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 567 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 568 | The importance, complexity of the patch, or severity of the bug it fixes must |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 569 | be indicated when relevant. A single upper-case word is preferred, among : |
| 570 | |
| 571 | - MINOR minor change, very low risk of impact. It is often the case for |
| 572 | code additions that don't touch live code. As a rule of thumb, a |
| 573 | patch tagged "MINOR" is safe enough to be backported to stable |
| 574 | branches. For a bug, it generally indicates an annoyance, nothing |
| 575 | more. |
| 576 | |
| 577 | - MEDIUM medium risk, may cause unexpected regressions of low importance or |
| 578 | which may quickly be discovered. In short, the patch is safe but |
| 579 | touches working areas and it is always possible that you missed |
| 580 | something you didn't know existed (eg: adding a "case" entry or |
| 581 | an error message after adding an error code to an enum). For a bug, |
| 582 | it generally indicates something odd which requires changing the |
| 583 | configuration in an undesired way to work around the issue. |
| 584 | |
| 585 | - MAJOR major risk of hidden regression. This happens when large parts of |
| 586 | the code are rearranged, when new timeouts are introduced, when |
| 587 | sensitive parts of the session scheduling are touched, etc... We |
| 588 | should only exceptionally find such patches in stable branches when |
| 589 | there is no other option to fix a design issue. For a bug, it |
| 590 | indicates severe reliability issues for which workarounds are |
| 591 | identified with or without performance impacts. |
| 592 | |
| 593 | - CRITICAL medium-term reliability or security is at risk and workarounds, |
| 594 | if they exist, might not always be acceptable. An upgrade is |
| 595 | absolutely required. A maintenance release may be emitted even if |
| 596 | only one of these bugs are fixed. Note that this tag is only used |
| 597 | with bugs. Such patches must indicate what is the first version |
| 598 | affected, and if known, the commit ID which introduced the issue. |
| 599 | |
| 600 | The expected length of the commit message grows with the importance of the |
| 601 | change. While a MINOR patch may sometimes be described in 1 or 2 lines, MAJOR |
| 602 | or CRITICAL patches cannot have less than 10-15 lines to describe exactly the |
| 603 | impacts otherwise the submitter's work will be considered as rough sabotage. |
| 604 | |
| 605 | For BUILD, DOC and CLEANUP types, this tag is not always relevant and may be |
| 606 | omitted. |
| 607 | |
| 608 | The area the patch applies to is quite important, because some areas are known |
| 609 | to be similar in older versions, suggesting a backport might be desirable, and |
| 610 | conversely, some areas are known to be specific to one version. The area is a |
| 611 | single-word lowercase name the contributor find clear enough to describe what |
| 612 | part is being touched. The following tags are suggested but not limitative : |
| 613 | |
| 614 | - examples example files. Be careful, sometimes these files are packaged. |
| 615 | |
| 616 | - tests regression test files. No code is affected, no need to upgrade. |
| 617 | |
| 618 | - init initialization code, arguments parsing, etc... |
| 619 | |
| 620 | - config configuration parser, mostly used when adding new config keywords |
| 621 | |
| 622 | - http the HTTP engine |
| 623 | |
| 624 | - stats the stats reporting engine |
| 625 | |
| 626 | - cli the stats socket CLI |
| 627 | |
| 628 | - checks the health checks engine (eg: when adding new checks) |
| 629 | |
| 630 | - sample the sample fetch system (new fetch or converter functions) |
| 631 | |
| 632 | - acl the ACL processing core or some ACLs from other areas |
| 633 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 634 | - filters everything related to the filters core |
| 635 | |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 636 | - peers the peer synchronization engine |
| 637 | |
| 638 | - lua the Lua scripting engine |
| 639 | |
| 640 | - listeners everything related to incoming connection settings |
| 641 | |
| 642 | - frontend everything related to incoming connection processing |
| 643 | |
| 644 | - backend everything related to LB algorithms and server farm |
| 645 | |
| 646 | - session session processing and flags (very sensible, be careful) |
| 647 | |
| 648 | - server server connection management, queueing |
| 649 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 650 | - spoe SPOE code |
| 651 | |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 652 | - ssl the SSL/TLS interface |
| 653 | |
| 654 | - proxy proxy maintenance (start/stop) |
| 655 | |
| 656 | - log log management |
| 657 | |
| 658 | - poll any of the pollers |
| 659 | |
| 660 | - halog the halog sub-component in the contrib directory |
| 661 | |
| 662 | - contrib any addition to the contrib directory |
| 663 | |
| 664 | Other names may be invented when more precise indications are meaningful, for |
| 665 | instance : "cookie" which indicates cookie processing in the HTTP core. Last, |
| 666 | indicating the name of the affected file is also a good way to quickly spot |
| 667 | changes. Many commits were already tagged with "stream_sock" or "cfgparse" for |
| 668 | instance. |
| 669 | |
| 670 | It is required that the type of change and the severity when relevant are |
| 671 | indicated, as well as the touched area when relevant as well in the patch |
| 672 | subject. Normally, we would have the 3 most often. The two first criteria should |
| 673 | be present before a first colon (':'). If both are present, then they should be |
| 674 | delimited with a slash ('/'). The 3rd criterion (area) should appear next, also |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 675 | followed by a colon. Thus, all of the following subject lines are valid : |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 676 | |
Willy Tarreau | 138544f | 2017-03-31 16:24:44 +0200 | [diff] [blame] | 677 | Examples of subject lines : |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 678 | - DOC: document options forwardfor to logasap |
| 679 | - DOC/MAJOR: reorganize the whole document and change indenting |
| 680 | - BUG: stats: connection reset counters must be plain ascii, not HTML |
| 681 | - BUG/MINOR: stats: connection reset counters must be plain ascii, not HTML |
| 682 | - MEDIUM: checks: support multi-packet health check responses |
| 683 | - RELEASE: Released version 1.4.2 |
| 684 | - BUILD: stats: stdint is not present on solaris |
| 685 | - OPTIM/MINOR: halog: make fgets parse more bytes by blocks |
| 686 | - REORG/MEDIUM: move syscall redefinition to specific places |
| 687 | |
| 688 | Please do not use square brackets anymore around the tags, because they induce |
| 689 | more work when merging patches, which need to be hand-edited not to lose the |
| 690 | enclosed part. |
| 691 | |
| 692 | In fact, one of the only square bracket tags that still makes sense is '[RFC]' |
| 693 | at the beginning of the subject, when you're asking for someone to review your |
| 694 | change before getting it merged. If the patch is OK to be merged, then it can |
| 695 | be merge as-is and the '[RFC]' tag will automatically be removed. If you don't |
| 696 | want it to be merged at all, you can simply state it in the message, or use an |
| 697 | alternate 'WIP/' prefix in front of your tag tag ("work in progress"). |
| 698 | |
| 699 | The tags are not rigid, follow your intuition first, and they may be readjusted |
| 700 | when your patch is merged. It may happen that a same patch has a different tag |
| 701 | in two distinct branches. The reason is that a bug in one branch may just be a |
| 702 | cleanup or safety measure in the other one because the code cannot be triggered. |
| 703 | |
| 704 | |
| 705 | Working with Git |
| 706 | ---------------- |
| 707 | |
| 708 | For a more efficient interaction between the mainline code and your code, you |
| 709 | are strongly encouraged to try the Git version control system : |
| 710 | |
| 711 | http://git-scm.com/ |
| 712 | |
| 713 | It's very fast, lightweight and lets you undo/redo your work as often as you |
| 714 | want, without making your mistakes visible to the rest of the world. It will |
| 715 | definitely help you contribute quality code and take other people's feedback |
| 716 | in consideration. In order to clone the HAProxy Git repository : |
| 717 | |
| 718 | $ git clone http://git.haproxy.org/git/haproxy.git/ (development) |
| 719 | |
| 720 | If you decide to use Git for your developments, then your commit messages will |
| 721 | have the subject line in the format described above, then the whole description |
| 722 | of your work (mainly why you did it) will be in the body. You can directly send |
| 723 | your commits to the mailing list, the format is convenient to read and process. |
| 724 | |
| 725 | It is recommended to create a branch for your work that is based on the master |
| 726 | branch : |
| 727 | |
| 728 | $ git checkout -b 20150920-fix-stats master |
| 729 | |
| 730 | You can then do your work and even experiment with multiple alternatives if you |
| 731 | are not completely sure that your solution is the best one : |
| 732 | |
| 733 | $ git checkout -b 20150920-fix-stats-v2 |
| 734 | |
| 735 | Then reorder/merge/edit your patches : |
| 736 | |
| 737 | $ git rebase -i master |
| 738 | |
| 739 | When you think you're ready, reread your whole patchset to ensure there is no |
Tim Düsterhus | 4896c44 | 2016-11-29 02:15:19 +0100 | [diff] [blame] | 740 | formatting or style issue : |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 741 | |
| 742 | $ git show master.. |
| 743 | |
| 744 | And once you're satisfied, you should update your master branch to be sure that |
Thiago Farina | 9f72a39 | 2016-04-01 16:43:50 -0300 | [diff] [blame] | 745 | nothing changed during your work (only needed if you left it unattended for days |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 746 | or weeks) : |
| 747 | |
| 748 | $ git checkout -b 20150920-fix-stats-rebased |
| 749 | $ git fetch origin master:master |
| 750 | $ git rebase master |
| 751 | |
Thiago Farina | 9f72a39 | 2016-04-01 16:43:50 -0300 | [diff] [blame] | 752 | You can build a list of patches ready for submission like this : |
Willy Tarreau | 11e334d9 | 2015-09-20 22:31:42 +0200 | [diff] [blame] | 753 | |
| 754 | $ git format-patch master |
| 755 | |
| 756 | The output files are the patches ready to be sent over e-mail, either via a |
| 757 | regular e-mail or via git send-email (carefully check the man page). Don't |
| 758 | destroy your other work branches until your patches get merged, it may happen |
| 759 | that earlier designs will be preferred for various reasons. Patches should be |
| 760 | sent to the mailing list : haproxy@formilux.org and CCed to relevant subsystem |
| 761 | maintainers or authors of the modified files if their address appears at the |
| 762 | top of the file. |
| 763 | |
| 764 | Please don't send pull-requests, they are really unconvenient. First, a pull |
| 765 | implies a merge operation and the code doesn't move fast enough to justify the |
| 766 | use of merges. Second, pull requests are not easily commented on by the |
| 767 | project's participants, contrary to e-mails where anyone is allowed to have an |
| 768 | opinion and to express it. |
| 769 | |
| 770 | -- end |