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

Http Content Range #83528

Merged

Conversation

td-pradecki
Copy link
Contributor

Content-Range parsing has been added to the http subsystem.
PR consist of changes in:

  • http_parser: Added parsing Content-Range header. Parsing is done by writing
    the output to the output structure called "http_content_range".
    Structure contains fields: start, end, total. Unit automatically translates to bytes.

  • http_client: If header field matches Content-Range scheme, fields are returned via
    http_content_range struct. cr_present flag indicates that Content-Range is present.

Changes are much needed because currently Zephyr doesn't support Content-Range header, which
is extensively used approach in http-based download apps.

Copy link

github-actions bot commented Jan 3, 2025

Hello @td-pradecki, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@@ -218,11 +218,16 @@ static int on_header_field(struct http_parser *parser, const char *at,
struct http_request,
internal.parser);
const char *content_len = "Content-Length";
uint16_t len;
const char *content_range = "Content-Range";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could change this to
const char content_range[]="Content-Range";
This way we avoid allocating a pointer and can use sizeof instead of strlen
Same change could be done for content-lenght.
Perhaps it could be even static.

@mrodgers-witekio
Copy link
Collaborator

FYI as an alternative approach it should be possible to handle the Content-Range (or any other header) at an application level by passing your own callbacks in the http_cb parameter of the struct http_request.

The approach in this PR also works, but hard-coding the response headers to be processed in the HTTP parser state machine might not be the most scalable approach if we want to handle more headers in future.

E.g. a quick example of what I mean by modifying the HTTP client sample app:

diff --git a/samples/net/sockets/http_client/src/main.c b/samples/net/sockets/http_client/src/main.c
index 5c89fb42b27..f3eee3aa74a 100644
--- a/samples/net/sockets/http_client/src/main.c
+++ b/samples/net/sockets/http_client/src/main.c
@@ -149,6 +149,23 @@ static int connect_socket(sa_family_t family, const char *server, int port,
        return ret;
 }
 
+int cb_header_field(struct http_parser *parser, const char *at, size_t length)
+{
+       LOG_INF("Header field: '%.*s'", length, at);
+       return 0;
+}
+
+int cb_header_value(struct http_parser *parser, const char *at, size_t length)
+{
+       LOG_INF("Header value: '%.*s'", length, at);
+       return 0;
+}
+
+static const struct http_parser_settings http_parser_cbs = {
+       .on_header_field = cb_header_field,
+       .on_header_value = cb_header_value,
+};
+
 static int run_queries(void)
 {
        struct sockaddr_in6 addr6;
@@ -201,6 +218,7 @@ static int run_queries(void)
                req.response = response_cb;
                req.recv_buf = recv_buf_ipv4;
                req.recv_buf_len = sizeof(recv_buf_ipv4);
+               req.http_cb = &http_parser_cbs;
 
                ret = http_client_req(sock4, &req, timeout, "IPv4 GET");

@td-pradecki td-pradecki force-pushed the http_content_range branch 2 times, most recently from d1bea07 to e7cbdd6 Compare January 7, 2025 09:47
@td-pradecki td-pradecki requested a review from jukkar January 7, 2025 14:26
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good just some nits.

case h_content_range_start:
if (parser->content_range_present) {
SET_ERRNO
(HPE_UNEXPECTED_CONTENT_RANGE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line break seems unnecessary?

Comment on lines 223 to 224
uint16_t content_len_len = sizeof(content_len)-1;
uint16_t content_range_len = sizeof(content_range)-1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird that checkpatch doesn't complain about this:

Suggested change
uint16_t content_len_len = sizeof(content_len)-1;
uint16_t content_range_len = sizeof(content_range)-1;
uint16_t content_len_len = sizeof(content_len) - 1;
uint16_t content_range_len = sizeof(content_range) - 1;

Content-Range hasn't been supported in zephyr. This change adds
Content-Range header parsing to http_parser module. Range start,
range end, and total size are supported. All units are currently
interpreted as bytes.
This is much needed change, because many applications responsible
for http data download are based on Content-Range approach.

Signed-off-by: Piotr Radecki <[email protected]>
Three tests has been added for Content-Range functionality in http
parser:
- test_content_range_supplied: Checks if parser handles range correctly.
- test_content_range_asterisk_total: Checks if parser interprets
astersk as no total size supplied.
- test_double_content_range_error: Checks if parser rejects header with
repeated Content-Range field.

Signed-off-by: Piotr Radecki <[email protected]>
jukkar
jukkar previously approved these changes Jan 15, 2025
@@ -160,6 +167,8 @@ struct http_parser {
uint64_t content_length; /* # bytes in body (0 if no Content-Length
* header)
*/
uint8_t content_range_present;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be a bool to make it clear the meaning of the field.

@jukkar jukkar dismissed their stale review January 15, 2025 11:45

Gave +1 too early

@td-pradecki td-pradecki requested a review from rlubos January 16, 2025 12:27
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good provided the remaining comment is addressed.

Content-Range functionality added in recent commits has been propagated
to http_client module. If "Content-Range" string is detected on header
field, Content-Range are returned via http_response structure.

Signed-off-by: Piotr Radecki <[email protected]>
@kartben kartben merged commit e2ddac3 into zephyrproject-rtos:main Jan 16, 2025
25 checks passed
Copy link

Hi @td-pradecki!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: HTTP HTTP client/server support area: Networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants