[asterisk-dev] dialog matching

[Steve Murphy]

On Wed, 2007-12-19 at 17:06 -0700, Steve Murphy wrote:

> Olle--
> 
> I thought to double check with you... the price of missing something
> is pretty high.
> 
> Put on your thinking cap, pull out some paper and a pencil, turn off
> the cell phone... this is going to require
> some thought!
> 
> 
> For reference, here's the search loop in find_call:
> 
> 
>         dialoglist_lock();
>         for (p = dialoglist; p; p = p->next) {
>                 /* In pedantic, we do not want packets with bad syntax
> to be connected to a PVT */
>                 int found = FALSE;
>                 if (ast_strlen_zero(p->callid))
>                         continue;
>                 if (req->method == SIP_REGISTER)
>                         found = (!strcmp(p->callid, callid));
>                 else
>                         found = (!strcmp(p->callid, callid) &&
>                            (!pedanticsipchecking || !tag ||
> ast_strlen_zero(p->theirtag) || !strcmp(p->theirtag, tag))) ;
> 
>                 ast_debug(5, "= %s Their Call ID: %s Their Tag %s Our
> tag: %s\n", 
>                                                  found ? "Found" : "No
> match", p->callid, p->theirtag, p->tag);
> 
>                 /* If we get a new request within an existing to-tag -
> check the to tag as well */
>                 if (pedanticsipchecking && found  && req->method !=
> SIP_RESPONSE) {     /* SIP Request */
>                         if (p->tag[0] == '\0' && totag[0]) {
>                                 /* We have no to tag, but they have.
> Wrong dialog */
>                                 found = FALSE;
>                         } else if (totag[0]) {                  /*
> Both have tags, compare them */
>                                 if (strcmp(totag, p->tag)) {
>                                         found = FALSE;          /*
> This is not our packet */
>                                 }
>                         }
>                         if (!found)
>                                 ast_debug(5, "= Being pedantic:
> This ... request: Call ID: %s Ourtag <null> Totag %s Method %s\n", 
>                                        p->callid, totag,
> sip_methods[req->method].text);
>                 }
> 
> 
>                 if (found) {
>                         /* Found the call */
>                         sip_pvt_lock(p);
>                         dialoglist_unlock();
>                         return p;
>                 }
>         }
>         dialoglist_unlock();
> 
> The object: remove the loop, and replace it with astobj2 hash table
> lookups.
> 
> 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...
> 
> There seem to be two main modes: with pedanticsipchecking ON, and
> pedanticsipchecking OFF.
> 
> ______________________________________________________________________
> 
> 
> 
> Things are simple in OFF mode:
> 
> fundamentally, whether req->method is SIP_REGISTER or not, found is
> true when a dialog's callid matches.
> No code after setting found could disqualify this choice.
> 
> 
> ______________________________________________________________________
> 
> 
> 
> Things get more complex in pedanticsipchecking ON mode:
> 
> It seems split into whether req->method is SIP_REGISTER/SIP_RESPONSE
> or not:
> 
> If req->method is SIP_REGISTER:
> 
>    if match(p->callid,callid)  AND (!totag[0] ||  (p->tag[0] &&
> match(p->tag,totag)) 
> 
> else 
> 
>     If req->method is SIP_RESPONSE:
> 
>             If callid matches AND ( empty(p->theirtag)  ||
> match(p->theirtag,tag)) 
> 
>     else
> 
>             If callid matches AND  (!totag[0] ||  (p->tag[0] &&
> match(p->tag,totag)) 
>                                         AND ( empty(p->theirtag)  ||
> match(p->theirtag,tag))  
> 
> (I notice in the code that there is :     ... ||  !tag  || ...   which
> will never be true, and can be dropped)
> 
> 
> ______________________________________________________________________



OK, terms like:                empty(p->theirtag)  ||
match(p->theirtag,tag) ,

which means: "if p->theirtag is null/empty, then it doesn't affect the
match, otherwise, it must match the tag.

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.


But:               (!totag[0] ||  (p->tag[0] && match(p->tag,totag)) 

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.
Nor can I define another table, or even just enter another row with the
tag field blank, BECAUSE THERE MIGHT BE A COLLISION.

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.

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. 

--Before the search starts, I see this code:

                if (ast_strlen_zero(totag) && (req->method == SIP_ACK 
                                                               ||
req->method == SIP_BYE 
                                                               ||
req->method == SIP_INFO )) {
                        ast_debug(5, "%s must have a to tag. dropping
callid: %s from: %s\n", 

sip_methods[req->method].text , callid, from );
                        return NULL;
                }

This doesn't protect the SIP_REGISTER case; and the other
non-SIP_RESPONSE cases:
        SIP_UNKNOWN,            /*!< Unknown response */
        SIP_OPTIONS,            /*!< Check capabilities of a device,
used for "ping" too */
        SIP_NOTIFY,             /*!< Status update, Part of the event
package standard, result of a SUBSCRIBE or a REFER */
        SIP_INVITE,             /*!< Set up a session */
        SIP_PRACK,              /*!< Reliable pre-call signalling. Not
supported in Asterisk. */
        SIP_REFER,              /*!< Refer to another URI (transfer) */
        SIP_SUBSCRIBE,          /*!< Subscribe for updates (voicemail,
session status, device status, presence) */
        SIP_MESSAGE,            /*!< Text messaging */
        SIP_UPDATE,             /*!< Update a dialog. We can send
UPDATE; but not accept it */
        SIP_CANCEL,             /*!< Cancel an INVITE */
        SIP_PUBLISH,            /*!< Not supported in Asterisk */
        SIP_PING,               /*!< Not supported at all, no standard
but still implemented out there */

So, is there a problem here or not?

murf


-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.digium.com/pipermail/asterisk-dev/attachments/20071220/d2ec4c54/attachment-0001.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3239 bytes
Desc: not available
Url : http://lists.digium.com/pipermail/asterisk-dev/attachments/20071220/d2ec4c54/attachment-0001.bin