search

enarx-archive / sevctl

Administrative utility for AMD SEV
Apache License 2.0
16 stars 5 forks source link

Replace reqwest with barebones HTTP client #66

Closed connorkuehl closed 3 years ago

connorkuehl commented 3 years ago

We are underutilizing reqwests in comparison to the downstream packaging complexity it adds for when this crate must vendor its sources.

This also shrinks the dependency graph.

Signed-off-by: Connor Kuehl ckuehl@redhat.com

connorkuehl commented 3 years ago

This still smells of NIH despite httparse taking care of the parsing. The Response struct I wrote is (as far as I can tell from the httparse documentation) necessary for filling a usability gap in client code (i.e., fn download).

edit: to elaborate on the usability gap I alluded to:

  1. Interpreting httparse::Response still requires no small amount of effort from client code since it's non-allocating, it's not tidied up into a data structure that makes lookups convenient (hashmap vs array). Checking for the retry header would still be an array traversal, which would just clutter up the call site.
  2. Returning an httparse::Response would require a C-like function where the caller allocates a buffer and passes it in so that the lifetime of the httparse::Response can align with it. Even if we did this, point 1 still applies.
  3. One could encapsulate the httparse::Response struct with a struct of our own, just much uglier and nontrivial lifetime decorations on the struct and its impl.

Because of the above points, I decided to fill that gap with a Response abstraction that copies whatever data it needs from the httparse::Response and organizes it in a way that's convenient to use in src/main.rs.

npmccallum commented 3 years ago

@connorkuehl I'm not worried about the NIH smell. We need to keep dependencies small.