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

Remove DNS handling from all C modules except net.dns. #3063

Closed
nwf opened this issue Apr 6, 2020 · 4 comments
Closed

Remove DNS handling from all C modules except net.dns. #3063

nwf opened this issue Apr 6, 2020 · 4 comments

Comments

@nwf
Copy link
Member

nwf commented Apr 6, 2020

This is actually an anti-feature request. Handling DNS in C is a pain and a source of object lifecycle bugs in our tree.

  • net.dns gets it right, though the documentation asserts that there is only one callback in flight at once, which appears false. I'll add that to my next stack of cleanups.

  • net's socket handling at least tries to get it right, using wait_dns to continue to pin the userdata in the registry until the DNS callbacks have fired. This is entangled in its state machine, which is pretty messy.

  • tls gets it wrong, at the time of this writing, possibly referencing freed memory if the user completely drops a tls socket while a DNS operation is in progress.

  • mqtt and websocket behave similarly, following pointers to possibly-dead memory.

Anyway, I'd like to simplify the C world by removing any pretense of doing DNS and require that the Lua application use net.dns whenever DNS resolution is requisite. Note that, for example, coap already does not support DNS, and even net's support is restricted to TCP and not UDP.

@pjsg
Copy link
Member

pjsg commented Apr 6, 2020

This raises an issue with sntp as the default invocation with (essentially) no arguments works well at the moment. It does need to resolve some dns names.

@nwf
Copy link
Member Author

nwf commented Apr 6, 2020

Good call. sntp.c dodges the lifecycle issue by having a static state object and not pinning anything to the Lua registry; as such, it's exempt from my ire in this anti-feature request. ;)
(And, FWIW, #2819 presently uses the net.udp dns function, but would be easy to make use net.dns instead.)

@marcelstoer
Copy link
Member

marcelstoer commented Apr 7, 2020

I'd like to simplify the C world by removing any pretense of doing DNS and require that the Lua application use net.dns whenever DNS resolution is requisite.

Purely speaking as a firmware user now: this is... no, please don't do this. No network library/framework I know of has an API that accepts IP addresses only. It would be a grand step backwards.

Handling DNS in C is a pain and a source of object lifecycle bugs in our tree.

Can't this be extracted into a shared piece of code that modules can include? I agree, it shouldn't be the module's responsibility to address cross-cutting concerns.

@nwf
Copy link
Member Author

nwf commented Apr 7, 2020

You're right; this is an over-reaction on my part. Threading reference counts around tls was not terrible; doing so for everybody else is probably a matter of a few days' half-hearted work.

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

No branches or pull requests

3 participants