@henningw commented on this pull request.
I have added some remarks, mostly related to error logging and name space
topics. Thank you.
> + *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA
+ *
+ */
+
+#ifndef _PEERSTATE_H
+#define _PEERSTATE_H
+
+#include "../../core/parser/parse_uri.h"
+struct str_list;
+
+#define PEERSTATE_EVENT_ROUTE "dialog:state_changed"
+
+typedef enum pstate
Please add the module name as prefix, as there is already a pstate variable in
the codebase.
> + NOT_INUSE, // according to dialog state
+ INUSE, // according to dialog and register state
+ RINGING, // according to dialog state
+ UNAVAILABLE, // according to register state
+ NA
+} pstate_t;
+
+static const char *pstate_strs[] __attribute__((unused)) = {
+ "NOT_INUSE", "INUSE", "RINGING", "UNAVAILABLE", "N/A"};
+
+#define PSTATE_TO_STR(s) \
+ ((s) >= 0 && (s) < (int)(sizeof(pstate_strs) / sizeof(pstate_strs[0])) \
+ ? pstate_strs[s]
\
+ : "UNKNOWN")
+
+struct dlginfo_cell
Please add the module name as prefix, as there is already a definition with
this name
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
+ *
+ */
+
+#include <string.h>
+#include <time.h>
+
+#include "../../core/mem/shm_mem.h"
+#include "../../core/locking.h"
+#include "../../core/str_hash.h"
+#include "../../core/dprint.h"
+#include "../../core/timer.h"
+#include "../../core/timer_proc.h"
+#include "peerstate_cache.h"
+
+struct peer_state
If this is used only here, please make it static. There is also already a
peer_state.
> +
+ LM_DBG("Created new peer entry '%.*s'\n", peer->len, peer->s);
+ }
+
+ // Save old values
+ old_state = p->state;
+ old_count = p->call_count;
+
+ // Process based on candidate state
+ if(candidate_state == RINGING) {
+ // EARLY event occurred
+ // Always increment counter for new dialog
+ p->call_count++;
+ new_count = p->call_count;
+
+ // Update state based on priority: INUSE > RINGING > NOT_INUSE
Just a comment, maybe it would be easier to use a switch statement
> + new_state = NOT_INUSE; // Peer was unavailable, now
> registered
+ else
+ new_state =
+ old_state; // Registration doesn't
interrupt active calls, NOT_INUSE, INUSE, or RINGING - no change
+ p->state = new_state;
+ p->timestamp = time(NULL);
+
+ LM_DBG("REGISTERED: peer='%.*s', old_state=%s, new_state=%s,
count=%d, "
+ "is_registered=%d\n",
+ peer->len, peer->s, PSTATE_TO_STR(old_state),
+ PSTATE_TO_STR(new_state), call_count,
p->is_registered);
+ } else if(candidate_state == UNAVAILABLE) {
+ // UNREGISTERED event occurred
+ // Update registration flag
+ p->is_registered = 0;
+ if(old_state == NOT_INUSE)
Just a comment, maybe it would be easier to use a switch statement
> + PKG_MEM_ERROR;
+ return -1;
+ }
+
+ lock_get(peer_state_lock);
+
+ for(i = 0; i < peer_state_hash_size; i++) {
+ p = peer_state_hash_table[i];
+ while(p) {
+ if(p->state == target_state) {
+ if(found >= capacity) {
+ capacity *= 2;
+ str *new_result =
+ (str
*)pkg_realloc(result, capacity * sizeof(str));
+ if(!new_result) {
+ lock_release(peer_state_lock);
There should be probably also some error logging here, e.g. PKG_MEM_ERROR
> + if(p->state == target_state) {
+ if(found >= capacity) {
+ capacity *= 2;
+ str *new_result =
+ (str
*)pkg_realloc(result, capacity * sizeof(str));
+ if(!new_result) {
+ lock_release(peer_state_lock);
+ pkg_free(result);
+ return -1;
+ }
+ result = new_result;
+ }
+
+ result[found].s = (char
*)pkg_malloc(p->peer.len + 1);
+ if(!result[found].s) {
+ lock_release(peer_state_lock);
There should be probably also some error logging here, e.g. PKG_MEM_ERROR
> + if(!result) {
+ PKG_MEM_ERROR;
+ return -1;
+ }
+
+ lock_get(peer_state_lock);
+
+ for(i = 0; i < peer_state_hash_size; i++) {
+ p = peer_state_hash_table[i];
+ while(p) {
+ if(found >= capacity) {
+ capacity *= 2;
+ str *new_result =
+ (str *)pkg_realloc(result,
capacity * sizeof(str));
+ if(!new_result) {
+ lock_release(peer_state_lock);
There should be probably also some error logging here, e.g. PKG_MEM_ERROR
> + while(p) {
+ if(found >= capacity) {
+ capacity *= 2;
+ str *new_result =
+ (str *)pkg_realloc(result,
capacity * sizeof(str));
+ if(!new_result) {
+ lock_release(peer_state_lock);
+ pkg_free(result);
+ return -1;
+ }
+ result = new_result;
+ }
+
+ result[found].s = (char *)pkg_malloc(p->peer.len + 1);
+ if(!result[found].s) {
+ lock_release(peer_state_lock);
There should be probably also some error logging here, e.g. PKG_MEM_ERROR
> +#define DEF_CALLER_ALWAYS_CONFIRMED 0
+#define DEF_USE_AVPS 0
+#define DEF_CALLER_AVP 0
+#define DEF_CALLEE_AVP 0
+#define DEF_REG_AVP 0
+
+#define DEF_ENABLE_NOTIFY_ON_TRYING 0
+#define DEF_DISABLE_CALLER_NOTIFY_FLAG -1
+#define DEF_DISABLE_CALLEE_NOTIFY_FLAG -1
+#define DEF_DISABLE_REG_NOTIFY -1
+#define DEF_DISABLE_DLG_NOTIFY -1
+#define DEF_CACHE_EXPIRE 3600
+#define DEF_CACHE_CLEANUP_INTERVAL 300
+#define DEF_CACHE_HASH_SIZE 4096
+
+#define MAX_CALLID_BUF_SIZE 1024
This define and the following one seems to be unused, then it should be removed
> +#define DEF_CALLEE_AVP 0
+#define DEF_REG_AVP 0
+
+#define DEF_ENABLE_NOTIFY_ON_TRYING 0
+#define DEF_DISABLE_CALLER_NOTIFY_FLAG -1
+#define DEF_DISABLE_CALLEE_NOTIFY_FLAG -1
+#define DEF_DISABLE_REG_NOTIFY -1
+#define DEF_DISABLE_DLG_NOTIFY -1
+#define DEF_CACHE_EXPIRE 3600
+#define DEF_CACHE_CLEANUP_INTERVAL 300
+#define DEF_CACHE_HASH_SIZE 4096
+
+#define MAX_CALLID_BUF_SIZE 1024
+#define STR_AT "@"
+
+struct dlg_binds dlg_api;
If its unsed only here, make it static. Otherwise add the module name as prefix
to it.
Also look to the following api variable.
> + uniq_id->len, uniq_id->s, peer->len, peer->s,
> PSTATE_TO_STR(pstate),
+ event_type);
+
+ /* Trigger event route */
+ trigger_event_route(pstate, prev_state, peer, uniq_id);
+
+ /* Prepare callback context */
+ memset(&ctx, 0, sizeof(peerstate_cb_ctx_t));
+ ctx.peer = *peer;
+ ctx.current_state = pstate;
+ ctx.previous_state = prev_state;
+ ctx.uniq_id = *uniq_id;
+ ctx.event_type = event_type;
+
+ /* Get additional info from cache */
+ if(get_peer_info(peer, NULL, &call_count, &is_registered) == 0) {
Should we log an error/notice/info message?
> + dlg->from_uri.s);
+
+ if(disable_caller_notify_flag != -1
+ && (request
+ && (request->flags & (1 <<
disable_caller_notify_flag))))
+ disable_caller_notify = 1;
+
+ if(disable_callee_notify_flag != -1
+ && (request
+ && (request->flags & (1 <<
disable_callee_notify_flag))))
+ disable_callee_notify = 1;
+
+ dlginfo = get_dialog_data(
+ dlg, type, disable_caller_notify,
disable_callee_notify);
+
+ if(dlginfo == NULL)
Should we log an error/notice/info message?
> +
+static void __dialog_loaded(
+ struct dlg_cell *dlg, int type, struct dlg_cb_params *_params)
+{
+ struct sip_msg *request = _params->req;
+ struct dlginfo_cell *dlginfo;
+
+ if(request == NULL || request->REQ_METHOD != METHOD_INVITE)
+ return;
+
+ LM_INFO("INVITE dialog loaded: from=%.*s\n", dlg->from_uri.len,
+ dlg->from_uri.s);
+
+ dlginfo = get_dialog_data(dlg, type, 0, 0);
+
+ if(dlginfo == NULL)
Should we log an error/notice/info message?
> +
+ if(!lst)
+ return -1;
+
+ buf.len = 0;
+ while(it) {
+ buf.len += it->s.len;
+ count++;
+ it = it->next;
+ }
+ if(count > 1)
+ buf.len += (count - 1);
+
+ buf.s = (char *)pkg_malloc(buf.len + 1);
+ if(!buf.s)
+ return -1;
Error logging, e.g. PKG_MEM_ERROR
> +<!DOCTYPE book PUBLIC "-//OASIS//DTD DocBook XML V4.4//EN"
+"http://www.oasis-open.org/docbook/xml/4.4/docbookx.dtd"; [
+
+<!ENTITY % docentities SYSTEM "../../../../doc/docbook/entities.xml">
+%docentities;
+
+]>
+
+<chapter>
+
+ <title>&adminguide;</title>
+
+ <section>
+ <title>Overview</title>
+ <para>
+ The peerstate module provides real-time peer state tracking for the
As discusses, should be re-phrased to not mixup with other projects:
"The peerstate module provides peer state tracking in real-time for the.."
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4537#pullrequestreview-3635006005
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4537/review/[email protected]>_______________________________________________
Kamailio - Development Mailing List -- [email protected]
To unsubscribe send an email to [email protected]
Important: keep the mailing list in the recipients, do not reply only to the
sender!
