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