Make mctp-req a generic tool to send *any* MCTP commands

[Peng85282]

A recent change to mctp-req makes it a tool to send only the echo command to work with mctp-echo.

However, a minor change would make the tool much more useful so it could send any MCTP commands instead.

[mkj]

This functionality was added in mctp-client #84

[Peng85282]

@mkj I did read the mctp-client.c source code. However, I'd believe it has two major limitations and mctp-req is still a better option.

1. mctp-client has a fixed MCTP Message Type lookup table. anytime there is a new message type, this table needs to be updated. For example, it doesn't include the type 7, 8, and 9 that were added to the spec years ago. Be honest, I don't see much value of using a user-friendly type name rather than just the type number while there is no user-friendly interface for the actual payload data.

2. mctp-client uses struct sockaddr_mctp, but not struct sockaddr_mctp_ext. This is the biggest issue. Because of this, it cannot send the Set EID command using only the physical address.

On the other hand, this patched mctp-req is compatible with the current "echo" function as well as the previous version that could send "any" data. It also covers all mctp-client functions.

[Peng85282]

BTW this patch added a timeout check for recvfrom.

[Peng85282]

I noticed the code relies on OS to free the allocated memory instead of explicit free(). Is it a common practice now?

[jk-ozlabs]

mctp-req is explicitly intended to pair with mctp-echo, with mctp-client targeted as a generic MCTP client.

If there are limitations to mctp-client that prevent your usage, I would be happy to modify it to suit. However, mctp-req is not the right place for that functionality.

[Peng85282]

@jk-ozlabs I understand initially mctp-req was created with mctp-echo together for testing purpose. However, I don't understand why we should limit ourselves rather than enhancing mctp-req with just a few small changes to do much more than that, especially when this tool is called mctp-req rather than mctp-echo-client.

If you have reviewed the chances, you'll see all changes are:

1. prepared payload data (for echo command) in main() instead of mctp_req(), so mctp_req() does only send/receive.
2. enabled mctp type to be specified in command line
3. added socket timeout check
4. updated usage message

After the change, mctp-req is 100% compatible with the current echo-request function. Unless some application is checking the usage message, all existing tools calling mctp-req should work as is.

The only thing that truly "changes" the intention of this mctp-req tool is the 2nd item that enables different mctp types, and this change takes just a few lines. Why should we duplicate 99% of the code just because it's not the original intention? This is not the medicine industry where a different name/description/purpose or adding a different flavor sugar coat makes it a new medicine.

On the other hand, I didn't say mctp-client is not valuable. To me, mctp-client should be further enhanced to handle the payload data, making it a truly user-friendly client tool, while this mctp-req, as another client tool, deals with only raw data bytes. mctp-req then will be very stable and mctp-client may need to be updated from time to time when MCTP spec gets updates.

Please reconsider this submission.

By the way, mctp-echo was called mctp-resp, which is clearly paired with mctp-req, but not today's name. Without looking into the source code, I believe most people would consider mctp-req as a generic tool to send any mctp request message.

[jk-ozlabs]

To me, mctp-client should be further enhanced to handle the payload data, making it a truly user-friendly client tool, while this mctp-req, as another client tool, deals with only raw data bytes

mctp-client will always be targeted for raw payload bytes; we are not planning to implement any non-DSP0236 payload handing in mctp-client. Anything beyond raw payload bytes (defined by upper-layer protocols) belongs in tooling for those protocols.

Conversely, mctp-req already does have payload definitions, in constructing the vendor type data.

mctp-client is the correct tool for performing arbitrary commands, using raw payload byte definitions. From what you have said so far, the things that are missing are:

• a facility to use an arbitrary type value
• support for extended addressing

and I think these would be fine to implement in mctp-client.

The only thing that truly "changes" the intention of this mctp-req tool is the 2nd item that enables different mctp types

... and we already have this multi-type-handling (plus payload parsing) in mctp-client.

These are both tiny utilities, and either approach would be super straightforward. What is your objection here, just the naming?

[Peng85282]

@jk-ozlabs, first of all, I acknowledge that this is your project, and I respect your decisions.

What is your objection here, just the naming?

I thought I had explained myself clearly, but obviously not. Please allow me to try again.

I made my changes last year, before mctp-client was added into the repository. I regret not submitting my work sooner; or we might have avoided today’s arguments.

I did review the source code of mctp-client, and in my opinion, is not a good add-on to this mctp project. It is very different and even does not follow the same the syntax/format of the data parameter mctp-req has. This inconsistency can be confusing for users, especially those who have been working with mctp-req for years. I believe I’m not the only one who patched mctp-req.

The naming is just another reason to support my argument that mctp-req should be a generic command-line tool for sending any MCTP commands, especially for users who are familiar with the original mctp project. If a dedicated tool is preferred solely for sending the MCTP echo requests, I’d suggest naming it mctp-echo-req.

The changes I made to mctp-req are intended to be 100% compatible with the current implementation. I’ve reviewed and fixed some issues, with limited tests on an OpenBMC setup, to ensure this. Existing mctp-req commands should continue to work without any modification. The changes should not cause any disruption. I don’t see a compelling reason to reject them.

I understand that mctp-req was originally created to pair with mctp-echo. However, many great tools and products evolve beyond their original purpose. Coca-Cola was created as a medicine; WD-40 was invented for missiles; Listerine was a surgical antiseptic; sildenafil was discovered to treat angina; GPU was not originally built for AI. The list goes on. Why limit mctp-req to its initial intent?

While I recognize that mctp-client could be patched to meet my needs, I see a philosophical difference between mctp-client and mctp-req. As the MCTP specification evolves to support new message types in the future, mctp-client will require updates whereas the new mctp-req does not. To me, mctp-client could serve as a more user-friendly command line tool while mctp-req remains as a script-oriented tool. Both could send “any” MCTP commands. However, if we remove the “user-friendly” aspect from mctp-client to avoid future updates, it essentially becomes a duplicate of mctp-req with a different name.

[jk-ozlabs]

Why limit mctp-req to its initial intent?

Because we already have a tool grown out of these wider requirements, mctp-client. We need one specifically for echo, and mctp-req is that.

From the very top of the mctp-req sources:

/* SPDX-License-Identifier: GPL-2.0 */
/*
 * mctp-req: MCTP echo requester
 *

I think it's pretty clear what its intent is.

As the MCTP specification evolves to support new message types in the future, mctp-client will require updates whereas the new mctp-req does not.

Currently, mctp-req does not support any flexibility in the message types. Your proposed patches add that. Your patches could be even simpler in adding arbitrary type support to the already-parameterised type in mctp-client.