Merge branch 'fix/incorrect_console_open_and_close_behaviour' into 'master'

fix(storage/vfs_console): stop new console opens from overwriting existing fds

Closes IDFGH-13502

See merge request espressif/esp-idf!32934

Author: Jiang Jiang Jian

components/esp_system/test_apps/.build-test-rules.yml

@@ -8,6 +8,8 @@ components/esp_system/test_apps/console:
   disable:
     - if: CONFIG_NAME == "serial_jtag_only" and SOC_USB_SERIAL_JTAG_SUPPORTED != 1
     - if: CONFIG_NAME == "serial_jtag_only_no_vfs" and SOC_USB_SERIAL_JTAG_SUPPORTED != 1
+  disable_test:
+    - if: CONFIG_NAME == "simple" and IDF_TARGET != "esp32"
 
 components/esp_system/test_apps/esp_system_unity_tests:
   disable:

components/esp_system/test_apps/console/main/test_app_main.c

@@ -5,6 +5,7 @@
  */
 #include <stdio.h>
 #include <assert.h>
+#include <string.h>
 #include "sdkconfig.h"
 
 #include "esp_rom_uart.h"
@@ -49,10 +50,29 @@ static void console_none_print(void)
 }
 #endif
 
+#if CONFIG_VFS_SUPPORT_IO
+static void console_open_close_check(void)
+{
+    printf("Opening /dev/console\n");
+    int fd = open("/dev/console", O_RDWR);
+    assert(fd >= 0 && "Could not open file");
+
+    const char *msg = "This should be printed to stdout\n";
+
+    write(fd, msg, strlen(msg));
+
+    printf("Closing /dev/console\n");
+    close(fd);
+
+    printf("This should be printed to stdout\n");
+}
+#endif // CONFIG_VFS_SUPPORT_IO
+
 void app_main(void)
 {
     printf("Hello World\n");
 
+#if CONFIG_VFS_SUPPORT_IO
     int fd = open("/dev/null", O_RDWR);
     assert(fd >= 0 && "Could not open file"); // Standard check
 
@@ -61,8 +81,14 @@ void app_main(void)
     assert(fd > 2 && "Incorrect file descriptor returned, stdin, stdout, stderr were not correctly assigned");
 
     close(fd);
+#endif // CONFIG_VFS_SUPPORT_IO
 
 #if CONFIG_ESP_CONSOLE_NONE
     console_none_print();
 #endif // CONFIG_ESP_CONSOLE_NONE
+
+#if CONFIG_VFS_SUPPORT_IO
+    console_open_close_check();
+#endif // CONFIG_VFS_SUPPORT_IO
+
 }

components/esp_system/test_apps/console/pytest_esp_system_console_tests.py

@@ -41,10 +41,43 @@ def test_esp_system_console_no_output_uart(dut: Dut) -> None:
     'port, flash_port, config',
     [
         pytest.param('/dev/serial_ports/ttyACM-esp32', '/dev/serial_ports/ttyUSB-esp32', 'serial_jtag_only', marks=JTAG_SERIAL_MARKS),
-        pytest.param('/dev/serial_ports/ttyACM-esp32', '/dev/serial_ports/ttyUSB-esp32', 'serial_jtag_only_no_vfs', marks=JTAG_SERIAL_MARKS),
     ],
     indirect=True,
 )
 def test_esp_system_console_only_serial_jtag(dut: Dut) -> None:
     dut.expect('2nd stage bootloader')
     dut.expect('Hello World')
+    dut.expect('Opening /dev/console')
+    dut.expect('This should be printed to stdout')
+    dut.expect('Closing /dev/console')
+    dut.expect('This should be printed to stdout')
+
+
+@pytest.mark.usb_serial_jtag
+@pytest.mark.parametrize(
+    'port, flash_port, config',
+    [
+        pytest.param('/dev/serial_ports/ttyACM-esp32', '/dev/serial_ports/ttyUSB-esp32', 'serial_jtag_only_no_vfs', marks=JTAG_SERIAL_MARKS),
+    ],
+    indirect=True,
+)
+def test_esp_system_console_only_serial_jtag_no_vfs(dut: Dut) -> None:
+    dut.expect('2nd stage bootloader')
+    dut.expect('Hello World')
+
+
+@pytest.mark.generic
+@pytest.mark.parametrize(
+    'config',
+    [
+        pytest.param('simple', marks=pytest.mark.supported_targets),
+    ],
+    indirect=True
+)
+def test_esp_system_console_correct_open_and_close(dut: Dut) -> None:
+    dut.expect('2nd stage bootloader')
+    dut.expect('Hello World')
+    dut.expect('Opening /dev/console')
+    dut.expect('This should be printed to stdout')
+    dut.expect('Closing /dev/console')
+    dut.expect('This should be printed to stdout')

components/esp_system/test_apps/console/sdkconfig.ci.console_none

@@ -1,2 +1,3 @@
 CONFIG_ESP_CONSOLE_NONE=y
+CONFIG_ESP_CONSOLE_SECONDARY_NONE=y
 CONFIG_VFS_SUPPORT_IO=y

components/esp_system/test_apps/console/sdkconfig.ci.simple

@@ -15,8 +15,8 @@
 #include "esp_vfs_console.h"
 #include "sdkconfig.h"
 #include "esp_private/startup_internal.h"
-#include "esp_vfs_null.h"
 #include "esp_private/nullfs.h"
+#include <sys/errno.h>
 
 #define STRINGIFY(s) STRINGIFY2(s)
 #define STRINGIFY2(s) #s
@@ -45,8 +45,18 @@ const static esp_vfs_fs_ops_t *primary_vfs = NULL;
 
 static vfs_console_context_t vfs_console = {0};
 
+static size_t s_open_count = 0;
+
 int console_open(const char * path, int flags, int mode)
 {
+    if (s_open_count > 0) {
+        // Underlying fd is already open, so just increment the open count
+        // and return the same fd
+
+        s_open_count++;
+        return 0;
+    }
+
 // Primary port open
 #if CONFIG_ESP_CONSOLE_UART
     vfs_console.fd_primary = open("/dev/uart/"STRINGIFY(CONFIG_ESP_CONSOLE_UART_NUM), flags, mode);
@@ -62,6 +72,8 @@ int console_open(const char * path, int flags, int mode)
 #if CONFIG_ESP_CONSOLE_SECONDARY_USB_SERIAL_JTAG
     vfs_console.fd_secondary = open("/dev/secondary", flags, mode);
 #endif
+
+    s_open_count++;
     return 0;
 }
 
@@ -82,6 +94,18 @@ int console_fstat(int fd, struct stat * st)
 
 int console_close(int fd)
 {
+    if (s_open_count == 0) {
+        errno = EBADF;
+        return -1;
+    }
+
+    s_open_count--;
+
+    // We don't actually close the underlying fd until the open count reaches 0
+    if (s_open_count > 0) {
+        return 0;
+    }
+
     // All function calls are to primary, except from write and close, which will be forwarded to both primary and secondary.
     close(vfs_console.fd_primary);
 #if CONFIG_ESP_CONSOLE_SECONDARY_USB_SERIAL_JTAG