Checklist for making changes to libpldm

Philosophy and influences

• Good Practices in Library Design, Implementation, and Maintenance - Ulrich Drepper

• How Do I Make This Hard to Misuse? - Rusty Russell

• What If I Don't Actually Like My Users? - Rusty Russell

• Red flags that indicate questionable quality - Lennart Poettering

• Not sure if this is a gcc bug or some weird corner of UB or what... - Andrew Zonenberg

• The Good, the Bad, and the Weird - Trail of Bits Blog

• Logic for Programmers - Hillel Wayne

• Parse, don’t validate - Alexis King

References

• C17 draft standard

• SEI CERT C Coding Standard

• Common Weakness Enumeration (CWE) - Software Development

Definitions

• Error condition: An invalid state reached at runtime, caused either by resource exhaustion, or incorrect use of the library's public APIs and data types.

• Invariant: A condition in the library's implementation that must never evaluate false.

• Public API: Any definitions and declarations under include/libpldm.

• Wire format: Any message structure defined in the DMTF PLDM protocol specifications.

Elaborations

• Resource exhaustion is always an error condition and never an invariant violation.

• An invariant violation is always a programming failure of the library's implementation, and never the result of incorrect use of the library's public APIs (see error condition).

• Corollaries of the above two points:

  • Incorrect use of public API functions is always an error condition, and is dealt with by returning an error code.

  • Incorrect use of static functions in the library's implementation is an invariant violation which may be established using assert().

• assert() is the recommended way to demonstrate invariants are upheld.

Adding a new API

• [ ] My new public message codec functions take a struct representing the message as a parameter

  • Function prototypes must not decompose the message to individual parameters. This approach is not ergonomic and is difficult to make type-safe. This is especially true for message decoding functions which must use pointers for out-parameters, where it has often become ambiguous whether the underlying memory represents a single object or an array.

• [ ] Each new struct I've defined is used in at least one new function I've added to the public API.

• [ ] My new public struct definitions are not marked __attribute__((packed))

• [ ] My new public struct definitions do not define a flexible array member, unless:

  • [ ] It's contained in an #ifndef __cplusplus macro guard, as flexible arrays are not specified by C++, and

  • [ ] I've implemented an accessor function so the array base pointer can be accessed from C++, and

  • [ ] It is defined as per the C17 specification by omitting the length[^1]

    • Note: Any array defined with length 1 is not a flexible array, and any access beyond the first element invokes undefined behaviour in both C and C++.

  • [ ] I've annotated the flexible array member with LIBPLDM_CC_COUNTED_BY()

[^1]: C17 draft specification, 6.7.2.1 Structure and union specifiers, paragraph 18.

• [ ] If my work interacts with the PLDM wire format, then I have done so using the msgbuf APIs found in src/msgbuf.h (and under src/msgbuf/) to minimise concerns around spatial memory safety and endian-correctness.

• [ ] All my error conditions are handled by returning an error code to the caller.

• [ ] All my invariants are tested using assert().

• [ ] I have not used assert() to evaluate any error conditions without also handling the error condition by returning an error code the the caller.

  • Release builds of the library are configured with assert() disabled (-Db_ndebug=if-release, which provides -DNDEBUG in CFLAGS).

• [ ] My new APIs return negative errno values on error and not PLDM completion codes.

  • [ ] The specific error values my function returns and their meaning in the context of the function call are listed in the API documentation.

• [ ] If I've added support for a new PLDM message type, then I've implemented both the encoder and decoder for that message. Note this applies for both request and response message types.

• [ ] My new function symbols are marked with LIBPLDM_ABI_TESTING in the implementation

• [ ] I've implemented test cases with reasonable branch coverage of each new function I've added

• [ ] I've guarded the test cases of functions marked LIBPLDM_ABI_TESTING so that they are not compiled when the corresponding function symbols aren't visible

• [ ] If I've added support for a new message type, then my commit message specifies all of:

  • [ ] The relevant DMTF specification by its DSP number and title
  • [ ] The relevant version of the specification
  • [ ] The section of the specification that defines the message type

• [ ] If my work impacts the public API of the library, then I've added an entry to CHANGELOG.md describing my work

Stabilising an existing API

• [ ] The API of interest is currently marked LIBPLDM_ABI_TESTING

• [ ] My commit message links to a publicly visible patch that makes use of the API

• [ ] My commit updates the annotation from LIBPLDM_ABI_TESTING to LIBPLDM_ABI_STABLE only for the function symbols demonstrated by the patch linked in the commit message.

• [ ] I've removed guards from the function's tests so they are always compiled

• [ ] If I've updated the ABI dump, then I've used the OpenBMC CI container to do so.

Updating an ABI dump

Each of the following must succeed:

• [ ] Enter the OpenBMC CI Docker container
  • Approximately: 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
• [ ] CC=gcc CXX=g++; [ $(uname -m) = 'x86_64' ] && meson setup -Dabi=deprecated,stable build
• [ ] meson compile -C build
• [ ] ./scripts/abi-dump-formatter < build/src/current.dump > abi/x86_64/gcc.dump

Removing an API

• [ ] If the function is marked LIBPLDM_ABI_TESTING, then I have removed it

• [ ] If the function is marked LIBPLDM_ABI_STABLE, then I have changed the annotation to LIBPLDM_ABI_DEPRECATED and left it in-place.

  • [ ] I have updated the ABI dump, or will mark the change as WIP until it has been.

• [ ] If the function is marked LIBPLDM_ABI_DEPRECATED, then I have removed it only after satisfying myself that each of the following is true:

  • [ ] There are no known users of the function left in the community
  • [ ] There has been at least one tagged release of libpldm subsequent to the API being marked deprecated

Renaming an API

A change to an API is a pure rename only if there are no additional behavioural changes. Renaming an API with no other behavioural changes is really two actions:

1. Introducing the new API name
2. Deprecating the old API name

• [ ] Only the name of the function has changed. None of its behaviour has changed.

• [ ] Both the new and the old functions are declared in the public headers

• [ ] I've aliased the old function name to the new function name via the libpldm_deprecated_aliases list in meson.build

• [ ] I've added a semantic patch to migrate users from the old name to the new name

• [ ] I've updated the ABI dump to capture the rename, or will mark the change as WIP until it has been.