Removing printf from Svc. Partially Fixed #1708

Author: LeStarch

Drv/LinuxSpiDriver/LinuxSpiDriverComponentImpl.cpp

@@ -13,7 +13,7 @@
 #include <Drv/LinuxSpiDriver/LinuxSpiDriverComponentImpl.hpp>
 #include <FpConfig.hpp>
 #include <Fw/Types/Assert.hpp>
-#include <Fw/Types/StringUtils.hpp>
+#include <Fw/Types/FileNameString.hpp>
 #include <cstdint>
 #include <unistd.h>
 #include <cstdio>
@@ -76,10 +76,11 @@ namespace Drv {
         NATIVE_INT_TYPE ret;
 
         // Open:
-        char devName[256];
-        Fw::StringUtils::format(devName, sizeof devName, "/dev/spidev%d.%d", device, select);
+        Fw::FileNameString devString;
+        Fw::FormatStatus formatStatus = devString.format("/dev/spidev%d.%d", device, select);
+        FW_ASSERT(formatStatus == Fw::FormatStatus::SUCCESS);
 
-        fd = ::open(devName, O_RDWR);
+        fd = ::open(devString.toChar(), O_RDWR);
         if (fd == -1) {
             this->log_WARNING_HI_SPI_OpenError(device,select,fd);
             return false;

Fw/Types/StringBase.cpp

@@ -53,25 +53,45 @@ bool StringBase::operator==(const CHAR* other) const {
     return (result == 0);
 }
 
-void StringBase::format(const CHAR* formatString, ...) {
+FormatStatus StringBase::format(const CHAR* formatString, ...) {
     va_list args;
     va_start(args, formatString);
-    this->vformat(formatString, args);
+    FormatStatus status = this->vformat(formatString, args);
     va_end(args);
+    return status;
 }
 
-void StringBase::vformat(const CHAR* formatString, va_list args) {
+FormatStatus StringBase::vformat(const CHAR* formatString, va_list args) {
     CHAR* us = const_cast<CHAR*>(this->toChar());
     SizeType cap = this->getCapacity();
     FW_ASSERT(us != nullptr);
-    FW_ASSERT(formatString != nullptr);
+
+    // Check format string
+    if (formatString == nullptr) {
+        return FormatStatus::INVALID_FORMAT_STRING;
+    }
+    FwSizeType total_needed_size = 0;
 #if FW_USE_PRINTF_FAMILY_FUNCTIONS_IN_STRING_FORMATTING
-    (void) vsnprintf(us, cap, formatString, args);
+    // Check that the API size type fits in fprime size type
+    static_assert(std::numeric_limits<FwSizeType>::max() >= std::numeric_limits<PlatformIntType>::max(),
+            "Range of PlarformIntType does not fit within range of FwSizeType");
+    PlatformIntType total_needed_size_api = vsnprintf(us, cap, formatString, args);
+    // Check for error return, or a type overflow
+    if (total_needed_size_api < 0) {
+        return FormatStatus::OTHER_ERROR;
+    }
+    total_needed_size = static_cast<FwSizeType>(total_needed_size_api);
 #else
+    total_needed_size = StringUtils::string_length(format_string, cap);
     *this = formatString;
 #endif
     // Force null terminate
     us[cap - 1] = 0;
+    // Check for overflow
+    if (total_needed_size >= cap) {
+        return FormatStatus::OVERFLOW;
+    }
+    return FormatStatus::SUCCESS;
 }
 
 bool StringBase::operator!=(const StringBase& other) const {

Fw/Types/StringBase.hpp

@@ -21,6 +21,15 @@
 #endif
 
 namespace Fw {
+
+//! \brief status of format
+enum class FormatStatus {
+    SUCCESS, //!< Format worked
+    OVERFLOW, //!< Format overflowed
+    INVALID_FORMAT_STRING, //!< Format provided invalid format string
+    OTHER_ERROR //!< An error was returned from an underlying call
+};
+
 class StringBase : public Serializable {
   public:
     using SizeType = NATIVE_UINT_TYPE;
@@ -62,8 +71,8 @@ class StringBase : public Serializable {
     StringBase& operator=(const CHAR* src);               //!< Assign CHAR*
     StringBase& operator=(const StringBase& src);         //!< Assign another StringBase
 
-    void format(const CHAR* formatString, ...);  //!< write formatted string to buffer
-    void vformat(const CHAR* formatString, va_list args);  //!< write formatted string to buffer using va_list
+    FormatStatus format(const CHAR* formatString, ...);  //!< write formatted string to buffer
+    FormatStatus vformat(const CHAR* formatString, va_list args);  //!< write formatted string to buffer using va_list
 
     virtual SerializeStatus serialize(SerializeBufferBase& buffer) const;                   //!< serialization function
     virtual SerializeStatus serialize(SerializeBufferBase& buffer, SizeType maxLen) const;  //!< serialization function

Fw/Types/StringUtils.cpp

@@ -73,10 +73,3 @@ FwSignedSizeType Fw::StringUtils::substring_find(const CHAR* source_string,
     // if we make it here, no matches were found
     return -1;
 }
-
-void Fw::StringUtils::format(CHAR* destination, Fw::StringBase::SizeType size, const CHAR* format, ...) {
-    va_list args;
-    va_start(args, format);
-    Fw::ExternalString(destination,  size).vformat(format, args);
-    va_end(args);
-}

Fw/Types/StringUtils.hpp

@@ -1,7 +1,11 @@
+/**
+ * Fw/Types/StringUtils.hpp:
+ *
+ * C-string helper utilities. Note: wherever possible, use Fw::StringBase and derived classes instead of raw C-strings.
+ */
 #ifndef FW_STRINGUTILS_HPP
 #define FW_STRINGUTILS_HPP
 #include <FpConfig.hpp>
-#include <Fw/Types/StringBase.hpp>
 
 namespace Fw {
 namespace StringUtils {
@@ -45,21 +49,6 @@ FwSizeType string_length(const CHAR* source, FwSizeType buffer_size);
  */
 FwSignedSizeType substring_find(const CHAR* source_string, FwSizeType source_size, const CHAR* sub_string, FwSizeType sub_size);
 
-//! \brief format a string and store into a destination buffer
-//!
-//! This function will format a string into a destination buffer. The destination buffer must be large enough to hold
-//! the formatted string. The format delegates to the standard C library function vsnprintf underneath the hood.
-//!
-//! \warning if used in execution (i.e. a file path), assert FW_USE_PRINTF_FAMILY_FUNCTIONS_IN_STRING_FORMATTING.
-//!          ```
-//!          static_assert(FW_USE_PRINTF_FAMILY_FUNCTIONS_IN_STRING_FORMATTING, "String formatting disabled")
-//!          ```
-//! \param destination: destination buffer to hold formatted string
-//! \param size: size of the destination buffer. Output will be truncated to this length
-//! \param format: constant format string
-//! \param ...: variable arguments to to pass to the format string
-void format(CHAR* destination, StringBase::SizeType size, const CHAR* format, ...);
-
 enum StringToNumberStatus {
     SUCCESSFUL_CONVERSION, //!< Output should be valid
     NULL_INPUT,     //!< A null string was supplied

Svc/ActiveTextLogger/ActiveTextLogger.cpp

@@ -72,19 +72,14 @@ namespace Svc {
                 severityString = "SEVERITY ERROR";
                 break;
         }
-
-        // TODO: Add calling task id to format string
-        char textStr[FW_INTERNAL_INTERFACE_STRING_MAX_SIZE];
-
-        (void) snprintf(textStr,
-                        FW_INTERNAL_INTERFACE_STRING_MAX_SIZE,
-                        "EVENT: (%" PRI_FwEventIdType ") (%" PRI_FwTimeBaseStoreType ":%" PRIu32 ",%" PRIu32 ") %s: %s\n",
-                        id, static_cast<FwTimeBaseStoreType>(timeTag.getTimeBase()), timeTag.getSeconds(), timeTag.getUSeconds(),
-                        severityString, text.toChar());
+        // Overflow is allowed and truncation accepted
+        Fw::InternalInterfaceString intText;
+        (void) intText.format("EVENT: (%" PRI_FwEventIdType ") (%" PRI_FwTimeBaseStoreType ":%" PRIu32 ",%" PRIu32 ") %s: %s\n",
+                              id, static_cast<FwTimeBaseStoreType>(timeTag.getTimeBase()), timeTag.getSeconds(), timeTag.getUSeconds(),
+                              severityString, text.toChar());
 
         // Call internal interface so that everything else is done on component thread,
         // this helps ensure consistent ordering of the printed text:
-        Fw::InternalInterfaceString intText(textStr);
         this->TextQueue_internalInterfaceInvoke(intText);
     }
 

Svc/ActiveTextLogger/LogFile.cpp

@@ -87,60 +87,40 @@ namespace Svc {
             this->m_openFile = false;
             this->m_file.close();
         }
+        Fw::FileNameString searchFilename;
+        Fw::FormatStatus formatStatus = searchFilename.format("%s", fileName);
 
         // If file name is too large, return failure:
-        FwSizeType fileNameSize = Fw::StringUtils::string_length(fileName, static_cast<FwSizeType>(Fw::String::STRING_SIZE));
-        if (fileNameSize == Fw::String::STRING_SIZE) {
+        if (formatStatus != Fw::FormatStatus::SUCCESS) {
             return false;
         }
 
-        U32 suffix = 0;
-        FwSignedSizeType tmp;
-        char fileNameFinal[Fw::String::STRING_SIZE];
-        (void) strncpy(fileNameFinal,fileName,
-                       Fw::String::STRING_SIZE);
-        fileNameFinal[Fw::String::STRING_SIZE-1] = 0;
-
         // Check if file already exists, and if it does try to tack on a suffix.
-        // Quit after 10 suffix addition tries (first try is w/ the original name).
+        // Quit after maxBackups suffix addition tries (first try is w/ the original name).
+        U32 suffix = 0;
         bool failedSuffix = false;
-        while (Os::FileSystem::getFileSize(fileNameFinal,tmp) == Os::FileSystem::OP_OK) {
-
-            // If the file name was the max size, then can't append a suffix,
-            // so just fail:
-            if (fileNameSize == (Fw::String::STRING_SIZE-1)) {
-                return false;
-            }
-
+        FwSignedSizeType fileSize = 0;
+        while (Os::FileSystem::getFileSize(searchFilename.toChar(), fileSize) == Os::FileSystem::OP_OK) {
             // Not able to create a new non-existing file in maxBackups tries, then mark that it failed:
             if (suffix >= maxBackups) {
                 failedSuffix = true;
                 break;
             }
-
-            NATIVE_INT_TYPE stat = snprintf(fileNameFinal,Fw::String::STRING_SIZE,
-                                            "%s%" PRIu32,fileName,suffix);
-
-            // If there was error, then just fail:
-            if (stat <= 0) {
+            // Format and check for error and overflows
+            formatStatus = searchFilename.format("%s%" PRIu32, fileName, suffix);
+            if (formatStatus != Fw::FormatStatus::SUCCESS) {
                 return false;
             }
-
-            // There should never be truncation:
-            FW_ASSERT(stat < Fw::String::STRING_SIZE);
-
             ++suffix;
         }
 
         // If failed trying to make a new file, just use the original file
         if (failedSuffix) {
-            (void) strncpy(fileNameFinal,fileName,
-                           Fw::String::STRING_SIZE);
-            fileNameFinal[Fw::String::STRING_SIZE-1] = 0;
+            searchFilename = fileName;
         }
 
         // Open the file (using CREATE so that it truncates an already existing file):
-        Os::File::Status stat = this->m_file.open(fileNameFinal, Os::File::OPEN_CREATE, Os::File::OverwriteType::OVERWRITE);
+        Os::File::Status stat = this->m_file.open(searchFilename.toChar(), Os::File::OPEN_CREATE, Os::File::OverwriteType::OVERWRITE);
 
         // Bad status when trying to open the file:
         if (stat != Os::File::OP_OK) {
@@ -149,7 +129,7 @@ namespace Svc {
 
         this->m_currentFileSize = 0;
         this->m_maxFileSize = maxSize;
-        this->m_fileName = fileNameFinal;
+        this->m_fileName = searchFilename;
         this->m_openFile = true;
 
         return true;

Svc/ActiveTextLogger/LogFile.hpp

@@ -6,7 +6,7 @@
 #ifndef SVCLOGFILE_HPP_
 #define SVCLOGFILE_HPP_
 
-#include <Fw/Types/String.hpp>
+#include <Fw/Types/FileNameString.hpp>
 #include <Os/File.hpp>
 #include <Os/FileSystem.hpp>
 
@@ -55,7 +55,7 @@ namespace Svc {
         // ----------------------------------------------------------------------
 
         // The name of the file to text logs to:
-        Fw::String m_fileName;
+        Fw::FileNameString m_fileName;
 
         // The file to write text logs to:
         Os::File m_file;

Svc/ActiveTextLogger/test/ut/ActiveTextLoggerTester.cpp

@@ -253,13 +253,13 @@ namespace Svc {
 
       printf("Testing file name larger than string size\n");
 
-      // Setup filename larger than 80 char:
-      char longFileName[Fw::String::STRING_SIZE + 1];
-      for (U32 i = 0; i < Fw::String::STRING_SIZE; ++i) {
+      // Setup filename larger than file name string can accept
+      // Maximum valid file name is Fw::FileNameString::STRING_SIZE, add 1 to be too long, and an extra for the \0
+      char longFileName[Fw::FileNameString::STRING_SIZE + 2];
+      for (U32 i = 0; i < Fw::FileNameString::STRING_SIZE + 1; ++i) {
           longFileName[i] = 'a';
       }
-      longFileName[Fw::String::STRING_SIZE] = 0;
-
+      longFileName[Fw::FileNameString::STRING_SIZE + 1] = 0;
       stat = this->component.set_log_file(longFileName,50);
 
       // Verify file not made:
@@ -269,11 +269,12 @@ namespace Svc {
                Os::FileSystem::getFileSize(longFileName,tmp));
 
       printf("Testing file name of max size and file already exists\n");
-      char longFileNameDup[Fw::String::STRING_SIZE];
-      for (U32 i = 0; i < Fw::String::STRING_SIZE; ++i) {
+      // Maximum valid file name is Fw::FileNameString::STRING_SIZE add one for \0
+      char longFileNameDup[Fw::FileNameString::STRING_SIZE + 1];
+      for (U32 i = 0; i < Fw::FileNameString::STRING_SIZE; ++i) {
           longFileNameDup[i] = 'a';
       }
-      longFileNameDup[Fw::String::STRING_SIZE-1] = 0;
+      longFileNameDup[Fw::FileNameString::STRING_SIZE] = 0;
 
       stat = this->component.set_log_file(longFileNameDup,50);
 
@@ -311,9 +312,6 @@ namespace Svc {
       char baseNameWithSuffix[128];
       U32 i;
       for (i = 0; i < 10; ++i) {
-
-          //printf("<< %i\n",i);
-
           stat = this->component.set_log_file(baseName,50);
 
           snprintf(baseNameWithSuffix, sizeof(baseNameWithSuffix), "%s%d",baseName,i);

Svc/AssertFatalAdapter/AssertFatalAdapterComponentImpl.cpp

@@ -118,9 +118,7 @@ namespace Svc {
 
       CHAR msg[Fw::StringBase::BUFFER_SIZE(FW_ASSERT_TEXT_SIZE)] = {0};
       Fw::defaultReportAssert(file,lineNo,numArgs,arg1,arg2,arg3,arg4,arg5,arg6,msg,sizeof(msg));
-      // fprintf(stderr... allocates large buffers on stack as stderr is unbuffered by the OS
-      // and this can conflict with the traditionally smaller stack sizes.
-      printf("%s\n", msg);
+      Fw::Logger::log("%s\n", msg);
 
       // Handle the case where the ports aren't connected yet
       if (not this->isConnected_Log_OutputPort(0)) {