Add connect/disconnect, reduce copy, simplify headers #23

billphipps · 2024-04-18T04:27:15Z

A number of changes:

Adds server-side external connect/disconnect handlers
Adds server transport callbacks that can trigger a connect or disconnect from the transport layer
Adds CommInit message to exchange client and server id's
Adds CommClose message which will trigger server to disconnect
Updated tests to now use CommClose to close servers async
Consolidated wh_transport.h into wh_comm.h to avoid circular includes
Consolidated wh_server_dma.h into wh_server.h to avoid circular includes
Eliminated declaring counter variables within for loops
Eliminated // comments. Now compiles clean for C90
Reordered/typed whServerContext and whClientContext to ensure packet buffer is 64-bit aligned
Exposed data ptr into internal buffer for CommServer and CommClient to eliminate extra copies and space
Reworked server handler to use internal buffer

Nearly all of these changes are necessary to make the Bernini and Aurix ports compile clean and align packet buffers correctly as well as use optimized memcpy.

src/wh_comm.c

src/wh_server.c

jpbland1 · 2024-04-18T09:03:46Z

wolfhsm/wh_comm.h

@@ -32,7 +32,8 @@
 enum {
    WH_COMM_HEADER_LEN = 8,    /* whCommHeader */
    WH_COMM_DATA_LEN = 1280,
-    WH_COMM_MTU = (WH_COMM_HEADER_LEN + WH_COMM_DATA_LEN)
+    WH_COMM_MTU = (WH_COMM_HEADER_LEN + WH_COMM_DATA_LEN),
+    WH_COMM_MTU_8 = (WH_COMM_MTU + 7) / 8,


I think we should call it WH_COMM_MTU_BITS, wolfSSL defines things as bits when referring to sizes in bits: #define SP_INT_BITS 4096

Hah! This is actually the rounded-up number of 8-byte uint64_t's needed to hold an MTU packet. How about I change this to WH_COMM_MTU_64?

Oooh yeah that is a little confusing, I had to double take this a few times to be sure, as +7/8 looks like rounding up bits to the next byte on first glance, but we are dealing with bytes and 64-bit words here.

Maybe one of the following, or a permutation thereof?
WH_COMM_MTU_ALIGN64_COUNT
WH_COMM_MTU_WORD64_COUNT
WH_COMM_MTU_ALIGN64_WORD_COUNT

I think explicitly calling out sizes and purpose is a good thing.

ah gotcha yeah thats much less confusing, I'd also add a comment

went with WH_COMM_MTU_U64_COUNT with a comment. Fixed in next push.

wolfhsm/wh_comm.h

wolfhsm/wh_server.h

test/wh_test_crypto.c

wolfhsm/wh_server.h

bigbrett · 2024-04-18T15:30:33Z

wolfhsm/wh_comm.h

@@ -32,7 +32,8 @@
 enum {
    WH_COMM_HEADER_LEN = 8,    /* whCommHeader */
    WH_COMM_DATA_LEN = 1280,
-    WH_COMM_MTU = (WH_COMM_HEADER_LEN + WH_COMM_DATA_LEN)
+    WH_COMM_MTU = (WH_COMM_HEADER_LEN + WH_COMM_DATA_LEN),
+    WH_COMM_MTU_8 = (WH_COMM_MTU + 7) / 8,


Oooh yeah that is a little confusing, I had to double take this a few times to be sure, as +7/8 looks like rounding up bits to the next byte on first glance, but we are dealing with bytes and 64-bit words here.

Maybe one of the following, or a permutation thereof?
WH_COMM_MTU_ALIGN64_COUNT
WH_COMM_MTU_WORD64_COUNT
WH_COMM_MTU_ALIGN64_WORD_COUNT

I think explicitly calling out sizes and purpose is a good thing.

bigbrett · 2024-04-18T15:33:31Z

wolfhsm/wh_comm.h

+typedef int (*whCommSetConnectedCb)(void* context, int connected);
+
+/* Status of whether a client is connected or not */
+enum {
+    WH_COMM_DISCONNECTED = 0,
+    WH_COMM_CONNECTED = 1,
+};


Thoughts on typing enums, especially when an argument is assumed to be one of a few values? I think it adds clarity as to what should be passed in, as opposed to relying on comments or a hopefully obvious enum name. I'm pro anything that lets code be more self-documenting. Something like:

/* Status of whether a client is connected or not */ typedef enum { WH_COMM_DISCONNECTED = 0, WH_COMM_CONNECTED = 1, } whCommConnectState; typedef int (*whCommSetConnectedCb)(void* context, whCommConnectState connected);

Concur. I was trying to keep the callbacks generic and untyped, but there's no reason not to force the type to an enum. Fixed in next push.

src/wh_server.c

bigbrett · 2024-04-18T18:12:17Z

test/wh_test_crypto.c

+    for (   wh_Server_SetConnected(server, am_connected);
+            am_connected == WH_COMM_CONNECTED;
+            wh_Server_GetConnected(server, &am_connected) ){


this is fun

Prolly too fancy. I'll switch it to a while structure. Fixed in next push

bigbrett · 2024-04-18T18:17:39Z

test/wh_test_clientserver.c

@@ -1235,6 +1236,7 @@ int whTest_ClientCfg(whClientConfig* clientCfg)
    WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK);
    WH_TEST_ASSERT_RETURN(avail_objects == NF_OBJECT_COUNT);

+    wh_Client_CommClose(client);


Suggested change

wh_Client_CommClose(client);

WH_TEST_RETURN_ON_FAIL(wh_Client_CommClose(client));

Fixed in next push.

Added refactor to backlog. Improved names

jpbland1

Looks good

billphipps added 2 commits April 18, 2024 00:17

Add connect, dataPtr, reorg struct features. Consolidated headers.

a06c0c4

Fix typo.

f039d11

billphipps requested review from bigbrett and jpbland1 April 18, 2024 04:27

jpbland1 previously requested changes Apr 18, 2024

View reviewed changes

bigbrett previously approved these changes Apr 18, 2024

View reviewed changes

Naming/testing fixes from peer review.

e433816

billphipps dismissed bigbrett’s stale review via e433816 April 18, 2024 19:45

Fix typos.

dba670c

billphipps requested a review from bigbrett April 18, 2024 19:50

jpbland1 self-requested a review April 18, 2024 20:16

jpbland1 approved these changes Apr 18, 2024

View reviewed changes

jpbland1 merged commit 5375227 into wolfSSL:main Apr 18, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connect/disconnect, reduce copy, simplify headers #23

Add connect/disconnect, reduce copy, simplify headers #23

billphipps commented Apr 18, 2024

jpbland1 Apr 18, 2024

billphipps Apr 18, 2024

bigbrett Apr 18, 2024 •

edited

Loading

jpbland1 Apr 18, 2024

billphipps Apr 18, 2024

bigbrett Apr 18, 2024 •

edited

Loading

bigbrett Apr 18, 2024

billphipps Apr 18, 2024

bigbrett Apr 18, 2024

billphipps Apr 18, 2024

bigbrett Apr 18, 2024

billphipps Apr 18, 2024

jpbland1 left a comment

	wh_Client_CommClose(client);
	WH_TEST_RETURN_ON_FAIL(wh_Client_CommClose(client));

Add connect/disconnect, reduce copy, simplify headers #23

Add connect/disconnect, reduce copy, simplify headers #23

Conversation

billphipps commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigbrett Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigbrett Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbland1 left a comment

Choose a reason for hiding this comment

bigbrett Apr 18, 2024 •

edited

Loading

bigbrett Apr 18, 2024 •

edited

Loading