mctpd: Add VendorDefinedMessageTypes property by msnidhin · Pull Request #136 · CodeConstruct/mctp

msnidhin · 2026-01-15T07:14:24Z

Implement Get VDM Support (0x06) control command to query vendor defined message capabilities from peers. Expose via D-Bus property a(yvu) containing format, vendor_id (PCIe 16-bit or IANA 32-bit), and command set. Includes positive test for both formats and negative tests for invalid responses (wrong lengths, invalid format, unsupported command).

jk-ozlabs

Nice! This looks good, just a few changes/comments inline.

jk-ozlabs · 2026-01-15T23:44:30Z

src/mctpd.c

+			goto out;
+
+		resp = (void *)buf;
+		if (resp->vendor_id_format !=


Minor: Since we re-use resp->vendor_id_format a few times, and the length causes a lot of wrapping, I'd suggest using a temporary (say, fmt) for this.

src/mctpd.c

jk-ozlabs · 2026-01-15T23:56:08Z

src/mctpd.c

+		peer->num_vdm_types++;
+
+		/* Use the next selector from the response. 0xFF indicates no more entries */
+		req.vendor_id_set_selector = resp->vendor_id_set_selector;


Might be cleaner to do the 0xff check here, and rc = 0; break if it matches. Then your while loop can be unconditional, and the error cases can just break, rather than needing a goto between scopes.

jk-ozlabs · 2026-01-15T23:57:37Z

src/mctpd.c

+	free(buf);
+	if (rc < 0) {
+		free(peer->vdm_types);
+		peer->vdm_types = NULL;


May be better to store these temporarily, and only update the peer-> members on success?

jk-ozlabs · 2026-01-15T23:59:03Z

src/mctpd.c

+		}
+	}
+
+	for (unsigned int i = 0; supports_vdm && i < max_retries; i++) {


The intent would be more obvious with supports_vdm as an explicit conditional, rather than hiding it in the loop conditional.

src/mctpd.c

jk-ozlabs · 2026-01-16T00:01:10Z

tests/test_mctpd.py

+    iface = mctpd.system.interfaces[0]
+    mctp = await mctpd_mctp_iface_obj(dbus, iface)
+
+    # Test 1: Invalid PCIe length


If these are different tests, please make them separate test functions. You can share the endpoint classes if that makes things easier.

jk-ozlabs · 2026-01-16T00:02:41Z

tests/test_mctpd.py

+""" Test that VendorDefinedMessageTypes property is queried and populated correctly """
+async def test_query_vdm_types(dbus, mctpd):
+    class VDMEndpoint(Endpoint):
+        async def handle_mctp_control(self, sock, addr, data):


This could go in the main Endpoint object, like the general type support, using a vdm_msg_types member as the data source.

msnidhin · 2026-01-18T05:38:15Z

Waiting for https://gerrit.openbmc.org/c/openbmc/phosphor-dbus-interfaces/+/86652 to finalize

jk-ozlabs

Looking good for the new spec, but a couple of things inline.

jk-ozlabs · 2026-02-19T08:05:18Z

tests/test_mctpd.py

    assert ep_types == query_types


+""" Test that VendorDefinedMessageTypes property is queried and populated correctly """


Docstrings should go inside the function definition, as the first line.

Please check out the spacing requirements in PEP257 too - no space immediately within the quotes, and """ on its own line for multi-line docstrings.

(this is all part of the new checks, which had landed since your last push)

jk-ozlabs · 2026-02-19T08:06:21Z

tests/test_mctpd.py

+    assert len(vdm_types) == 0
+
+
+""" Test VDM query with invalid PCIe length """


We can probably deduce this from the function name. If you add a docstring, make it a little more explanatory (or drop if not needed)

jk-ozlabs · 2026-02-19T08:07:40Z

tests/mctpenv/__init__.py

+                    resp = resp + list(cur_vdm[1].to_bytes(4, 'big'))
+                else:
+                    await sock.send(raddr, bytes(hdr + [0x02]))
+                    return


I'd suggest handing this error case before constructing the response.

However, a value other than 0 or 1 would indicate an issue in the test suite, no?

jk-ozlabs · 2026-02-19T08:10:15Z

src/mctpd.c

+			if (rc < 0)
+				return rc;
+
+			rc = sd_bus_message_append(reply, "u",


The new spec defines this as q, no?

jk-ozlabs · 2026-02-19T08:13:28Z

src/mctpd.c

 			0,
 			SD_BUS_VTABLE_PROPERTY_CONST),
+	SD_BUS_PROPERTY("VendorDefinedMessageTypes",
+			"a(yvu)",


jk-ozlabs · 2026-02-19T08:16:22Z

src/mctpd.c

+	if (supports_vdm) {
+		for (unsigned int i = 0; i < max_retries; i++) {
+			rc = query_get_peer_vdm_types(peer);
+			// Success


Probably don't need this comment.

src/mctpd.c

jk-ozlabs · 2026-02-19T08:22:38Z

src/mctpd.c

+			break;
+
+		/* Check for minimum length of PCIe VDM*/
+		expect_size = sizeof(*resp) + sizeof(struct mctp_vdm_pcie_data);


Maybe just check for the base here, then apply the full expect_size check in the same place you are doing for PCIe (and that will allow you to handle the !PCIE_FORMAT && !IANA_FORMAT as an else-case there)

Implement Get VDM Support (0x06) control command to query vendor defined message capabilities from peers. Expose via D-Bus property a(yvq) containing format, vendor_id (PCIe 16-bit or IANA 32-bit), and command set. Includes positive test for both formats and negative tests for invalid responses (wrong lengths, invalid format, unsupported command). Signed-off-by: Nidhin MS <nidhin.ms@intel.com>

jk-ozlabs

Looks good, thanks!

jk-ozlabs · 2026-02-23T09:40:31Z

src/mctpd.c

+			peer->num_vdm_types = num_vdm_types;
+			rc = 0;
+			break;
+		}


We have a remote memory exhaustion problem here: the remote endpoint could return the same set selector repeatedly, causing an infinite allocation of vdm_types.

I have a fix in progress, will merge along with this.

We have a remote memory exhaustion problem here: the remote endpoint could return the same set selector repeatedly, causing an infinite allocation of vdm_types.

I have a fix in progress, will merge along with this.

Thank You for merging and fixing the issue !!

msnidhin force-pushed the vdm_types branch 3 times, most recently from fa05dfb to dad0d6c Compare January 15, 2026 07:51

jk-ozlabs requested changes Jan 16, 2026

View reviewed changes

msnidhin force-pushed the vdm_types branch 3 times, most recently from 171d7fd to a964b55 Compare February 19, 2026 06:13

jk-ozlabs requested changes Feb 19, 2026

View reviewed changes

msnidhin force-pushed the vdm_types branch 2 times, most recently from a7e5ece to cb182be Compare February 20, 2026 04:55

msnidhin requested a review from jk-ozlabs February 20, 2026 04:57

jk-ozlabs approved these changes Feb 23, 2026

View reviewed changes

jk-ozlabs reviewed Feb 23, 2026

View reviewed changes

jk-ozlabs merged commit cb182be into CodeConstruct:main Feb 23, 2026
3 checks passed

		assert ep_types == query_types


		""" Test that VendorDefinedMessageTypes property is queried and populated correctly """

		assert len(vdm_types) == 0


		""" Test VDM query with invalid PCIe length """

Comments

Conversation

msnidhin commented Jan 15, 2026

Uh oh!

jk-ozlabs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msnidhin commented Jan 18, 2026

Uh oh!

jk-ozlabs left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jk-ozlabs left a comment

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants