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

implement kqueue support, add server header to error pages, fix openssl warning with 1.1.x #66

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

danmeuk
Copy link

@danmeuk danmeuk commented Sep 7, 2021

This pull request adds kqueue support to aprsc - tested on FreeBSD 13.0-RELEASE/amd64.
Currently testing/running on T2GB without any issues noticed.

73, 2E0NNX

Use version check to leave it in for older OpenSSL.
This wont include things handled by libevent internally (e.g. bad method).
It does include 404 errors etc however.
@danmeuk
Copy link
Author

danmeuk commented Sep 7, 2021

I also looked to fix bug #22 at least partly. 404 errors etc should now include a Server header, but method not allowed will not.

Very brief fix to ssl.c due to OPENSSL_config() being deprecated in OpenSSL 1.1.x so that it's only used on older versions.

…es if needed)

Error pages now return server header correctly.
@danmeuk danmeuk changed the title implement kqueue support implement kqueue support, add server header to error pages, fix openssl warning with 1.1.x Sep 7, 2021
Copy link
Owner

@hessu hessu left a comment

Choose a reason for hiding this comment

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

Hey, thank you! Looks good in principle. Just a few small items to fix.

Please leave src/configure out from the PR, it's a bit difficult to review - I will regenerate it here instead. Maybe it wouldn't really belong in the source repository after all. Thanks!

@@ -86,6 +95,11 @@ int xpoll_free(struct xpoll_t *xp)
#ifdef XP_USE_EPOLL
close(xp->epollfd);
xp->epollfd = -1;
#endif
#ifdef XP_USE_KQUEUE
if (xp->kq > 0)
Copy link
Owner

Choose a reason for hiding this comment

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

If this is a file descriptor, should probably compare with >= 0, as 0 is a valid file descriptor and -1 is the "nope we don't have it" value.

@@ -18,7 +18,7 @@
* Thanks!
*/

#define VERSION_BRANCH ""
#define VERSION_BRANCH "-2E0NNX-1"
Copy link
Owner

Choose a reason for hiding this comment

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

This branch name probably doesn't belong to the PR, it'd look weird in master. :)

@hessu
Copy link
Owner

hessu commented Oct 10, 2021

I just noticed this PR contains multiple different unrelated changes; it would be easier to review and consider separate independent and non-related changes if they were separate PRs. It should be easy to make new branches from master (git checkout -b feature-a master), and then "git cherry-pick commit-id-a" to pick the changes individually to separate branches.

Also, I would prefer to not merge the revert commit. If you wish to revert some changes and make a new commit with fixes after a PR has been submitted, you can do a "rebase -i master" to start an interactive rebase, and specify "s" (squash) for the revert & new commits to merge them to the initial commit being fixed.

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.

2 participants