mctpd: Add VendorDefinedMessageTypes property #136
Conversation
msnidhin
force-pushed
the
vdm_types
branch
3 times, most recently
from
fa05dfb to
dad0d6c
Compare
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). Signed-off-by: Nidhin MS <[email protected]>
jk-ozlabs
left a comment
jk-ozlabs
left a comment
There was a problem hiding this comment.
Nice! This looks good, just a few changes/comments inline.
| goto out; | ||
|
|
||
| resp = (void *)buf; | ||
| if (resp->vendor_id_format != |
There was a problem hiding this comment.
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.
| goto out; | ||
|
|
||
| /* Check for minimum length of PCIe VDM*/ | ||
| expect_size = sizeof(*resp); |
There was a problem hiding this comment.
This is a bit fragile given the somewhat quirky definition of struct mctp_ctrl_resp_get_vdm_support. We're really including spce for the IANA-format response (because the union will be the max size possible), but then relying on the later fields being absent from the struct, which coincidentally aligns with the correct size for a PCIe format.
We should fix that definition to not include format-specific data at all. In which case, a fixed value here might be more appropriate.
| 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; |
There was a problem hiding this comment.
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.
| free(buf); | ||
| if (rc < 0) { | ||
| free(peer->vdm_types); | ||
| peer->vdm_types = NULL; |
There was a problem hiding this comment.
May be better to store these temporarily, and only update the peer-> members on success?
| } | ||
| } | ||
|
|
||
| for (unsigned int i = 0; supports_vdm && i < max_retries; i++) { |
There was a problem hiding this comment.
The intent would be more obvious with supports_vdm as an explicit conditional, rather than hiding it in the loop conditional.
| "a(yvu)", | ||
| bus_endpoint_get_prop, | ||
| 0, | ||
| SD_BUS_VTABLE_PROPERTY_CONST), |
There was a problem hiding this comment.
Please add to the dbus API spec (in docs/mctpd.md) too.
| iface = mctpd.system.interfaces[0] | ||
| mctp = await mctpd_mctp_iface_obj(dbus, iface) | ||
|
|
||
| # Test 1: Invalid PCIe length |
There was a problem hiding this comment.
If these are different tests, please make them separate test functions. You can share the endpoint classes if that makes things easier.
| """ 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): |
There was a problem hiding this comment.
This could go in the main Endpoint object, like the general type support, using a vdm_msg_types member as the data source.
2 participants
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).