[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
URL:
  <http://savannah.nongnu.org/patch/?9525>

                 Summary: httpd: add Websocket support, fix lwip_strnicmp
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: flannelhead
            Submitted on: Thu 21 Dec 2017 09:12:19 PM UTC
                Category: apps
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None

    _______________________________________________________

Details:

This patch adds Websocket support to the httpd. The original work was done
based on older lwIP by lujji: https://github.com/lujji/esp-httpd

I integrated his work into the current upstream httpd code. Broadcast support
was also added in the process.

Also, this patch fixes the lwip_strnicmp implementation which I found to be
faulty when I needed to implement strnistr to be used in the Websocket
implementation.



    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Thu 21 Dec 2017 09:12:19 PM UTC  Name:
0001-httpd-integrate-Websocket-support.patch  Size: 34KiB   By: flannelhead

<http://savannah.nongnu.org/patch/download.php?file_id=42700>

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
Follow-up Comment #1, patch #9525 (project lwip):

Pushed the fix for strnicmp in a modified version without side-effect in
boolean expression

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
Follow-up Comment #2, patch #9525 (project lwip):

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. :)

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
Update of patch #9525 (project lwip):

             Assigned to:                    None => goldsimon              

    _______________________________________________________

Follow-up Comment #3:

Simon is the httpd guy :-)

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
Follow-up Comment #4, patch #9525 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
Follow-up Comment #5, patch #9525 (project lwip):

Simon, thank you for the feedback.

As for the SHA1 and base64 code, the origin is here:
https://github.com/B-Con/crypto-algorithms
They are released into the public domain and there's even no license file in
the origin repo.

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).

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
Follow-up Comment #6, patch #9525 (project lwip):

> 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...

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
Follow-up Comment #7, patch #9525 (project lwip):

Good to hear that. I'll follow up with a refactored patch and let's discuss it
further then. :)

Still, could you please give a suggestion where should I relocate the base64
and SHA1 functions decoupled from SMTP and PPP, respectively?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
Follow-up Comment #8, patch #9525 (project lwip):

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 :-)

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
Follow-up Comment #9, patch #9525 (project lwip):

Thanks, that seems like a fair assumption with good reasoning and indeed it
makes maintenance easier.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support, fix lwip_strnicmp

Simon Goldschmidt
Update of patch #9525 (project lwip):

             Assigned to:               goldsimon => None                  


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support

Simon Goldschmidt
Update of patch #9525 (project lwip):

                 Summary: httpd: add Websocket support, fix lwip_strnicmp =>
httpd: add Websocket support

    _______________________________________________________

Follow-up Comment #10:

Adjusted the summary as Dirk said he already pushed the strnicmp thing

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support

Simon Goldschmidt
Follow-up Comment #11, patch #9525 (project lwip):

Hello Sakari,

may I ask how the work is going with adding websockets? I would be interested
in the outcome.


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support

Simon Goldschmidt
Follow-up Comment #12, patch #9525 (project lwip):

Should the websocket API not resemble the Raw TCP API? I mean stuff like:

ws_recv(ws_pcb *, ...)
ws_close(...)
ws_sent(...)

And a way to detect closing of a socket, like calling the recv callback with
NULL pbuf.

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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9525] httpd: add Websocket support

Simon Goldschmidt
Follow-up Comment #13, patch #9525 (project lwip):

If you want something that resembles the raw tcp API, please create a new
altcp layer.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: [patch #9525] httpd: add Websocket support

Sakari Kapanen
Hi all,

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.

Best regards,
Sakari

ma 1. lokak. 2018 klo 16.16 Simon Goldschmidt <[hidden email]> kirjoitti:
Follow-up Comment #13, patch #9525 (project lwip):

If you want something that resembles the raw tcp API, please create a new
altcp layer.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9525>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel