mctp-req: Make mctp-req a generic mctp client #82
Conversation
Changes mctp-req to take and receive arbitrary mctp messages. Also makes a change to include it in the install by default. Testing send a simple get-eid command to an endpoint at eid 8: ``` mctp-req type 0 eid 8 data 0x80:0x02 0x00:0x02:0x00:0x08:0x01:0x00 mctp-req type 0 eid 9 data 0x80:0x02 0x00:0x02:0x00:0x09:0x01:0x00 ``` testing default eid stays at 8 ``` mctp-req type 0 data 0x80:0x02 0x00:0x02:0x00:0x08:0x01:0x00 ``` testing corner cases for parsing ``` mctp-req data 0x80:0x02 mctp-req [eid <eid>] [net <net>] type <type> [ifindex <ifindex> lladdr <hwaddr>] data <data> default eid 8 net 1 mctp-req type 0 mctp-req [eid <eid>] [net <net>] type <type> [ifindex <ifindex> lladdr <hwaddr>] data <data> default eid 8 net 1 mctp-req type 0 data mctp-req: extra argument data mctp-req [eid <eid>] [net <net>] type <type> [ifindex <ifindex> lladdr <hwaddr>] data <data> default eid 8 net 1 mctp-req data type 0 mctp-req: extra argument 0 mctp-req [eid <eid>] [net <net>] type <type> [ifindex <ifindex> lladdr <hwaddr>] data <data> default eid 8 net 1 ``` Signed-off-by: Marc Olberding <marcolberding@gmail.com>
| "payload mismatch at byte 0x%zx; " | ||
| "sent 0x%02x, received 0x%02x", | ||
| i, exp, buf[i]); | ||
| for (i = 0; i < recvlen; ++i) { |
There was a problem hiding this comment.
There was some back and forth on whether we should use space delimited or colon delimited hex strings. This uses colon delimited strings for the sake of consistency between send and receive side, as the send side was already using colon delimited strings. Part of me thinks that json would be a better interface on both sides as we can just use jq to parse it instead of handrolled bash, but open to thoughts.
There was a problem hiding this comment.
I find the proposed output fairly difficult to scan - there are a lot of superfluous characters in:
0x00:0x02:0x00:0x08:0x01:0x00
would it work to use mctp_hexdump for the output format?
If you're looking for programmatic interfaces here (to support json or scripted automation), I would suggest that we would want something other than mctp-req for that. This is really intended as a debug tool, rather than something for production MCTP communication.
(that's why we're not currently installing it by default...)
There was a problem hiding this comment.
I didn't really have production flows in mind per se, but mostly for things like hardware bring up where you need to prove out functionality quickly. Having scripts that you can hand out to people before full firmware support is up is a boon and in my mind, a hard to use interface won't stop people from sneaking in bad hacks like using this in a production flow. That said I'm not going to die on this hill and json isn't the end all be all, my thought was that it would allow the user to see the debug info we currently output while not making scripting more difficult than it has to be.
There was a problem hiding this comment.
ok, sounds like we're on the same page then; I have seen a lot of scripting hacks escape into production though!
For the bringup support objective, I would suggest focus on a human-readable interface, provided we don't stray too far from "possible to parse".
Consistency is good; would space-separated work for both input and output?
There was a problem hiding this comment.
I think that's a reasonable compromise.
There was a problem hiding this comment.
... and maybe assume hex in data, so the 0x could be optional.
There was a problem hiding this comment.
I don't really have a strong opinion, it would be a little odd to use decimal here in any case, but we should probably spell that out in the usage text
|
|
||
| // Code Construct allocation | ||
| static const uint8_t VENDOR_TYPE_ECHO[3] = { 0xcc, 0xde, 0xf0 }; | ||
| static const uint8_t MCTP_TYPE_VENDOR_PCIE = 0x7e; |
There was a problem hiding this comment.
We still want to be able to interact with the echo service; I would prefer if we can still describe vendor types here - or even retaining a shortcut for sending an echo request.
There was a problem hiding this comment.
I'm fine with having an echo shortcut, my original thinking was that its not terribly difficult to script the existing behavior but would something like mctp-req echo instead of specifying data and type make sense to you?
There was a problem hiding this comment.
I think so. I was initially imagining something like
mctp-req type pci:ccde len 5
... but then we also need the subtype byte (0xf0), which is vendor specific. We can get around that by using the
data parameter, but now you have to specify all the data :/ So using a generic format for vendor types isn't really practical.
Would it work to allow named type aliases? like:
mctp req type echo len 5
and even extending this to existing types:
mctp req type control data 0x02
There was a problem hiding this comment.
mctp-req type echo len 5
mctp-req type control data 0x02
Could probably skip the type keyword for those cases?
There was a problem hiding this comment.
To be honest, it really does feel like echo should live in a script that uses mctp-req or we just make a separate executable that is an arbitrary mctp client
Yep, the primary purpose of req is to be an echo client. It's now sounding like a separate program would be a better approach.
I'm happy to rename mctp-req if you'd like to use that for a "generic send-a-message" utility.
There was a problem hiding this comment.
Do you want the echo command to do the existing byte verification as well still?
Ah, yes, there is that too. Yes, I would like to keep that.
There was a problem hiding this comment.
Yeah, I'll gather the feedback here and put up a cleaned up pr for a generic send a message client. Let's call it mctp-client, also let's get the bike shedding on the name done now :)
There was a problem hiding this comment.
Sorry if you were up for more bikeshedding, mctp-client sounds great :D
There was a problem hiding this comment.
Is it even a sub one hundred line change on an obscure utility if it doesn't have thirty comments? :P
Seriously though, I appreciate all the feedback
| "payload mismatch at byte 0x%zx; " | ||
| "sent 0x%02x, received 0x%02x", | ||
| i, exp, buf[i]); | ||
| for (i = 0; i < recvlen; ++i) { |
There was a problem hiding this comment.
I find the proposed output fairly difficult to scan - there are a lot of superfluous characters in:
0x00:0x02:0x00:0x08:0x01:0x00
would it work to use mctp_hexdump for the output format?
If you're looking for programmatic interfaces here (to support json or scripted automation), I would suggest that we would want something other than mctp-req for that. This is really intended as a debug tool, rather than something for production MCTP communication.
(that's why we're not currently installing it by default...)
| } else if (!strcmp(optname, "len")) { | ||
| if (tmp > 64 * 1024) | ||
| errx(EXIT_FAILURE, "Bad len"); | ||
| len = tmp; |
There was a problem hiding this comment.
I'd like to retain the len behaviour for when the data isn't important (ie, for echo)
There was a problem hiding this comment.
Are there other use cases than echo for when we don't care about the data? This was mostly trying to make it difficult to misuse, i.e accidentally truncating data with length for example
There was a problem hiding this comment.
Not really, just echo. However, data and len should be mutually exclusive: either use the data provided, or generate len bytes.
Changes mctp-req to take and receive arbitrary mctp messages. Also makes a change to include it in the install by default.
Testing
send a simple get-eid command to an endpoint at eid 8:
testing default eid stays at 8
testing corner cases for parsing