Skip to content

include: net: http: rework http headers #51141

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 12, 2022
Merged

include: net: http: rework http headers #51141

merged 4 commits into from
Oct 12, 2022

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Oct 10, 2022 •
edited
Loading

Some housekeeping to make #46758 slightly easier

  • move enum http_method to its own header under http/
  • create scripts/net/enumerate_http_status.py
  • add enum http_status in a separate header under http/
  • move remaining http headers to http/ directory

@cfriedt cfriedt requested review from gmarull and jukkar October 10, 2022 17:54
@cfriedt cfriedt self-assigned this Oct 10, 2022
@cfriedt cfriedt force-pushed the rework-net-http-headers branch 2 times, most recently from 13439c2 to 201a6eb Compare October 10, 2022 20:27
@jukkar
Copy link
Member

jukkar commented Oct 11, 2022

Now there are http include files in two directories. Why not put all the http related files in to same place, either to include/zephyr/net or include/zephyr/net/http?

@cfriedt
Copy link
Member Author

cfriedt commented Oct 11, 2022

Now there are http include files in two directories. Why not put all the http related files in to same place, either to include/zephyr/net or include/zephyr/net/http?

I was aiming to do it gradually - could move everything to http/ though, if that makes sense.

@cfriedt
Copy link
Member Author

cfriedt commented Oct 11, 2022

@stephanosio - looks like the license issue may be a false positive. MIT is permissive, so should be compatible with Apache, unless I'm mistaken.

@stephanosio
Copy link
Member

@stephanosio - looks like the license issue may be a false positive. MIT is permissive, so should be compatible with Apache, unless I'm mistaken.

It is not a false positive. Integrating any non-Apache 2.0 code requires TSC approval; if approved, the check result can be overridden.

https://docs.zephyrproject.org/latest/contribute/external.html#submission-and-review-process

@jukkar
Copy link
Member

jukkar commented Oct 11, 2022

It is not a false positive. Integrating any non-Apache 2.0 code requires TSC approval; if approved, the check result can be overridden.

The code already exists in Zephyr, @cfriedt just moved it to another header and kept the license. I just wonder if we could just mark the license as Apache in the new header, after all it is just some enums we are copying here.

@cfriedt
Copy link
Member Author

cfriedt commented Oct 11, 2022

It is not a false positive. Integrating any non-Apache 2.0 code requires TSC approval; if approved, the check result can be overridden.

The code already exists in Zephyr, @cfriedt just moved it to another header and kept the license. I just wonder if we could just mark the license as Apache in the new header, after all it is just some enums we are copying here.

I'm inclined to go with @jukkar 's suggestion. For what equates to effectively constants, I think it's safe to relicense.

@cfriedt cfriedt mentioned this pull request Oct 11, 2022
Closed
2 tasks
Christopher Friedt added 3 commits October 11, 2022 20:41
include: net: http: move http methods to separate header
627c21a 
Previously, HTTP method enumerations were only defined within
the `http_parser.h`, which may not be ideal for all use cases.

This commit moves the `enum http_method` definition to a
dedicated header in a dedicated `http` subdirectory.

Signed-off-by: Christopher Friedt <[email protected]>
scripts: net: add enumerate_http_status.py
bb1728c 
Add a script to extract HTTP status values and format them
in a way that is both human readable and machine parseable.

Each line of output is of the form:

```
HTTP_{key}_{upper_val} = {key}, /**< val */
```

Signed-off-by: Christopher Friedt <[email protected]>
include: net: http: define http response status codes
a794796 
Provide a common header enumerating HTTP response status codes.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt cfriedt force-pushed the rework-net-http-headers branch from 7c63582 to 7a4d438 Compare October 12, 2022 00:41
jukkar
jukkar reviewed Oct 12, 2022
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Looks reasonable. For the latest commit include: net: http: rename http_x.h http/x.h, it would be great to see the motivation/reasoning why this change in the commit message. Now the commit msg only states the obvious which can be seen also in the actual change.

include: net: http: rename http_x.h http/x.h
449e00f 
Some minor housekeeping prior to adding an http server
implementation. There are already a number of http headers
and that number will likely increase with subsequent work.
Moving them into a common directory cleans up the
`include/net` directory a bit.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt cfriedt force-pushed the rework-net-http-headers branch from 7a4d438 to 449e00f Compare October 12, 2022 11:13
@cfriedt cfriedt requested a review from jukkar October 12, 2022 12:22
@cfriedt cfriedt changed the title [WIP] rework net http headers include: net: http: rework http headers Oct 12, 2022
@cfriedt cfriedt marked this pull request as ready for review October 12, 2022 12:24
@zephyrbot zephyrbot added the area: Sockets Networking sockets label Oct 12, 2022
@cfriedt cfriedt merged commit dbe2c0d into zephyrproject-rtos:main Oct 12, 2022
@cfriedt cfriedt deleted the rework-net-http-headers branch October 12, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jukkar jukkar jukkar approved these changes

@ycsin ycsin ycsin approved these changes

@gmarull gmarull Awaiting requested review from gmarull

@rlubos rlubos Awaiting requested review from rlubos

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@pfalcon pfalcon Awaiting requested review from pfalcon

@Navin-Sankar Navin-Sankar Awaiting requested review from Navin-Sankar

@nandojve nandojve Awaiting requested review from nandojve

@mengxianglinx mengxianglinx Awaiting requested review from mengxianglinx

@SeppoTakalo SeppoTakalo Awaiting requested review from SeppoTakalo

Assignees

@cfriedt cfriedt

Labels

area: hawkBit area: LWM2M area: Networking area: Sockets Networking sockets

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cfriedt @jukkar @stephanosio @ycsin @zephyrbot