[net-diag] invoke the callback when failed to get the response (#5653)

This commit fixes the bug that the Network Diagnostic callback is not
called when we failed to get the response (e.g. the request
timeouted). This results in no command output in the CLI.

.github/workflows/simulation-1.2.yml

@@ -228,6 +228,7 @@ jobs:
       MULTIPLY: 3
       PYTHONUNBUFFERED: 1
       VERBOSE: 1
+      OTBR_COMMIT: "3cf6ff1126dca96803252514691f40d0554046bf" # Date:   Mon Oct 26 15:27:04 2020 +0800
     steps:
       - uses: actions/checkout@v2
       - name: Build OTBR Docker

include/openthread/instance.h

@@ -53,7 +53,7 @@ extern "C" {
  * @note This number versions both OpenThread platform and user APIs.
  *
  */
-#define OPENTHREAD_API_VERSION (41)
+#define OPENTHREAD_API_VERSION (42)
 
 /**
  * @addtogroup api-instance

include/openthread/netdiag.h

@@ -283,13 +283,18 @@ otError otThreadGetNextDiagnosticTlv(const otMessage *      aMessage,
 /**
  * This function pointer is called when Network Diagnostic Get response is received.
  *
+ * @param[in]  aError        The error when failed to get the response.
  * @param[in]  aMessage      A pointer to the message buffer containing the received Network Diagnostic
- *                           Get response payload.
- * @param[in]  aMessageInfo  A pointer to the message info for @p aMessage.
+ *                           Get response payload. Available only when @p aError is `OT_ERROR_NONE`.
+ * @param[in]  aMessageInfo  A pointer to the message info for @p aMessage. Available only when
+ *                           @p aError is `OT_ERROR_NONE`.
  * @param[in]  aContext      A pointer to application-specific context.
  *
  */
-typedef void (*otReceiveDiagnosticGetCallback)(otMessage *aMessage, const otMessageInfo *aMessageInfo, void *aContext);
+typedef void (*otReceiveDiagnosticGetCallback)(otError              aError,
+                                               otMessage *          aMessage,
+                                               const otMessageInfo *aMessageInfo,
+                                               void *               aContext);
 
 /**
  * This function registers a callback to provide received raw Network Diagnostic Get response payload.

src/cli/cli.cpp

@@ -4249,28 +4249,34 @@ otError Interpreter::ProcessNetworkDiagnostic(uint8_t aArgsLength, char *aArgs[]
     return error;
 }
 
-void Interpreter::HandleDiagnosticGetResponse(otMessage *aMessage, const otMessageInfo *aMessageInfo, void *aContext)
+void Interpreter::HandleDiagnosticGetResponse(otError              aError,
+                                              otMessage *          aMessage,
+                                              const otMessageInfo *aMessageInfo,
+                                              void *               aContext)
 {
     static_cast<Interpreter *>(aContext)->HandleDiagnosticGetResponse(
-        *aMessage, *static_cast<const Ip6::MessageInfo *>(aMessageInfo));
+        aError, aMessage, static_cast<const Ip6::MessageInfo *>(aMessageInfo));
 }
 
-void Interpreter::HandleDiagnosticGetResponse(const otMessage &aMessage, const Ip6::MessageInfo &)
+void Interpreter::HandleDiagnosticGetResponse(otError aError, const otMessage *aMessage, const Ip6::MessageInfo *)
 {
     uint8_t               buf[16];
     uint16_t              bytesToPrint;
     uint16_t              bytesPrinted = 0;
-    uint16_t              length       = otMessageGetLength(&aMessage) - otMessageGetOffset(&aMessage);
+    uint16_t              length;
     otNetworkDiagTlv      diagTlv;
     otNetworkDiagIterator iterator = OT_NETWORK_DIAGNOSTIC_ITERATOR_INIT;
-    otError               error    = OT_ERROR_NONE;
+
+    SuccessOrExit(aError);
 
     OutputFormat("DIAG_GET.rsp/ans: ");
 
+    length = otMessageGetLength(aMessage) - otMessageGetOffset(aMessage);
+
     while (length > 0)
     {
         bytesToPrint = (length < sizeof(buf)) ? length : sizeof(buf);
-        otMessageRead(&aMessage, otMessageGetOffset(&aMessage) + bytesPrinted, buf, bytesToPrint);
+        otMessageRead(aMessage, otMessageGetOffset(aMessage) + bytesPrinted, buf, bytesToPrint);
 
         OutputBytes(buf, static_cast<uint8_t>(bytesToPrint));
 
@@ -4281,7 +4287,7 @@ void Interpreter::HandleDiagnosticGetResponse(const otMessage &aMessage, const I
     OutputLine("");
 
     // Output Network Diagnostic TLV values in standard YAML format.
-    while ((error = otThreadGetNextDiagnosticTlv(&aMessage, &iterator, &diagTlv)) == OT_ERROR_NONE)
+    while ((aError = otThreadGetNextDiagnosticTlv(aMessage, &iterator, &diagTlv)) == OT_ERROR_NONE)
     {
         uint16_t column = 0;
         switch (diagTlv.mType)
@@ -4358,7 +4364,13 @@ void Interpreter::HandleDiagnosticGetResponse(const otMessage &aMessage, const I
         }
     }
 
-    OutputResult(error == OT_ERROR_NOT_FOUND ? OT_ERROR_NONE : error);
+    if (aError == OT_ERROR_NOT_FOUND)
+    {
+        aError = OT_ERROR_NONE;
+    }
+
+exit:
+    OutputResult(aError);
 }
 
 void Interpreter::OutputSpaces(uint16_t aCount)

src/cli/cli.hpp

@@ -485,8 +485,11 @@ class Interpreter
     static void HandleLinkPcapReceive(const otRadioFrame *aFrame, bool aIsTx, void *aContext);
 
 #if OPENTHREAD_FTD || OPENTHREAD_CONFIG_TMF_NETWORK_DIAG_MTD_ENABLE
-    void        HandleDiagnosticGetResponse(const otMessage &aMessage, const Ip6::MessageInfo &aMessageInfo);
-    static void HandleDiagnosticGetResponse(otMessage *aMessage, const otMessageInfo *aMessageInfo, void *aContext);
+    void HandleDiagnosticGetResponse(otError aError, const otMessage *aMessage, const Ip6::MessageInfo *aMessageInfo);
+    static void HandleDiagnosticGetResponse(otError              aError,
+                                            otMessage *          aMessage,
+                                            const otMessageInfo *aMessageInfo,
+                                            void *               aContext);
     void        OutputSpaces(uint16_t aCount);
     void        OutputMode(const otLinkModeConfig &aMode, uint16_t aColumn);
     void        OutputConnectivity(const otNetworkDiagConnectivity &aConnectivity, uint16_t aColumn);

src/core/thread/network_diagnostic.cpp

@@ -141,17 +141,18 @@ void NetworkDiagnostic::HandleDiagnosticGetResponse(Coap::Message *         aMes
                                                     const Ip6::MessageInfo *aMessageInfo,
                                                     otError                 aResult)
 {
-    VerifyOrExit(aResult == OT_ERROR_NONE);
-    VerifyOrExit(aMessage && aMessage->GetCode() == Coap::kCodeChanged);
-
-    otLogInfoNetDiag("Received diagnostic get response");
+    SuccessOrExit(aResult);
+    VerifyOrExit(aMessage->GetCode() == Coap::kCodeChanged, aResult = OT_ERROR_FAILED);
 
+exit:
     if (mReceiveDiagnosticGetCallback)
     {
-        mReceiveDiagnosticGetCallback(aMessage, aMessageInfo, mReceiveDiagnosticGetCallbackContext);
+        mReceiveDiagnosticGetCallback(aResult, aMessage, aMessageInfo, mReceiveDiagnosticGetCallbackContext);
+    }
+    else
+    {
+        otLogDebgNetDiag("Received diagnostic get response, error = %s", otThreadErrorToString(aResult));
     }
-
-exit:
     return;
 }
 
@@ -171,7 +172,7 @@ void NetworkDiagnostic::HandleDiagnosticGetAnswer(Coap::Message &aMessage, const
 
     if (mReceiveDiagnosticGetCallback)
     {
-        mReceiveDiagnosticGetCallback(&aMessage, &aMessageInfo, mReceiveDiagnosticGetCallbackContext);
+        mReceiveDiagnosticGetCallback(OT_ERROR_NONE, &aMessage, &aMessageInfo, mReceiveDiagnosticGetCallbackContext);
     }
 
     SuccessOrExit(Get<Tmf::TmfAgent>().SendEmptyAck(aMessage, aMessageInfo));