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

PRACK refactoring #630

Merged
merged 1 commit into from
Jan 23, 2023
Merged

Conversation

maximilianfridrich
Copy link
Contributor

@maximilianfridrich maximilianfridrich commented Jan 4, 2023

  • Add awaiting_prack bool to sipsess to be able to check whether the sipsess is still awaiting a PRACK to a 1xx response with SDP.
  • Refactor PRACK logic to remove ht_prack which was unused and unneeded.
  • Add logic to be conformant to RFC 3262.

Related:

@sreimers sreimers added this to the v2.12.0 milestone Jan 4, 2023
@maximilianfridrich maximilianfridrich force-pushed the prack_refactoring branch 2 times, most recently from 4a6da0c to fbc6b91 Compare January 16, 2023 08:32
@maximilianfridrich
Copy link
Contributor Author

I am quite confused as to why some checks are failing. I rebased and added an additional test to retest. Maybe an issue with the build agent? Could someone trigger a re-check?

int err;

if (!req || req->tmr.th)
return -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

better to return EINVAL here ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* @return true if session is waiting for a PRACK to a 1xx containing SDP,
* false otherwise
*/
bool sipsess_awaiting_prack(struct sipsess *sess)
Copy link
Contributor

Choose a reason for hiding this comment

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

can sess be const ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Done.

*
* @return True if a target refresh is currently allowed, otherwise false
*/
bool sipsess_refresh_allowed(struct sipsess *sess)
Copy link
Contributor

Choose a reason for hiding this comment

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

can sess be const ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Done.

@alfredh
Copy link
Contributor

alfredh commented Jan 20, 2023

/home/runner/work/re/re/current/src/sipsess/sess.c:334:6: error: conflicting types for ‘sipsess_awaiting_prack’; have ‘_Bool(const struct sipsess *)’

@maximilianfridrich
Copy link
Contributor Author

/home/runner/work/re/re/current/src/sipsess/sess.c:334:6: error: conflicting types for ‘sipsess_awaiting_prack’; have ‘_Bool(const struct sipsess *)’

Sorry, that was a bit careless. My time is somewhat limited currently, but my intention is to fix the retest issue we are seeing. Should we switch this PR and the related ones to draft for the time being? It could be a few days until I have time to look at it again.

Add awaiting_sdp_prack bool to sipsess to be able to check whether the
sipsess is still awaiting a PRACK to a 1xx response with SDP. Further,
refactor PRACK logic to remove ht_prack which was unused and unneeded.
@alfredh
Copy link
Contributor

alfredh commented Jan 23, 2023

looks good now. should we merge it to main?

@maximilianfridrich
Copy link
Contributor Author

I think @cspiel1 will look at it again later today, so maybe we can give him a little bit of time. The more reviewers the better.

@cspiel1 cspiel1 merged commit 416340c into baresip:main Jan 23, 2023
@maximilianfridrich maximilianfridrich deleted the prack_refactoring branch January 24, 2023 07:09
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.

4 participants