Openthread: Significant number of PropertySet handlers do not return error code correctly

[jmw-google]

While debugging a thread network name set issue, RQ and I found that plenty of handlers in src/ncp/ncp_base_mtd.cpp does not return a non-okay error code when it fails, i.e.

template <> otError NcpBase::HandlePropertySet<SPINEL_PROP_NET_NETWORK_NAME>(void)
{
    const char *string = nullptr;
    otError     error  = OT_ERROR_NONE;

    SuccessOrExit(mDecoder.ReadUtf8(string));

    error = otThreadSetNetworkName(mInstance, string);

exit:
    return error;
}

As you can see, if the decoder fails, the return code is OT_ERROR_NONE, instead of the expected error code from decoder.

There are plenty of other places where the code is not consistently populate all the error code back to its caller, i.e. in line 1020 of the same file:

        SuccessOrExit(mEncoder.OpenStruct());
        SuccessOrExit(mEncoder.WriteUintPacked(SPINEL_PROP_IPV6_ML_PREFIX));
        SuccessOrExit(error = mEncoder.WriteIp6Address(addr));             // Mesh local prefix
        SuccessOrExit(error = mEncoder.WriteUint8(OT_IP6_PREFIX_BITSIZE)); // Prefix length (in bits)
        SuccessOrExit(mEncoder.CloseStruct());

This results in a more difficult debug procedure, since sometimes the caller receives OT_ERROR_NONE even if there are failures within the function.

[abtink]

@jmw-google thanks for submitting the issue.

I just pushed PR https://github.com/openthread/openthread/pull/5830 which addresses this.

I would like to encourage you to submit PRs for issues you may find. Contributions to OpenThread are always welcome. For smaller fixes like this one, there is no need to submit an issue on GitHub first (can submit PR directly).

I went through the NcpBase code and could find four handlers where the parameter decoding was not setting error:

• In the set handlers for NETWORK_NAME, THREAD_MODE, and STREAM_NET_INSECURE, and
• In the insert handler of MAC_FIXED_RSS.

If you know/see other cases, can you please mention them (I can add it to same PR or you can submit PRs on them later). Thanks.

I am asking this to try to double check with you in case there are some cases I may have missed (mainly since I see that you used the terms "significant" and "plenty" in the description, and I would not consider 4 out of maybe 200 spinel properties we have as "plenty" 😃).

---

On encoder side, while I agree there was an issue with the EncodeOperationalDataset(), this pattern (of not setting error in every SuccessOrExit()) itself is intentional and it is actually used in some other places. The idea is that the error would be set at the top of the method to a common error code, and then we use SuccessOrExit() without setting error and after all the statement are done (if all are successful) error would set to OT_ERROR_NONE. This is a small optimization and can lead to smaller code size on some toolchains.

That said, in the PR, I changed this (and the other case for "msg buffer counter" get handler) to set error in every SuccessOrExit() check to harmonize the code with other get handlers (and avoid confusion).

[darconeous]

@abtink: We were debugging an issue and once we figured out what was going wrong I asked @jmw-google to quickly file a bug before we forgot about it. I believe the use of the words "significant" and "plenty" originated from me.

I believe "Plenty" was originally referring to the number of lines missing error attributions, rather than the number of affected properties. After a few edits I think it crept into applying to the properties as well. Looking at the diff, I'm sure you got them all, thanks for the quick turn-around!