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

utils: add janus_strlcat helper #2792

Merged
merged 1 commit into from
Nov 3, 2021
Merged

Conversation

tmatth
Copy link
Contributor

@tmatth tmatth commented Oct 28, 2021

This will log an error if the output is truncated.

@tmatth tmatth force-pushed the strlcat-helper branch 3 times, most recently from dee572e to 1683d9f Compare October 28, 2021 15:29
@tmatth tmatth marked this pull request as draft October 28, 2021 15:30
@tmatth
Copy link
Contributor Author

tmatth commented Oct 28, 2021

N.B. this can take a backseat to #2793 (I'll rebase it after that gets merged) but I think would be useful for the rest of the codebase.

@lminiero
Copy link
Member

I think it makes sense to merge yours first (in order to check when truncation is indeed the cause of the issue), and then expand it with @JanFellner's patch to make it extend the buffer rather than fail (and maybe print the error you have now in case the reallocation fails, for instance).

@JanFellner
Copy link
Contributor

Works now on my side. So the buffer size was really the issue.

@tmatth tmatth marked this pull request as ready for review October 28, 2021 18:28
@tmatth
Copy link
Contributor Author

tmatth commented Oct 28, 2021

I think it makes sense to merge yours first (in order to check when truncation is indeed the cause of the issue), and then expand it with @JanFellner's patch to make it extend the buffer rather than fail (and maybe print the error you have now in case the reallocation fails, for instance).

Sure just wanted to make sure I wasn't scooping @JanFellner's work but this should be a lower impact/risk change.

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

Apart from a tiny code-style nit (I know, I'm the worst 😆 ), this looks like a good fence to me. I already know it will cause me more than a little headache when re-aligning multistream (it will likely cause many conflicts), but that's indeed needed. I'll make a couple of tests, then merge and address the multistream conflicts so that we can start addressing the SDP size there.

utils.c Outdated Show resolved Hide resolved
postprocessing/janus-pp-rec.c Show resolved Hide resolved
This will log an error if the output is truncated.
@lminiero
Copy link
Member

lminiero commented Nov 3, 2021

Thanks for the quick fix! Merging then, and heading down the multistream merge conflicts hole...

@lminiero lminiero merged commit 943558d into meetecho:master Nov 3, 2021
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.

None yet

3 participants