Skip to content

Conversation

@jukkar
Copy link
Member

@jukkar jukkar commented May 10, 2017

This PR moves most of the http-server application code into a dedicated library. The library can be used to create HTTP server instances. The library supports both HTTP and HTTPS connections, and both IPv4 and IPv6 can be used at the same time. The PR modifies http-server to use this new API.

@jukkar jukkar requested review from nashif, pfalcon and tbursztyka May 10, 2017 12:19
@jukkar
Copy link
Member Author

jukkar commented May 10, 2017

Just noticed that I forgot to fix the README.rst file in http-server, will do that in next version.

@jukkar jukkar force-pushed the http-server branch 2 times, most recently from 6edabc0 to 67d6590 Compare May 10, 2017 13:29
dbkinder
dbkinder previously approved these changes May 10, 2017
Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

+1 for doc changes

Copy link
Member

Choose a reason for hiding this comment

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

Don't we have a k_panic() now?

Copy link
Member

Choose a reason for hiding this comment

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

snprintf() returns an integer, and you're accumulating its return value on an unsigned short. In the minimal libc, it can return EOF, which is defined to -1; I don't know about newlib. It's better to store the return value on an integer, check for overflow (if (r < 0 || r >= size_param)), and only then accumulate to the offset variable -- making sure it's not going to go past MAX_UINT16.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, good point.

Copy link
Member

@lpereira lpereira May 10, 2017

Choose a reason for hiding this comment

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

Doing a linear search here means that the order which URLs were added to the urls array matters when matching. You might want to consider either using a prefix tree (which is costly WRT memory), or, instead of returning the first hit, storing the last match/prefix size, and only changing that if the last stored size is larger than the current one. Alternatively you can sort them by prefix length as well and compare the larger prefixes first; just have to figure out when to sort (maybe every time a new route is added).

This way, if you had something like "/sensors" to list all the sensors in a board, and "/sensors/temperature" to get the temperature sensor, you would never match "/sensors" if asked for a specific one.

Copy link
Contributor

Choose a reason for hiding this comment

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

instead of returning the first hit, storing the last match/prefix size, and only changing that if the last stored size is larger than the current one.

I'd say this is the practical solution for embedded system like Zephyr.

Copy link
Member Author

Choose a reason for hiding this comment

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

This functionality comes from existing http-server application and in this in this first conversion commit (from app to lib) I would rather postpone change a bit later.

Copy link
Contributor

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

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

So, as I found in my previous testing, this server is not compliant and doesn't allow to send HTTP headers in all cases. I imagine it expects headers to be the part of the same packet as the initial request string. Just try telnet on this server.

It also not compliant with HTTP/1.0, force-uses pipelined protocol even with it. I'd check whether Connection: close header is handled properly with HTTP/1.1 but again, I can't enter HTTP headers.

Overall, this patch is the step in the right direction, and given that it's scope is to untangle server lib from samples, I give +1, but it has a long way to go beyond that...

@jukkar
Copy link
Member Author

jukkar commented May 12, 2017

Overall, this patch is the step in the right direction, and given that it's scope is to untangle server lib from samples, I give +1, but it has a long way to go beyond that...

I fully agree, there is still much to do to get this spec compliant. The purpose of this patch was to separate the core http server functionality from sample app, no effort was made here to make fully compliant server and/or optimize the performance etc.

jukkar added 4 commits May 12, 2017 12:34
The net_sample_app_init() is now able to wait that both IPv4
and IPv6 addresses are setup before continuing.

Signed-off-by: Jukka Rissanen <[email protected]>
The CFLAGS and object path were set incorrectly for Bluetooth.

Signed-off-by: Jukka Rissanen <[email protected]>
This helper copies desired amount of data from network packet
buffer info a user provided linear buffer.

Signed-off-by: Jukka Rissanen <[email protected]>
When a new UDP or TCP connection handler is to be registered,
we need to check if identical handler has already been created.
If a duplicate is found, the registering call will return -EALREADY.

The earlier code did not check this but allowed two identical
handlers to be created. The latter handler was never called in
this case.

Signed-off-by: Jukka Rissanen <[email protected]>
@jukkar
Copy link
Member Author

jukkar commented May 12, 2017

New version with snprintf() fixes and replacing the k_thread_spawn() with k_thread_create(). I did not change the panic() function at this point, probably the panic() could be removed all together.

This commit creates a HTTP server library. So instead of creating
a complex HTTP server application for serving HTTP requests, the
developer can use the HTTP server API to create HTTP server
insteances. This commit also adds support for creating HTTPS servers.

Signed-off-by: Jukka Rissanen <[email protected]>
@nashif nashif merged commit a174d2e into zephyrproject-rtos:master May 13, 2017
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.

5 participants