Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rework example C test code #497

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

rework example C test code #497

wants to merge 23 commits into from

Conversation

cpu
Copy link
Member

@cpu cpu commented Dec 9, 2024

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.c fairly substantially.

I started by making small incremental improvements but eventually gave up and bit off a more substantial rewrite of client.c that tries to pull out more structure with some new types. I think it better aligns with the tlsclient-mio.rs example 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.

@cpu cpu self-assigned this Dec 9, 2024
tests/client.c Outdated Show resolved Hide resolved
tests/client.c Outdated Show resolved Hide resolved
tests/common.c Outdated Show resolved Hide resolved
tests/client.c Outdated Show resolved Hide resolved
tests/server.c Outdated Show resolved Hide resolved
@cpu
Copy link
Member Author

cpu commented Dec 9, 2024

added 3 commits 6 minutes ago

Just removing code that's unused, and moving code unchanged from common.c -> server.c where appropriate.

@cpu
Copy link
Member Author

cpu commented Dec 12, 2024

@jsha is this a branch you're interested in reviewing? It's only test code but also a fairly significant update.

@cpu
Copy link
Member Author

cpu commented Dec 12, 2024

cpu force-pushed the cpu-c-tidy branch from dffcfdc to b94cb0c

rebased & integrated the ECH bits from #485 into the client.c rewrite commit.

@cpu
Copy link
Member Author

cpu commented Dec 16, 2024

Added a commit on the end that does the equivalent of #504 but for the updated client.c code. If we merge this first I'll close #504. If we merge #504 first, this branch is ready afterwards.

cpu added 21 commits December 16, 2024 12:08
* 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.
cpu added 2 commits December 16, 2024 12:13
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.
@cpu
Copy link
Member Author

cpu commented Dec 16, 2024

cpu force-pushed the cpu-c-tidy branch from e760777 to d3c8797

#504 landed first so I rebased here and folded in the ech updates to the client.c rewrite commit.

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.

Do less HTTP in test client.c/server.c examples
2 participants