Hi,
thanks for the explanations of your implementatioin.
I was not aware of the optional spaces before/after the “;” and “=”.
(I thought I could have it with higher performance using a single osip_strcasecmp
instead of calling osip_strncasecmp, strstr and strchr.)
thus, your implementation for parsing is fine – ACK !
ACK also for your additional changes.
From my point of view we are ready for commit.
Br,
christoph
From: Aymeric Moizard [mailto:address@hidden]
Sent: Mittwoch, 28. Juni 2017 14:23
To: FEICHTER Christoph <Christoph.FEICHTER@frequentis.com >
Cc: address@hidden
Subject: Re: implicit subscription
inline!
2017-06-26 23:28 GMT+02:00 FEICHTER Christoph <Christoph.FEICHTER@
frequentis.com >:hi,
sorry – my fault.
everything fine !
your patch v4 partly works.
Nevertheless I have some improvements / hardening issues; attached as v5.
here are my explanations:
- jcallback.c, function cb_rcv2xx
for performance reasons I put the handling of NOTIFY and REFER in an else-if cascadeok!
- jcallback.c, function cb_rcv2xx
eXcall_api.c, function eXosip_call_send_answer
I re-worked the parsing a bit:o searching for the expires-parameter did not work, due to the different length of “active” and “pending”
o you forgot to take into account the “;” between “active”/”pending” and the “expires” parameter
o the length of “expires” is 7, not 6 chars
à I added const char’s for the strings to search for; so we can use strlen to get the right lengthPlease check again my version!!!
///CHECK 6 char for "active" or 7 char for "pending"
if (0 == osip_strncasecmp (sub_state->hvalue, "active", 6)
||0 == osip_strncasecmp (sub_state->hvalue, "pending", 7)) {
//In the next strstr call, we are searching STARTING FROM HVALUE+6 for "expires" occurence.
-> this means we start on the ";expires=180" if "active" was found.
-> this means we start of the g;expires=180" if "pending" was found.
const char *tmp = strstr(sub_state->hvalue+6, "expires");
-> in both case, the first occurence is found and tmp pointer is on first letter of "expires"
const char *ss_expires = NULL;
if (tmp!=NULL) {
//Here, we search for "=" starting 7 letter after "expires", so in both case, we are on "=" and ss_expires
//ends up being at tmp+7
ss_expires = strchr(tmp+7, '=');
-> conclusion, my code handles correctly both "active" and "pending".
-> my code handle correctly any additionnal space before or after ";" and any other before or after "=".
-> this is why I do prefer my code! see below for more info on SEMI, EQUAL...
o I did not like the double “if (ss_expires!=NULL)”
Accepted! ;)
o I use “;expires=” for searching for the expires-parameter
My code modification were made to accept any space (and LWS) inside the string! As I pointed above.
Looking at rfc3265, here is the definition of Subscription-State
Subscription-State = "Subscription-State" HCOLON substate-value*( SEMI subexp-params )substate-value = "active" / "pending" / "terminated"/ extension-substatesubexp-params = ("reason" EQUAL event-reason-value)/ ("expires" EQUAL delta-seconds)/ ("retry-after" EQUAL delta-seconds)/ generic-paramI have no much doubt that SEMI and EQUAL can contains space and other LWS...
o check “sub_state->hvalue != NULL” was missing
otherwise crash in case of empty “Subscription-State” headerGood point!
o check “refer_sub->hvalue != NULL” was missing
otherwise crash in case of empty “Refer-Sub” headerGood point!
- Although the expires parameter should be present in the Subscription-State header of the NOTIFY
I added to use the default (excontext->implicit_subscription_expires ) in this case.Right. My patch didn't fallback to default. I have modified my version to include that
jd->implicit_subscription_
expire_time = now + excontext->implicit_ subscription_expires;
I also moved the log to show when expires is not present. Also, my code also use default
value when expires is not showing a good value... (negative one or above 600.)
Additionnal change:
At the end of "eXosip_call_terminate_with_
reason", your code do not include my change: when a subscription exist, it will remove the dialog when sending a BYE.
Instead my code avoid to delete the dialog if implicit_subscription_expire_
time has a value.
This is required to send NOTIFY after sending a BYE.
Other minor udp.c change were accepted!
Here is the newer v6 patch...
I should commit it by tomorrow. However, if anything is wrong let me know!
Regards
Aymeric
br,
Christoph