Issue #1498: Initial framework for HTTP/1.x observer #1501
Conversation
src/CMakeLists.txt
Outdated
| observers/http1/http1_observer.c | ||
| observers/http1/decoder.c |
There was a problem hiding this comment.
Why is one file prefixed but not the other?
There was a problem hiding this comment.
Good call - I'll add the http1_ prefix to the decoder.c file
src/observers/http1/decoder.h
Outdated
| @@ -0,0 +1,141 @@ | |||
| #ifndef http1_observer_decoder_H | |||
There was a problem hiding this comment.
Isn't the convention to make these all caps?
There was a problem hiding this comment.
Regarding the location of the decoder code, where it is now under the observer does not enforce any kind of separation. Maybe that's a desired coupling? If it stays here, it's easy to see how it would grow mutual dependencies with the other observer code.
There was a problem hiding this comment.
Isn't the convention to make these all caps?
I don't believe the convention is all caps but I'll update the ifdef to be consistent with other existing header files.
There was a problem hiding this comment.
Regarding the location of the decoder code, where it is now under the observer does not enforce any kind of separation. Maybe that's a desired coupling? If it stays here, it's easy to see how it would grow mutual dependencies with the other observer code.
That's a good point - I'll follow up with a separate PR to re-organize the file layout to make the decoupling clearer. Stay tuned.
src/observers/http1/decoder.h
Outdated
| #define HTTP1_VERSION_1_1 "HTTP/1.1" | ||
| #define HTTP1_VERSION_1_0 "HTTP/1.0" | ||
|
|
||
| typedef struct h1_decode_config_t h1_decode_config_t; |
There was a problem hiding this comment.
I don't think we want two conventions for HTTP stuff: h1 and http1 (h2 and http2) with no principled distinction.
There was a problem hiding this comment.
Existing precedent (adaptors) use the full "http1_" and "http2_" prefix for API. I'll update this code to follow that convention.
src/observers/http1/decoder.h
Outdated
| // Invoked when the HTTP request/response messages have been completely encoded/decoded and the transaction is complete. | ||
| // The request_context will never be used by the decoder again on return from this call. | ||
| // | ||
| int (*transaction_complete)(h1_decode_connection_t *hconn, uintptr_t request_context); |
There was a problem hiding this comment.
Is "transaction" the conventional term here?
There was a problem hiding this comment.
ChatGPT says yes, transaction is the convention.
src/observers/http1/decoder.h
Outdated
| struct h1_decode_config_t { | ||
| // | ||
| // Decoder callbacks | ||
| // | ||
| // Callbacks invoked when parsing incoming raw connection data. These callbacks are invoked from the | ||
| // h1_decode_connection_rx_data() call. These callbacks should return 0 on success or non-zero on error. A non-zero | ||
| // return code is used as the return code from h1_decode_connection_rx_data() | ||
| // |
There was a problem hiding this comment.
The docs say this is decoder configuration, and it's in a decoder.h file, but it's called h1_decode_config (note no R).
There was a problem hiding this comment.
Good call - I'm going to rename this to qd_http1_decoder_config_t.
This patch just checkpoints the start of the decoder implementation. Does not resolve Issue skupperproject#1498.
This patch just checkpoints the start of the decoder implementation. Does not resolve Issue #1498.