rework example C test code #497
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ctz
approved these changes
Dec 9, 2024
Just removing code that's unused, and moving code unchanged from common.c -> server.c where appropriate. |
|
@jsha is this a branch you're interested in reviewing? It's only test code but also a fairly significant update. |
* Where there is already a `typedef struct foo { ... } foo;` prefer
writing `foo` over `struct foo` throughout.
* Where there isn't already a `typedef`, (e.g. `enum demo_result`) add
one and use it consistently throughout.
Previously we were checking the result of `socket(2)` for the error result (-1) but weren't jumping to the clean up label. This meant we'd fall through to calling `bind(2)` with `sockfd == -1`. Instead, jump directly to jail^H^H^H^Hcleanup.
Instead of using `memset(3)` we can lean on C99 empty structure
initialization syntax, `struct foo = { 0 };`
In C23+ we could use the even more simple `struct foo = {};` syntax, but
C99 seems like the best compatibility goal for now.
* Always use `rustls_result` or `demo_result`, not `unsigned int` or `int`. * Always name `rustls_result` vars `rr` and `demo_result` vars `dr`. * Don't check `rustls_result` instances against `DEMO_XXX` defines and don't check `demo_result` instances against `RUSTLS_RESULT_XXX` defines. * Use `const` and smaller scopes where possible.
* Avoid direct `fprintf()` to `stderr` - we have `LOG` and `LOG_SIMPLE` for that. They handle adding the program name and a `\n`. * Use `print_error()` when printing an error about a `rustls_result`. It handles making a friendly textual description for the `rustls_result` numeric code.
* Try to consolidate declarations and assignments where possible. * Condense scopes where possible. * Use `const` where possible.
* Try to consolidate declarations and assignments where possible. * Condense scopes where possible. * Use `const` where possible.
For Win32, `common.h` redefines `EAGAIN` to `WSAEWOULDBLOCK`, and `EWOULDBLOCK` to `WSAEWOULDBLOCK`. On Unix, `errno.h` defines `EWOULDBLOCK` as `EAGAIN`. Thus, there's no situation where `errno` is either `EAGAIN` _or_ `EWOULDBLOCK` - both are the same value and checking just `EWOULDBLOCK` is sufficient in both client/server.
* Try to consolidate declarations and assignments where possible. * Condense scopes where possible. * Use `const` where possible.
Missed a couple with the previous round.
The `rustls_connection_get_alpn_protocol()` function takes an out param of type `const uint8_t*` for the ALPN data and an out param of type `size_t` for the length. We were using these with `printf()` for a `%.*s` specifier. This requires that we cast the len to `int` (already done), as well as casting the `const uint8_t*` to `const char*` (done as of this commit). Along the way, move the declarations of the two out vars closer to the only code that uses them.
Both `client.c` and `server.c` defined a `do_read()` function that was almost line-for-line identical. The only meaningful difference was the server version returned `DEMO_EOF` earlier for a `n == 0` read. I can't see any reason why this shouldn't be done for the client, so let's hoist that version into `common.c` and use it in both binaries.
Rather than duplicating the same logic in client/server, let's do it in once in `common.c`. Also consistently log the protocol version and ALPN status for both client/server.
* Fix off-by-one in client argc checking. * Move server argv processing up front. * Add a new optional arg for the client for the number of requests to do (defaulting to 3, matching existing behaviour).
The `copy_plaintext_to_buffer()` helper is called in `do_read()`. It may return `DEMO_OK` (0) when there's no more bytes available to copy. In this case it's not very interesting to log from `do_read()`.
Rather than log when we're done writing, log when we've written some bytes. This is more interesting and we already have logging that will kick in when we break the loop.
For `server.c` the changes are fairly minor since it was already a relatively straight-forward and self-contained example: * Handle a potential `EAGAIN` `demo_result` from `write_tls()`. * Add a `server.h` that is presently unused, but allows keeping the compilation rules simple by treating server/client symmetrically. * Break the connection handling loop when we've both sent a response and the rustls connection requires no more writes. This effectively closes the connection after a response has been written, without waiting on the peer to do so. We want to do this since we don't process the HTTP request to learn if the client wanted `Connection: keep-alive` or `Connection: close`. For `client.c`, the changes are more extensive: * Add a `client.h` so we can forward declare everything interesting. This allows `client.c` to match our preferred Rust standard of "top down ordering" * Extract out a `demo_client_options` struct and a `options_from_env()` function for handling options based on the environment. * Extract out a `new_tls_config()` function that takes a pointer to `demo_client_options` and returns a `rustls_client_config`. * Extract out a `demo_client_request_options` struct for per-request options (hostname, port, path, whether to use vectored I/O). * Pull out a `demo_client_connection` struct for managing the state associated with a connection (socket fd, rustls_connection, conndata, closing stae, etc). * Rework existing logic around the new types. * Simplify the request handling to better match tls-client-mio.rs in the Rustls examples. Notably we _do not_ process the HTTP response, instead we just read whatever data we get and blast it to stdout. A new timeout on `select()` ensures that if the server doesn't close the connection after writing a response we will time out waiting for more data and do it ourselves. With the update to server.c to close the connection after writing a response this won't kick in, but is helpful for testing against servers that may let the conn linger even though we send `Connection: close`. * Care is taken to still treat unclean closure as an error condition. * Various other small improvements are made where possible.
There was no `write_all()` defined in `common.c`, so the `common.h` prototype wasn't worth keeping.
After the client rewrite some of the pieces in common are now server specific. Move these bits into `server.c`. For now I've put them into `server.c` in the order required for compilation pending a larger top-down order rewrite.
It's been a week now so I'm going to merge this with the one review since it's ultimately just test code. I'd like to move on to some other tasks that will create conflicts with this branch and keeping it up to date but unmerged for longer doesn't seem like the best use of time. Happy to iterate on any future feedback as needed. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was mainly intended to be a follow-up/fix for the issues noted in #485 (now tracked in #496) related to the client logic & chunked encoding, but I ended up rewriting
client.cfairly substantially.I started by making small incremental improvements but eventually gave up and bit off a more substantial rewrite of
client.cthat tries to pull out more structure with some new types. I think it better aligns with thetlsclient-mio.rsexample upstream but would be open to feedback. If folks aren't feeling this bigger rewrite for whatever reason I suspect I can drop that commit and implement just what's needed to resolve #496 - WDYT? I prefer where this landed, but I acknowledge it's a chonkier diff.I'm not 100% confident I've nailed the "clean close" semantics while trying to port the logic from
tlsclient-mio.rs- I would encourage reviews to focus their attention on that aspect if possible.