[Home]

Summary:ASTERISK-17791: [patch] realtime peer update fails, because "" is not a valide int value for lastms
Reporter:Marcello Ceschia (marcelloceschia)Labels:
Date Opened:2011-05-04 02:56:58Date Closed:2011-07-20 11:26:12
Priority:TrivialRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Registration
Versions:1.8.4 Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20110505__issue19223.diff.txt
( 1) chan_sip-update-lastms-on-realtime-peer_branch18.patch
Description:The issue also present on asterisk 1.6.x
Comments:By: Tilghman Lesher (tilghman) 2011-05-05 10:47:17

A -1 value is NOT equivalent to empty.  -1 means the peer did not respond.  Blank means the peer is untested.
By: Marcello Ceschia (marcelloceschia) 2011-05-05 11:00:00

But empty is not a valide int value for databases.
Maybe it should 0 but not empty!
By: Tilghman Lesher (tilghman) 2011-05-05 11:05:31

Or maybe it should be NULL, as I've attempted to do in this patch.
By: Tilghman Lesher (tilghman) 2011-05-05 11:07:47

BTW, 0 means that the peer responded very quickly, within 0 ms.  There is no equivalent numeric value to "untested", only NULL or blank.
By: Marcello Ceschia (marcelloceschia) 2011-05-05 11:17:55

It can not be NULL, because this field is required. And how to parse a NULL value to int?


I think this value must be -1, because this call is in destroy_association(struct sip_peer *peer)
and after remove a registration the device is tested and do not response
By: Tilghman Lesher (tilghman) 2011-05-05 20:39:05

It can be NULL, and making the field NULLable has nothing to do with whether the field is required or not.

The database must take on the attributes that we find useful in Asterisk; Asterisk is not modeled on top of the database.  Asterisk specifically expects NULL for those values which are not specified.  Alternatively, you can create the field as a char type.

Again, -1 means SOMETHING DIFFERENT than NULL.  You MAY NOT conflate the two values.


By: Marcello Ceschia (marcelloceschia) 2011-05-06 04:20:59

I know the differnce of NULL and not NULL values.
Looking into contrib/realtime/postgresql/realtime.sql, lastms is defined as

lastms integer DEFAULT -1 NOT NULL

and thats the truth people know about asterisk realtime.
I dont say that -1 is the right value in this case, but destroy_association is called in expire_register and in build_peer.
Within build_peer lastms is explicitly set to -1:

if ((nowtime - regseconds) > 0) {
destroy_association(peer);
memset(&peer->addr, 0, sizeof(peer->addr));
peer->lastms = -1;
ast_debug(1, "Bah, we're expired (%d/%d/%d)!\n", (int)(nowtime - regseconds), (int)regseconds, (int)nowtime);
}

In expire_register lastms isnt set, but we know that peer is expired so it can not be unknown.
Setting the value to NULL means asterisk does not parse this value and so the lastms has the default 0 value.

Retire my patch and close this issue.
By: Jonathan Rose (jrose) 2011-07-20 11:26:12.464-0500

It's been patched.  For some reason the commit message wasn't applied to the issue though and I'm not sure which revision it was.