wsServer icon indicating copy to clipboard operation
wsServer copied to clipboard

ability to specify binding host, vague ipv6 support

Open refutationalist opened this issue 2 years ago • 5 comments

I wanted to be able to bind to localhost where I use wsServer for reverse proxying purposes, which means it would also be helpful if I could see which client port it was connecting from. One of the localhost interfaces I bind to is ::1, so some changes were necessary to fit ipv6 into it.

I guess this modernizes things a bit?

The main API change is that ws_socket requires a host char*, though setting it to NULL returns it to the previous behavior. Also, the port is also a char* now.

refutationalist avatar May 07 '22 14:05 refutationalist

Hello!

  • a) I suspected that might be the case, but would need help on that as I've done exactly zero windows development. Probably should have mentioned that in the original PR, but it was pretty late in the day. Willing to do what I can.
  • b) That makes sense as service names don't help us very much, I just wasn't seeing any type conversions and decided to stick with that.
  • c) That strikes me as good. I appreciate the desire to keep the parameter list under control, as long as I can still control which interfaces I'm binding to.
  • d) I'm not primarily a developer, so I'm new at this PR deal. I'll make whatever changes would be easiest, which seems to be changing the email address on my end.

refutationalist avatar May 13 '22 03:05 refutationalist

Hello!

  • a) I suspected that might be the case, but would need help on that as I've done exactly zero windows development. Probably should have mentioned that in the original PR, but it was pretty late in the day. Willing to do what I can.

Looking at the Windows docs, the functions getaddrinfo() and getnameinfo() (from <netdb.h>) are already included in the Windows '<ws2tcpip.h>' header, which is already included in the wsServer.

The only header you need to add is <netdb.h> for builds in Unix-like environments, or rather for non-Windows environments. Something like this should work.

  • d) I'm not primarily a developer, so I'm new at this PR deal. I'll make whatever changes would be easiest, which seems to be changing the email address on my end.

No worries, I'm open to receiving contributions from anyone, and helping within my possibilities as well =).


Edit: You can change the authorship of your 4 commits by rebasing them and then force-pushing your branch, something like:

# Rebase
$ git rebase --onto HEAD~4 --exec \
    "git commit --amend --author='Sam Mulvey <right-email-here>' --no-edit" HEAD~4

# Force-push them
$ git push --force-with-lease

Theldus avatar May 13 '22 06:05 Theldus

Just a quick note:I haven't forgotten about this PR, summer is a crunch time for me and I haven't been able to spend a lot of time in front of the editor. I'll be back at it as soon as I can.

refutationalist avatar Jun 22 '22 07:06 refutationalist

Just a quick note:I haven't forgotten about this PR, summer is a crunch time for me and I haven't been able to spend a lot of time in front of the editor. I'll be back at it as soon as I can.

Hey @refutationalist,

No worries, If you do not mind, I can work on what we discussed earlier to get this PR merged, basically: fix the build for Windows, and the email address used. In fact I was hoping you'd give me an 'ok' to work on it, since it's quite simple.

The remaining I can address later, as they require major changes and I prefer to do them myself.

Theldus avatar Jun 23 '22 06:06 Theldus

On 6/22/22 23:38, Davidson Francis wrote:

If you do not mind, I can work on what we discussed earlier to get this PR merged, basically: fix the build for Windows, and the email address used. In fact I was hoping you'd give me an 'ok' to work on it, since it's quite simple.

The remaining I can address later, as they require major changes and I prefer to do them myself.

Absolutely no problem.  Thank you!

refutationalist avatar Jun 23 '22 07:06 refutationalist

I will wait this features too. I use this library and it work perfect (thanks for your work, @Theldus). But, I need to bind localhost too and this PR look promising. Has the work been stopped?

arhiv6 avatar Aug 02 '23 08:08 arhiv6

I will wait this features too. I use this library and it work perfect (thanks for your work, @Theldus). But, I need to bind localhost too and this PR look promising. Has the work been stopped?

Hi @arhiv6, thanks for the compliments, This project is still maintained, it's just hard to find the time to add new features, while bug fixes are (generally) simpler to fix right away.

I still intend to retake this PR and merge it, and looking at the date, it's been 1 year since my last interaction with it =/. I will look at this PR more fondly, I promise.

As for the localhost binding, in the current state, wsServer works on localhost without any problem, the difference is that at the moment it is not possible to limit the server to only localhost.

Theldus avatar Aug 02 '23 16:08 Theldus

Hi @refutationalist and @arhiv6, The new PR #82 finally reviews all the points I covered here and is finally merged. If you encounter any issues with the implementation, please let me know.

That said, I will close this PR and again, thank you very much for the contribution.

Theldus avatar Nov 28 '23 06:11 Theldus