Skip to content

Conversation

@AlexLanzano
Copy link
Member

No description provided.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a generic timeout utility and wires client-side response timeouts into crypto operations, plus unit tests for the timeout helper.

Changes:

  • Add a generic timeout module (wh_timeout.[ch]) based on WH_GETTIME_US() and expose it via configuration (WOLFHSM_CFG_ENABLE_TIMEOUT) and a new error code WH_ERROR_TIMEOUT.
  • Extend whClientContext/whClientConfig and add wh_Client_RecvResponseTimeout, then route all crypto client receive paths through a new _recvCryptoResponse helper that uses the timeout when enabled.
  • Add unit tests for the timeout helper and enable timeout support in the test configuration.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
wolfhsm/wh_timeout.h Declares timeout context/config types and the timeout API used by the client; documentation establishes that timeoutUs == 0 disables the timeout.
wolfhsm/wh_settings.h Documents WOLFHSM_CFG_ENABLE_TIMEOUT as enabling timeout helpers and client response timeouts; also defines WH_GETTIME_US(), which the timeout code relies on.
wolfhsm/wh_error.h Introduces WH_ERROR_TIMEOUT to signal an expired timeout from client operations.
wolfhsm/wh_client.h Adds a per-client respTimeout context, an optional respTimeout config, and declares wh_Client_RecvResponseTimeout behind WOLFHSM_CFG_ENABLE_TIMEOUT.
test/wh_test_timeout.h Declares the whTest_Timeout unit test entry point.
test/wh_test_timeout.c Implements unit tests for wh_Timeout_*, including callback invocation, stop/disable behavior, and bad-argument handling.
test/wh_test.c Wires whTest_Timeout() into the unit test suite when WOLFHSM_CFG_ENABLE_TIMEOUT is defined.
test/config/wolfhsm_cfg.h Enables WOLFHSM_CFG_ENABLE_TIMEOUT in the test configuration and ensures a valid time source via WOLFHSM_CFG_PORT_GETTIME.
src/wh_timeout.c Implements the timeout helper functions; currently treats timeoutUs == 0 as an error in wh_Timeout_Start, which conflicts with the documented “0 disables” semantics and impacts higher-level usage.
src/wh_client_crypto.c Introduces _recvCryptoResponse and switches all crypto receive loops to use it; with timeout support enabled this always goes through wh_Client_RecvResponseTimeout and the per-client respTimeout.
src/wh_client.c Initializes the optional respTimeout context in wh_Client_Init and adds wh_Client_RecvResponseTimeout, which loops on WH_ERROR_NOTREADY until a valid response arrives or the timeout expires.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +211 to +233
int wh_Client_RecvResponseTimeout(whClientContext *c,
uint16_t *out_group, uint16_t *out_action,
uint16_t *out_size, void* data, whTimeoutCtx *timeout)
{
int ret;

if ((c == NULL) || (timeout == NULL)) {
return WH_ERROR_BADARGS;
}

ret = wh_Timeout_Start(timeout);
if (ret != WH_ERROR_OK) {
return ret;
}

do {
ret = wh_Client_RecvResponse(c, out_group, out_action, out_size, data);
if ((ret == WH_ERROR_NOTREADY) && wh_Timeout_Expired(timeout)) {
return WH_ERROR_TIMEOUT;
}
} while (ret == WH_ERROR_NOTREADY);

return ret;
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wh_Client_RecvResponseTimeout assumes the supplied whTimeoutCtx is fully configured and immediately calls wh_Timeout_Start, propagating any non-WH_ERROR_OK return value. Given that wh_Timeout_Start currently returns WH_ERROR_BADARGS when timeoutUs == 0, this means that using a "disabled" timeout (timeout set to 0 per the docs in wh_timeout.h) causes every receive to fail with an error rather than behave like an infinite wait, which is surprising relative to the configuration comment for timeoutUs. It would be more robust either to special-case a disabled timeout here (e.g., bypass the timeout loop and delegate directly to wh_Client_RecvResponse), or to update the timeout API so that starting a disabled timeout is a no-op success instead of an error, aligning behavior with the documented "0 disables the timeout" semantics.

Copilot uses AI. Check for mistakes.
Comment on lines +61 to +62
timeout->startUs = 0;
return WH_ERROR_BADARGS;
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wh_Timeout_Start treats a configured timeout of 0 microseconds as an error and returns WH_ERROR_BADARGS, even though the API docs for wh_Timeout_Set describe timeoutUs == 0 as "disables the timeout" and wh_Timeout_Expired treats timeoutUs == 0 as disabled. This makes it impossible to use wh_Client_RecvResponseTimeout with a "disabled" timeout (for example when respTimeout->timeoutUs is 0), since every call will immediately fail on wh_Timeout_Start rather than behaving like an infinite-wait receive. Consider either making wh_Timeout_Start a no-op that returns success when timeoutUs == 0 (leaving the timeout effectively disabled), or clearly documenting and enforcing that callers must never invoke Start on a disabled timeout and updating higher-level callers accordingly.

Suggested change
timeout->startUs = 0;
return WH_ERROR_BADARGS;
/* Disabled timeout: no-op, report success. */
timeout->startUs = 0;
return WH_ERROR_OK;

Copilot uses AI. Check for mistakes.
Comment on lines +173 to +174
ret = wh_Client_RecvResponseTimeout(ctx, group, action, size, data,
ctx->respTimeout);
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With WOLFHSM_CFG_ENABLE_TIMEOUT enabled, _recvCryptoResponse always routes crypto responses through wh_Client_RecvResponseTimeout using ctx->respTimeout, but wh_Client_Init only initializes respTimeout when config->respTimeout is non-NULL. If a client is built with timeout support but leaves config->respTimeout as NULL (or sets a config whose timeoutUs is 0), ctx->respTimeout remains all-zero and wh_Client_RecvResponseTimeout will immediately fail with WH_ERROR_BADARGS from wh_Timeout_Start, breaking all crypto calls. Consider either requiring a non-NULL timeout config in wh_Client_Init when timeout support is compiled in (and failing init otherwise), or having _recvCryptoResponse fall back to wh_Client_RecvResponse when no timeout is configured or when the configured timeout is effectively disabled.

Suggested change
ret = wh_Client_RecvResponseTimeout(ctx, group, action, size, data,
ctx->respTimeout);
/* If no effective timeout is configured (e.g., timeoutUs == 0), fall back
* to the non-timeout receive API to avoid WH_ERROR_BADARGS from
* wh_Timeout_Start. */
if (ctx->respTimeout.timeoutUs != 0U) {
ret = wh_Client_RecvResponseTimeout(ctx, group, action, size, data,
ctx->respTimeout);
}
else {
ret = wh_Client_RecvResponse(ctx, group, action, size, data);
}

Copilot uses AI. Check for mistakes.
@AlexLanzano AlexLanzano marked this pull request as draft January 28, 2026 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants