Checklist for making changes to `libpldm`

General practices

Organise your work

Solve only one problem per patch

Commit messages

The commit message subject is prefixed with the name of the impacted subsystem
If the change adds support for a new message type, then the 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
The commit message is written in imperative mood

Describe your changes in imperative mood, e.g. “make xyzzy do frotz” instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour.

The commit message describes testing practices only if the discussion is substantive.
- The description must contain enough information for someone else to reproduce the setup and verify the results.
- A section on testing should not be added if there was no specific testing performed

Changelog

New changelog entries document work impacting the users of the library
- For example:
  - Changes to the public headers under include/libpldm
  - Bug-fixes whose impact is observable at runtime
New changelog entries are added at the top of their category's list
Work that has no user impact does not have a new changelog entry
- For example, refactoring the implementation in a way that:
  - Doesn't change the public headers under include/libpldm
  - Doesn't change observable runtime behaviour

Adding a new API

Organising declarations and definitions

Related macros, structs and functions are declared or defined together
- Avoid inserting them each at significantly different line locations
Message-length macros should immediately follow the definition of the related struct.
Message-related API elements are defined in PLDM command-code order
- Refer to the command code tables in each specification
- This order is prefered because the PLDM command-codes must be stable, unlike the organisation of sections in the specification texts.
Avoid unnecessary forward-declarations
- Macros using structs or functions should be declared following those structs or functions, so the context is clear

Naming macros, functions and types

API design

If support is added for a new PLDM message type, then both the encoder and decoder are defined for that message.
- This applies for both request and response message types.
APIs are designed so their implementation does not require heap allocation.
- Prefer defining iterators over the message buffer to extract sub-structures from variable-length messages. Iterators avoid both requiring heap allocation in the implementation or splitting the API to allow the caller to allocate appropriate space. Instead, the caller is provided with an on-stack struct containing the extracted sub-structure.

encode_*() APIs exchange with the caller the size of the destination buffer and the length of the encoded data using an in-out buffer length parameter
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 defined is used in at least one new function added to the public API.
New public struct definitions are not marked __attribute__((packed))
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
- The change implements 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¹
  - 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++.
- Flexible array members are annotated with LIBPLDM_CC_COUNTED_BY()

Strings

PLDM strings are not assumed to be NUL-terminated
Strings are passed-to and received-from libpldm APIs in the appropriately encoded form
- Strings are not encoded or decoded in the libpldm implementation
- The PLDM specifications utilise multiple string encodings. We avoid prescribing a string handling implementation by deferring that responsibility to the caller.
Strings communicated via PLDM messages are referred to using a message struct member of type struct variable_field.
- In both the message encode and decode cases struct variable_field points to a data source: For decode operations the points to the relevant span in the encoded message body.
- If the string is terminated with a NUL value then the span represented by the struct variable_field shall include the NUL terminator appropriate for the encoding.

ABI control

New function symbols are marked with LIBPLDM_ABI_TESTING in the implementation
Test cases of functions marked LIBPLDM_ABI_TESTING are guarded so that they are not compiled when the corresponding function symbols aren't visible

Error handling and invariants

All error conditions are handled by returning an error code to the caller.
All invariants are tested using assert().
assert() is not used 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).
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.

Implementation

If the work interacts with the PLDM wire format, then it does so using the msgbuf APIs found in src/msgbuf.h (and under src/msgbuf/) to minimise concerns around spatial memory safety and endian-correctness.
goto is used to clean up resources acquired prior to encountering an error condition
- Replication of resource cleanup across multiple error paths is error-prone, especially when multiple, dependent resources have been acquired.
acquired resources are released in stack-order
- This should be the case regardless of whether we're in the happy path at the end of object lifetime or an error path during construction.
Variables are declared in reverse-christmas-tree (inverted pyramid) order in any block scopes added or changed.

Testing

Test cases are provided with reasonable branch coverage of each new function I've added

OEM/vendor-specific APIs

The wire format for all OEM messages is documented under docs/oem/${OEM_NAME}/
Public OEM APIs are declared or defined under include/libpldm/oem/${OEM_NAME}/, and installed to the same relative location.
Public OEM APIs are implemented under src/oem/${OEM_NAME}/
Tests for OEM APIs are implemented under tests/oem/${OEM_NAME}/

The ${OEM_NAME} folder must be created with the name of the OEM/vendor in lower case.

Finally, the OEM name must be added to the list of choices for the oem meson option, and the meson.build files updated throughout the tree to guard integration of the OEM extensions.

Stabilising an existing API

The API of interest is currently marked LIBPLDM_ABI_TESTING
The commit message links to a publicly visible patch that makes use of the API
The 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.
Relevant guards are removed from the function's tests so they are always compiled
The OpenBMC CI container has been used to update the ABI dump.

Removing an API

If the function is marked LIBPLDM_ABI_TESTING, then it has been removed
If the function is marked LIBPLDM_ABI_STABLE, then the annotation is changed 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 has only been removed after:
- 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:

Introducing the new API name
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
The old function name has been aliased to the new function name via the libpldm_deprecated_aliases list in meson.build
A semantic patch has been added to migrate users from the old name to the new name

The ABI dump has been updated accordingly.

Fixing Implementation Defects

A Fixes tag is present, identifying the change introducing the defect.

The change fixing the bug includes test cases demonstrating that the bug is fixed.

Background

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.

The ABI lifecycle

---
title: libpldm symbol lifecycle
---
stateDiagram-v2
    direction LR
    [*] --> Testing: Add
    Testing --> Testing: Change
    Testing --> [*]: Remove
    Testing --> Stable: Stabilise
    Stable --> Deprecated: Deprecate
    Deprecated --> [*]: Remove

The ABI of the library produced by the build is controlled using the abi meson option. The following use cases determine how the abi option should be specified:

Use Case	Meson Configuration
Production	`-Dabi=deprecated,stable`
Maintenance	`-Dabi=stable`
Development	`-Dabi=deprecated,stable,testing`

Maintenance

Applications and libraries that depend on libpldm can identify how to migrate off of deprecated APIs by constraining the library ABI to the stable category. This will force the compiler identify any call-sites that try to link against deprecated symbols.

Development

Applications and libraries often require functionality that doesn't yet exist in libpldm. The work is thus in two parts:

Add the required APIs to libpldm
Use the new APIs from libpldm in the dependent application or library

Adding APIs to a library is a difficult task. Generally, once an API is exposed in the library's ABI, any changes to the API risk breaking applications already making use of it. To make sure we have more than one shot at getting an API right, all new APIs must first be exposed in the testing category. Concretely:

Patches adding new APIs MUST mark them as testing and MUST NOT mark them as stable.

Marking functions as testing, stable or deprecated

Three macros are provided through config.h (automatically included for all translation units) to mark functions as testing, stable or deprecated:

LIBPLDM_ABI_TESTING
LIBPLDM_ABI_STABLE
LIBPLDM_ABI_DEPRECATED

These annotations go immediately before your function signature:

LIBPLDM_ABI_TESTING
pldm_requester_rc_t pldm_transport_send_msg(struct pldm_transport *transport,
                                            pldm_tid_t tid,
                                            const void *pldm_req_msg,
                                            size_t req_msg_len)
{
    ...
}

What does it mean to mark a function as stable?

Marking a function as stable makes the following promise to users of the library:

We will not remove or change the symbol name, argument count, argument types, return type, or interpretation of relevant values for the function before first marking it as LIBPLDM_ABI_DEPRECATED and then subsequently creating a tagged release

Marking a function as stable does not promise that it is free of implementation bugs. It is just a promise that the prototype won't change without notice.

Given this, it is always okay to implement functions marked stable in terms of functions marked testing inside of libpldm. If we remove or change the prototype of a function marked testing the only impact is that we need to fix up any call sites of that function in the same patch.

Requirements for stabilising a function

To move a function from the testing category to the stable category, it's required that patches demonstrating use of the function in a dependent application or library be linked in the commit message of the stabilisation change. We require this to demonstrate that the implementer has considered its use in context before preventing us from making changes to the API.

Building a dependent application or library against a testing ABI

Meson is broadly used in the OpenBMC ecosystem, the historical home of libpldm. Meson's subprojects are a relatively painless way of managing dependencies for the purpose of developing complex applications and libraries. Use of libpldm as a subproject is both supported and encouraged.

libpldm's ABI can be controlled from a parent project through meson's subproject configuration syntax:

meson setup ... -Dlibpldm:abi=deprecated,stable,testing ...

Updating an ABI dump

To update the ABI dump you'll need to build an appropriate OpenBMC CI container image of your own. Some hints on how to do this locally can be found in the openbmc/docs repository. You can list your locally built images with docker images.

Assuming:

export OPENBMC_CI_IMAGE=openbmc/ubuntu-unit-test:2024-W21-ce361f95ff4fa669

the ABI dump can be updated with:

docker run \
  --cap-add=sys_admin \
  --rm=true \
  --privileged=true \
  -u $USER \
  -w $(pwd) \
  -v $(pwd):$(pwd) \
  -e MAKEFLAGS= \
  -t $OPENBMC_CI_IMAGE \
  ./scripts/abi-dump-updater

Changelog

The purpose of the changelog is to highlight changes that might interest users of the library who are not also developers of the library. As such, reorganisation or refactoring of implementation should not feature there. That type of work is captured in the commit history, which is readily available to libpldm developers.

Changelog entries broadly fall into one of two cases

Simple: Listing the commit title is enough

For example:
- fru: Add enum values for all possible commands
Complex: Users need to pay attention to specific details

For example:
- Returned error values for the following stable APIs have changed their semantics:
  - decode_descriptor_type_length_value()
  - decode_event_message_buffer_size_resp()
  - decode_get_numeric_effecter_value_resp()
  - decode_get_sensor_reading_resp()
  - decode_get_state_sensor_readings_resp()
  - decode_numeric_sensor_data()
  - decode_sensor_op_data()
  No new error values will be returned, but existing error values may be returned under new conditions.