api: add ABI compatibility check and payload validation

[rjarry]

Annotate every API request and event with GR_REQ()/GR_EVENT() macros that associate request codes with their payload types. These macros generate compile-time size constants and can be overridden by client libraries to extract metadata differently.

The server uses these constants to reject requests with truncated payloads (EMSGSIZE) before dispatching to handlers. On the client side, gr_api_client_impl.h overrides the macros to build a runtime exchange registry via constructors, enabling response size validation and a generic code-to-name lookup that replaces hand-maintained switch statements.

At build time, abidiff compares the current API headers against the previous git revision. If breaking struct layout changes are detected and GR_API_VERSION has not been bumped, the build fails. The GR_HELLO handshake now negotiates on this integer instead of comparing version strings, so that compatible builds from different commits can interoperate.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@devtools/gen_version_h.sh`:
- Around line 22-35: The hashing step in gen_version_h.sh omits project
compile-time defines so api_hash (computed in the api_hash variable using $cc -E
-P $flags) can differ from full builds; update the script to include the same
defines used by the build system by appending the project's C/CXX compilation
defines (e.g., meson-provided defines or CFLAGS/CXXFLAGS/CPPFLAGS like
-D_GNU_SOURCE, -DALLOW_EXPERIMENTAL_API) into the flags variable before
preprocessing so the preprocessed output matches full-build macros when
computing api_hash.

In `@main/api.c`:
- Around line 204-214: The handler hello currently dereferences request as a
full struct gr_hello_req and reads api_hash without validating the payload size;
add a guard at the top of hello to ensure request is non-NULL and that the
provided payload length (from the api_ctx passed into hello, e.g.
ctx->payload_len) is at least sizeof(struct gr_hello_req); if not, return
api_out(EBADMSG, 0, NULL) to reject short/malformed GR_HELLO payloads before
accessing req->api_hash. Ensure the check uses the same symbols (hello, struct
gr_hello_req, api_ctx, api_out, EBADMSG, GR_HELLO) so the protection applies to
truncated buffers read by read_cb.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ec49cdea-654e-413a-8e5f-7c18ecc3fd30

📥 Commits

Reviewing files that changed from the base of the PR and between a5520bf and 246c81b.

📒 Files selected for processing (7)

• api/gr_api.h
• api/gr_api_client_impl.h
• api/meson.build
• devtools/gen_version_h.sh
• frr/meson.build
• main/api.c
• meson.build

💤 Files with no reviewable changes (1)

• api/meson.build

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@devtools/gen_version_h.sh`:
- Line 35: The api_hash assignment pipeline using "$cc -E -P $flags -x c
/dev/null | sed ... | sha256sum | cut ..." can silently succeed even if the
compiler fails; modify the script to enable strict failure detection (e.g. set
-o pipefail or explicitly run the preprocessor into a temp file), check the exit
status of the preprocessor invocation ($cc -E -P ...) before feeding its output
to sed/sha256sum, and if the preprocessor fails emit an error and exit non-zero
so that api_hash (the variable assigned from that pipeline) is never populated
with an invalid empty hash and the header generation aborts.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 346fcaed-f497-4a9f-b472-b41888a6ce3e

📥 Commits

Reviewing files that changed from the base of the PR and between 246c81b and 82f62fc.

📒 Files selected for processing (7)

• api/gr_api.h
• api/gr_api_client_impl.h
• api/meson.build
• devtools/gen_version_h.sh
• frr/meson.build
• main/api.c
• meson.build

💤 Files with no reviewable changes (1)

• api/meson.build

✅ Files skipped from review due to trivial changes (3)

• api/gr_api_client_impl.h
• frr/meson.build
• api/gr_api.h

🚧 Files skipped from review as they are similar to previous changes (1)

• main/api.c

[david-marchand]

I did not review yet, just some high level question/suggestion.

Would it help to keep a single version number, but map it to this digest the series introduces?

The idea is to have the digest as a comment in the header defining the version number, and have the build framework throw an error if the newly computed digest does not match.

This should allow adding new structures/features but still be compatible for existings features.

It could also help the other way around, like if grout changes its internal behavior for a strictly identical digest and we bump the version number so that external consumer must be updated.

[rjarry]

Hmm, let me see if I understood your suggestion. You're proposing that we add a manual:

// gr_api.h
#define GR_API_VERSION 1

And we bump it every time we think we make a breaking change in the API.

Then, during build time, we compute the hash based on API header contents.

But I don't understand how you can "compare" a hash with a number. There is a chicken and egg problem. That would mean, we need to store the hash in git to compare it.

And also, that would mean you cannot decide to bump the number because the internal handling of the API warrants a change. If the hash didn't change, the build system will error out because the version number changed without any hash change.

Did I misunderstand something?

[david-marchand]

The digest would be stored along the version.
Like:

#define GR_API_VERSION 1 // digest: XXXXXXXXXXXXXXXXXXXXXXXXXX

[rjarry]

Ah, so at runtime you would only check GR_API_VERSION, and during build you would check the digest?

[david-marchand]

Yes, the build time check is to avoid API breaking change going unnoticed.
We still need to check manually when build fails, and a version bump must then be debatted.

Another option could be to ping libabigail people, if they have better ideas.

[rjarry]

I will see what I can do. That seems more flexible than just the compile time generated hash.

[christophefontaine]

Should we validate only against payload smaller, or should we rather check for the exact length ?

[rjarry]

It would be better to check for exact length, but some requests have variable length (GR_IFACE_ADD, GR_NH_ADD). So we can only enforce a minimum size.

[rjarry]

For reference, here is the output of the new check:

$ PREV_REVISION=HEAD^^^ make 
ninja: Entering directory `build'
[1/1] Generating api_check with a custom command
Checking for API changes between v0.14.3-297-g5e76f273 and v0.14.3-300-g7ac5ff29-dirty

1 changed type unreachable from any public interface:

  [C] 'struct gr_hello_req' changed:
    type size changed from 128 to 132 (in bytes)
    1 data member insertion:
      'uint32_t api_version', at offset 0 (in bytes) at gr_api.h:147:1
    1 data member change:
      'char version[128]' offset changed from 0 to 4 (in bytes) (by +4 bytes)

breaking API changes
GR_API_VERSION changed from  to 1

[david-marchand]

One reference was left behind:

rjarry/api-version-hash:main/api.c:// GR_EVENT_IFACE_UNKNOWN | 0x0000 |

[rjarry]

It is unused anywhere. I didn't think necessary to keep this.

[david-marchand]

Yes, then please remove the last occurence.

[david-marchand]

Why break ABI?

IIUC, GR_LOG_PACKETS_SET was 0xCAFE0010 so far.

[rjarry]

I though that we could simplify the definitions of these request/event codes with enums and at this point (commit 4bb6da7) there is virtually no abi compatibility possible since clients built on a different commit hash from the server get rejected anyway.

[david-marchand]

Yes true.
Now, la récréation est finie.
No more gratuitous breakage.

[david-marchand]

I would have kept the REQUEST_TYPE macro.

enum gr_main_requests : uint32_t {
	GR_HELLO = REQUEST_TYPE(GR_MAIN_MODULE, 0x1981),

[rjarry]

Since IDs are now part of enums, this would be used only once per module. Do you think it still has any value?

[david-marchand]

I don't mind.

[david-marchand]

Same here, some ABI breakage, if I count correctly.

[rjarry]

Yes, see the reasoning in my previous comment.

[david-marchand]

Please cleanup main/api.c.

I see:
// GR_EVENT_IFACE_UNKNOWN | 0x0000 |

But also:
main/api.c:// GR_EVENT_IFACE_ADD | 0x0001 |
modules/infra/api/gr_infra.h: GR_EVENT_IFACE_ADD = 0xacdc1001u,

main/api.c:// GR_EVENT_NEXTHOP_UPDATE -> | 0x0102 | -> vec of evutil_socket_t
modules/infra/api/gr_nexthop.h: GR_EVENT_NEXTHOP_NEW = 0xd0171001,
modules/infra/api/gr_nexthop.h: GR_EVENT_NEXTHOP_DELETE,
modules/infra/api/gr_nexthop.h: GR_EVENT_NEXTHOP_UPDATE,

The series lgtm otherwise.

[rjarry]

I have added back a GR_MSG_TYPE() macro which does what REQUEST_TYPE() and EVENT_TYPE() did, without the duplication. Also added a few assertions that we don't have duplicate request/event type ids and that gr_api_client_impl.h is included first.