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## References 25 26- [C17 draft standard][c17-draft-standard] 27 28[c17-draft-standard]: 29 https://web.archive.org/web/20181230041359if_/http://www.open-std.org/jtc1/sc22/wg14/www/abq/c17_updated_proposed_fdis.pdf 30 31## Definitions 32 33- **Error condition**: An invalid state reached at runtime, caused either by 34 resource exhaustion, or incorrect use of the library's public APIs and data 35 types. 36 37- **Invariant**: A condition in the library's implementation that must never 38 evaluate false. 39 40- **Public API**: Any definitions and declarations under `include/libpldm`. 41 42## Elaborations 43 44- Resource exhaustion is always an error condition and never an invariant 45 violation. 46 47- An invariant violation is always a programming failure of the library's 48 implementation, and never the result of incorrect use of the library's public 49 APIs (see error condition). 50 51- Corollaries of the above two points: 52 53 - Incorrect use of public API functions is always an error condition, and is 54 dealt with by returning an error code. 55 56 - Incorrect use of static functions in the library's implementation is an 57 invariant violation which may be established using `assert()`. 58 59- `assert()` is the recommended way to demonstrate invariants are upheld. 60 61## Adding a new API 62 63- [ ] My new public `struct` definitions are _not_ marked 64 `__attribute__((packed))` 65 66- [ ] If my work interacts with the PLDM wire format, then I have done so using 67 the `msgbuf` APIs found in `src/msgbuf.h` (and under `src/msgbuf/`) to 68 minimise concerns around spatial memory safety and endian-correctness. 69 70- [ ] All my error conditions are handled by returning an error code to the 71 caller. 72 73- [ ] All my invariants are tested using `assert()`. 74 75- [ ] I have not used `assert()` to evaluate any error conditions without also 76 handling the error condition by returning an error code the the caller. 77 78 - Release builds of the library are configured with `assert()` disabled 79 (`-Db_ndebug=if-release`, which provides `-DNDEBUG` in `CFLAGS`). 80 81- [ ] My new APIs return negative `errno` values on error and not PLDM 82 completion codes. 83 84 - [ ] The specific error values my function returns and their meaning in the 85 context of the function call are listed in the API documentation. 86 87- [ ] If I've added support for a new PLDM message type, then I've implemented 88 both the encoder and decoder for that message. Note this applies for both 89 request _and_ response message types. 90 91- [ ] My new function symbols are marked with `LIBPLDM_ABI_TESTING` in the 92 implementation 93 94- [ ] I've implemented test cases with reasonable branch coverage of each new 95 function I've added 96 97- [ ] I've guarded the test cases of functions marked `LIBPLDM_ABI_TESTING` so 98 that they are not compiled when the corresponding function symbols aren't 99 visible 100 101- [ ] If I've added support for a new message type, then my commit message 102 specifies all of: 103 104 - [ ] The relevant DMTF specification by its DSP number and title 105 - [ ] The relevant version of the specification 106 - [ ] The section of the specification that defines the message type 107 108- [ ] If my work impacts the public API of the library, then I've added an entry 109 to `CHANGELOG.md` describing my work 110 111## Stabilising an existing API 112 113- [ ] The API of interest is currently marked `LIBPLDM_ABI_TESTING` 114 115- [ ] My commit message links to a publicly visible patch that makes use of the 116 API 117 118- [ ] My commit updates the annotation from `LIBPLDM_ABI_TESTING` to 119 `LIBPLDM_ABI_STABLE` only for the function symbols demonstrated by the 120 patch linked in the commit message. 121 122- [ ] I've removed guards from the function's tests so they are always compiled 123 124- [ ] If I've updated the ABI dump, then I've used the OpenBMC CI container to 125 do so. 126 127## Updating an ABI dump 128 129Each of the following must succeed: 130 131- [ ] Enter the OpenBMC CI Docker container 132 - Approximately: 133 `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` 134- [ ] `CC=gcc CXX=g++; [ $(uname -m) = 'x86_64' ] && meson setup -Dabi=deprecated,stable build` 135- [ ] `meson compile -C build` 136- [ ] `./scripts/abi-dump-formatter < build/src/current.dump > abi/x86_64/gcc.dump` 137 138## Removing an API 139 140- [ ] If the function is marked `LIBPLDM_ABI_TESTING`, then I have removed it 141 142- [ ] If the function is marked `LIBPLDM_ABI_STABLE`, then I have changed the 143 annotation to `LIBPLDM_ABI_DEPRECATED` and left it in-place. 144 145- [ ] If the function is marked `LIBPLDM_ABI_DEPRECATED`, then I have removed it 146 only after satisfying myself that each of the following is true: 147 148 - [ ] There are no known users of the function left in the community 149 - [ ] There has been at least one tagged release of `libpldm` subsequent to 150 the API being marked deprecated 151 152## Testing my changes 153 154Each of the following must succeed when executed in order. Note that to avoid 155[googletest bug #4232][googletest-issue-4232] you must avoid using GCC 12 156(shipped in Debian Bookworm). 157 158[googletest-issue-4232]: https://github.com/google/googletest/issues/4232 159 160- [ ] `meson setup -Dabi-compliance-check=disabled build` 161- [ ] `meson compile -C build && meson test -C build` 162 163- [ ] `meson configure --buildtype=release build` 164- [ ] `meson compile -C build && meson test -C build` 165 166- [ ] `meson configure --buildtype=debug build` 167- [ ] `meson configure -Dabi=deprecated,stable build` 168- [ ] `meson compile -C build && meson test -C build` 169