Address review comments

examples/all-clusters-app/linux/AppOptions.cpp

@@ -22,6 +22,7 @@
 #include <app/server/CommissioningWindowManager.h>
 #include <app/server/Server.h>
 
+using namespace chip;
 using namespace chip::ArgParser;
 using namespace chip::app::Clusters::DiagnosticLogs;
 
@@ -37,9 +38,14 @@ constexpr uint16_t kOptionCrashFilePath              = 0xFF05;
 
 static chip::Credentials::Examples::TestHarnessDACProvider mDacProvider;
 
-static char mEndUserSupportLogFileDesignator[kLogFileDesignatorMaxLen];
-static char mNetworkDiagnosticsLogFileDesignator[kLogFileDesignatorMaxLen];
-static char mCrashLogFileDesignator[kLogFileDesignatorMaxLen];
+chip::Optional<std::string> mEndUserSupportLogFilePath;
+chip::Optional<std::string> mNetworkDiagnosticsLogFilePath;
+chip::Optional<std::string> mCrashLogFilePath;
+
+bool AppOptions::IsNullOrEmpty(const char * value)
+{
+    return (value == nullptr || strlen(value) == 0 || strncmp(value, "", strlen(value)) == 0);
+}
 
 bool AppOptions::HandleOptions(const char * program, OptionSet * options, int identifier, const char * name, const char * value)
 {
@@ -55,30 +61,24 @@ bool AppOptions::HandleOptions(const char * program, OptionSet * options, int id
         break;
     }
     case kOptionEndUserSupportFilePath: {
-        if (strlen(value) > kLogFileDesignatorMaxLen)
+        if (!IsNullOrEmpty(value))
         {
-            PrintArgError("%s: Invalid file path length. Must be less that %d: %d\n", program, kLogFileDesignatorMaxLen,
-                          strlen(value));
+            mEndUserSupportLogFilePath.SetValue(std::string{ value });
         }
-        strncpy(mEndUserSupportLogFileDesignator, value, strlen(value));
         break;
     }
     case kOptionNetworkDiagnosticsFilePath: {
-        if (strlen(value) > kLogFileDesignatorMaxLen)
+        if (!IsNullOrEmpty(value))
         {
-            PrintArgError("%s: Invalid file path length. Must be less that %d: %d\n", program, kLogFileDesignatorMaxLen,
-                          strlen(value));
+            mNetworkDiagnosticsLogFilePath.SetValue(std::string{ value });
         }
-        strncpy(mNetworkDiagnosticsLogFileDesignator, value, strlen(value));
         break;
     }
     case kOptionCrashFilePath: {
-        if (strlen(value) > kLogFileDesignatorMaxLen)
+        if (!IsNullOrEmpty(value))
         {
-            PrintArgError("%s: Invalid file path length. Must be less that %d: %d\n", program, kLogFileDesignatorMaxLen,
-                          strlen(value));
+            mCrashLogFilePath.SetValue(std::string{ value });
         }
-        strncpy(mCrashLogFileDesignator, value, strlen(value));
         break;
     }
     default:
@@ -123,17 +123,17 @@ chip::Credentials::DeviceAttestationCredentialsProvider * AppOptions::GetDACProv
     return &mDacProvider;
 }
 
-char * AppOptions::GetEndUserSupportLogFileDesignator()
+Optional<std::string> AppOptions::GetEndUserSupportLogFilePath()
 {
-    return mEndUserSupportLogFileDesignator;
+    return mEndUserSupportLogFilePath;
 }
 
-char * AppOptions::GetNetworkDiagnosticsLogFileDesignator()
+Optional<std::string> AppOptions::GetNetworkDiagnosticsLogFilePath()
 {
-    return mNetworkDiagnosticsLogFileDesignator;
+    return mNetworkDiagnosticsLogFilePath;
 }
 
-char * AppOptions::GetCrashLogFileDesignator()
+Optional<std::string> AppOptions::GetCrashLogFilePath()
 {
-    return mCrashLogFileDesignator;
+    return mCrashLogFilePath;
 }

examples/all-clusters-app/linux/AppOptions.h

@@ -1,6 +1,6 @@
 /*
  *
- *    Copyright (c) 2022 Project CHIP Authors
+ *    Copyright (c) 2022-2023 Project CHIP Authors
  *    All rights reserved.
  *
  *    Licensed under the Apache License, Version 2.0 (the "License");
@@ -27,11 +27,13 @@ class AppOptions
 public:
     static chip::ArgParser::OptionSet * GetOptions();
     static chip::Credentials::DeviceAttestationCredentialsProvider * GetDACProvider();
-    static char * GetEndUserSupportLogFileDesignator();
-    static char * GetNetworkDiagnosticsLogFileDesignator();
-    static char * GetCrashLogFileDesignator();
+    static chip::Optional<std::string> GetEndUserSupportLogFilePath();
+    static chip::Optional<std::string> GetNetworkDiagnosticsLogFilePath();
+    static chip::Optional<std::string> GetCrashLogFilePath();
 
 private:
     static bool HandleOptions(const char * program, chip::ArgParser::OptionSet * options, int identifier, const char * name,
                               const char * value);
+
+    static bool IsNullOrEmpty(const char * value);
 };

examples/all-clusters-app/linux/BUILD.gn

@@ -25,7 +25,6 @@ source_set("chip-all-clusters-common") {
     "${chip_root}/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp",
     "${chip_root}/examples/all-clusters-app/all-clusters-common/src/bridged-actions-stub.cpp",
     "${chip_root}/examples/all-clusters-app/all-clusters-common/src/concentration-measurement-instances.cpp",
-    "${chip_root}/examples/all-clusters-app/linux/diagnostic-logs-provider-delegate-impl.cpp",
     "${chip_root}/examples/all-clusters-app/all-clusters-common/src/dishwasher-alarm-stub.cpp",
     "${chip_root}/examples/all-clusters-app/all-clusters-common/src/dishwasher-mode.cpp",
     "${chip_root}/examples/all-clusters-app/all-clusters-common/src/fan-stub.cpp",
@@ -38,6 +37,7 @@ source_set("chip-all-clusters-common") {
     "${chip_root}/examples/all-clusters-app/all-clusters-common/src/static-supported-modes-manager.cpp",
     "${chip_root}/examples/all-clusters-app/all-clusters-common/src/static-supported-temperature-levels.cpp",
     "${chip_root}/examples/all-clusters-app/all-clusters-common/src/tcc-mode.cpp",
+    "${chip_root}/examples/all-clusters-app/linux/diagnostic-logs-provider-delegate-impl.cpp",
     "AllClustersCommandDelegate.cpp",
     "AppOptions.cpp",
     "WindowCoveringManager.cpp",

examples/all-clusters-app/linux/diagnostic-logs-provider-delegate-impl.cpp

@@ -22,28 +22,42 @@
 using namespace chip;
 using namespace chip::app::Clusters::DiagnosticLogs;
 
-constexpr uint16_t kChunkSizeZero = 0;
-
 LogProvider LogProvider::sInstance;
 
 LogSessionHandle LogProvider::sLogSessionHandle;
 
+// TODO: Fix #30477 - Add support for multiple log collection sessions
+
 LogSessionHandle LogProvider::StartLogCollection(IntentEnum logType)
 {
 
+    // We can handle only one log collection session. Return kInvalidLogSessionHandle
+    // if we are in the middle of another log collection session.
+    if (mIsInALogCollectionSession)
+    {
+        return kInvalidLogSessionHandle;
+    }
+
+    mIsInALogCollectionSession  = true;
     mTotalNumberOfBytesConsumed = 0;
 
-    // Open the file of type
-    const char * fileName = GetLogFilePath(logType);
-    if (fileName != nullptr)
+    // Open the log file for reading.
+    Optional<std::string> filePath = GetLogFilePath(logType);
+    if (filePath.HasValue())
     {
-        mFileStream.open(fileName, std::ios_base::binary | std::ios_base::in);
+        mFileStream.open(filePath.Value().c_str(), std::ios_base::binary | std::ios_base::in);
         if (!mFileStream.good())
         {
             ChipLogError(BDX, "Failed to open the log file");
             return kInvalidLogSessionHandle;
         }
         sLogSessionHandle++;
+
+        // If the session handle rolls over to UINT16_MAX which is invalid, reset to 0.
+        if (sLogSessionHandle == kInvalidLogSessionHandle)
+        {
+            sLogSessionHandle = 0;
+        }
         mLogSessionHandle = sLogSessionHandle;
     }
     else
@@ -53,79 +67,89 @@ LogSessionHandle LogProvider::StartLogCollection(IntentEnum logType)
     return mLogSessionHandle;
 }
 
-uint64_t LogProvider::GetNextChunk(LogSessionHandle logSessionHandle, chip::MutableByteSpan & outBuffer, bool & outIsEOF)
+CHIP_ERROR LogProvider::GetNextChunk(LogSessionHandle logSessionHandle, MutableByteSpan & outBuffer, bool & outIsEOF)
 {
-    if (logSessionHandle != mLogSessionHandle && outBuffer.size() == 0)
+    if (!mIsInALogCollectionSession)
     {
-        return kChunkSizeZero;
+        return CHIP_ERROR_INCORRECT_STATE;
     }
 
     if (!mFileStream.is_open())
     {
         ChipLogError(BDX, "File is not open");
-        return kChunkSizeZero;
+        return CHIP_ERROR_INCORRECT_STATE;
+    }
+
+    mFileStream.seekg(mFileStream.end);
+    long long fileSize = mFileStream.tellg();
+
+    long long remainingBytesToBeRead = fileSize - static_cast<long long>(mTotalNumberOfBytesConsumed);
+
+    if ((remainingBytesToBeRead >= kMaxLogContentSize && outBuffer.size() < kMaxLogContentSize) ||
+        (remainingBytesToBeRead < kMaxLogContentSize && static_cast<long long>(outBuffer.size()) < remainingBytesToBeRead))
+    {
+        ChipLogError(BDX, "Buffer is too small to read the next chunk from file");
+        return CHIP_ERROR_INVALID_ARGUMENT;
     }
 
     mFileStream.seekg(static_cast<long long>(mTotalNumberOfBytesConsumed));
-    mFileStream.read(reinterpret_cast<char *>(outBuffer.data()), kLogContentMaxSize);
+    mFileStream.read(reinterpret_cast<char *>(outBuffer.data()), kMaxLogContentSize);
 
     if (!(mFileStream.good() || mFileStream.eof()))
     {
         ChipLogError(BDX, "Failed to read the log file");
         mFileStream.close();
-        return kChunkSizeZero;
+        return CHIP_ERROR_INCORRECT_STATE;
     }
 
     uint64_t bytesRead = static_cast<uint64_t>(mFileStream.gcount());
-    outIsEOF           = (mFileStream.peek() == EOF);
+    outBuffer.reduce_size(bytesRead);
+    outIsEOF = (mFileStream.peek() == EOF);
 
     mTotalNumberOfBytesConsumed += bytesRead;
-    return bytesRead;
+    return CHIP_NO_ERROR;
 }
 
 void LogProvider::EndLogCollection(LogSessionHandle logSessionHandle)
 {
-    if (logSessionHandle == mLogSessionHandle && mFileStream.is_open())
+    if (mFileStream.is_open())
     {
         mFileStream.close();
     }
+    mIsInALogCollectionSession = false;
 }
 
 uint64_t LogProvider::GetTotalNumberOfBytesConsumed(LogSessionHandle logSessionHandle)
 {
-    if (logSessionHandle == mLogSessionHandle)
-    {
-        return mTotalNumberOfBytesConsumed;
-    }
-    return kChunkSizeZero;
+    return mTotalNumberOfBytesConsumed;
 }
 
-const char * LogProvider::GetLogFilePath(IntentEnum logType)
+Optional<std::string> LogProvider::GetLogFilePath(IntentEnum logType)
 {
     switch (logType)
     {
     case IntentEnum::kEndUserSupport:
-        return mEndUserSupportLogFileDesignator;
+        return mEndUserSupportLogFilePath;
     case IntentEnum::kNetworkDiag:
-        return mNetworkDiagnosticsLogFileDesignator;
+        return mNetworkDiagnosticsLogFilePath;
     case IntentEnum::kCrashLogs:
-        return mCrashLogFileDesignator;
+        return mCrashLogFilePath;
     default:
-        return nullptr;
+        return NullOptional;
     }
 }
 
-void LogProvider::SetEndUserSupportLogFileDesignator(const char * logFileName)
+void LogProvider::SetEndUserSupportLogFilePath(Optional<std::string> logFilePath)
 {
-    strncpy(mEndUserSupportLogFileDesignator, logFileName, strlen(logFileName));
+    mEndUserSupportLogFilePath = logFilePath;
 }
 
-void LogProvider::SetNetworkDiagnosticsLogFileDesignator(const char * logFileName)
+void LogProvider::SetNetworkDiagnosticsLogFilePath(Optional<std::string> logFilePath)
 {
-    strncpy(mNetworkDiagnosticsLogFileDesignator, logFileName, strlen(logFileName));
+    mNetworkDiagnosticsLogFilePath = logFilePath;
 }
 
-void LogProvider::SetCrashLogFileDesignator(const char * logFileName)
+void LogProvider::SetCrashLogFilePath(Optional<std::string> logFilePath)
 {
-    strncpy(mCrashLogFileDesignator, logFileName, strlen(logFileName));
+    mCrashLogFilePath = logFilePath;
 }

examples/all-clusters-app/linux/include/diagnostic-logs-provider-delegate-impl.h

@@ -40,35 +40,37 @@ class LogProvider : public LogProviderDelegate
 public:
     LogSessionHandle StartLogCollection(IntentEnum logType);
 
-    uint64_t GetNextChunk(LogSessionHandle logSessionHandle, chip::MutableByteSpan & outBuffer, bool & outIsEOF);
+    CHIP_ERROR GetNextChunk(LogSessionHandle logSessionHandle, MutableByteSpan & outBuffer, bool & outIsEOF);
 
     void EndLogCollection(LogSessionHandle logSessionHandle);
 
     uint64_t GetTotalNumberOfBytesConsumed(LogSessionHandle logSessionHandle);
 
-    void SetEndUserSupportLogFileDesignator(const char * logFileName);
+    void SetEndUserSupportLogFilePath(Optional<std::string> logFilePath);
 
-    void SetNetworkDiagnosticsLogFileDesignator(const char * logFileName);
+    void SetNetworkDiagnosticsLogFilePath(Optional<std::string> logFilePath);
 
-    void SetCrashLogFileDesignator(const char * logFileName);
+    void SetCrashLogFilePath(Optional<std::string> logFilePath);
 
     LogProvider(){};
 
     ~LogProvider(){};
 
-    static inline LogProvider & GetInstance() { return sInstance; }
+    static inline LogProvider & getLogProvider() { return sInstance; }
 
 private:
-    const char * GetLogFilePath(IntentEnum logType);
+    Optional<std::string> GetLogFilePath(IntentEnum logType);
 
-    char mEndUserSupportLogFileDesignator[kLogFileDesignatorMaxLen];
-    char mNetworkDiagnosticsLogFileDesignator[kLogFileDesignatorMaxLen];
-    char mCrashLogFileDesignator[kLogFileDesignatorMaxLen];
+    Optional<std::string> mEndUserSupportLogFilePath;
+    Optional<std::string> mNetworkDiagnosticsLogFilePath;
+    Optional<std::string> mCrashLogFilePath;
 
     std::ifstream mFileStream;
 
     LogSessionHandle mLogSessionHandle;
 
+    bool mIsInALogCollectionSession;
+
     uint64_t mTotalNumberOfBytesConsumed;
 };
 

examples/all-clusters-app/linux/main-common.cpp

@@ -20,8 +20,8 @@
 #include "AppOptions.h"
 #include "WindowCoveringManager.h"
 #include "air-quality-instance.h"
-#include "include/diagnostic-logs-provider-delegate-impl.h"
 #include "dishwasher-mode.h"
+#include "include/diagnostic-logs-provider-delegate-impl.h"
 #include "include/tv-callbacks.h"
 #include "laundry-washer-controls-delegate-impl.h"
 #include "laundry-washer-mode.h"
@@ -265,9 +265,10 @@ void emberAfWindowCoveringClusterInitCallback(chip::EndpointId endpoint)
 using namespace chip::app::Clusters::DiagnosticLogs;
 void emberAfDiagnosticLogsClusterInitCallback(chip::EndpointId endpoint)
 {
-    ChipLogProgress(NotSpecified, "SetDefaultLogProviderDelegate");
-    DiagnosticLogsServer::Instance().SetDefaultLogProviderDelegate(endpoint, &LogProvider::getLogProvider());
-    LogProvider::getLogProvider().SetEndUserSupportLogFileDesignator(AppOptions::GetEndUserSupportLogFileDesignator());
-    LogProvider::getLogProvider().SetNetworkDiagnosticsLogFileDesignator(AppOptions::GetNetworkDiagnosticsLogFileDesignator());
-    LogProvider::getLogProvider().SetCrashLogFileDesignator(AppOptions::GetCrashLogFileDesignator());
+    ChipLogProgress(NotSpecified, "SetLogProviderDelegate");
+    DiagnosticLogsServer::Instance().SetLogProviderDelegate(endpoint, &LogProvider::getLogProvider());
+
+    LogProvider::getLogProvider().SetEndUserSupportLogFilePath(AppOptions::GetEndUserSupportLogFilePath());
+    LogProvider::getLogProvider().SetNetworkDiagnosticsLogFilePath(AppOptions::GetNetworkDiagnosticsLogFilePath());
+    LogProvider::getLogProvider().SetCrashLogFilePath(AppOptions::GetCrashLogFilePath());
 }