[SR-Users] A question about the code

[Daniel-Constantin Mierla]

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


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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-users/attachments/20160504/0b1b94b4/attachment.html>