Keep SNMP values also in memory to avoid file reading

Implement proper functions for other parts of the stack to properly
interact with SNMP.

Found with ART Tester case "Topology discovery check (non-PN)" and a
device running on Raspberry Pi (with an almost full SD card file system)

Use full path for the cmake generator expression in src/CMakeLists.txt and
test/CMakeLists.txt to work with cmake 3.19.2

src/CMakeLists.txt

@@ -14,6 +14,8 @@
 #*******************************************************************/
 
 # NOTE: add headers to make them show up in an IDE
+# NOTE: Use full path for the <$<BOOL:${PNET_OPTION_SNMP}> expression
+#       to work with certain cmake versions
 target_sources (profinet PRIVATE
   ${PROFINET_SOURCE_DIR}/include/pnet_api.h
   pf_includes.h
@@ -66,7 +68,7 @@ target_sources (profinet PRIVATE
   common/pf_eth.c
   common/pf_file.c
   common/pf_lldp.c
-  common/pf_snmp.c
+  $<$<BOOL:${PNET_OPTION_SNMP}>:${PROFINET_SOURCE_DIR}/src/common/pf_snmp.c>
   common/pf_udp.c
   common/pf_alarm.h
   common/pf_cpm.h

src/common/pf_file.c

@@ -45,9 +45,6 @@
  * as large as the longest filename used, including termination.
  */
 CC_STATIC_ASSERT (PNET_MAX_FILENAME_SIZE >= sizeof (PF_FILENAME_IP));
-CC_STATIC_ASSERT (PNET_MAX_FILENAME_SIZE >= sizeof (PF_FILENAME_SYSCONTACT));
-CC_STATIC_ASSERT (PNET_MAX_FILENAME_SIZE >= sizeof (PF_FILENAME_SYSNAME));
-CC_STATIC_ASSERT (PNET_MAX_FILENAME_SIZE >= sizeof (PF_FILENAME_SYSLOCATION));
 CC_STATIC_ASSERT (PNET_MAX_FILENAME_SIZE >= sizeof (PF_FILENAME_IM));
 CC_STATIC_ASSERT (PNET_MAX_FILENAME_SIZE >= sizeof (PF_FILENAME_DIAGNOSTICS));
 CC_STATIC_ASSERT (PNET_MAX_FILENAME_SIZE >= sizeof (PF_FILENAME_PDPORT_1));

src/common/pf_file.h

@@ -26,9 +26,6 @@ extern "C" {
  * PNET_MAX_FILENAME_SIZE (termination included).
  */
 #define PF_FILENAME_IP          "pnet_data_ip.bin"
-#define PF_FILENAME_SYSCONTACT  "pnet_data_syscontact.bin"
-#define PF_FILENAME_SYSNAME     "pnet_data_sysname.bin"
-#define PF_FILENAME_SYSLOCATION "pnet_data_syslocation.bin"
 #define PF_FILENAME_IM          "pnet_data_im.bin"
 #define PF_FILENAME_DIAGNOSTICS "pnet_data_diagnostics.bin"
 #define PF_FILENAME_PDPORT_1    "pnet_data_pdport_1.bin"

src/common/pf_snmp.c

@@ -17,6 +17,9 @@
  * @file
  * @brief Helper functions for use by platform-dependent SNMP server (agent).
  *
+ * Some SNMP related data needs to be stored to file between invocations.
+ * We read from file only at startup, but writing to file might happen any time.
+ *
  */
 
 #ifdef UNIT_TEST
@@ -38,6 +41,15 @@
 #define STRINGIFY(s)   STRINGIFIED (s)
 #define STRINGIFIED(s) #s
 
+/* The configurable constant PNET_MAX_FILENAME_SIZE should be at least
+ * as large as the longest filename used, including termination.
+ */
+CC_STATIC_ASSERT (
+   PNET_MAX_FILENAME_SIZE >= sizeof (PF_FILENAME_SNMP_SYSCONTACT));
+CC_STATIC_ASSERT (PNET_MAX_FILENAME_SIZE >= sizeof (PF_FILENAME_SNMP_SYSNAME));
+CC_STATIC_ASSERT (
+   PNET_MAX_FILENAME_SIZE >= sizeof (PF_FILENAME_SNMP_SYSLOCATION));
+
 /**
  * Log result of variable load operation
  *
@@ -225,30 +237,132 @@ static void pf_snmp_encode_signal_delays (
    encoded->line_propagation_delay_ns = plain->cable_delay_local;
 }
 
-void pf_snmp_get_system_name (pnet_t * net, pf_snmp_system_name_t * name)
+void pf_snmp_data_init (pnet_t * net)
 {
    const char * directory = pf_cmina_get_file_directory (net);
+   pf_snmp_data_t * snmp = &net->snmp_data;
    int error;
    int result;
 
-   CC_STATIC_ASSERT (sizeof (name->string) >= PNAL_HOSTNAME_MAX_SIZE);
+   CC_STATIC_ASSERT (
+      sizeof (snmp->system_name.string) >= PNAL_HOSTNAME_MAX_SIZE);
 
-   error = pf_file_load (directory, PF_FILENAME_SYSNAME, name, sizeof (*name));
+   /* sysContact */
+   error = pf_file_load (
+      directory,
+      PF_FILENAME_SNMP_SYSCONTACT,
+      &snmp->system_contact,
+      sizeof (snmp->system_contact));
    if (error)
    {
-      result = pnal_get_hostname (name->string);
+      snmp->system_contact.string[0] = '\0';
+   }
+   snmp->system_contact.string[sizeof (snmp->system_contact.string) - 1] = '\0';
+   pf_snmp_log_loaded_variable (
+      error,
+      "sysContact",
+      snmp->system_contact.string);
+
+   /* sysName */
+   error = pf_file_load (
+      directory,
+      PF_FILENAME_SNMP_SYSNAME,
+      &snmp->system_name,
+      sizeof (snmp->system_name));
+   if (error)
+   {
+      result = pnal_get_hostname (snmp->system_name.string);
       if (result != 0)
       {
-         name->string[0] = '\0';
+         snmp->system_name.string[0] = '\0';
       }
    }
-   name->string[sizeof (name->string) - 1] = '\0';
-   pf_snmp_log_loaded_variable (error, "sysName", name->string);
+   snmp->system_name.string[sizeof (snmp->system_name.string) - 1] = '\0';
+   pf_snmp_log_loaded_variable (error, "sysName", snmp->system_name.string);
+
+   /* sysLocation */
+   error = pf_file_load (
+      directory,
+      PF_FILENAME_SNMP_SYSLOCATION,
+      &snmp->system_location,
+      sizeof (snmp->system_location));
+   if (error)
+   {
+      /* Use "IM_Tag_Location" from I&M1 */
+      pf_fspm_get_im_location (net, snmp->system_location.string);
+   }
+   snmp->system_location.string[sizeof (snmp->system_location.string) - 1] =
+      '\0';
+   pf_snmp_log_loaded_variable (
+      error,
+      "sysLocation",
+      snmp->system_location.string);
+}
+
+void pf_snmp_remove_data_files (const char * file_directory)
+{
+   pf_file_clear (file_directory, PF_FILENAME_SNMP_SYSCONTACT);
+   pf_file_clear (file_directory, PF_FILENAME_SNMP_SYSNAME);
+   pf_file_clear (file_directory, PF_FILENAME_SNMP_SYSLOCATION);
+}
+
+void pf_snmp_data_clear (pnet_t * net)
+{
+   const char * p_file_directory = pf_cmina_get_file_directory (net);
+   pf_snmp_data_t * snmp = &net->snmp_data;
+
+   LOG_DEBUG (PF_SNMP_LOG, "SNMP(%d): Clearing SNMP data.\n", __LINE__);
+   pf_snmp_remove_data_files (p_file_directory);
+
+   memset (
+      snmp->system_contact.string,
+      '\0',
+      sizeof (snmp->system_contact.string));
+   memset (snmp->system_name.string, '\0', sizeof (snmp->system_name.string));
+   memset (
+      snmp->system_location.string,
+      '\0',
+      sizeof (snmp->system_location.string));
+}
+
+void pf_snmp_fspm_im_location_ind (pnet_t * net)
+{
+   const char * p_file_directory = pf_cmina_get_file_directory (net);
+   pf_snmp_data_t * snmp = &net->snmp_data;
+
+   /* Location data is stored in two different files: the file with
+    * I&M data and a file used by SNMP containing a larger version
+    * of the device's location. The larger version has precedence
+    * over the I&M version, so we need to delete the larger one */
+   pf_file_clear (p_file_directory, PF_FILENAME_SNMP_SYSLOCATION);
+
+   /* Use "IM_Tag_Location" from I&M1 */
+   pf_fspm_get_im_location (net, snmp->system_location.string);
+   snmp->system_location.string[sizeof (snmp->system_location.string) - 1] =
+      '\0';
+
+   LOG_DEBUG (
+      PF_SNMP_LOG,
+      "SNMP(%d): I&M1 location has been updated, set SNMP SysLocation to %s.\n",
+      __LINE__,
+      snmp->system_location.string);
+}
+
+void pf_snmp_get_system_name (pnet_t * net, pf_snmp_system_name_t * name)
+{
+   const pf_snmp_data_t * snmp = &net->snmp_data;
+
+   snprintf (
+      name->string,
+      sizeof (name->string),
+      "%s",
+      snmp->system_name.string);
 }
 
 int pf_snmp_set_system_name (pnet_t * net, const pf_snmp_system_name_t * name)
 {
    const char * directory = pf_cmina_get_file_directory (net);
+   pf_snmp_data_t * snmp = &net->snmp_data;
    pf_snmp_system_name_t temporary_buffer;
    int res;
 
@@ -257,9 +371,16 @@ int pf_snmp_set_system_name (pnet_t * net, const pf_snmp_system_name_t * name)
     * Instead, we just save the sysName to file as to ensure it is persistent
     * across restarts.
     */
+
+   snprintf (
+      snmp->system_name.string,
+      sizeof (snmp->system_name.string),
+      "%s",
+      name->string);
+
    res = pf_file_save_if_modified (
       directory,
-      PF_FILENAME_SYSNAME,
+      PF_FILENAME_SNMP_SYSNAME,
       name,
       &temporary_buffer,
       sizeof (*name));
@@ -272,20 +393,13 @@ void pf_snmp_get_system_contact (
    pnet_t * net,
    pf_snmp_system_contact_t * contact)
 {
-   const char * directory = pf_cmina_get_file_directory (net);
-   int error;
+   const pf_snmp_data_t * snmp = &net->snmp_data;
 
-   error = pf_file_load (
-      directory,
-      PF_FILENAME_SYSCONTACT,
-      contact,
-      sizeof (*contact));
-   if (error)
-   {
-      contact->string[0] = '\0';
-   }
-   contact->string[sizeof (contact->string) - 1] = '\0';
-   pf_snmp_log_loaded_variable (error, "sysContact", contact->string);
+   snprintf (
+      contact->string,
+      sizeof (contact->string),
+      "%s",
+      snmp->system_contact.string);
 }
 
 int pf_snmp_set_system_contact (
@@ -293,12 +407,19 @@ int pf_snmp_set_system_contact (
    const pf_snmp_system_contact_t * contact)
 {
    const char * directory = pf_cmina_get_file_directory (net);
+   pf_snmp_data_t * snmp = &net->snmp_data;
    pf_snmp_system_contact_t temporary_buffer;
    int res;
 
+   snprintf (
+      snmp->system_contact.string,
+      sizeof (snmp->system_contact.string),
+      "%s",
+      contact->string);
+
    res = pf_file_save_if_modified (
       directory,
-      PF_FILENAME_SYSCONTACT,
+      PF_FILENAME_SNMP_SYSCONTACT,
       contact,
       &temporary_buffer,
       sizeof (*contact));
@@ -311,22 +432,13 @@ void pf_snmp_get_system_location (
    pnet_t * net,
    pf_snmp_system_location_t * location)
 {
-   const char * directory = pf_cmina_get_file_directory (net);
-   int error;
-
-   error = pf_file_load (
-      directory,
-      PF_FILENAME_SYSLOCATION,
-      location,
-      sizeof (*location));
-   if (error)
-   {
-      /* Use "IM_Tag_Location" from I&M1 */
-      pf_fspm_get_im_location (net, location->string);
-   }
-   location->string[sizeof (location->string) - 1] = '\0';
+   const pf_snmp_data_t * snmp = &net->snmp_data;
 
-   pf_snmp_log_loaded_variable (error, "sysLocation", location->string);
+   snprintf (
+      location->string,
+      sizeof (location->string),
+      "%s",
+      snmp->system_location.string);
 }
 
 int pf_snmp_set_system_location (
@@ -334,12 +446,19 @@ int pf_snmp_set_system_location (
    const pf_snmp_system_location_t * location)
 {
    const char * directory = pf_cmina_get_file_directory (net);
+   pf_snmp_data_t * snmp = &net->snmp_data;
    pf_snmp_system_location_t temporary_buffer;
    int res;
 
+   snprintf (
+      snmp->system_location.string,
+      sizeof (snmp->system_location.string),
+      "%s",
+      location->string);
+
    res = pf_file_save_if_modified (
       directory,
-      PF_FILENAME_SYSLOCATION,
+      PF_FILENAME_SNMP_SYSLOCATION,
       location,
       &temporary_buffer,
       sizeof (*location));

src/common/pf_snmp.h

@@ -107,6 +107,13 @@ extern "C" {
 
 #include "pf_lldp.h"
 
+/* SNMP-related filenames used by p-net stack. A filename may not be longer
+ * than PNET_MAX_FILENAME_SIZE (termination included).
+ */
+#define PF_FILENAME_SNMP_SYSCONTACT  "pnet_data_syscontact.bin"
+#define PF_FILENAME_SNMP_SYSNAME     "pnet_data_sysname.bin"
+#define PF_FILENAME_SNMP_SYSLOCATION "pnet_data_syslocation.bin"
+
 /**
  * System name (sysName).
  *
@@ -253,6 +260,44 @@ typedef struct pf_snmp_signal_delay
    uint32_t line_propagation_delay_ns;
 } pf_snmp_signal_delay_t;
 
+/**
+ * Initialize SNMP related data, by reading from file.
+ *
+ * @param net              InOut: The p-net stack instance.
+ */
+void pf_snmp_data_init (pnet_t * net);
+
+/**
+ * Clear SNMP related data.
+ *
+ * Clears in-memory data and calls pf_snmp_remove_data_files().
+ *
+ * Note that it does not clear the IM_Tag_Location" from I&M1
+ * (which typically is the same as SNMP SysLocation).
+ *
+ * @param net              InOut: The p-net stack instance.
+ */
+void pf_snmp_data_clear (pnet_t * net);
+
+/**
+ * Removes SNMP related data files.
+ *
+ * Used by pf_snmp_data_clear() and other operations.
+ *
+ * @param net              InOut: The p-net stack instance.
+ */
+void pf_snmp_remove_data_files (const char * file_directory);
+
+/**
+ * Update SNMP SysLocation with the present I&M1 location value.
+ *
+ * Does remove the corresponding file. Upon next startup will we use the
+ * I&M location value.
+ *
+ * @param net              InOut: The p-net stack instance.
+ */
+void pf_snmp_fspm_im_location_ind(pnet_t * net);
+
 /**
  * Get system description.
  *

src/device/pf_cmina.c

@@ -295,14 +295,14 @@ int pf_cmina_set_default_cfg (pnet_t * net, uint16_t reset_mode)
          /* Reset I&M data */
          (void)pf_fspm_clear_im_data (net);
 
+#if PNET_OPTION_SNMP
          /* According to section 8.4 "Behavior to ResetToFactory" in
           * "Test case specification: Behavior" the MIB data should be reset
           * in all supported reset modes. This seems to contradict
           * PN-AL-Services ch. 6.3.11.3.2.
           */
-         pf_file_clear (p_file_directory, PF_FILENAME_SYSCONTACT);
-         pf_file_clear (p_file_directory, PF_FILENAME_SYSNAME);
-         pf_file_clear (p_file_directory, PF_FILENAME_SYSLOCATION);
+         pf_snmp_data_clear(net);
+#endif
       }
 
       if (reset_mode == 2 || reset_mode >= 3)
@@ -326,9 +326,9 @@ int pf_cmina_set_default_cfg (pnet_t * net, uint16_t reset_mode)
 
          pf_file_clear (p_file_directory, PF_FILENAME_IP);
          pf_file_clear (p_file_directory, PF_FILENAME_DIAGNOSTICS);
-         pf_file_clear (p_file_directory, PF_FILENAME_SYSCONTACT);
-         pf_file_clear (p_file_directory, PF_FILENAME_SYSNAME);
-         pf_file_clear (p_file_directory, PF_FILENAME_SYSLOCATION);
+#if PNET_OPTION_SNMP
+         pf_snmp_data_clear(net);
+#endif
          pf_pdport_reset_all (net);
       }
 
@@ -1282,9 +1282,9 @@ int pf_cmina_remove_all_data_files (const char * file_directory)
    pf_file_clear (file_directory, PF_FILENAME_IM);
    pf_file_clear (file_directory, PF_FILENAME_IP);
    pf_file_clear (file_directory, PF_FILENAME_DIAGNOSTICS);
-   pf_file_clear (file_directory, PF_FILENAME_SYSCONTACT);
-   pf_file_clear (file_directory, PF_FILENAME_SYSNAME);
-   pf_file_clear (file_directory, PF_FILENAME_SYSLOCATION);
+#if PNET_OPTION_SNMP
+   pf_snmp_remove_data_files (file_directory);
+#endif
    pf_pdport_remove_data_files (file_directory);
 
    return 0;

src/device/pf_fspm.c

@@ -857,7 +857,6 @@ int pf_fspm_cm_write_ind (
    int ret = -1;
    pf_get_info_t get_info;
    uint16_t pos = 0;
-   const char * p_file_directory = pf_cmina_get_file_directory (net);
 
    if (p_write_request->index <= PF_IDX_USER_MAX)
    {
@@ -941,12 +940,14 @@ int pf_fspm_cm_write_ind (
                   net->fspm_cfg.im_1_data.im_tag_location);
                ret = 0;
 
+#if PNET_OPTION_SNMP
                /* Location data is stored in two different files: the file with
                 * I&M data and a file used by SNMP containing a larger version
                 * of the device's location. The larger version has precedence
                 * over the I&M version, so we need to delete the larger one.
                 */
-               pf_file_clear (p_file_directory, PF_FILENAME_SYSLOCATION);
+               pf_snmp_fspm_im_location_ind(net);
+#endif
             }
             else
             {

src/device/pnet_api.c

@@ -66,7 +66,8 @@ int pnet_init_only (pnet_t * net, const pnet_cfg_t * p_cfg)
    pf_pdport_init (net);
 
    /* Configure SNMP server if enabled */
-#if PNET_OPTION_SNMP == 1
+#if PNET_OPTION_SNMP
+   pf_snmp_data_init (net);
    if (pnal_snmp_init (net, &p_cfg->pnal_cfg) != 0)
    {
       LOG_ERROR (PNET_LOG, "Failed to configure SNMP\n");

src/pf_types.h

@@ -2572,6 +2572,13 @@ typedef struct pf_port
    pf_lldp_port_t lldp;
 } pf_port_t;
 
+typedef struct pf_snmp_data
+{
+   pf_snmp_system_contact_t system_contact;
+   pf_snmp_system_name_t system_name;
+   pf_snmp_system_location_t system_location;
+} pf_snmp_data_t;
+
 struct pnet
 {
    uint32_t pnal_buf_alloc_cnt;
@@ -2649,6 +2656,10 @@ struct pnet
       pf_port_iterator_t link_monitor_iterator;
       uint32_t link_monitor_timeout; /* Scheduler timeout instance. */
    } pf_interface;
+
+#if PNET_OPTION_SNMP
+   pf_snmp_data_t snmp_data;
+#endif
 };
 
 /**

test/CMakeLists.txt

@@ -48,7 +48,7 @@ target_sources(pf_test PRIVATE
   test_ppm.cpp
   test_ptcp.cpp
   test_scheduler.cpp
-  test_snmp.cpp
+  $<$<BOOL:${PNET_OPTION_SNMP}>:${PROFINET_SOURCE_DIR}/test/test_snmp.cpp>
   utils_for_testing.h
   utils_for_testing.cpp
 
@@ -91,7 +91,7 @@ target_sources(pf_test PRIVATE
   ${PROFINET_SOURCE_DIR}/src/common/pf_ppm.c
   ${PROFINET_SOURCE_DIR}/src/common/pf_ptcp.c
   ${PROFINET_SOURCE_DIR}/src/common/pf_scheduler.c
-  ${PROFINET_SOURCE_DIR}/src/common/pf_snmp.c
+  $<$<BOOL:${PNET_OPTION_SNMP}>:${PROFINET_SOURCE_DIR}/src/common/pf_snmp.c>
   ${PROFINET_SOURCE_DIR}/src/common/pf_udp.c
   )
 

test/test_snmp.cpp

@@ -176,7 +176,8 @@ TEST_F (SnmpTest, SnmpGetLocationGivenLargeString)
    pf_snmp_system_location_t actual;
 
    memset (&actual, 0xff, sizeof (actual));
-   mock_pf_file_save (NULL, PF_FILENAME_SYSLOCATION, &stored, sizeof (stored));
+   mock_pf_file_save (NULL, PF_FILENAME_SNMP_SYSLOCATION, &stored, sizeof (stored));
+   pf_snmp_data_init (net);
 
    pf_snmp_get_system_location (net, &actual);
 
@@ -190,7 +191,8 @@ TEST_F (SnmpTest, SnmpGetLocationGivenSmallString)
    pf_snmp_system_location_t actual;
 
    memset (&actual, 0xcd, sizeof (actual));
-   mock_pf_file_save (NULL, PF_FILENAME_SYSLOCATION, &stored, sizeof (stored));
+   mock_pf_file_save (NULL, PF_FILENAME_SNMP_SYSLOCATION, &stored, sizeof (stored));
+   pf_snmp_data_init (net);
 
    pf_snmp_get_system_location (net, &actual);
 
@@ -208,6 +210,7 @@ TEST_F (SnmpTest, SnmpGetLocationGivenError)
    mock_file_data.is_load_failing = true;
    memset (&actual, 0xff, sizeof (actual));
    mock_pf_fspm_save_im_location (net, expected);
+   pf_snmp_data_init (net);
 
    pf_snmp_get_system_location (net, &actual);
 
@@ -224,7 +227,7 @@ TEST_F (SnmpTest, SnmpSetLocationGivenLargeString)
    error = pf_snmp_set_system_location (net, &written);
 
    EXPECT_EQ (error, 0);
-   EXPECT_STREQ (mock_file_data.filename, PF_FILENAME_SYSLOCATION);
+   EXPECT_STREQ (mock_file_data.filename, PF_FILENAME_SNMP_SYSLOCATION);
    EXPECT_EQ (mock_file_data.size, sizeof (written));
    EXPECT_EQ (memcmp (mock_file_data.object, &written, sizeof (written)), 0);
    EXPECT_STREQ (mock_fspm_data.im_location, "1234567890123456789012");
@@ -239,7 +242,7 @@ TEST_F (SnmpTest, SnmpSetLocationGivenSmallString)
    error = pf_snmp_set_system_location (net, &written);
 
    EXPECT_EQ (error, 0);
-   EXPECT_STREQ (mock_file_data.filename, PF_FILENAME_SYSLOCATION);
+   EXPECT_STREQ (mock_file_data.filename, PF_FILENAME_SNMP_SYSLOCATION);
    EXPECT_EQ (mock_file_data.size, sizeof (written));
    EXPECT_EQ (memcmp (mock_file_data.object, &written, sizeof (written)), 0);
    EXPECT_STREQ (mock_fspm_data.im_location, "small                 ");