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[c17-draft-standard]: 191 https://web.archive.org/web/20181230041359/http://www.open-std.org/jtc1/sc22/wg14/www/abq/c17_updated_proposed_fdis.pdf 192 193### ABI control 194 195- [ ] My new function symbols are marked with `LIBPLDM_ABI_TESTING` in the 196 implementation 197 198- [ ] I've guarded the test cases of functions marked `LIBPLDM_ABI_TESTING` so 199 that they are not compiled when the corresponding function symbols aren't 200 visible 201 202### Error handling and invariants 203 204- [ ] All my error conditions are handled by returning an error code to the 205 caller. 206 207- [ ] All my invariants are tested using `assert()`. 208 209- [ ] I have not used `assert()` to evaluate any error conditions without also 210 handling the error condition by returning an error code the the caller. 211 212 - Release builds of the library are configured with `assert()` disabled 213 (`-Db_ndebug=if-release`, which provides `-DNDEBUG` in `CFLAGS`). 214 215- [ ] My new APIs return negative `errno` values on error and not PLDM 216 completion codes. 217 218 - [ ] The specific error values my function returns and their meaning in the 219 context of the function call are listed in the API documentation. 220 221### Implementation 222 223- [ ] If my work interacts with the PLDM wire format, then I have done so using 224 the `msgbuf` APIs found in `src/msgbuf.h` (and under `src/msgbuf/`) to 225 minimise concerns around spatial memory safety and endian-correctness. 226 227- [ ] I've used `goto` to clean up resources acquired prior to encountering an 228 error condition 229 230 - Replication of resource cleanup across multiple error paths is error-prone, 231 especially when multiple, dependent resources have been acquired. 232 233- [ ] I've released acquired resources in stack-order 234 235 - This should be the case regardless of whether we're in the happy path at the 236 end of object lifetime or an error path during construction. 237 238### Testing 239 240- [ ] I've implemented test cases with reasonable branch coverage of each new 241 function I've added 242 243### Maintenance 244 245- [ ] If I've added support for a new message type, then my commit message 246 specifies all of: 247 248 - [ ] The relevant DMTF specification by its DSP number and title 249 - [ ] The relevant version of the specification 250 - [ ] The section of the specification that defines the message type 251 252- [ ] If my work impacts the public API of the library, then I've added an entry 253 to `CHANGELOG.md` describing my work 254 255## Stabilising an existing API 256 257- [ ] The API of interest is currently marked `LIBPLDM_ABI_TESTING` 258 259- [ ] My commit message links to a publicly visible patch that makes use of the 260 API 261 262- [ ] My commit updates the annotation from `LIBPLDM_ABI_TESTING` to 263 `LIBPLDM_ABI_STABLE` only for the function symbols demonstrated by the 264 patch linked in the commit message. 265 266- [ ] I've removed guards from the function's tests so they are always compiled 267 268- [ ] If I've updated the ABI dump, then I've used the OpenBMC CI container to 269 do so. 270 271## Updating an ABI dump 272 273To update the ABI dump you'll need to build an appropriate OpenBMC CI container 274image of your own. Some hints on how to do this locally can be found [in the 275openbmc/docs repository][openbmc-docs-local-ci]. You can list your locally built 276images with `docker images`. 277 278[openbmc-docs-local-ci]: 279 https://github.com/openbmc/docs/blob/master/testing/local-ci-build.md 280 281Assuming: 282 283```shell 284export OPENBMC_CI_IMAGE=openbmc/ubuntu-unit-test:2024-W21-ce361f95ff4fa669 285``` 286 287the ABI dump can be updated with: 288 289```shell 290docker run \ 291 --cap-add=sys_admin \ 292 --rm=true \ 293 --privileged=true \ 294 -u $USER \ 295 -w $(pwd) \ 296 -v $(pwd):$(pwd) \ 297 -e MAKEFLAGS= \ 298 -t $OPENBMC_CI_IMAGE \ 299 ./scripts/abi-dump-updater 300``` 301 302## Removing an API 303 304- [ ] If the function is marked `LIBPLDM_ABI_TESTING`, then I have removed it 305 306- [ ] If the function is marked `LIBPLDM_ABI_STABLE`, then I have changed the 307 annotation to `LIBPLDM_ABI_DEPRECATED` and left it in-place. 308 309 - [ ] I have updated the ABI dump, or will mark the change as WIP until it has 310 been. 311 312- [ ] If the function is marked `LIBPLDM_ABI_DEPRECATED`, then I have removed it 313 only after satisfying myself that each of the following is true: 314 315 - [ ] There are no known users of the function left in the community 316 - [ ] There has been at least one tagged release of `libpldm` subsequent to 317 the API being marked deprecated 318 319## Renaming an API 320 321A change to an API is a pure rename only if there are no additional behavioural 322changes. Renaming an API with no other behavioural changes is really two 323actions: 324 3251. Introducing the new API name 3262. Deprecating the old API name 327 328- [ ] Only the name of the function has changed. None of its behaviour has 329 changed. 330 331- [ ] Both the new and the old functions are declared in the public headers 332 333- [ ] I've aliased the old function name to the new function name via the 334 `libpldm_deprecated_aliases` list in `meson.build` 335 336- [ ] I've added a [semantic patch][coccinelle] to migrate users from the old 337 name to the new name 338 339[coccinelle]: https://coccinelle.gitlabpages.inria.fr/website/ 340 341- [ ] I've updated the ABI dump to capture the rename, or will mark the change 342 as WIP until it has been. 343 344## Fixing Implementation Defects 345 346- [ ] My change fixing the bug includes a [Fixes tag][linux-kernel-fixes-tag] 347 identifying the change introducing the defect. 348 349[linux-kernel-fixes-tag]: 350 https://docs.kernel.org/process/submitting-patches.html#describe-your-changes 351 352- [ ] My change fixing the bug includes test cases demonstrating that the bug is 353 fixed. 354