[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [osip-dev] implicit subscription

From: FEICHTER Christoph
Subject: Re: [osip-dev] implicit subscription
Date: Wed, 28 Jun 2017 12:56:49 +0000



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.






From: Aymeric Moizard [mailto:address@hidden
Sent: Mittwoch, 28. Juni 2017 14:23
To: FEICHTER Christoph <address@hidden>
Cc: address@hidden
Subject: Re: implicit subscription




2017-06-26 23:28 GMT+02:00 FEICHTER Christoph <address@hidden>:



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 cascade


-          jcallback.c, function cb_rcv2xx
eXcall_api.c, function
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 length

Please 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-substate
subexp-params        =   ("reason" EQUAL event-reason-value)
                          / ("expires" EQUAL delta-seconds)
                          / ("retry-after" EQUAL delta-seconds)
                          / generic-param

I 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” header

Good point! 

o   check “refer_sub->hvalue != NULL” was missing
otherwise crash in case of empty “Refer-Sub” header

Good 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!










reply via email to

[Prev in Thread] Current Thread [Next in Thread]