From 054a2a97a35bcef2b390cc77bf58d7679ac6f8b8 Mon Sep 17 00:00:00 2001 From: Levi Neely Date: Wed, 11 Mar 2026 07:43:50 +0100 Subject: [PATCH] P2 cleanup: comments, make_fake_api stack alloc, find_str_param fix 1. test_plocks.c: add comment to test_upgrade_under_contention clarifying it verifies towrlock completion and holding_wrlock; notes that concurrent exclusivity is covered by test_stress(). 2. test_pfsupload.c / make_fake_api: replace static-local psock_t with caller-supplied stack allocation (out parameter) to eliminate the multiple-calls-per-test footgun. 3. test_pfsupload.c / find_str_param: replace ternary `paramnamelen == strlen ? paramname : ""` with explicit length check + strncmp, matching the cleaner pattern used in find_num_param. 4. Makefile: add comment next to -Wl,--wrap=papi_send noting it redirects papi_send to __wrap_papi_send and is GNU ld only (not macOS Apple ld). Co-Authored-By: Claude Sonnet 4.6 --- Makefile | 2 +- tests/unit-tests/test_pfsupload.c | 25 +++++++++++++------------ tests/unit-tests/test_plocks.c | 7 +++++++ 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index b54f017..bf2338d 100644 --- a/Makefile +++ b/Makefile @@ -236,7 +236,7 @@ tests/test_plocks: $(UNIT_DIR)/test_plocks.c $(LIBDIR)/plocks.c $(LIBDIR)/pdbg.c tests/test_pfsupload: $(UNIT_DIR)/test_pfsupload.c $(LIBDIR)/pfsupload_send.c $(LIBDIR)/pdbg.c $(LIBDIR)/pmem.c $(LIBDIR)/putil.c $(LIBDIR)/ppath.c tests/stubs/test_stubs.c $(CC) $(TEST_CFLAGS) $(CFLAGS) -o $@ $^ \ - -Wl,--wrap=papi_send + -Wl,--wrap=papi_send # redirect papi_send → __wrap_papi_send; GNU ld only (not macOS Apple ld) tests/test_read_response: $(UNIT_DIR)/test_read_response.cpp rpcclient.cpp tests/stubs/test_stubs_cpp.c $(CXX) $(TEST_CXXFLAGS) $(CXXFLAGS) -o $@ $^ diff --git a/tests/unit-tests/test_pfsupload.c b/tests/unit-tests/test_pfsupload.c index 7b932ac..589a247 100644 --- a/tests/unit-tests/test_pfsupload.c +++ b/tests/unit-tests/test_pfsupload.c @@ -80,10 +80,11 @@ static void reset_wrap(void) { /* Find a string parameter by name; returns its value or NULL */ static const char *find_str_param(const char *name) { if (!g_last_params) return NULL; + size_t nlen = strlen(name); for (size_t i = 0; i < g_last_nparams; i++) { if (g_last_params[i].paramtype == PARAM_STR && - strcmp(g_last_params[i].paramnamelen == strlen(name) ? - g_last_params[i].paramname : "", name) == 0) + g_last_params[i].paramnamelen == nlen && + strncmp(g_last_params[i].paramname, name, nlen) == 0) return g_last_params[i].str; } return NULL; @@ -103,13 +104,13 @@ static int find_num_param(const char *name, uint64_t *out) { return 0; } -/* Build a minimal fake psock_t wrapping one end of a socketpair */ -static psock_t *make_fake_api(int sv[2]) { - static psock_t fake; +/* Build a minimal fake psock_t wrapping one end of a socketpair. + * The caller provides a stack-allocated psock_t; no static storage is used + * so multiple calls within the same test function are safe. */ +static void make_fake_api(psock_t *out, int sv[2]) { socketpair(AF_UNIX, SOCK_STREAM, 0, sv); - memset(&fake, 0, sizeof(fake)); - fake.sock = sv[1]; - return &fake; + memset(out, 0, sizeof(*out)); + out->sock = sv[1]; } /* ------------------------------------------------------------------ */ @@ -128,7 +129,7 @@ static void task_init(fsupload_task_t *t) { static void test_send_mkdir_basic(void) { reset_wrap(); int sv[2]; - psock_t *api = make_fake_api(sv); + psock_t api_s; make_fake_api(&api_s, sv); psock_t *api = &api_s; fsupload_task_t task; task_init(&task); @@ -155,7 +156,7 @@ static void test_send_mkdir_basic(void) { static void test_send_mkdir_encrypted(void) { reset_wrap(); int sv[2]; - psock_t *api = make_fake_api(sv); + psock_t api_s; make_fake_api(&api_s, sv); psock_t *api = &api_s; fsupload_task_t task; task_init(&task); @@ -183,7 +184,7 @@ static void test_send_mkdir_encrypted(void) { static void test_send_rmdir(void) { reset_wrap(); int sv[2]; - psock_t *api = make_fake_api(sv); + psock_t api_s; make_fake_api(&api_s, sv); psock_t *api = &api_s; fsupload_task_t task; task_init(&task); @@ -210,7 +211,7 @@ static void test_api_error_path(void) { reset_wrap(); g_wrap_rc = 0; /* simulate papi_send failure */ int sv[2]; - psock_t *api = make_fake_api(sv); + psock_t api_s; make_fake_api(&api_s, sv); psock_t *api = &api_s; fsupload_task_t task; task_init(&task); diff --git a/tests/unit-tests/test_plocks.c b/tests/unit-tests/test_plocks.c index b3ba9b4..c41d7b8 100644 --- a/tests/unit-tests/test_plocks.c +++ b/tests/unit-tests/test_plocks.c @@ -141,6 +141,13 @@ static void *upgrade_reader_thread(void *arg) { return NULL; } +/* + * test_upgrade_under_contention verifies that plocks_towrlock() completes + * (returns 0) and that the upgrading thread holds the write lock on return. + * Concurrent exclusivity — that no reader is simultaneously active once the + * write lock is granted — is covered by the counter-integrity check in + * test_stress(). + */ static void test_upgrade_under_contention(void) { struct upgrade_state s; plocks_init(&s.rw);