<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 TRANSITIONAL//EN">
<HTML>
<HEAD>
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; CHARSET=UTF-8">
<META NAME="GENERATOR" CONTENT="GtkHTML/3.14.3">
</HEAD>
<BODY>
On Wed, 2007-12-19 at 17:06 -0700, Steve Murphy wrote:<BR>
<BLOCKQUOTE TYPE=CITE>
<FONT COLOR="#000000">Olle--</FONT><BR>
<BR>
<FONT COLOR="#000000">I thought to double check with you... the price of missing something is pretty high.</FONT><BR>
<BR>
<FONT COLOR="#000000">Put on your thinking cap, pull out some paper and a pencil, turn off the cell phone... this is going to require</FONT><BR>
<FONT COLOR="#000000">some thought!</FONT><BR>
<BR>
<BR>
<FONT COLOR="#000000">For reference, here's the search loop in find_call:</FONT><BR>
<BR>
<BR>
<FONT COLOR="#000000"> dialoglist_lock();</FONT><BR>
<FONT COLOR="#000000"> for (p = dialoglist; p; p = p->next) {</FONT><BR>
<FONT COLOR="#000000"> /* In pedantic, we do not want packets with bad syntax to be connected to a PVT */</FONT><BR>
<FONT COLOR="#000000"> int found = FALSE;</FONT><BR>
<FONT COLOR="#000000"> if (ast_strlen_zero(p->callid))</FONT><BR>
<FONT COLOR="#000000"> continue;</FONT><BR>
<FONT COLOR="#000000"> if (req->method == SIP_REGISTER)</FONT><BR>
<FONT COLOR="#000000"> found = (!strcmp(p->callid, callid));</FONT><BR>
<FONT COLOR="#000000"> else</FONT><BR>
<FONT COLOR="#000000"> found = (!strcmp(p->callid, callid) &&</FONT><BR>
<FONT COLOR="#000000"> (!pedanticsipchecking </FONT><FONT COLOR="#ff0000">|| !tag ||</FONT><FONT COLOR="#000000"> ast_strlen_zero(p->theirtag) || !strcmp(p->theirtag, tag))) ;</FONT><BR>
<BR>
<FONT COLOR="#000000"> ast_debug(5, "= %s Their Call ID: %s Their Tag %s Our tag: %s\n", </FONT><BR>
<FONT COLOR="#000000"> found ? "Found" : "No match", p->callid, p->theirtag, p->tag);</FONT><BR>
<BR>
<FONT COLOR="#000000"> /* If we get a new request within an existing to-tag - check the to tag as well */</FONT><BR>
<FONT COLOR="#000000"> if (pedanticsipchecking && found && req->method != SIP_RESPONSE) { /* SIP Request */</FONT><BR>
<FONT COLOR="#000000"> if (p->tag[0] == '\0' && totag[0]) {</FONT><BR>
<FONT COLOR="#000000"> /* We have no to tag, but they have. Wrong dialog */</FONT><BR>
<FONT COLOR="#000000"> found = FALSE;</FONT><BR>
<FONT COLOR="#000000"> } else if (totag[0]) { /* Both have tags, compare them */</FONT><BR>
<FONT COLOR="#000000"> if (strcmp(totag, p->tag)) {</FONT><BR>
<FONT COLOR="#000000"> found = FALSE; /* This is not our packet */</FONT><BR>
<FONT COLOR="#000000"> }</FONT><BR>
<FONT COLOR="#000000"> }</FONT><BR>
<FONT COLOR="#000000"> if (!found)</FONT><BR>
<FONT COLOR="#000000"> ast_debug(5, "= Being pedantic: This ... request: Call ID: %s Ourtag <null> Totag %s Method %s\n", </FONT><BR>
<FONT COLOR="#000000"> p->callid, totag, sip_methods[req->method].text);</FONT><BR>
<FONT COLOR="#000000"> }</FONT><BR>
<BR>
<BR>
<FONT COLOR="#000000"> if (found) {</FONT><BR>
<FONT COLOR="#000000"> /* Found the call */</FONT><BR>
<FONT COLOR="#000000"> sip_pvt_lock(p);</FONT><BR>
<FONT COLOR="#000000"> dialoglist_unlock();</FONT><BR>
<FONT COLOR="#000000"> return p;</FONT><BR>
<FONT COLOR="#000000"> }</FONT><BR>
<FONT COLOR="#000000"> }</FONT><BR>
<FONT COLOR="#000000"> dialoglist_unlock();</FONT><BR>
<BR>
<FONT COLOR="#000000">The object: remove the loop, and replace it with astobj2 hash table lookups.</FONT><BR>
<BR>
<FONT COLOR="#000000">I checked Luigi's astobj2 branch, where he put dialogs in a table, but he implements the above as a linear traversal, which is not my goal. I want to improve performance, not just convert the code to be astobj2 based...</FONT><BR>
<BR>
<FONT COLOR="#000000">There seem to be two main modes: with pedanticsipchecking ON, and pedanticsipchecking OFF.</FONT><BR>
<HR NOSHADE>
<BR>
<BR>
<BR>
<BR>
<FONT COLOR="#000000">Things are simple in OFF mode:</FONT><BR>
<BR>
<FONT COLOR="#000000">fundamentally, whether req->method is SIP_REGISTER or not, found is true when a dialog's callid matches.</FONT><BR>
<FONT COLOR="#000000">No code after setting found could disqualify this choice.</FONT><BR>
<BR>
<HR NOSHADE>
<BR>
<BR>
<BR>
<BR>
<FONT COLOR="#000000">Things get more complex in pedanticsipchecking ON mode:</FONT><BR>
<BR>
<FONT COLOR="#000000">It seems split into whether req->method is SIP_REGISTER/SIP_RESPONSE or not:</FONT><BR>
<BR>
<FONT COLOR="#000000">If req->method is SIP_REGISTER:</FONT><BR>
<BR>
<FONT COLOR="#000000"> if match(p->callid,callid) AND (!totag[0] || (p->tag[0] && match(p->tag,totag)) </FONT><BR>
<BR>
<FONT COLOR="#000000">else </FONT><BR>
<BR>
<FONT COLOR="#000000"> If req->method is SIP_RESPONSE:</FONT><BR>
<BR>
<FONT COLOR="#000000"> If callid matches AND ( empty(p->theirtag) || match(p->theirtag,tag)) </FONT><BR>
<BR>
<FONT COLOR="#000000"> else</FONT><BR>
<BR>
<FONT COLOR="#000000"> If callid matches AND (!totag[0] || (p->tag[0] && match(p->tag,totag)) </FONT><BR>
<FONT COLOR="#000000"> AND ( empty(p->theirtag) || match(p->theirtag,tag)) </FONT><BR>
<BR>
<FONT COLOR="#000000">(I notice in the code that there is : ... </FONT><FONT COLOR="#ff0000">|| !tag ||</FONT><FONT COLOR="#000000"> ... which will never be true, and can be dropped)</FONT><BR>
<BR>
<HR NOSHADE>
<BR>
</BLOCKQUOTE>
<BR>
<BR>
OK, terms like: <FONT COLOR="#000000">empty(p->theirtag) || match(p->theirtag,tag)</FONT> ,<BR>
<BR>
which means: "if p->theirtag is null/empty, then it doesn't affect the match, otherwise, it must match the tag.<BR>
<BR>
This I can handle just fine, most likely by trying to ao2_find with theirtag set, and if no match, look for a null theirtag match.<BR>
<BR>
<BR>
But: <FONT COLOR="#000000">(!totag[0] || (p->tag[0] && match(p->tag,totag)) </FONT><BR>
<BR>
means that if the totag is empty, then it matches whatever p->tag might be in the db. This is impossible to replicate in hashtable searches, because the search term is blank, and will not match any entries where the field is not blank.<BR>
Nor can I define another table, or even just enter another row with the tag field blank, BECAUSE THERE MIGHT BE A COLLISION.<BR>
<BR>
I have come to the conclusion that this is not necessarily a weakness in the trying to use hash tables. but could be a weakness in the linear search algorithm itself. If totag is empty, then you could conceivably fetch the wrong dialog from the db, which would probably just be the first one you hit in your traversal.<BR>
<BR>
So, TELL ME, please, that if totag is empty, that You Never Will See more than one dialog with the same callid, and mayhaps with different tag field values, or better yet, with different theirtag values. <BR>
<BR>
--Before the search starts, I see this code:<BR>
<BR>
if (ast_strlen_zero(totag) && (req->method == SIP_ACK <BR>
|| req->method == SIP_BYE <BR>
|| req->method == SIP_INFO )) {<BR>
ast_debug(5, "%s must have a to tag. dropping callid: %s from: %s\n", <BR>
sip_methods[req->method].text , callid, from );<BR>
return NULL;<BR>
}<BR>
<BR>
This doesn't protect the SIP_REGISTER case; and the other non-SIP_RESPONSE cases:<BR>
SIP_UNKNOWN, /*!< Unknown response */<BR>
SIP_OPTIONS, /*!< Check capabilities of a device, used for "ping" too */<BR>
SIP_NOTIFY, /*!< Status update, Part of the event package standard, result of a SUBSCRIBE or a REFER */<BR>
SIP_INVITE, /*!< Set up a session */<BR>
SIP_PRACK, /*!< Reliable pre-call signalling. Not supported in Asterisk. */<BR>
SIP_REFER, /*!< Refer to another URI (transfer) */<BR>
SIP_SUBSCRIBE, /*!< Subscribe for updates (voicemail, session status, device status, presence) */<BR>
SIP_MESSAGE, /*!< Text messaging */<BR>
SIP_UPDATE, /*!< Update a dialog. We can send UPDATE; but not accept it */<BR>
SIP_CANCEL, /*!< Cancel an INVITE */<BR>
SIP_PUBLISH, /*!< Not supported in Asterisk */<BR>
SIP_PING, /*!< Not supported at all, no standard but still implemented out there */<BR>
<BR>
So, is there a problem here or not?<BR>
<BR>
murf<BR>
<BR>
<BR>
</BODY>
</HTML>