strophe / libstrophe

A simple, lightweight C library for writing XMPP clients
http://strophe.im/libstrophe
Other
401 stars 163 forks source link

Add API to register callback to set socket options #202

Closed nosnilmot closed 2 years ago

nosnilmot commented 2 years ago

The registered callback function will be called before connect() is called on the socket, allowing tweaking of much more than just keepalive settings.

This deprecates xmpp_conn_set_keepalive(), which will be ignored if it is called for a conn object that already has a sockopt callback set.

Includes helper/example callback function to set default keepalive parameters that some library users may find useful.

This comes from discussion on PR #199.

Examples have been modified to leverage new API:

sock.c is no longer completely standalone from the rest of strophe, as it needed to know about xmpp_conn_t and xmpp_sockopt_callback We could either:

nosnilmot commented 2 years ago

Do you prefer I squash commits & force push, or will you squash/rebase during merge?

sjaeckel commented 2 years ago

Do you prefer I squash commits & force push, or will you squash/rebase during merge?

Please always squash/fixup, rebase and force-push!

sjaeckel commented 2 years ago

Can you please check whether I missed something in 163e34d or if that's OK and still does everything we want and discussed?

You could also squash b798e90 into af22d87 if you want to.

nosnilmot commented 2 years ago

Can you please check whether I missed something in 163e34d or if that's OK and still does everything we want and discussed?

hmm, this introduces some subtle behaviour changes:

I think the existing behaviour is restored with 0001773a62d6b73ce896b904691c8a06803117bc.

* This function is only effective on a disconnected object.

That's a good point, and slight loss of functionality. Should we call the callback immediately for a connected object? or change xmpp_conn_set_sockopt_callback() to return the socket (or NULL if not connected) so the user can choose?

You could also squash b798e90 into af22d87 if you want to.

Will wait to hear if you want to keep 0001773a62d6b73ce896b904691c8a06803117bc or not before cleaning up / rewriting git history.

sjaeckel commented 2 years ago

I think the existing behaviour is restored with 0001773.

Thanks! Please keep it, squash stuff together and rebase.

* This function is only effective on a disconnected object.

That's a good point, and slight loss of functionality. Should we call the callback immediately for a connected object? or change xmpp_conn_set_sockopt_callback() to return the socket (or NULL if not connected) so the user can choose?

We're not sure whether libstrophe will only have a single socket per connection in the future (some comment from @pasis in the past), so returning the socket is no option. I don't see a reason why we shouldn't call the callback immediately for a connected object. IIRC some of the socket options can only be applied to disconnected sockets, if that's the case we should write down in our documentation that the user has to take care of that!

nosnilmot commented 2 years ago

I think the existing behaviour is restored with 0001773.

Thanks! Please keep it, squash stuff together and rebase.

done.

I don't see a reason why we shouldn't call the callback immediately for a connected object.

added.

IIRC some of the socket options can only be applied to disconnected sockets, if that's the case we should write down in our documentation that the user has to take care of that!

I'd like to think it was obvious that if you want to set options that can only be set on disconnnected sockets, then the callback should be registered before connection, but I have added a note to that effect anyway.