RegisterResponder API #98

msnidhin · 2025-07-25T06:52:13Z

GetMsgType response is hardcoded and always return only control message support.
Create a Dbus method to register MCTP types and reply the same in GetMsgType and GetVersion

Tested by sending GetMsgType and GetVersion from another EP
Default responses for GetMsgType and GetVersion for type 0
From command code
5, 0, 1, 0 and 4, 0, 4, 241, 240, 255, 0, 241, 241, 255, 0, 241, 242, 255, 0, 241, 243, 241, 0

After calling
busctl call au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1 au.com.codeconstruct.MCTP1 RegisterResponder yau 5 1 0xF1F2F3F4
Response for GetMsgType and GetVersion for type 5
5, 0, 2, 0, 5 and 4, 0, 1, 244, 243, 242, 241

jk-ozlabs

Thanks for the contribution. A couple of comments inline, and can you add some tests too?

jk-ozlabs · 2025-07-25T06:56:47Z

src/mctpd.c

+	size_t count;
+
+	// Assume array index follows same order. Msg type at msg_types[0]
+	// will have version_counts[0] versions stored at versions[0]


Better to use an array of structs for this, rather than three separate arrays. You'll probably find that this simplifies the registration and setup too.

You probably don't need this parent struct, as you only ever create one of them. All of the other counted arrays use that style.

I thought associating version with msg type will be easy with this parent struct
Like

struct msg_type_support { uint8_t type; size_t num_versions; uint32_t *versions; }; struct msg_type_support* supported_msg_types; size_t num_msg_types; // Use like supported_msg_types[0].type; supported_msg_types[0].versions;

Yep, that looks better.

This is changed now

jk-ozlabs · 2025-07-25T06:58:09Z

src/mctpd.c

+	uint32_t *ctrl_cmd_versions = NULL;
+	ctx->supported_msg_types.msg_types = NULL;
+	ctx->supported_msg_types.versions = NULL;
+	ctx->supported_msg_types.version_counts = NULL;
+
 	ctx->mctp_timeout = 250000; // 250ms
 	ctx->default_role = ENDPOINT_ROLE_BUS_OWNER;
+	ctx->supported_msg_types.count = 0;
+
+	// Default to supporting only control messages
+	ctx->supported_msg_types.msg_types = malloc(1);
+	if (!ctx->supported_msg_types.msg_types) {
+		warnx("Out of memory for supported message types");
+		goto oom_err;
+	}
+	ctrl_cmd_versions = malloc(sizeof(uint32_t) * 4);
+	if (!ctrl_cmd_versions) {
+		warnx("Out of memory for versions");
+		goto oom_err;
+	}
+	ctx->supported_msg_types.versions = malloc(sizeof(uint32_t *));
+	if (!ctx->supported_msg_types.versions) {
+		warnx("Out of memory for versions");
+		goto oom_err;
+	}
+	ctx->supported_msg_types.version_counts = malloc(sizeof(size_t));
+	if (!ctx->supported_msg_types.version_counts) {
+		warnx("Out of memory for version counts");
+		goto oom_err;
+	}
+
+	ctx->supported_msg_types.count = 1;
+	ctx->supported_msg_types.msg_types[0] = MCTP_CTRL_HDR_MSG_TYPE;
+	ctrl_cmd_versions[0] = htonl(0xF1F0FF00);
+	ctrl_cmd_versions[1] = htonl(0xF1F1FF00);
+	ctrl_cmd_versions[2] = htonl(0xF1F2FF00);
+	ctrl_cmd_versions[3] = htonl(0xF1F3F100);
+	ctx->supported_msg_types.versions[0] = ctrl_cmd_versions;
+	ctx->supported_msg_types.version_counts[0] = 4;
+
+	return;
+oom_err:
+	free(ctrl_cmd_versions);
+	free(ctx->supported_msg_types.msg_types);
+	free(ctx->supported_msg_types.versions);
+	free(ctx->supported_msg_types.version_counts);
+	ctx->supported_msg_types.msg_types = NULL;
+	ctx->supported_msg_types.versions = NULL;
+	ctx->supported_msg_types.version_counts = NULL;


These aren't really config options, so this init doesn't belong in the config_defaults setup.

Sure. Can you suggest a better place to put thsese ?

Just where we first set up the struct ctx. I would suggest just a helper function called from main.

Moved to a new function

jk-ozlabs · 2025-07-25T06:58:31Z

src/mctpd.c

+	method_register_responder,
+	0),
+	SD_BUS_VTABLE_END,
+};


Any dbus API changes should be documented in mctpd.md

Also, how are you thinking of handling unregister?

Leaving this to later is fine, we may at least need to accommodate a design for that in the registration function.

I just want to add that if we choose to require the message type lifetime to be bound to a D-Bus peer, existing sd_bus_track API might be used. If D-Bus peer disappears, we can remove the type(s) registered by the peer (which should probably be a daemon of some kinds)

Sounds like a great idea; we would just sd_bus_track_add_sender from the source.

... In which case we do not need any lifetime-management considerations in the proposed API. We would just need to document that the lifetime is managed by mctpd.

Unregister handling is pending. Will push that also.

GetMsgType response is hardcoded and always return only control message support. Read supported message types from config file and reply in control message Tested by sending GetMsgType and GetVersion from another EP Default responses for GetMsgType and GetVersion for type 0 5, 0, 1, 0 and 4, 0, 4, 241, 240, 255, 0, 241, 241, 255, 0, 241, 242, 255, 0, 241, 243, 241, 0 After calling busctl call au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1 au.com.codeconstruct.MCTP1 RegisterResponder yau 5 1 0xF1F2F3F4 Response for GetMsgType and GetVersion for type 5 5, 0, 2, 0, 5 and 4, 0, 1, 244, 243, 242, 241 Signed-off-by: Nidhin MS <[email protected]>

msnidhin · 2025-08-01T08:09:42Z

Thanks for the contribution. A couple of comments inline, and can you add some tests too?

Is the test steps in doc still valid ?
I was getting an error in ninja -C obj test command
../src/mctp-netlink.h:37:23: error: ‘IFNAMSIZ’ undeclared here (not in a function)

jk-ozlabs · 2025-08-05T01:20:43Z

../src/mctp-netlink.h:37:23: error: ‘IFNAMSIZ’ undeclared here (not in a function)

that's odd, we should get if.h; but I'll add an explicit include. What environment are you compiling this on?

msnidhin · 2025-08-05T09:03:38Z

../src/mctp-netlink.h:37:23: error: ‘IFNAMSIZ’ undeclared here (not in a function)
that's odd, we should get if.h; but I'll add an explicit include. What environment are you compiling this on?

Im compiling from an Ubuntu server system. Ubuntu 22.04 LTS
gcc (Ubuntu 13.1.0-8ubuntu1~22.04) 13.1.0

jk-ozlabs · 2025-08-07T02:45:23Z

(the PR fixing this has been merged, feel free to rebase for testing)

msnidhin · 2025-08-07T04:36:35Z

(the PR fixing this has been merged, feel free to rebase for testing)

Hi. Sorry if this is getting long. Im hit with another issue
../src/mctp-bench.c:322:2: error: #error No ALLOCTAG ioctl available
322 | #error No ALLOCTAG ioctl available

Is there any prerequisites for the environment to run the test? Im trying that two commands only
meson setup obj and ninja -C obj test

jk-ozlabs · 2025-08-07T05:01:27Z

The issue is that the kernel headers you are building against predate a lot of the MCTP support. Even if we get the build working, you are unlikely to be able to actually run any of the binaries produced on that system.

That said, if you want to just run the tests (they're independent of running kernel version), you may be able to work-around the above issue by disabling the mctp-bench build.

Something like:

diff --git a/meson.build b/meson.build
index f93d28c..fc4e0bb 100644
--- a/meson.build
+++ b/meson.build
@@ -71,9 +71,9 @@ executable('mctp-echo',
     sources: ['src/mctp-echo.c'] + util_sources,
 )
 
-executable('mctp-bench',
-    sources: ['src/mctp-bench.c'] + util_sources,
-)
+#executable('mctp-bench',
+#    sources: ['src/mctp-bench.c'] + util_sources,
+#)
 
 executable('mctp-client',
     sources: ['src/mctp-client.c'] + util_sources,

jk-ozlabs · 2025-08-07T05:06:47Z

I see that the original release of 22.04 used 5.15, which would explain those errors. However, there has subsequently been an update to 22.04 that brings it up to 6.8, which would be fine for these builds.

jk-ozlabs requested changes Jul 25, 2025

View reviewed changes

msnidhin force-pushed the get_msg_types branch from e55310e to b79fdd2 Compare August 1, 2025 08:05

RegisterResponder API #98

Are you sure you want to change the base?

RegisterResponder API #98

Uh oh!

Conversation

msnidhin commented Jul 25, 2025

Uh oh!

jk-ozlabs left a comment

Choose a reason for hiding this comment

Uh oh!

jk-ozlabs Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msnidhin commented Aug 1, 2025

Uh oh!

jk-ozlabs commented Aug 5, 2025

Uh oh!

msnidhin commented Aug 5, 2025

Uh oh!

jk-ozlabs commented Aug 7, 2025

Uh oh!

msnidhin commented Aug 7, 2025

Uh oh!

jk-ozlabs commented Aug 7, 2025

Uh oh!

jk-ozlabs commented Aug 7, 2025

Uh oh!

Uh oh!

jk-ozlabs Jul 25, 2025 •

edited

Loading