mgmt: mcumgr: stat_mgmt: stat_mgmt_list always send legacy behavior

[nandojve]

Describe the bug

The MCUmgr statistics only work correctly when selecting MCUMGR_SMP_LEGACY_RC_BEHAVIOUR. This flag is correctly handled for stat_mgmt_show command but it is missing for stat_mgmt_list command.

It is easy to compare both functions on this points:

zephyr/subsys/mgmt/mcumgr/grp/stat_mgmt/src/stat_mgmt.c

Lines 153 to 163 in 4b4bba4

	if (IS_ENABLED(CONFIG_MCUMGR_SMP_LEGACY_RC_BEHAVIOUR)) {
	ok = zcbor_tstr_put_lit(zse, "rc") &&
	zcbor_int32_put(zse, MGMT_ERR_EOK);
	}
	if (ok) {
	ok = zcbor_tstr_put_lit(zse, "name") &&
	zcbor_tstr_encode(zse, &name) &&
	zcbor_tstr_put_lit(zse, "fields") &&
	zcbor_map_start_encode(zse, counter);
	}

zephyr/subsys/mgmt/mcumgr/grp/stat_mgmt/src/stat_mgmt.c

Lines 203 to 206 in 4b4bba4

	ok = zcbor_tstr_put_lit(zse, "rc") &&
	zcbor_int32_put(zse, MGMT_ERR_EOK) &&
	zcbor_tstr_put_lit(zse, "stat_list") &&
	zcbor_list_start_encode(zse, counter);

Expected behavior

The MCUmgr statistics should work correctly for all protocol versions.

Impact

Field devices can not report their groups correctly when using a tool that not allow to use the MCUMGR_SMP_LEGACY_RC_BEHAVIOUR.

Environment (please complete the following information):

• OS: Linux
• Toolchain Zephyr SDK: 0.16.4
• mainline

Additional context

Tested with https://github.com/intercreate/smpmgr

[thedjnK]

The MCUmgr statistics only work correctly when selecting MCUMGR_SMP_LEGACY_RC_BEHAVIOUR

Then your tool is broken, this Kconfig exists specifically for the go tool, which had a deprecated warning added to the documention a long time ago, and whose documentation was fully removed from zephyr earlier this year. If you use another tool and it requires this then the fault is with the tool

Just saw the PR and even more confused, the tool does not work without the patch because it receives an rc? Then the tool is even more broken that originally thought, response with rc = 0 and response without rc are equivalent, if the tool can't handle that then the tool is flawed

[nandojve]

@thedjnK ,

Can you explain why one command send the RC and the another not?

[thedjnK]

@thedjnK ,

Can you explain why one command send the RC and the another not?

Probably an oversight when it was added, but the extra rc field being there should not breaks tools when rc is 0 (PR is fine)

[nandojve]

Ok,

response with rc = 0 and response without rc are equivalent,

This means that if I drop rc field it still comply with a V2 protocol. Is this assumption correct? I mean, a tool that only works with V2 will be able to handle the response. In this case, can I assume that rc field on V2 protocol means always an error ?

[thedjnK]

Ok,

response with rc = 0 and response without rc are equivalent,

This means that if I drop rc field it still comply with a V2 protocol. Is this assumption correct? I mean, a tool that only works with V2 will be able to handle the response. In this case, can I assume that rc field on V2 protocol means always an error ?

No rc has nothing to do with v2, it was added before such a protocol existed, it is because you don't need an error variable if there is no error, it is superfluous. See #42590 for details

[nandojve]

@thedjnK ,

Thank you for point out those info it help me to clarify some concepts since I still green in this area.
In the case of the PR #80477, I think it is fair to move in. At least we ensure behavior for both commands.

[JPHutchins]

@nandojve Is any action needed at smp/smpclient/smpmgr to fix this?

Looking at the spec I see no rc field in the success response: https://docs.zephyrproject.org/latest/services/device_mgmt/smp_groups/smp_group_2.html

I see no indication that the behavior should be different for V1 or V2. Presence of rc = 0 should cause validation to fail because that is not a valid success response of the form:

{
    (str)"stat_list" :  [
        (str)<stat_group_name>, ...
    ]
}

and rather indicates that the SMP server has sent a malformed response.