dziegel: thanks for merging the fix. Have you yet had time to review the
patch? Would you consider this feature appropriate for merging to upstream
httpd? If there is anything I could do to help this get merged, I'm willing to
do so. :)
I really don't know if this fits. Anyway, a few generic comments:
- don't add third party code without a license that fits. I can't even see a
license at all for base64 and sha1
- when using sha1, use it like ppp does (polarssl/mbedtls)
- base64 encoding is used in smtp, too. We need to combine this instead of
adding it a second time. Again, watch out for licensing issues here!
As to the actual web socket implementation: I won't add it like this to
httpd.c. That file is too big and unreadable like it is now, adding yet more
code would be bad. You might try to add a httpd_websockets.c file and come off
with minimal changes to integrate this into httpd.c
However, I agree with your point about avoiding duplication of the SHA1 and
base64 functions. I had a look at the SMTP code, and the base64 function seems
easy to split out of that code. What is, in your opinion, the best place in
the code tree to locate the standalone base64 function?
About SHA1: I can change that to use PolarSSL/mbedTLS conditionally (I think
you're referring to pppcrypt.h). In their current state, the PolarSSL files
seem to be quite coupled to the PPP code, having includes of ppp_opts.h and
conditional compilation based on whether PPP support is selected. I think they
would have to be split out as well. What's your opinion about this? Where
should the files be relocated?
Finally, I completely agree that httpd.c is huge, and I'll do my best to
refactor the WebSocket support out of the main file. It will indeed be much
more maintainable that way.
With the mentioned changes in place, would you then more likely to consider
this for merging? Also, are there any other things I would have to consider?
It seems you're moving more and more towards pool-based memory allocation, so
it would seem preferable to account for this in the WebSocket implementation
(like I've done to some extent already in the patch).
> With the mentioned changes in place, would you then more likely
> to consider this for merging?
I don't know that yet. With the mentioned changes, I'd be willing to take a
look at it in depth. I'm not at all opposed to having web socket support in a
separate file. And if the diff to httpd.c is really minimal, I guess this
might go in, yes.
If that initial work is done, maybe we might be able to better separate the
SSI code as well...
The best thing I can come up with is to just expect mbedtls is present. They
provide everything we need and we don't need to implement things twice.
The code copy from PolarSSL was at a time where that library had moved to GPL
and the copy we have was the last BSD version. With mbedtls having a
compatible license, I don't thing it's good to stay with the code copy.
The downside is of course that we make non-secure things depend on mbedtls,
but I'm willing to take that for not having to maintain more code :-)
Should the websocket API not resemble the Raw TCP API? I mean stuff like:
ws_recv(ws_pcb *, ...)
And a way to detect closing of a socket, like calling the recv callback with
I used the proposed API some time ago, and if I remember correctly it had a
single, shared, receive callback for all open sockets. Also it gave no
indication that a socket was closed for inactivity or remote request, making
user's cleanup hard.
Unfortunately I have not had time to work on this - probably it would need some rethinking in order to better fit the lwip interfaces and be more useful.
If anyone would like to tackle this, feel free to do so. My contribution would probably have been ~splitting the proposed websocket code to a separate file and adding only minimal changes to httpd.c. However this isn't probably optimal.
ma 1. lokak. 2018 klo 16.16 Simon Goldschmidt <[hidden email]> kirjoitti: