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- [The byte order fallacy - command center][command-center-byte-order-fallacy] 45 46[command-center-byte-order-fallacy]: 47 https://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html 48 49- [what’s the most annoying thing about writing C - Jessica 50 Paquette][barrelshifter-annoying-c] 51 52[barrelshifter-annoying-c]: 53 https://mastodon.social/@barrelshifter/114016810912730423 54 55- [The Power of Ten: Rules for Developing Safety Critical Code - NASA/JPL 56 Laboratory for Reliable Software][nasa-reliable-code] 57 58[nasa-reliable-code]: 59 https://www.cs.otago.ac.nz/cosc345/resources/nasa-10-rules.pdf 60 61- [C Isn't A Programming Language Anymore - Aria Desires][aria-c-is-a-protocol] 62 63[aria-c-is-a-protocol]: https://faultlore.com/blah/c-isnt-a-language/ 64 65## References 66 67- [The C programming language - C Standards Committee][c-language] 68 69[c-language]: https://www.c-language.org/ 70 71- [SEI CERT C Coding Standard][sei-cert-c-coding-standard] 72 73[sei-cert-c-coding-standard]: 74 https://wiki.sei.cmu.edu/confluence/display/c/SEI+CERT+C+Coding+Standard 75 76- [Common Weakness Enumeration (CWE) - Software 77 Development][common-weakness-enumeration-sw] 78 79[common-weakness-enumeration-sw]: 80 https://cwe.mitre.org/data/definitions/699.html 81 82## Definitions 83 84- **Error condition**: An invalid state reached at runtime, caused either by 85 resource exhaustion, or incorrect use of the library's public APIs and data 86 types. 87 88- **Invariant**: A condition in the library's implementation that must never 89 evaluate false. 90 91- **Public API**: Any definitions and declarations under `include/libpldm`. 92 93- **Wire format**: Any message structure defined in the DMTF PLDM protocol 94 specifications. 95 96## Elaborations 97 98- Resource exhaustion is always an error condition and never an invariant 99 violation. 100 101- An invariant violation is always a programming failure of the library's 102 implementation, and never the result of incorrect use of the library's public 103 APIs (see error condition). 104 105- Corollaries of the above two points: 106 107 - Incorrect use of public API functions is always an error condition, and is 108 dealt with by returning an error code. 109 110 - Incorrect use of static functions in the library's implementation is an 111 invariant violation which may be established using `assert()`. 112 113- `assert()` is the recommended way to demonstrate invariants are upheld. 114 115## Adding a new API 116 117### Naming macros, functions and types 118 119- [ ] All publicly exposed macros, types and functions relating to the PLDM 120 specifications must be prefixed with either `pldm_` or `PLDM_` as 121 appropriate 122 123 - The only (temporary) exception are the `encode_*()` and `decode_*()` 124 function symbols 125 126- [ ] All publicly exposed macros, types and functions relating to the library 127 implementation must be prefixed with `libpldm_` or `LIBPLDM_` 128 129- [ ] All `pldm_`-prefixed symbols must also name the related specification. For 130 example, for DSP0248 Platform Monitoring and Control, the symbol prefix 131 should be `pldm_platform_`. 132 133- [ ] All enum members must be prefixed with the type name 134 135### API design 136 137- [ ] If I've added support for a new PLDM message type, then I've defined both 138 the encoder and decoder for that message. 139 140 - This applies for both request _and_ response message types. 141 142- [ ] I've designed my APIs so their implementation does not require heap 143 allocation. 144 145 - Prefer [defining iterators][libpldm-iterator] over the message buffer to 146 extract sub-structures from variable-length messages. Iterators avoid both 147 requiring heap allocation in the implementation or splitting the API to 148 allow the caller to allocate appropriate space. Instead, the caller is 149 provided with an on-stack struct containing the extracted sub-structure. 150 151[libpldm-iterator]: 152 https://github.com/openbmc/libpldm/commit/3a2c6589c5660d2066b612bae28ca393a8aa1c2b 153 154- [ ] My new public message codec functions take a `struct` representing the 155 message as a parameter 156 157 - Function prototypes must _not_ decompose the message to individual 158 parameters. This approach is not ergonomic and is difficult to make 159 type-safe. This is especially true for message decoding functions which must 160 use pointers for out-parameters, where it has often become ambiguous whether 161 the underlying memory represents a single object or an array. 162 163- [ ] Each new `struct` I've defined is used in at least one new function I've 164 added to the public API. 165 166- [ ] My new public `struct` definitions are _not_ marked 167 `__attribute__((packed))` 168 169- [ ] My new public `struct` definitions do _not_ define a flexible array 170 member, unless: 171 172 - [ ] It's contained in an `#ifndef __cplusplus` macro guard, as flexible 173 arrays are not specified by C++, and 174 175 - [ ] I've implemented an accessor function so the array base pointer can be 176 accessed from C++, and 177 178 - [ ] It is defined as per the C17 specification by omitting the length[^1] 179 180 - Note: Any array defined with length 1 is _not_ a flexible array, and any 181 access beyond the first element invokes undefined behaviour in both C and 182 C++. 183 184 - [ ] I've annotated the flexible array member with `LIBPLDM_CC_COUNTED_BY()` 185 186[^1]: 187 [C17 draft specification][c17-draft-standard], 6.7.2.1 Structure and union 188 specifiers, paragraph 18. 189 190### ABI control 191 192- [ ] My new function symbols are marked with `LIBPLDM_ABI_TESTING` in the 193 implementation 194 195- [ ] I've guarded the test cases of functions marked `LIBPLDM_ABI_TESTING` so 196 that they are not compiled when the corresponding function symbols aren't 197 visible 198 199### Error handling and invariants 200 201- [ ] All my error conditions are handled by returning an error code to the 202 caller. 203 204- [ ] All my invariants are tested using `assert()`. 205 206- [ ] I have not used `assert()` to evaluate any error conditions without also 207 handling the error condition by returning an error code the the caller. 208 209 - Release builds of the library are configured with `assert()` disabled 210 (`-Db_ndebug=if-release`, which provides `-DNDEBUG` in `CFLAGS`). 211 212- [ ] My new APIs return negative `errno` values on error and not PLDM 213 completion codes. 214 215 - [ ] The specific error values my function returns and their meaning in the 216 context of the function call are listed in the API documentation. 217 218### Implementation 219 220- [ ] If my work interacts with the PLDM wire format, then I have done so using 221 the `msgbuf` APIs found in `src/msgbuf.h` (and under `src/msgbuf/`) to 222 minimise concerns around spatial memory safety and endian-correctness. 223 224- [ ] I've used `goto` to clean up resources acquired prior to encountering an 225 error condition 226 227 - Replication of resource cleanup across multiple error paths is error-prone, 228 especially when multiple, dependent resources have been acquired. 229 230- [ ] I've released acquired resources in stack-order 231 232 - This should be the case regardless of whether we're in the happy path at the 233 end of object lifetime or an error path during construction. 234 235### Testing 236 237- [ ] I've implemented test cases with reasonable branch coverage of each new 238 function I've added 239 240### Maintenance 241 242- [ ] If I've added support for a new message type, then my commit message 243 specifies all of: 244 245 - [ ] The relevant DMTF specification by its DSP number and title 246 - [ ] The relevant version of the specification 247 - [ ] The section of the specification that defines the message type 248 249- [ ] If my work impacts the public API of the library, then I've added an entry 250 to `CHANGELOG.md` describing my work 251 252## Stabilising an existing API 253 254- [ ] The API of interest is currently marked `LIBPLDM_ABI_TESTING` 255 256- [ ] My commit message links to a publicly visible patch that makes use of the 257 API 258 259- [ ] My commit updates the annotation from `LIBPLDM_ABI_TESTING` to 260 `LIBPLDM_ABI_STABLE` only for the function symbols demonstrated by the 261 patch linked in the commit message. 262 263- [ ] I've removed guards from the function's tests so they are always compiled 264 265- [ ] If I've updated the ABI dump, then I've used the OpenBMC CI container to 266 do so. 267 268## Updating an ABI dump 269 270To update the ABI dump you'll need to build an appropriate OpenBMC CI container 271image of your own. Some hints on how to do this locally can be found [in the 272openbmc/docs repository][openbmc-docs-local-ci]. You can list your locally built 273images with `docker images`. 274 275[openbmc-docs-local-ci]: 276 https://github.com/openbmc/docs/blob/master/testing/local-ci-build.md 277 278Assuming: 279 280``` 281export OPENBMC_CI_IMAGE=openbmc/ubuntu-unit-test:2024-W21-ce361f95ff4fa669 282``` 283 284the ABI dump can be updated with: 285 286``` 287docker run \ 288 --cap-add=sys_admin \ 289 --rm=true \ 290 --privileged=true \ 291 -u $USER \ 292 -w $(pwd) \ 293 -v $(pwd):$(pwd) \ 294 -e MAKEFLAGS= \ 295 -t $OPENBMC_CI_IMAGE \ 296 ./scripts/abi-dump-updater 297``` 298 299## Removing an API 300 301- [ ] If the function is marked `LIBPLDM_ABI_TESTING`, then I have removed it 302 303- [ ] If the function is marked `LIBPLDM_ABI_STABLE`, then I have changed the 304 annotation to `LIBPLDM_ABI_DEPRECATED` and left it in-place. 305 306 - [ ] I have updated the ABI dump, or will mark the change as WIP until it has 307 been. 308 309- [ ] If the function is marked `LIBPLDM_ABI_DEPRECATED`, then I have removed it 310 only after satisfying myself that each of the following is true: 311 312 - [ ] There are no known users of the function left in the community 313 - [ ] There has been at least one tagged release of `libpldm` subsequent to 314 the API being marked deprecated 315 316## Renaming an API 317 318A change to an API is a pure rename only if there are no additional behavioural 319changes. Renaming an API with no other behavioural changes is really two 320actions: 321 3221. Introducing the new API name 3232. Deprecating the old API name 324 325- [ ] Only the name of the function has changed. None of its behaviour has 326 changed. 327 328- [ ] Both the new and the old functions are declared in the public headers 329 330- [ ] I've aliased the old function name to the new function name via the 331 `libpldm_deprecated_aliases` list in `meson.build` 332 333- [ ] I've added a [semantic patch][coccinelle] to migrate users from the old 334 name to the new name 335 336[coccinelle]: https://coccinelle.gitlabpages.inria.fr/website/ 337 338- [ ] I've updated the ABI dump to capture the rename, or will mark the change 339 as WIP until it has been. 340 341## Fixing Implementation Defects 342 343- [ ] My change fixing the bug includes a [Fixes tag][linux-kernel-fixes-tag] 344 identifying the change introducing the defect. 345 346[linux-kernel-fixes-tag]: 347 https://docs.kernel.org/process/submitting-patches.html#describe-your-changes 348 349- [ ] My change fixing the bug includes test cases demonstrating that the bug is 350 fixed. 351