Add C csm-tcp-router-client SDK (multi-platform, VS2026, CMake, tests)#40
Conversation
Agent-Logs-Url: https://github.com/NEVSTOP-LAB/CSM-TCP-Router-App/sessions/ea6c42b2-b6f8-4ebe-8438-786601832ef5 Co-authored-by: nevstop <8196752+nevstop@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new portable C (C99) client SDK under SDK/c/ for the CSM-TCP-Router project, including cross-platform build tooling (CMake + VS2026), an in-process mock server, and a test suite with CI.
Changes:
- Introduces the C SDK public API (
include/) and implementation (src/) with a background receive thread, protocol codec, and sync/async APIs. - Adds unit + integration tests (including a cross-platform in-process
MockServer) and example programs. - Adds build & developer workflows: CMake project, VS2026 solution/projects, and a GitHub Actions workflow running
ctestacross OSes (+ sanitizers on Linux).
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/C_SDK.yml | CI workflow for building/testing the C SDK on Linux/macOS/Windows + sanitizers on Linux |
| SDK/c/.gitignore | Ignores CMake and Visual Studio build artifacts for the C SDK |
| SDK/c/CMakeLists.txt | Cross-platform CMake build for library, tests, and examples |
| SDK/c/CHANGELOG.md | Initial changelog entry for C SDK v0.1.0 |
| SDK/c/LICENSE | MIT license file for the C SDK distribution |
| SDK/c/README.md | English documentation for the C SDK (features, build, API, tests) |
| SDK/c/README.zh-cn.md | Chinese documentation for the C SDK (features, build, API, tests) |
| SDK/c/examples/basic_usage.c | Example showing connect/ping/list and basic command flow |
| SDK/c/examples/subscribe_status.c | Example showing status subscription callback usage |
| SDK/c/include/csm_tcp_router_client.h | Public C API: types, protocol helpers, client API, docs |
| SDK/c/src/csm_tcp_router_client.c | Single-file implementation: platform shims, protocol codec, client logic, threading |
| SDK/c/tests/mock_server.c | Cross-platform in-process mock server implementation used by integration tests |
| SDK/c/tests/mock_server.h | Mock server public test fixture header |
| SDK/c/tests/test_client.c | Unit tests covering client lifecycle + invalid args + unreachable server behavior |
| SDK/c/tests/test_harness.h | Minimal assertion/test harness used by the C test suite |
| SDK/c/tests/test_integration.c | End-to-end tests against the in-process mock server |
| SDK/c/tests/test_main.c | Test runner that registers and executes all tests |
| SDK/c/tests/test_protocol.c | Unit tests for protocol encode/decode/parse helpers |
| SDK/c/vs2026/README.md | Documentation for building/running tests via Visual Studio 2026 |
| SDK/c/vs2026/csm_tcp_router_client.sln | VS2026 solution containing library + test executable projects |
| SDK/c/vs2026/csm_tcp_router_client/csm_tcp_router_client.vcxproj | VS2026 static library project configuration |
| SDK/c/vs2026/csm_tcp_router_client/csm_tcp_router_client.vcxproj.filters | VS project filters for source/header organization |
| SDK/c/vs2026/csm_tcp_router_client.tests/csm_tcp_router_client.tests.vcxproj | VS2026 console test executable project configuration |
| SDK/c/vs2026/csm_tcp_router_client.tests/csm_tcp_router_client.tests.vcxproj.filters | VS test project filters for source/header organization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| r->cmd = (char *)malloc(strlen(cmd_text) + 1); strcpy(r->cmd, cmd_text); | ||
| r->type = type; | ||
| size_t dl = strlen(data); | ||
| r->data = (uint8_t *)malloc(dl); if (dl) memcpy(r->data, data, dl); |
There was a problem hiding this comment.
ms_set_response_typed() does not check allocation results before using them: malloc(strlen(cmd_text)+1) is immediately passed to strcpy, and malloc(dl) can be NULL but is still used by memcpy when dl > 0. This can crash tests under low-memory conditions or sanitizers. Add NULL checks and free r on failure before returning.
| r->cmd = (char *)malloc(strlen(cmd_text) + 1); strcpy(r->cmd, cmd_text); | |
| r->type = type; | |
| size_t dl = strlen(data); | |
| r->data = (uint8_t *)malloc(dl); if (dl) memcpy(r->data, data, dl); | |
| r->cmd = (char *)malloc(strlen(cmd_text) + 1); | |
| if (!r->cmd) { | |
| free(r); | |
| return; | |
| } | |
| strcpy(r->cmd, cmd_text); | |
| r->type = type; | |
| size_t dl = strlen(data); | |
| if (dl > 0) { | |
| r->data = (uint8_t *)malloc(dl); | |
| if (!r->data) { | |
| free(r->cmd); | |
| free(r); | |
| return; | |
| } | |
| memcpy(r->data, data, dl); | |
| } |
| static int g_wsa_lock_inited = 0; | ||
| static int g_wsa_refcount = 0; | ||
|
|
||
| static void csm_wsa_lock_init_once(void) { | ||
| /* Process-wide one-time init. Safe because csm_client_create is | ||
| * the only caller and the OS guarantees serialisation of the very | ||
| * first call by virtue of program startup ordering; subsequent | ||
| * concurrent callers race only on the lock-init itself, but | ||
| * InitializeCriticalSection is idempotent-safe enough for our use | ||
| * since we only ever protect refcount. In practice clients should | ||
| * create the first instance from a single thread. | ||
| */ | ||
| if (!g_wsa_lock_inited) { | ||
| csm_mutex_init(&g_wsa_lock); | ||
| g_wsa_lock_inited = 1; | ||
| } |
There was a problem hiding this comment.
csm_wsa_lock_init_once() is not actually thread-safe: multiple concurrent callers can race on g_wsa_lock_inited and call InitializeCriticalSection on the same storage more than once, which is undefined behavior. Use a proper one-time init primitive (e.g., InitOnceExecuteOnce, InterlockedCompareExchange guard, or a function-local static initialized via InitOnce) to guarantee single initialization of g_wsa_lock.
| static int g_wsa_lock_inited = 0; | |
| static int g_wsa_refcount = 0; | |
| static void csm_wsa_lock_init_once(void) { | |
| /* Process-wide one-time init. Safe because csm_client_create is | |
| * the only caller and the OS guarantees serialisation of the very | |
| * first call by virtue of program startup ordering; subsequent | |
| * concurrent callers race only on the lock-init itself, but | |
| * InitializeCriticalSection is idempotent-safe enough for our use | |
| * since we only ever protect refcount. In practice clients should | |
| * create the first instance from a single thread. | |
| */ | |
| if (!g_wsa_lock_inited) { | |
| csm_mutex_init(&g_wsa_lock); | |
| g_wsa_lock_inited = 1; | |
| } | |
| static INIT_ONCE g_wsa_lock_init_once_state = INIT_ONCE_STATIC_INIT; | |
| static int g_wsa_refcount = 0; | |
| static BOOL CALLBACK csm_wsa_lock_init_once_callback(PINIT_ONCE init_once, | |
| PVOID param, | |
| PVOID *context) { | |
| (void)init_once; | |
| (void)param; | |
| (void)context; | |
| csm_mutex_init(&g_wsa_lock); | |
| return TRUE; | |
| } | |
| static void csm_wsa_lock_init_once(void) { | |
| InitOnceExecuteOnce(&g_wsa_lock_init_once_state, | |
| csm_wsa_lock_init_once_callback, | |
| NULL, | |
| NULL); |
| if (c->stop_flag) break; | ||
| if (csm_recv_all(c->sock, header, CSM_HEADER_SIZE) != 0) break; | ||
| uint32_t data_len = csm_unpack_be32(header); | ||
| uint8_t *body = NULL; | ||
| if (data_len > 0) { | ||
| body = (uint8_t *)malloc(data_len); | ||
| if (!body) break; | ||
| if (csm_recv_all(c->sock, body, data_len) != 0) { |
There was a problem hiding this comment.
The receive thread reads c->stop_flag and c->sock without holding state_lock (or using atomics), while csm_client_disconnect() mutates them under the lock. This introduces a data race (and can be flagged by TSAN/UBSan). Consider making these fields atomic, or always reading them under state_lock (e.g., snapshot stop_flag/sock under lock each loop iteration).
| if (c->stop_flag) break; | |
| if (csm_recv_all(c->sock, header, CSM_HEADER_SIZE) != 0) break; | |
| uint32_t data_len = csm_unpack_be32(header); | |
| uint8_t *body = NULL; | |
| if (data_len > 0) { | |
| body = (uint8_t *)malloc(data_len); | |
| if (!body) break; | |
| if (csm_recv_all(c->sock, body, data_len) != 0) { | |
| int stop_flag; | |
| csm_socket_t sock; | |
| csm_mutex_lock(&c->state_lock); | |
| stop_flag = c->stop_flag; | |
| sock = c->sock; | |
| csm_mutex_unlock(&c->state_lock); | |
| if (stop_flag) break; | |
| if (sock == CSM_INVALID_SOCKET) break; | |
| if (csm_recv_all(sock, header, CSM_HEADER_SIZE) != 0) break; | |
| uint32_t data_len = csm_unpack_be32(header); | |
| uint8_t *body = NULL; | |
| if (data_len > 0) { | |
| body = (uint8_t *)malloc(data_len); | |
| if (!body) break; | |
| if (csm_recv_all(sock, body, data_len) != 0) { |
| #define CSM_BUILD_LIBRARY 1 | ||
|
|
There was a problem hiding this comment.
This translation unit unconditionally defines CSM_BUILD_LIBRARY. Build/ABI selection macros like this should be controlled by the build system (CMake/MSBuild) rather than hard-coded in the source, otherwise consumers that compile the .c directly or build a shared library can end up with incorrect CSM_API import/export decoration. Consider removing this #define and relying on target_compile_definitions / project preprocessor definitions instead.
| #define CSM_BUILD_LIBRARY 1 |
| ioctlsocket(sock, FIONBIO, &mode); | ||
| #else | ||
| int flags = fcntl(sock, F_GETFL, 0); | ||
| fcntl(sock, F_SETFL, flags | O_NONBLOCK); |
There was a problem hiding this comment.
On POSIX, the return value from fcntl(sock, F_GETFL, 0) is not checked. If it returns -1, the subsequent F_SETFL uses an invalid flags value, which can break the non-blocking connect logic. Handle F_GETFL/F_SETFL failures (e.g., treat as CSM_ERR_CONNECTION and try the next addrinfo) to avoid undefined socket state.
| fcntl(sock, F_SETFL, flags | O_NONBLOCK); | |
| if (flags == -1 || fcntl(sock, F_SETFL, flags | O_NONBLOCK) == -1) { | |
| csm_close_socket(sock); | |
| sock = CSM_INVALID_SOCKET; | |
| continue; | |
| } |
| /** Retrieve information about the last CSM_ERR_SERVER returned by *client*. | ||
| * | ||
| * Returns CSM_OK and fills *out_err* if a server error has been observed | ||
| * since the last successful call, otherwise returns CSM_ERR_STATE. |
There was a problem hiding this comment.
The doc says server errors are returned only if observed "since the last successful call", which implies the stored error state should be cleared after returning CSM_OK. The implementation never clears has_server_error, so repeated calls will keep returning the same error. Either update the doc to match the current behavior or clear the flag in csm_client_last_server_error() after copying last_server_error.
| /** Retrieve information about the last CSM_ERR_SERVER returned by *client*. | |
| * | |
| * Returns CSM_OK and fills *out_err* if a server error has been observed | |
| * since the last successful call, otherwise returns CSM_ERR_STATE. | |
| /** Retrieve information about the last CSM_ERR_SERVER observed by *client*. | |
| * | |
| * Returns CSM_OK and fills *out_err* if a server error has been observed; | |
| * otherwise returns CSM_ERR_STATE. |
|
@copilot 根据Comment修复 |
Agent-Logs-Url: https://github.com/NEVSTOP-LAB/CSM-TCP-Router-App/sessions/de04612d-484c-4905-9c04-06c451a01e15 Co-authored-by: nevstop <8196752+nevstop@users.noreply.github.com>
…teOnce Agent-Logs-Url: https://github.com/NEVSTOP-LAB/CSM-TCP-Router-App/sessions/de04612d-484c-4905-9c04-06c451a01e15 Co-authored-by: nevstop <8196752+nevstop@users.noreply.github.com>
已根据 6 条评审意见修复（commit
ASan + UBSan 下 22 条测试全部通过。 |
Plan
InitOnceExecuteOncefor true one-time init; redundantg_wsa_lock_initedflag removed (cleanup also routed throughInitOnceExecuteOnce).stop_flag/sockunderstate_lockeach iteration.#define CSM_BUILD_LIBRARY(controlled by build system only).csm_do_connectchecksfcntlreturn values and skips to the next addrinfo on failure.csm_client_last_server_errordoc to match implemented behaviour.All 22 tests pass on Release and under ASan + UBSan on Linux.