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

Hardening/2724 mhd to use poll #2751

Merged
merged 9 commits into from
Dec 5, 2016
Merged

Conversation

kzangeli
Copy link
Member

@kzangeli kzangeli commented Dec 5, 2016

Made MHD use poll and not select.
Also, changed a little bit how serverMode was set - made it cleaner.

Fixes #2724.

@kzangeli kzangeli added this to the 1.7.0 milestone Dec 5, 2016
@@ -0,0 +1 @@
- Fix: libmicrohttpd now uses poll() and not select() (#2724)
Copy link
Member

@fgalan fgalan Dec 5, 2016

Choose a reason for hiding this comment

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

Maybe epoll and #2166 should be mentionces here. In addition, explain the "functional" nature of the change. Suggestion:

- Fix:  libmicrohttpd now uses poll()/epoll() and not select() so fds greater than 1024 can be used for incomming connections  (#2724, #2166)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good.
Fixed in 3614f3b

// while in MHD 0.9.51, the name of the define has changed to MHD_USE_EPOLL.
// So, to support both names, we need a ifdef/else cpp directive here.
//
#ifdef MHD_USE_EPOLL
Copy link
Member

Choose a reason for hiding this comment

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

Looks smart, as this will work if we some day upgrade MHD. However, it wouldn't be a problem also to "stick" to 0.9.48 now and change the code in the future :)

NTC

@@ -379,7 +369,7 @@ PaArgument paArgs[] =
{ "-subCacheIval", &subCacheInterval, "SUBCACHE_IVAL", PaInt, PaOpt, 60, 0, 3600, SUB_CACHE_IVAL_DESC },
{ "-noCache", &noCache, "NOCACHE", PaBool, PaOpt, false, false, true, NO_CACHE },
{ "-connectionMemory", &connectionMemory, "CONN_MEMORY", PaUInt, PaOpt, 64, 0, 1024, CONN_MEMORY_DESC },
{ "-maxConnections", &maxConnections, "MAX_CONN", PaUInt, PaOpt, FD_SETSIZE - 4, 0, FD_SETSIZE - 4, MAX_CONN_DESC },
{ "-maxConnections", &maxConnections, "MAX_CONN", PaUInt, PaOpt, 1020, 0, PaNL, MAX_CONN_DESC },
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a comment about that 1020 comes from old implementation based in select and it has been kept for legacy reasons could be useful (in the future, we could easily forget where this magic 1020 came from).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 39ca80e

@crbrox
Copy link
Member

crbrox commented Dec 5, 2016

LGTM

1 similar comment
@fgalan
Copy link
Member

fgalan commented Dec 5, 2016

LGTM

@fgalan fgalan merged commit a21724e into master Dec 5, 2016
@fgalan fgalan deleted the hardening/2724_mhd_to_use_poll branch December 5, 2016 11:09
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.

3 participants