1# Checklist for making changes to `libpldm` 2 3## Philosophy and influences 4 5- [Good Practices in Library Design, Implementation, and Maintenance - Ulrich 6 Drepper][goodpractice] 7 8[goodpractice]: https://www.akkadia.org/drepper/goodpractice.pdf 9 10- [How Do I Make This Hard to Misuse? - Rusty Russell][rusty-api-scale-good] 11 12[rusty-api-scale-good]: https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html 13 14- [What If I Don't Actually Like My Users? - Rusty Russell][rusty-api-scale-bad] 15 16[rusty-api-scale-bad]: https://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html 17 18- [Red flags that indicate questionable quality - Lennart 19 Poettering][poettering-library-red-flags] 20 21[poettering-library-red-flags]: 22 https://mastodon.social/@pid_eins/112517953375791453 23 24- [Not sure if this is a gcc bug or some weird corner of UB or what... - Andrew 25 Zonenberg][azonenberg-packed-struct] 26 27[azonenberg-packed-struct]: https://ioc.exchange/@azonenberg/112535511250395148 28 29- [The Good, the Bad, and the Weird - Trail of Bits 30 Blog][trail-of-bits-weird-machines] 31 32[trail-of-bits-weird-machines]: 33 https://blog.trailofbits.com/2018/10/26/the-good-the-bad-and-the-weird/ 34 35- [Logic for Programmers - Hillel Wayne][logic-for-programmers] 36 37[logic-for-programmers]: https://leanpub.com/logic 38 39- [Parse, don’t validate - Alexis King][alexis-king-parse-dont-validate] 40 41[alexis-king-parse-dont-validate]: 42 https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/ 43 44## References 45 46- [C17 draft standard][c17-draft-standard] 47 48[c17-draft-standard]: 49 https://web.archive.org/web/20181230041359if_/http://www.open-std.org/jtc1/sc22/wg14/www/abq/c17_updated_proposed_fdis.pdf 50 51- [SEI CERT C Coding Standard][sei-cert-c-coding-standard] 52 53[sei-cert-c-coding-standard]: 54 https://wiki.sei.cmu.edu/confluence/display/c/SEI+CERT+C+Coding+Standard 55 56- [Common Weakness Enumeration (CWE) - Software 57 Development][common-weakness-enumeration-sw] 58 59[common-weakness-enumeration-sw]: 60 https://cwe.mitre.org/data/definitions/699.html 61 62## Definitions 63 64- **Error condition**: An invalid state reached at runtime, caused either by 65 resource exhaustion, or incorrect use of the library's public APIs and data 66 types. 67 68- **Invariant**: A condition in the library's implementation that must never 69 evaluate false. 70 71- **Public API**: Any definitions and declarations under `include/libpldm`. 72 73- **Wire format**: Any message structure defined in the DMTF PLDM protocol 74 specifications. 75 76## Elaborations 77 78- Resource exhaustion is always an error condition and never an invariant 79 violation. 80 81- An invariant violation is always a programming failure of the library's 82 implementation, and never the result of incorrect use of the library's public 83 APIs (see error condition). 84 85- Corollaries of the above two points: 86 87 - Incorrect use of public API functions is always an error condition, and is 88 dealt with by returning an error code. 89 90 - Incorrect use of static functions in the library's implementation is an 91 invariant violation which may be established using `assert()`. 92 93- `assert()` is the recommended way to demonstrate invariants are upheld. 94 95## Adding a new API 96 97### Naming macros, functions and types 98 99- [ ] All publicly exposed macros, types and functions relating to the PLDM 100 specifications must be prefixed with either `pldm_` or `PLDM_` as 101 appropriate 102 103 - The only (temporary) exception are the `encode_*()` and `decode_*()` 104 function symbols 105 106- [ ] All publicly exposed macros, types and functions relating to the library 107 implementation must be prefixed with `libpldm_` or `LIBPLDM_` 108 109- [ ] All `pldm_`-prefixed symbols must also name the related specification. For 110 example, for DSP0248 Platform Monitoring and Control, the symbol prefix 111 should be `pldm_platform_`. 112 113- [ ] All enum members must be prefixed with the type name 114 115### All other concerns 116 117- [ ] My new public message codec functions take a `struct` representing the 118 message as a parameter 119 120 - Function prototypes must _not_ decompose the message to individual 121 parameters. This approach is not ergonomic and is difficult to make 122 type-safe. This is especially true for message decoding functions which must 123 use pointers for out-parameters, where it has often become ambiguous whether 124 the underlying memory represents a single object or an array. 125 126- [ ] Each new `struct` I've defined is used in at least one new function I've 127 added to the public API. 128 129- [ ] My new public `struct` definitions are _not_ marked 130 `__attribute__((packed))` 131 132- [ ] My new public `struct` definitions do _not_ define a flexible array 133 member, unless: 134 135 - [ ] It's contained in an `#ifndef __cplusplus` macro guard, as flexible 136 arrays are not specified by C++, and 137 138 - [ ] I've implemented an accessor function so the array base pointer can be 139 accessed from C++, and 140 141 - [ ] It is defined as per the C17 specification by omitting the length[^1] 142 143 - Note: Any array defined with length 1 is _not_ a flexible array, and any 144 access beyond the first element invokes undefined behaviour in both C and 145 C++. 146 147 - [ ] I've annotated the flexible array member with `LIBPLDM_CC_COUNTED_BY()` 148 149[^1]: 150 [C17 draft specification][c17-draft-standard], 6.7.2.1 Structure and union 151 specifiers, paragraph 18. 152 153- [ ] If my work interacts with the PLDM wire format, then I have done so using 154 the `msgbuf` APIs found in `src/msgbuf.h` (and under `src/msgbuf/`) to 155 minimise concerns around spatial memory safety and endian-correctness. 156 157- [ ] All my error conditions are handled by returning an error code to the 158 caller. 159 160- [ ] All my invariants are tested using `assert()`. 161 162- [ ] I have not used `assert()` to evaluate any error conditions without also 163 handling the error condition by returning an error code the the caller. 164 165 - Release builds of the library are configured with `assert()` disabled 166 (`-Db_ndebug=if-release`, which provides `-DNDEBUG` in `CFLAGS`). 167 168- [ ] My new APIs return negative `errno` values on error and not PLDM 169 completion codes. 170 171 - [ ] The specific error values my function returns and their meaning in the 172 context of the function call are listed in the API documentation. 173 174- [ ] If I've added support for a new PLDM message type, then I've implemented 175 both the encoder and decoder for that message. Note this applies for both 176 request _and_ response message types. 177 178- [ ] My new function symbols are marked with `LIBPLDM_ABI_TESTING` in the 179 implementation 180 181- [ ] I've implemented test cases with reasonable branch coverage of each new 182 function I've added 183 184- [ ] I've guarded the test cases of functions marked `LIBPLDM_ABI_TESTING` so 185 that they are not compiled when the corresponding function symbols aren't 186 visible 187 188- [ ] If I've added support for a new message type, then my commit message 189 specifies all of: 190 191 - [ ] The relevant DMTF specification by its DSP number and title 192 - [ ] The relevant version of the specification 193 - [ ] The section of the specification that defines the message type 194 195- [ ] If my work impacts the public API of the library, then I've added an entry 196 to `CHANGELOG.md` describing my work 197 198## Stabilising an existing API 199 200- [ ] The API of interest is currently marked `LIBPLDM_ABI_TESTING` 201 202- [ ] My commit message links to a publicly visible patch that makes use of the 203 API 204 205- [ ] My commit updates the annotation from `LIBPLDM_ABI_TESTING` to 206 `LIBPLDM_ABI_STABLE` only for the function symbols demonstrated by the 207 patch linked in the commit message. 208 209- [ ] I've removed guards from the function's tests so they are always compiled 210 211- [ ] If I've updated the ABI dump, then I've used the OpenBMC CI container to 212 do so. 213 214## Updating an ABI dump 215 216Each of the following must succeed: 217 218- [ ] Enter the OpenBMC CI Docker container 219 - Approximately: 220 `docker run --cap-add=sys_admin --rm=true --privileged=true -u $USER -w $(pwd) -v $(pwd):$(pwd) -e MAKEFLAGS= -it openbmc/ubuntu-unit-test:2024-W21-ce361f95ff4fa669` 221- [ ] `CC=gcc CXX=g++; [ $(uname -m) = 'x86_64' ] && meson setup -Dabi=deprecated,stable build` 222- [ ] `meson compile -C build` 223- [ ] `./scripts/abi-dump-formatter < build/src/current.dump > abi/x86_64/gcc.dump` 224 225## Removing an API 226 227- [ ] If the function is marked `LIBPLDM_ABI_TESTING`, then I have removed it 228 229- [ ] If the function is marked `LIBPLDM_ABI_STABLE`, then I have changed the 230 annotation to `LIBPLDM_ABI_DEPRECATED` and left it in-place. 231 232 - [ ] I have updated the ABI dump, or will mark the change as WIP until it has 233 been. 234 235- [ ] If the function is marked `LIBPLDM_ABI_DEPRECATED`, then I have removed it 236 only after satisfying myself that each of the following is true: 237 238 - [ ] There are no known users of the function left in the community 239 - [ ] There has been at least one tagged release of `libpldm` subsequent to 240 the API being marked deprecated 241 242## Renaming an API 243 244A change to an API is a pure rename only if there are no additional behavioural 245changes. Renaming an API with no other behavioural changes is really two 246actions: 247 2481. Introducing the new API name 2492. Deprecating the old API name 250 251- [ ] Only the name of the function has changed. None of its behaviour has 252 changed. 253 254- [ ] Both the new and the old functions are declared in the public headers 255 256- [ ] I've aliased the old function name to the new function name via the 257 `libpldm_deprecated_aliases` list in `meson.build` 258 259- [ ] I've added a [semantic patch][coccinelle] to migrate users from the old 260 name to the new name 261 262[coccinelle]: https://coccinelle.gitlabpages.inria.fr/website/ 263 264- [ ] I've updated the ABI dump to capture the rename, or will mark the change 265 as WIP until it has been. 266