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 216To update the ABI dump you'll need to build an appropriate OpenBMC CI container 217image of your own. Some hints on how to do this locally can be found [in the 218openbmc/docs repository][openbmc-docs-local-ci]. You can list your locally built 219images with `docker images`. 220 221[openbmc-docs-local-ci]: 222 https://github.com/openbmc/docs/blob/master/testing/local-ci-build.md 223 224Assuming: 225 226``` 227export OPENBMC_CI_IMAGE=openbmc/ubuntu-unit-test:2024-W21-ce361f95ff4fa669 228``` 229 230the ABI dump can be updated with: 231 232``` 233docker run \ 234 --cap-add=sys_admin \ 235 --rm=true \ 236 --privileged=true \ 237 -u $USER \ 238 -w $(pwd) \ 239 -v $(pwd):$(pwd) \ 240 -e MAKEFLAGS= \ 241 -t $OPENBMC_CI_IMAGE \ 242 ./scripts/abi-dump-updater 243``` 244 245## Removing an API 246 247- [ ] If the function is marked `LIBPLDM_ABI_TESTING`, then I have removed it 248 249- [ ] If the function is marked `LIBPLDM_ABI_STABLE`, then I have changed the 250 annotation to `LIBPLDM_ABI_DEPRECATED` and left it in-place. 251 252 - [ ] I have updated the ABI dump, or will mark the change as WIP until it has 253 been. 254 255- [ ] If the function is marked `LIBPLDM_ABI_DEPRECATED`, then I have removed it 256 only after satisfying myself that each of the following is true: 257 258 - [ ] There are no known users of the function left in the community 259 - [ ] There has been at least one tagged release of `libpldm` subsequent to 260 the API being marked deprecated 261 262## Renaming an API 263 264A change to an API is a pure rename only if there are no additional behavioural 265changes. Renaming an API with no other behavioural changes is really two 266actions: 267 2681. Introducing the new API name 2692. Deprecating the old API name 270 271- [ ] Only the name of the function has changed. None of its behaviour has 272 changed. 273 274- [ ] Both the new and the old functions are declared in the public headers 275 276- [ ] I've aliased the old function name to the new function name via the 277 `libpldm_deprecated_aliases` list in `meson.build` 278 279- [ ] I've added a [semantic patch][coccinelle] to migrate users from the old 280 name to the new name 281 282[coccinelle]: https://coccinelle.gitlabpages.inria.fr/website/ 283 284- [ ] I've updated the ABI dump to capture the rename, or will mark the change 285 as WIP until it has been. 286 287## Fixing Implementation Defects 288 289- [ ] My change fixing the bug includes a [Fixes tag][linux-kernel-fixes-tag] 290 identifying the change introducing the defect. 291 292[linux-kernel-fixes-tag]: 293 https://docs.kernel.org/process/submitting-patches.html#describe-your-changes 294 295- [ ] My change fixing the bug includes test cases demonstrating that the bug is 296 fixed. 297