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

-- 
Daniel-Constantin Mierla
http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio World Conference, Berlin, May 18-20, 2016 - http://www.kamailioworld.com