-
Notifications
You must be signed in to change notification settings - Fork 83
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
sipreg: supports PNS custom contact URI #837
Conversation
Please review these linting (ccheck.py) errors:
|
Done |
test/sipreg.c
Outdated
contact_hdr = sip_msg_hdr(srv->sip_msgs[srv->n_register_req - 1], | ||
SIP_HDR_CONTACT); | ||
err = re_regex(contact_hdr->val.p, contact_hdr->val.l, | ||
";" CPARAMS ">;expires=", &contact_match); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really wanted to add some test. I have the full matching regex for both deprecated or not, took me a while to figure out it's an interesting version of regex.... So I'd just settle on this instead of having the deprecated version testing some other stuff while this one just some substr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess in that case I can just use strstr
, happy to change if anyone think it's important enough for a test case..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please complete the test! Then we'll see if we need more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please complete the test! Then we'll see if we need more.
The test already confirms if cparam is supplied it's added correctly, currently there's no way to add cparam through the deprecated interface. IMHO, this should be a good enough test case.
What I meant above is that, originally I wanted to define the test to be "validate the whole Contact URI string", thus one less branch dependent test, just supply different expression based on the flag. Particularly since it's now possible through the new sip message capturing.
However since the regex implementation doesn't support wildcard, after quite a lot of effort I don't think it's worth it.
What I meant above is just that I'm effectively using regex to do a substr test, bit of a waste, but then it's a test cases so..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. But then contact_match
is unused. There is no "character class" ([...]
) in your regex expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not used, just testing it's found. Just wanted to do better than tmp
.. perhaps better differs upon beholder, in this case it confuses unfortunately. Character class was what I meant by a lot of effort.. we're talking about correctly testing the new interface + verifying it doesn't break existing behavior, which is done here; versus, introducing a new and possibly better test.
I actually could've done quite a lot with [^;];[^;]
, but then they're variable based on actual test case callers so it's going to introduce limitation. If it were my code base it's a hard no that introduces more troubles and code than it solves, but i'm not the maintainer here so if you insist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial attempt extended regex, before discovering it's not standard regex impl, if you could suggest how to translate, happy to update:
err = re_regex(contact_hdr->val.p, contact_hdr->val.l, deprecated ?
"^<sip:[^>]+>;.*$" : "^<sip:([^;]+;)+some-param=test;other-param=123;>;.*$", ...)
TEST_ERR(err)
If we stick to just validating the actual new behavior, can swap out that contact_match
with a NULL, I thought the segfault I got was from using NULL. Or switch to a simple substring test.
Note that I've already gotten a sense of the level of test in the code base for similar intrusiveness then try to tune it up a step, and that resulted in the previous commit.
There is typo in the first commit message/PR title. |
|
Fixed.. apologies for the churn.. I'll spend some time to setup my editor.. |
Not sure how safe it is changing the interfaces, using it to show what's the change needed for baresip/baresip#2600
Both re and baresip do build and test passed after the change.