Skip to content

mctp-req: Make mctp-req a generic mctp client #82

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

mctp-req: Make mctp-req a generic mctp client #82

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ executable('mctp',

executable('mctp-req',
sources: ['src/mctp-req.c'] + util_sources,
install: true
)

executable('mctp-echo',
Expand Down
125 changes: 55 additions & 70 deletions src/mctp-req.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,18 @@

static const int DEFAULT_NET = 1;
static const mctp_eid_t DEFAULT_EID = 8;
static const size_t DEFAULT_LEN = 1;

// Code Construct allocation
static const uint8_t VENDOR_TYPE_ECHO[3] = { 0xcc, 0xde, 0xf0 };
static const uint8_t MCTP_TYPE_VENDOR_PCIE = 0x7e;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that works for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mctp-req type echo len 5
mctp-req type control data 0x02

Could probably skip the type keyword for those cases?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@molbear molbear Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if you were up for more bikeshedding, mctp-client sounds great :D

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/* lladdrlen != -1 to ignore ifindex/lladdr */
static int mctp_req(unsigned int net, mctp_eid_t eid,
unsigned int ifindex, uint8_t *lladdr, int lladdrlen,
uint8_t *data, size_t len)
uint8_t *data, size_t len, uint8_t type)
{
struct sockaddr_mctp_ext addr;
unsigned char *payload, *buf;
unsigned char *rxbuf;
socklen_t addrlen;
int rc, sd, val;
size_t i, buf_len;
size_t recvlen;
size_t i;

sd = socket(AF_MCTP, SOCK_DGRAM, 0);
if (sd < 0)
Expand All @@ -46,34 +42,15 @@ static int mctp_req(unsigned int net, mctp_eid_t eid,
addr.smctp_base.smctp_family = AF_MCTP;
addr.smctp_base.smctp_network = net;
addr.smctp_base.smctp_addr.s_addr = eid;
addr.smctp_base.smctp_type = MCTP_TYPE_VENDOR_PCIE;
addr.smctp_base.smctp_type = type;
addr.smctp_base.smctp_tag = MCTP_TAG_OWNER;
printf("req: sending to (net %d, eid %d), type 0x%x\n",
net, eid, addr.smctp_base.smctp_type);

buf_len = len + sizeof(VENDOR_TYPE_ECHO);
buf = malloc(buf_len);
if (!buf)
err(EXIT_FAILURE, "malloc");
memcpy(buf, VENDOR_TYPE_ECHO, sizeof(VENDOR_TYPE_ECHO));
payload = &buf[sizeof(VENDOR_TYPE_ECHO)];

if (data) {
memcpy(payload, data, len);
} else {
for (i = 0; i < len; i++)
payload[i] = i & 0xff;
}

/* extended addressing */
if (lladdrlen != -1) {
addrlen = sizeof(struct sockaddr_mctp_ext);
addr.smctp_halen = lladdrlen;
memcpy(addr.smctp_haddr, lladdr, lladdrlen);
addr.smctp_ifindex = ifindex;
printf(" ext ifindex %d ha[0]=0x%02x len %hhu\n",
addr.smctp_ifindex,
addr.smctp_haddr[0], addr.smctp_halen);
val = 1;
rc = setsockopt(sd, SOL_MCTP, MCTP_OPT_ADDR_EXT, &val, sizeof(val));
if (rc < 0)
Expand All @@ -83,71 +60,64 @@ static int mctp_req(unsigned int net, mctp_eid_t eid,


/* send data */
rc = sendto(sd, buf, buf_len, 0,
rc = sendto(sd, data, len, 0,
(struct sockaddr *)&addr, addrlen);
if (rc != (int)buf_len)
err(EXIT_FAILURE, "sendto(%zd)", buf_len);
if (rc != (int)len)
err(EXIT_FAILURE, "sendto(%zd)", len);

/* receive response */
addrlen = sizeof(addr);
rc = recvfrom(sd, buf, buf_len, MSG_TRUNC,
recvlen = recvfrom(sd, NULL, 0, MSG_PEEK | MSG_TRUNC,
NULL, 0);
if (recvlen < 0)
err(EXIT_FAILURE, "recvfrom(MSG_PEEK)");

rxbuf = calloc(recvlen, sizeof(*rxbuf));
if (!rxbuf)
err(EXIT_FAILURE, "calloc");

rc = recvfrom(sd, rxbuf, recvlen, MSG_TRUNC,
(struct sockaddr *)&addr, &addrlen);
if (rc < 0)
err(EXIT_FAILURE, "recvfrom");
else if ((size_t)rc != buf_len)
else if ((size_t)rc != recvlen)
errx(EXIT_FAILURE, "unexpected length: got %d, exp %zd",
rc, buf_len);
rc, recvlen);

if (!(addrlen == sizeof(struct sockaddr_mctp_ext) ||
addrlen == sizeof(struct sockaddr_mctp)))
errx(EXIT_FAILURE, "unknown recv address length %d, exp %zu or %zu)",
addrlen, sizeof(struct sockaddr_mctp_ext),
sizeof(struct sockaddr_mctp));


printf("req: message from (net %d, eid %d) type 0x%x len %zd\n",
addr.smctp_base.smctp_network, addr.smctp_base.smctp_addr.s_addr,
addr.smctp_base.smctp_type,
len);
if (addrlen == sizeof(struct sockaddr_mctp_ext)) {
printf(" ext ifindex %d ha[0]=0x%02x len %hhu\n",
addr.smctp_ifindex,
addr.smctp_haddr[0], addr.smctp_halen);
}

if (memcmp(buf, VENDOR_TYPE_ECHO, sizeof(VENDOR_TYPE_ECHO)) != 0) {
errx(EXIT_FAILURE, "unexpected vendor ID");
}

for (i = 0; i < len; i++) {
uint8_t exp = data ? data[i] : i & 0xff;
if (payload[i] != exp)
errx(EXIT_FAILURE,
"payload mismatch at byte 0x%zx; "
"sent 0x%02x, received 0x%02x",
i, exp, buf[i]);
for (i = 0; i < recvlen; ++i) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that's fine

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a reasonable compromise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and maybe assume hex in data, so the 0x could be optional.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

printf("0x%02x", rxbuf[i]);
if (i != (recvlen - 1))
printf(":");
}

printf("\n");
return 0;
}

static void usage(void)
{
fprintf(stderr, "mctp-req [eid <eid>] [net <net>] [ifindex <ifindex> lladdr <hwaddr>] [len <len>]\n");
fprintf(stderr, "default eid %d net %d len %zd\n",
DEFAULT_EID, DEFAULT_NET, DEFAULT_LEN);
fprintf(stderr, "mctp-req [eid <eid>] [net <net>] type <type> [ifindex <ifindex> lladdr <hwaddr>] data <data>\n");
fprintf(stderr, "default eid %d net %d\n",
DEFAULT_EID, DEFAULT_NET);
}

int main(int argc, char ** argv)
{
uint8_t *data, lladdr[MAX_ADDR_LEN];
int lladdrlen = -1, datalen = -1;
unsigned int net = DEFAULT_NET;
mctp_eid_t eid = DEFAULT_EID;
size_t len = DEFAULT_LEN, sz;
char *endp, *optname, *optval;
mctp_eid_t eid = DEFAULT_EID;
bool valid_parse, valid_type;
unsigned int tmp, ifindex;
bool valid_parse;
uint8_t type;
size_t sz;
int i;

if (!(argc % 2)) {
Expand All @@ -156,8 +126,14 @@ int main(int argc, char ** argv)
return 255;
}

data = NULL;
if (argc < 5) {
usage();
return EXIT_FAILURE;
}

ifindex = 0;
data = NULL;
valid_type = false;

for (i = 1; i < argc; i += 2) {
optname = argv[i];
Expand All @@ -174,12 +150,13 @@ int main(int argc, char ** argv)
if (tmp > 0xff)
errx(EXIT_FAILURE, "Bad net");
net = tmp;
} else if (!strcmp(optname, "type")) {
type = tmp;
if (type > 0xff)
errx(EXIT_FAILURE, "Bad type");
valid_type = true;
} else if (!strcmp(optname, "ifindex")) {
ifindex = tmp;
} else if (!strcmp(optname, "len")) {
if (tmp > 64 * 1024)
errx(EXIT_FAILURE, "Bad len");
len = tmp;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to retain the len behaviour for when the data isn't important (ie, for echo)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, just echo. However, data and len should be mutually exclusive: either use the data provided, or generate len bytes.

} else if (!strcmp(optname, "data")) {
sz = (strlen(optval) + 2) / 3;
data = malloc(sz);
Expand Down Expand Up @@ -207,8 +184,16 @@ int main(int argc, char ** argv)
}
}

if (data)
len = datalen;
if (!datalen || !data) {
printf("data is a required field\n");
usage();
return EXIT_FAILURE;
}
if (!valid_type) {
printf("type is a required field\n");
usage();
return EXIT_FAILURE;
}

return mctp_req(net, eid, ifindex, lladdr, lladdrlen, data, len);
return mctp_req(net, eid, ifindex, lladdr, lladdrlen, data, datalen, type);
}