Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snom patch: Last_ keywords extension #260

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
2 changes: 1 addition & 1 deletion include/call.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ class call : virtual public task, virtual public listener, public virtual socket

char * get_header_field_code(const char * msg, const char * code);
char * get_last_header(const char * name);
char * get_last_request_uri();
char * get_last_header_uri(const char *name);
unsigned long hash(const char * msg);

typedef std::map <std::string, int> file_line_map;
Expand Down
1 change: 1 addition & 0 deletions include/sip_parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#define MAX_HEADER_LEN 2049

char *get_call_id(const char *msg);
char *get_param_tag(const char *msg, const char *name, const char *shortname);
char *get_peer_tag(const char *msg);

int get_method(char *msg);
Expand Down
34 changes: 34 additions & 0 deletions regress/github-#0260/run
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#!/bin/sh
# This regression test is a part of SIPp.
# Author: Pietro Bertera, Snom Technology AG, 2016

. "`dirname "$0"`/../functions"; init

sippbg -m 1 -sf uas.xml -p 5070 -i 127.0.0.1 \
-timeout 4 -timeout_error -trace_logs \
-log_file log.log >/dev/null 2>&1

job=$!

sippfg -m 1 -sf uac.xml -i 127.0.0.1 -p 5060 127.0.0.1:5070 \
-timeout 4 -timeout_error >/dev/null 2>&1

status=$?
wait $job || status=1


if test $status -ne 0; then
fail "process failure"
elif ! grep -e ^LAST\ Contact\|Contact:\ \<sip:[email protected]:5060\> log.log > /dev/null; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:[email protected]:5060>' log.log

fail "[last_Contact] header not found"
elif ! grep -e ^LAST\ Contact:\|Contact:\ \<sip:[email protected]:5060\> log.log > /dev/null; then
fail "[last_Contact:] header not found"
elif ! grep -e ^LAST\ From:value\|\"Tom\ Jones\"\ \<sip:[email protected]\>\;tag=SIPpTag001$ log.log > /dev/null; then
fail "[last_From:value] not found"
elif ! grep -e ^LAST\ To:uri\|sip:[email protected]$ log.log > /dev/null; then
fail "[last_To:uri] not found"
elif ! grep -e ^LAST\ From:param.tag\|SIPpTag001$ log.log > /dev/null; then
fail "[last_From:param.tag] not found"
fi
Copy link
Member

@wdoekes wdoekes Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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\[email protected]>;tag=SIPpTag001$' log.log; then
    fail "From header not found"
...


ok
54 changes: 54 additions & 0 deletions regress/github-#0260/uac.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?xml version="1.0" encoding="ISO-8859-1"?>
<!DOCTYPE scenario SYSTEM "sipp.dtd">
<scenario>
<send retrans="500" start_txn="invite">
<![CDATA[

INVITE sip:[service]@[remote_ip]:[remote_port] SIP/2.0
Via: SIP/2.0/[transport] [local_ip]:[local_port];branch=justafake
From: "Tom Jones" <sip:[email protected]>;tag=SIPpTag00[call_number]
To: "Fromage" <sip:[email protected]>
Call-ID: [call_id]
CSeq: 1 INVITE
Contact: <sip:sipp@[local_ip]:[local_port]>
Content-Length: 0

]]>
</send>

<recv response="200" response_txn="invite" rrs="true"/>

<send retrans="500" ack_txn="invite">
<![CDATA[

ACK [next_url] SIP/2.0
Via: SIP/2.0/[transport] [local_ip]:[local_port];branch=justafake
From: "Tom Jones" <sip:[email protected]>;tag=SIPpTag00[call_number]
To: "Fromage" <sip:[email protected]>[peer_tag_param]
Call-ID: [call_id]
CSeq: 1 ACK
Contact: sip:sipp@[local_ip]:[local_port]
Content-Length: 0

]]>
</send>

<recv request="BYE"/>

<send>
<![CDATA[

SIP/2.0 200 OK
[last_Via:]
[last_From:]
[last_To:]
[last_Call-ID:]
[last_CSeq:]
Contact: <sip:[local_ip]:[local_port];transport=[transport]>
Content-Length: 0

]]>
</send>

<timewait milliseconds="500"/>
</scenario>
53 changes: 53 additions & 0 deletions regress/github-#0260/uas.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?xml version="1.0" encoding="ISO-8859-1"?>
<!DOCTYPE scenario SYSTEM "sipp.dtd">
<scenario>
<recv request="INVITE"/>

<send retrans="500">
<![CDATA[

SIP/2.0 200 OK
[last_Contact]
[last_via]
From: [last_From:value]
To: [last_To:value];tag=[pid]SIPpTag00[call_number]
Call-ID: [last_Call-ID:value]
CSeq: [last_CSeq:value]
Contact: sip:sipp@[local_ip]:[local_port]
Content-Length: 0

]]>
</send>

<recv request="ACK" rrs="true">
<action>
<log message="LAST Contact|[last_Contact]" />
<log message="LAST Contact:|[last_Contact:]" />
<log message="LAST Contact:uri|[last_Contact:uri]"/>
<log message="LAST Via:value[last_Via:value]"/>
<log message="LAST From:value|[last_From:value]"/>
<log message="LAST To:value|[last_To:value]"/>
<log message="LAST To:uri|[last_To:uri]"/>
<log message="LAST From:param.tag|[last_From:param.tag]"/>
</action>
</recv>

<send retrans="500">
<![CDATA[

BYE [next_url] SIP/2.0
Via: SIP/2.0/[transport] [local_ip]:[local_port];branch=[branch]
From: [last_To:value]
To: [last_From:value]
Call-ID: [call_id]
CSeq: 2 BYE
Contact: sip:sipp@[local_ip]:[local_port]
Content-Length: 0

]]>
</send>

<recv response="200"/>

<timewait milliseconds="500"/>
</scenario>
53 changes: 43 additions & 10 deletions src/call.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -920,36 +920,69 @@ char * call::get_last_header(const char * name)
ERROR("call::get_last_header: Header to parse bigger than %d (%zu)", MAX_HEADER_LEN, strlen(name));
}

char header_name[MAX_HEADER_LEN];
const char *sep = strrchr(name, ':');

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

snprintf(header_name, len - strlen(sep) + 2, name);

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

@wdoekes wdoekes Oct 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!strcmp(sep, ":value")) { /* [last_Header:value] */
; /* done */
} else if (!strcmp(sep, ":uri")) { /* [last_Header:uri] */
char * uri = get_last_header_uri(header_name);
int uri_len = strlen(uri);
memmove(value, uri, uri_len);
value[uri_len] = '\0';
free(uri);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if (!strcmp(sep, ":param.tag")) { /* [last_Header:param.tag] */
snprintf(header_name, len - 9, "%s", name);
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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.. copies the NUL along.

} else {
char with_colon[MAX_HEADER_LEN];
sprintf(with_colon, "%s:", name);
return get_header(last_recv_msg, with_colon, false);
ERROR("Token %s not valid in %s", sep, name);
}
return value;
}

/* Return the last request URI from the To header. On any error returns the
/* Return the last request URI from the header. On any error returns the
* empty string. The caller must free the result. */
char * call::get_last_request_uri()
char * call::get_last_header_uri(const char *name)
{
char * tmp;
char * tmp2;
char * last_request_uri;
char * last_header;
int tmp_len;

char * last_To = get_last_header("To:");
if (!last_To) {
if (!name || !strlen(name)) {
return strdup("");
}

last_header = get_last_header(name);

if (!last_header) {
return strdup("");
}

tmp = strchr(last_To, '<');
tmp = strchr(last_header, '<');
if (!tmp) {
return strdup("");
}
tmp++;

tmp2 = strchr(last_To, '>');
tmp2 = strchr(last_header, '>');
if (!tmp2) {
return strdup("");
}
Expand Down Expand Up @@ -2201,7 +2234,7 @@ char* call::createSendingMessage(SendingMessage *src, int P_index, char *msg_buf
}
break;
case E_Message_Last_Request_URI: {
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);
Copy link
Member

@wdoekes wdoekes Oct 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

break;
Expand Down
11 changes: 8 additions & 3 deletions src/sip_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,17 @@ static const char *internal_hdrend(const char *ptr);

/*************************** Mini SIP parser (externals) ***************/

char * get_peer_tag(const char *msg)
char * get_param_tag(const char *msg, const char *name, const char *shortname)
{
static char tag[MAX_HEADER_LEN];
const char * to_hdr;
const char * ptr;
int tag_i = 0;

/* Find start of header */
to_hdr = internal_find_header(msg, "To", "t", true);
to_hdr = internal_find_header(msg, name, shortname, true);
if (!to_hdr) {
WARNING("No valid To: header in reply");
WARNING("No valid %s: header in reply", name);
return NULL;
}

Expand Down Expand Up @@ -109,6 +109,11 @@ char * get_peer_tag(const char *msg)
return tag;
}

char * get_peer_tag(const char *msg)
{
return get_param_tag(msg, "To", "t");
Copy link
Member

@wdoekes wdoekes Oct 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be:

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

That'd be even more generic.

}

char * get_header_content(const char* message, const char * name)
{
return get_header(message, name, true);
Expand Down