Snom patch: Last_ keywords extension

[pbertera]

This patch add the support for the following new scenario keywords:

• Last_Contact_URI: returning the last Contact URI value
• Last_From_Tag: extracting the last from-tag
• Last_To_Tag: extracting the last to-tag

All the attribution for this patch should go to Snom Technology AG

[wdoekes]

This is not the way to move forward. If you look at the syntax suggested in #132, I would envision these to be called like:

[last_Contact.uri], [last_From.param.tag], [last_To.param.tag] or similar.

See https://www.opensips.org/Documentation/Script-Tran-2-2 for ideas.

I don't mind if only ".value", ".uri" and ".param.tag" are implemented, but at least it would provide a solid syntax to build more features on, instead of of adding E_Message* for every individually requested feature.

[pbertera]

Yes, you are right, I agree with this more general approach instead of adding more E_Message*.

Here: master...Snomio:last_extensions I'm working on a solution for this, the patch implements:

• last_XXX:value
• last_XXX:param.tag
• last_XXX:uri

So should cover this and #258, your feedback are appreciated,

Thanks,

[pbertera]

Could you look at the code ?
If is sufficiently clean / correct I'll work on the documentation.

Thanks

[wdoekes]

This could be done without so much nesting with negative tests instead:

if test $status -ne 0; then
    fail "process failure"
elif ! grep -q '^LAST From: "Tom Jones" <sip:tom\.jones@wales.uk>;tag=SIPpTag001$' log.log; then
    fail "From header not found"
...

[wdoekes]

Your "key names" don't correspond to the lookups:

Suggest:
LAST Contact|[last_Contact]
LAST Via:value|[last_Via:value]
etc..

[wdoekes]

This function is too big. The code in the if(strcmp..){...} blocks should be delegated to helper methods and duplicate code should be avoided.

For example:

        if (!sep) { /* [last_Header] */
            sprintf(with_colon, "%s:", name);
            return get_header(last_recv_msg, with_colon, false);
        } 

        /* [last_Header:...] */
        sprintf(with_colon, "%.*s", (sep - name + 1), name);
        if (!strcmp(sep, ":")) {
            return get_header(last_recv_msg, with_colon, false);
        }

        char *value = get_header(last_recv_msg, with_colon, true);
        if (!strcmp(sep, ":value")) {
            ; /* done */
        } else if (!strcmp(sep, ":uri")) {
            extract_header_uri(value); /* reuse mem in value (memmove?) */
        } else if (....) {
            ...
        } else {
            ERROR("Token %s not valid in %s", sep, name);
        }

        return value;

[wdoekes]

This should not be called get_last_request_uri but get_last_uri_from or something.

[wdoekes]

get_header() returns a static buffer (yuck, I know).

You can't start returning malloc'ed memory all of a sudden: memory leaks.

[wdoekes]

This should be implemented in sip_parser.cpp instead. See get_peer_tag.

[pbertera]

Thanks for all the hints, can you give me your feedback ?
Thanks a lot,

[wdoekes]

I'm sorry for being picky. But if we're changing the source, it should be for the better.

[wdoekes]

Please use single-quotes instead. And -q so you don't have to redirect to /dev/null. (And with -F you can do regular string comparisons without worrying about special regex tokens.)

grep -qF 'LAST Contact|Contact: <sip:sipp@127.0.0.1:5060>' log.log

[wdoekes]

This could also be:

get_param(msg, "tag", "To", "t");

That'd be even more generic.

[wdoekes]

Why not this?

dest += sprintf(dest, "%s", get_last_header("To:uri"));

And drop the entire get_last_header_uri() method.

The get_uri-code could be implemented in sip_parser.cpp instead as get_first_uri(const char *msg, const char *name, const char *shortname) or something, with the same semantics as the other get_* functions.

[pbertera]

Why you wrote get_first_uri ? maybe you are thinking about multiple URI in the header ?

[wdoekes]

memmove(value, last_tag, strlen(last_tag) + 1);

.. copies the NUL along.

[wdoekes]

Apparently you're not using value get_header(last_recv_msg, header_name, true); here, move it to the ; /* done */ instead.

It would be only useful here if we used functions that operate on that single header directly later on, e.g.:

• get_param(value, "tag"); or extract_param(value, "tag");
• get_first_uri(value); or extract_first_uri(value);

They could certainly be implemented and would clean up things some more.

[wdoekes]

If get_last_header_uri is dropped, we get rid of this unusual free() here (because of the different semantics of get_last_header_uri).

[pbertera]

@wdoekes no pickiness, unfortunately my programming skills are very weak. Your fine grained review is a warranty for the quality of SIPp, thanks a lot.
I hope the code now is an acceptable level of quality, if yes I'll write an example scenario and I'all add the note to the changelog.
Many thanks,

[wdoekes]

Sorry for the delay. I've been busy. I'll try to get to this in the coming week.

[wdoekes]

Note to self: this fails:

Testing github-#0152: failed (got 0 calls, and 0 failed)