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

fix srtp_remove_stream to use SSRC in host byte order #670

Merged
merged 3 commits into from
Jan 4, 2024

Conversation

pabuhler
Copy link
Member

This fixes the inconsistency in SSRC byte order usage in the public api. Now all usage should be in host byte order.

#220

This fixes the inconsistency in SSRC byte order usage in the public api. Now all usage should be in host byte order.

cisco#220
@pabuhler pabuhler requested a review from paulej December 27, 2023 19:56
@JonathanLennox
Copy link

See @jesup 's comment on #220 at #220 (comment): I don't think changing this while leaving the function name and signature the same is a good idea. Even at an API-breaking change like 3.0.0, this won't have any compiler warning to tell someone porting to the new API that they need to change something.

@pabuhler
Copy link
Member Author

@JonathanLennox fair point, so what about the new name, srtp_delete_stream() ?
Other options could be to change from add/remove to create/destroy ...
Just changing the name though will not ensure people take notice of the change in parameters.

Copy link
Contributor

@paulej paulej left a comment

Choose a reason for hiding this comment

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

I agree with changes like this for consistency. The app developer should never have to call htons() or htonl() to use the libsrtp APIs, IMO. I also agree with Jonathan's comment from Randell that we shouldn't make a change that might blindside developers. Perhaps just rename the API slightly? (We could keep both the old and a new one, but I think that's less desirable. Upgrading SRTP and having a clean interface should not be hard, especially if these kinds of breaking changes are documented in one obvious place.)

@paulej
Copy link
Contributor

paulej commented Dec 27, 2023

Maybe srtp_stream_add and srtp_stream_remove? Just changing the order, but I prefer to have the noun before the action in APIs. APIs are mixed in this regard, too.

@pabuhler
Copy link
Member Author

Maybe srtp_stream_add and srtp_stream_remove? Just changing the order, but I prefer to have the noun before the action in APIs. APIs are mixed in this regard, too.

I think I like that, how about, I rename to srtp_stream_remove / add / update and add a note to the documentation of srtp_stream_remove about the change in byte order. It will also end up in the CHANGES file.

Can make a new task to ensure all API's are reviewed for naming consistency.

@paulej
Copy link
Contributor

paulej commented Dec 28, 2023

I think I like that, how about, I rename to srtp_stream_remove / add / update and add a note to the documentation of srtp_stream_remove about the change in byte order. It will also end up in the CHANGES file.

Can make a new task to ensure all API's are reviewed for naming consistency.

Sounds good to me.

This is both part of cisco#672 as well as trying to ensure that
the parameter change for remove stream is noticed.
@pabuhler pabuhler requested a review from paulej January 2, 2024 07:55
@pabuhler pabuhler merged commit c823a57 into cisco:main Jan 4, 2024
33 checks passed
@pabuhler pabuhler deleted the fix-byte-order-remove-stream branch January 4, 2024 07:04
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