Hello,

Alex is right, but the new code example doesn't do exactly like the old one ...

On 04/05/16 06:19, Alex Balashov wrote:

Nathan,

I think this is actually deliberate. The return value of an assignment operation is always going to be true, so it's just an "overly clever" way of compactly setting the value of 'ret' to -2 within the same conditional evaluation.

The aim there is compactness, though it does obscure readability, and should probably be refactored into:

      if(ptr && !(VALID_CONTACT(ptr,act_time)
         && allowed_method(_m,ptr))) {
          ret = -2;
          goto done;
      }

Sometimes programmers like to do things like this because variety is the spice of life, but certainly, they are likely to elicit bewilderment in others. :-)

Indeed the condition does what is expected -- just to clarify better for those that want to understand more, it can be also written like:

if( (ptr) && ( !VALID_CONTACT(ptr,act_time) || !(ret=-2) || !allowed_method(_m,ptr)))

It is also important to know that in C the evaluation of an expression stops when its final result cannot change anymore.

The evaluation is:

- if not a valid contact then the ret=-2 and allow_method() are not executed anymore, because the condition is already true. The ret was initialized to -1, so the function returns -1

- if valid contact, ret=-2 is always executed, but now with ! (negation) is always false, so allow_method() is also executed and goto done happens if the method is not accepted by the target, in this case function returns -2

- if the method is allowed, then the ret is -2, but few lines later is set to 1.

An alternative of the IF block will be:

if(!(VALID_CONTACT(ptr,act_time)) goto done; if(!allowed_method(_m,ptr))) { ret = -2; goto done; }

Anyhow, looking at the code I spotted a minor issue -- ptr should not be checked anymore, because the line before the IF is accessing it. So if it is NULL, then it will crash at the line before. However, the function setting the ptr was successful, so ptr should not be null there - the all code:

res = ul.get_urecord_by_ruid(_d, ahash, &inst, &r, &ptr); if(res<0) { LM_DBG("temp gruu '%.*s' not found in usrloc\n", aor.len, ZSW(aor.s)); return -1; }

aor = *ptr->aor; /* test if un-expired and suported contact */ if( (ptr) && !(VALID_CONTACT(ptr,act_time) && (ret=-2) && allowed_method(_m,ptr)))

This is the part for temporary gruu lookup, my guess it's not much used out there. I will refactor to handle better the ptr and show more clear the conditions.

Cheers, Daniel

On 04/05/16 08:18, Alex Balashov wrote:

On 05/04/2016 02:09 AM, Daniel-Constantin Mierla wrote:

...

the new code example doesn't do exactly like the old one

D'oh! You're right. I was careless in my analysis of that statement, and didn't see that the negation was on the outside.

It sounds like the GRUU-related commit that introduced this was

https://github.com/kamailio/kamailio/commit/fd9fe6e683ca3c446daa043035072a37...

I don't know who is to blame? ;-)

:-) -- knowing that I tend to use clear statements, looking at the patch the expression was more or less inherited from previous version, respectively the patch removes:

- while ( (ptr) && - !(VALID_CONTACT(ptr,act_time) && (ret=-2) && allowed_method(_m,ptr))) - ptr = ptr->next; - if (ptr==0) {

Cheers, Daniel