Snom patch: Last_ keywords extension by pbertera · Pull Request #260 · SIPp/sipp

pbertera · 2016-10-01T15:44:54Z

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 · 2016-10-02T09:14:15Z

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 · 2016-10-04T00:20:09Z

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 · 2016-10-06T07:16:24Z

regress/github-#0260/run

+    fi
+else
+    fail "process failure"
+fi


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 · 2016-10-06T07:19:19Z

regress/github-#0260/uas.xml

+      <log message="LAST To: [last_To:value]"/>
+      <log message="LAST To:uri: [last_To:uri]"/>
+      <log message="LAST Contact:uri: [last_Contact:uri]"/>
+      <log message="LAST From-tag: [last_From:param.tag]"/>


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

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

wdoekes · 2016-10-06T07:34:18Z

src/call.cpp

+                return ret;
+            }
+            ERROR("Token %s not valid in %s", sep, name);
+        }


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 · 2016-10-06T07:36:01Z

include/call.hpp

    char * get_last_header(const char * name);
-    char * get_last_request_uri();
+    char * get_last_tag (const char *name);
+    char * get_last_request_uri(const char *name);


This should not be called get_last_request_uri but get_last_uri_from or something.

wdoekes · 2016-10-06T07:39:18Z

src/call.cpp

+                snprintf(with_colon, len - 2, "%s", name);
+                char * last_header_uri = get_last_request_uri(with_colon);
+                char * ret;
+                if (!(ret = (char *)malloc(strlen(last_header_uri + 1)))){


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

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

wdoekes · 2016-10-06T07:39:55Z

src/call.cpp


+/* Return the last tag from the header line. On any error returns the
+ * empty string.  The caller must free the result. */
+char * call::get_last_tag(const char *name)


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

…om-last-extensions

pbertera · 2016-10-07T18:09:37Z

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 · 2016-10-08T12:57:05Z

regress/github-#0260/run

+
+if test $status -ne 0; then
+    fail "process failure"
+elif ! grep -e ^LAST\ Contact\|Contact:\ \<sip:sipp@127.0.0.1:5060\> log.log > /dev/null; then


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 · 2016-10-08T13:01:40Z

src/sip_parser.cpp


+char * get_peer_tag(const char *msg)
+{
+    return get_param_tag(msg, "To", "t");


This could also be:

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

That'd be even more generic.

wdoekes · 2016-10-08T13:04:41Z

src/call.cpp

-            char * last_request_uri = get_last_request_uri();
+            char * last_request_uri = get_last_header_uri("To:");
            dest += sprintf(dest, "%s", last_request_uri);
            free(last_request_uri);


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.

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

wdoekes · 2016-10-08T13:13:19Z

src/call.cpp

+        char * last_tag = get_param_tag(last_recv_msg, header_name, "");
+        int last_tag_len = strlen(last_tag);
+        memmove(value, last_tag, last_tag_len);
+        value[last_tag_len] = '\0';


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

.. copies the NUL along.

wdoekes · 2016-10-08T13:15:39Z

src/call.cpp

    if (name[len - 1] == ':') {
        return get_header(last_recv_msg, name, false);
+    }
+    char *value = get_header(last_recv_msg, header_name, true);


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 · 2016-10-08T13:16:22Z

src/call.cpp

+        int uri_len = strlen(uri);
+        memmove(value, uri, uri_len);
+        value[uri_len] = '\0';
+        free(uri);


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 · 2016-10-20T22:28:55Z

@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 · 2016-11-06T12:23:59Z

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

wdoekes · 2016-12-21T08:37:29Z

Note to self: this fails:

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

pbertera added a commit to Snomio/sipp that referenced this pull request Oct 1, 2016

FIX: Fixed SIPp#260 testcase

6cc918c

pbertera added a commit to Snomio/sipp that referenced this pull request Oct 1, 2016

MOD: Disabled github-SIPp#260 testcase, seems broken in Travis CI

18697fe

wdoekes added question request-feedback and removed question labels Oct 2, 2016

wdoekes mentioned this pull request Oct 4, 2016

Snom patch: header value syntax #258

Closed

pbertera force-pushed the snom-last-extensions branch from 18697fe to 3a9f928 Compare October 5, 2016 10:30

Support for last_XX:value, last_XX:uri, last_XX:param.tag

060d7ca

pbertera force-pushed the snom-last-extensions branch from 324cf71 to 060d7ca Compare October 5, 2016 10:47

pbertera commented Oct 5, 2016

View reviewed changes

Support for last_XX:value, last_XX:uri, last_XX:param.tag

909cae7

pbertera force-pushed the snom-last-extensions branch from 060d7ca to 909cae7 Compare October 5, 2016 11:58

wdoekes requested changes Oct 6, 2016

View reviewed changes

pbertera added 6 commits October 6, 2016 22:45

Merge branch 'snom-last-extensions' of github.com:Snomio/sipp into sn…

e755a4c

…om-last-extensions

Code review

6a3ec98

Fixed regtest

3344940

testcase debugging for travis falure

9442f8d

travis testcase fix

a6575cd

fixed regtest

b82bfff

wdoekes requested changes Oct 8, 2016

View reviewed changes

pbertera added 3 commits October 20, 2016 22:17

Fixed github-SIPp#260/run

8a4ad21

Code review

9b99ed2

Unit tests

e52e597

wdoekes force-pushed the master branch from 3759a15 to 4d0c1dd Compare April 23, 2018 15:35

wdoekes force-pushed the master branch from 7b68356 to 90ab1ef Compare May 27, 2019 15:49

Conversation

pbertera commented Oct 1, 2016

Uh oh!

wdoekes commented Oct 2, 2016

Uh oh!

pbertera commented Oct 4, 2016

Uh oh!

pbertera left a comment

Choose a reason for hiding this comment

Uh oh!

wdoekes Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdoekes Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbertera commented Oct 7, 2016

Uh oh!

wdoekes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdoekes Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdoekes Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdoekes Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbertera commented Oct 20, 2016

Uh oh!

wdoekes commented Nov 6, 2016

Uh oh!

wdoekes commented Dec 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wdoekes Oct 6, 2016 •

edited

Loading

wdoekes Oct 6, 2016 •

edited

Loading

wdoekes Oct 8, 2016 •

edited

Loading

wdoekes Oct 8, 2016 •

edited

Loading

wdoekes Oct 8, 2016 •

edited

Loading