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

Return status code instead of bool in IParser #737

Closed
gavv opened this issue Jun 23, 2024 · 3 comments
Closed

Return status code instead of bool in IParser #737

gavv opened this issue Jun 23, 2024 · 3 comments
Assignees
Labels
easy hacks The solution is expected to be straightforward even if you are new to the project help wanted An important and awaited task but we have no human resources for it yet refactoring
Milestone

Comments

@gavv
Copy link
Member

gavv commented Jun 23, 2024

Summary

packet::IParser is interface for protocol-specific deserializators. See documentation.

Currently its methods return bool (true on success or false on error). We need to replace bool with status::StatusCode and return code that described why the operation failed.

Implementation

  • Update IParser interface and implementations:
    • rtp::Parser
    • fec::Parser
    • rtcp::Parser
  • Report appropriate statuses:
    • when allocation failed, return StatusNoMem
    • when buffer is too small, return StatusBadBuffer
    • when packet is malformed, return StatusBadPacket
    • on success, return StatusOK
  • Update users of parsers and composers. They should forward status to upper level:
    • fec::BlockReader
    • rtcp::Communicator

Testing

  • Add unit tests for rtp::Parser and fec::Parser that check returned statuses in case of errors.
  • Add unit tests for components which use parsers, and check that they forward statuses from parser to the upper level (e.g. if Parser fails with StatusNoMem, BlockReader::read() returns StatusNoMem). One test per component should be enough.
@gavv gavv added refactoring help wanted An important and awaited task but we have no human resources for it yet labels Jun 23, 2024
@gavv gavv added this to Roc Toolkit Jul 6, 2024
@gavv gavv moved this to Help wanted in Roc Toolkit Jul 6, 2024
@gavv gavv added the much-needed This issue is needed most among other help-wanted issues label Jul 13, 2024
@gavv gavv changed the title Return status code instead of bool in IParser & IComposer Return status code instead of bool in IParser Jul 26, 2024
@gavv gavv added easy hacks The solution is expected to be straightforward even if you are new to the project and removed much-needed This issue is needed most among other help-wanted issues labels Jul 26, 2024
@sprinkuls
Copy link

hey, is it alright if i take this up?

@gavv
Copy link
Member Author

gavv commented Oct 2, 2024

@sprinkuls Sure, thanks!

@gavv gavv added this to the next milestone Jan 16, 2025
gavv pushed a commit to sprinkuls/roc-toolkit that referenced this issue Jan 16, 2025
gavv added a commit to gavv/roc-toolkit that referenced this issue Jan 16, 2025
- Add ROC_ATTR_NODISCARD
- Forward status in fec::BlockReader
- Logging
- Use StatusBadPacket instead of StatusBadBuffer
- Use LONGS_EQUAL
@gavv
Copy link
Member Author

gavv commented Jan 16, 2025

Implementation merged, for tests I've created a separate issue: #792

@gavv gavv closed this as completed Jan 16, 2025
@github-project-automation github-project-automation bot moved this from Help wanted to Done in Roc Toolkit Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy hacks The solution is expected to be straightforward even if you are new to the project help wanted An important and awaited task but we have no human resources for it yet refactoring
Projects
Status: Done
Development

No branches or pull requests

2 participants