From a30a33b6617deb9abe6d281b237f83fd079537ef Mon Sep 17 00:00:00 2001 From: Jean-Roland Gosse Date: Fri, 19 Jul 2024 11:23:01 +0200 Subject: [PATCH] Fix no router segfault (#549) * feat: add rc func that decrement without clearing * build: add no_router test * fix: missing newline eof --- .github/workflows/build-check.yaml | 14 ++ CMakeLists.txt | 3 +- include/zenoh-pico/collections/refcount.h | 16 +++ src/api/api.c | 2 +- tests/no_router.py | 155 ++++++++++++++++++++++ tests/z_refcount_test.c | 8 ++ 6 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/no_router.py diff --git a/.github/workflows/build-check.yaml b/.github/workflows/build-check.yaml index 37fab7d0b..96545a251 100644 --- a/.github/workflows/build-check.yaml +++ b/.github/workflows/build-check.yaml @@ -228,3 +228,17 @@ jobs: - name: Kill Zenoh router if: always() run: kill ${{ steps.run-zenoh.outputs.zenohd-pid }} + + no_routeur: + name: Test examples without router + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Build & test pico + run: | + sudo apt install -y ninja-build + CMAKE_GENERATOR=Ninja make + python3 ./build/tests/no_router.py + timeout-minutes: 5 diff --git a/CMakeLists.txt b/CMakeLists.txt index edeabe8e6..d564bd435 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -185,7 +185,7 @@ set(Z_FEATURE_SUBSCRIPTION 1 CACHE STRING "Toggle subscription feature") set(Z_FEATURE_QUERY 1 CACHE STRING "Toggle query feature") set(Z_FEATURE_QUERYABLE 1 CACHE STRING "Toggle queryable feature") set(Z_FEATURE_RAWETH_TRANSPORT 0 CACHE STRING "Toggle raw ethernet transport feature") -set(Z_FEATURE_INTEREST 0 CACHE STRING "Toggle interest feature") # Toggle to 1 when protocol changes are merged +set(Z_FEATURE_INTEREST 1 CACHE STRING "Toggle interest feature") add_definition(Z_FEATURE_MULTI_THREAD=${Z_FEATURE_MULTI_THREAD}) add_definition(Z_FEATURE_PUBLICATION=${Z_FEATURE_PUBLICATION}) add_definition(Z_FEATURE_SUBSCRIPTION=${Z_FEATURE_SUBSCRIPTION}) @@ -405,6 +405,7 @@ if(UNIX OR MSVC) configure_file(${PROJECT_SOURCE_DIR}/tests/fragment.py ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/fragment.py COPYONLY) configure_file(${PROJECT_SOURCE_DIR}/tests/single_thread.py ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/single_thread.py COPYONLY) configure_file(${PROJECT_SOURCE_DIR}/tests/attachment.py ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/attachment.py COPYONLY) + configure_file(${PROJECT_SOURCE_DIR}/tests/no_router.py ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/no_router.py COPYONLY) enable_testing() add_test(z_data_struct_test ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/z_data_struct_test) diff --git a/include/zenoh-pico/collections/refcount.h b/include/zenoh-pico/collections/refcount.h index d4b0d5e4e..8b548fdc9 100644 --- a/include/zenoh-pico/collections/refcount.h +++ b/include/zenoh-pico/collections/refcount.h @@ -279,6 +279,22 @@ extern "C++" { z_free(p->in); \ return true; \ } \ + static inline _Bool name##_rc_decr(name##_rc_t *p) { \ + if (_ZP_RC_OP_DECR_AND_CMP_STRONG(1)) { \ + if (_ZP_RC_OP_DECR_AND_CMP_WEAK(1)) { \ + return false; \ + } \ + _ZP_RC_OP_SYNC \ + z_free(p->in); \ + return true; \ + } \ + if (_ZP_RC_OP_DECR_AND_CMP_WEAK(1)) { \ + return false; \ + } \ + _ZP_RC_OP_SYNC \ + z_free(p->in); \ + return true; \ + } \ static inline name##_weak_t name##_weak_null(void) { \ name##_weak_t p; \ p.in = NULL; \ diff --git a/src/api/api.c b/src/api/api.c index 4ac72ebd0..67baad6d5 100644 --- a/src/api/api.c +++ b/src/api/api.c @@ -867,7 +867,7 @@ int8_t z_open(z_owned_session_t *zs, z_owned_config_t *config) { // Open session int8_t ret = _z_open(&zs->_rc, &config->_val); if (ret != _Z_RES_OK) { - _z_session_rc_drop(&zs->_rc); + _z_session_rc_decr(&zs->_rc); z_session_null(zs); z_config_drop(config); return ret; diff --git a/tests/no_router.py b/tests/no_router.py new file mode 100644 index 000000000..0e1d2c5a5 --- /dev/null +++ b/tests/no_router.py @@ -0,0 +1,155 @@ +import argparse +import os +from signal import SIGINT +import subprocess +import sys +import time + +# Specify the directory for the binaries +DIR_EXAMPLES = "build/examples" + + +def test_all(): + print("*** Pub & sub test ***") + test_status = 0 + + # Expected output & status + z_expected_status = 255 + z_expected_output = '''Opening session... +Unable to open session!''' + + print("Start subscriber") + # Start z_sub + z_sub_command = f"stdbuf -oL -eL ./{DIR_EXAMPLES}/z_sub" + z_sub_process = subprocess.Popen( + z_sub_command, shell=True, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True + ) + # Wait for z_sub to finish + z_sub_process.wait() + + print("Start publisher") + # Start z_pub + z_pub_command = f"stdbuf -oL -eL ./{DIR_EXAMPLES}/z_pub" + z_pub_process = subprocess.Popen( + z_pub_command, shell=True, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True + ) + # Wait for z_pub to finish + z_pub_process.wait() + + print("Start queryable") + # Start z_queryable + z_queryable_command = f"stdbuf -oL -eL ./{DIR_EXAMPLES}/z_queryable" + z_queryable_process = subprocess.Popen( + z_queryable_command, + shell=True, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + # Wait for z_queryable to finish + z_queryable_process.wait() + + print("Start query") + # Start z_query + z_query_command = f"stdbuf -oL -eL ./{DIR_EXAMPLES}/z_get" + z_query_process = subprocess.Popen( + z_query_command, + shell=True, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + # Wait for z_query to finish + z_query_process.wait() + + print("Check publisher status & output") + # Check the exit status of z_pub + z_pub_status = z_pub_process.returncode + if z_pub_status == z_expected_status: + print("z_pub status valid") + else: + print(f"z_pub status invalid, expected: {z_expected_status}, received: {z_pub_status}") + test_status = 1 + + # Check output of z_pub + z_pub_output = z_pub_process.stdout.read() + if z_expected_output in z_pub_output: + print("z_pub output valid") + else: + print("z_pub output invalid:") + print(f"Expected: \"{z_expected_output}\"") + print(f"Received: \"{z_pub_output}\"") + test_status = 1 + + print("Check subscriber status & output") + # Check the exit status of z_sub + z_sub_status = z_sub_process.returncode + if z_sub_status == z_expected_status: + print("z_sub status valid") + else: + print(f"z_sub status invalid, expected: {z_expected_status}, received: {z_sub_status}") + test_status = 1 + + # Check output of z_sub + z_sub_output = z_sub_process.stdout.read() + if z_expected_output in z_sub_output: + print("z_sub output valid") + else: + print("z_sub output invalid:") + print(f"Expected: \"{z_expected_output}\"") + print(f"Received: \"{z_sub_output}\"") + test_status = 1 + + print("Check query status & output") + # Check the exit status of z_query + z_query_status = z_query_process.returncode + if z_query_status == z_expected_status: + print("z_query status valid") + else: + print(f"z_query status invalid, expected: {z_expected_status}," f" received: {z_query_status}") + test_status = 1 + + # Check output of z_query + z_query_output = z_query_process.stdout.read() + if z_expected_output in z_query_output: + print("z_query output valid") + else: + print("z_query output invalid:") + print(f'Expected: "{z_expected_output}"') + print(f'Received: "{z_query_output}"') + test_status = 1 + + print("Check queryable status & output") + # Check the exit status of z_queryable + z_queryable_status = z_queryable_process.returncode + if z_queryable_status == z_expected_status: + print("z_queryable status valid") + else: + print(f"z_queryable status invalid, expected: {z_expected_status}," f" received: {z_queryable_status}") + test_status = 1 + + # Check output of z_queryable + z_queryable_output = z_queryable_process.stdout.read() + if z_expected_output in z_queryable_output: + print("z_queryable output valid") + else: + print("z_queryable output invalid:") + print(f'Expected: "{z_expected_output}"') + print(f'Received: "{z_queryable_output}"') + test_status = 1 + + # Return value + return test_status + + +if __name__ == "__main__": + EXIT_STATUS = 0 + + # Test all examples + if test_all() == 1: + EXIT_STATUS = 1 + + # Exit + sys.exit(EXIT_STATUS) diff --git a/tests/z_refcount_test.c b/tests/z_refcount_test.c index e5ad98374..91e7392e3 100644 --- a/tests/z_refcount_test.c +++ b/tests/z_refcount_test.c @@ -222,6 +222,13 @@ void test_overflow(void) { assert(dwk1.in == NULL); } +void test_decr(void) { + _dummy_rc_t drc1 = _dummy_rc_new(); + _dummy_rc_t drc2 = _dummy_rc_clone(&drc1); + assert(!_dummy_rc_decr(&drc2)); + assert(_dummy_rc_decr(&drc1)); +} + int main(void) { test_rc_null(); test_rc_size(); @@ -239,5 +246,6 @@ int main(void) { test_weak_copy(); test_weak_upgrade(); test_overflow(); + test_decr(); return 0; }