modules_k/dialog: Simplify and refactor parts of reference counter

handling.

- Remove special handling for dialogs in the "deleted" state to
  allow accessing such dialogs (e.g., from the configuration
  script).
  Besides making the code easier to understand, this also fixes a
  bug where the reference counter would be decremented after
  detecting a spiral (dlg_handlers.c) even though get_dlg() does
  not increment it for "deleted" dialogs.
- Adapt interfaces for lookup_dlg() and get_dlg() accordingly,
  i.e., remove "del" parameter and update in-code documentation.
- Replace direct increments on a dialog structure's ref variable
  by calls to ref_dlg_unsafe().
- Move {un}ref_dlg_unsafe() definitions towards the head of the
  file to make declaration available earlier.
- Improve store_dlg_in_tm():
    * Return and evaluate result code.
    * Replace second register call by passing unreference_dialog()
      as release function to the first register call.
- Document various places in the code dealing with reference
  counting.

modules_k/dialog/dialog.c

@@ -1139,9 +1139,10 @@ static int w_dlg_get(struct sip_msg *msg, char *ci, char *ft, char *tt)
 		return -1;
 	}
 
-	dlg = get_dlg(&sc, &sf, &st, &dir, NULL);
+	dlg = get_dlg(&sc, &sf, &st, &dir);
 	if(dlg==NULL)
 		return -1;
+    /* set current dialog pointer - re-use ref increment from dlg_get() above */
 	current_dlg_pointer = dlg;
 	_dlg_ctx.dlg = dlg;
 	_dlg_ctx.dir = dir;
@@ -1433,7 +1434,7 @@ static void rpc_end_dlg_entry_id(rpc_t *rpc, void *c) {
 
 	if (rpc->scan(c, "ddS", &h_entry, &h_id, &rpc_extra_hdrs) < 2) return;
 
-	dlg = lookup_dlg(h_entry, h_id, NULL);
+	dlg = lookup_dlg(h_entry, h_id);
 	if(dlg){
 		dlg_bye_all(dlg, (rpc_extra_hdrs.len>0)?&rpc_extra_hdrs:NULL);
 		unref_dlg(dlg, 1);

modules_k/dialog/dlg_handlers.c

@@ -647,7 +647,7 @@ static void unref_new_dialog(void *dialog)
 static void unreference_dialog(void *dialog)
 {
     // if the dialog table is gone, it means the system is shutting down.
-    if (!d_table)
+    if (!dialog || !d_table)
         return;
     unref_dlg((struct dlg_cell*)dialog, 1);
 }
@@ -663,69 +663,43 @@ void dlg_tmcb_dummy(struct cell* t, int type, struct tmcb_params *param)
 	return;
 }
 
-/*!
- * \brief Release a transaction from a dialog
- * \param t transaction
- * \param type type of the entered callback
- * \param param saved dialog structure in the callback
- */
-static void release_dlg_from_tm(struct cell* t,
-                                int type,
-                                struct tmcb_params *param)
-{
-    struct dlg_cell *dlg = get_dialog_from_tm(t);
-
-    if (!dlg)
-    {
-        LM_ERR("Failed to get and unref dialog from transaction!");
-        return;
-    }
-
-    unreference_dialog(dlg);
-}
-
 /*!
  * \brief Register a transaction on a dialog
  * \param t transaction
  * \param type type of the entered callback
  * \param param saved dialog structure in the callback
  */
-static void store_dlg_in_tm(struct sip_msg* msg,
+static int store_dlg_in_tm(struct sip_msg* msg,
                             struct cell* t,
                             struct dlg_cell *dlg)
 {
     if( !msg || msg == FAKED_REPLY || !t || !dlg)
     {
         LM_ERR("invalid parameter msg(%p), t(%p), dlg(%p)\n", msg, t, dlg);
-        return;
+        return -1;
     }
 
     if(get_dialog_from_tm(t))
     {
         LM_NOTICE("dialog %p is already set for this transaction!\n",dlg);
-        return;
+        return 1;
     }
 
+	// facilitate referencing of dialog through TMCB_MAX
     if( d_tmb.register_tmcb (msg,
                              t,
                              TMCB_MAX,
                              dlg_tmcb_dummy,
-                             (void*)dlg, 0)<0 )
+                             (void*)dlg, unreference_dialog)<0 )
     {
         LM_ERR("failed cache in T the shortcut to dlg %p\n",dlg);
-        return;
+        return -3;
     }
 
+	// registering succeeded, we must increase the reference counter
     ref_dlg(dlg, 1);
 
-    if (d_tmb.register_tmcb (msg,
-                             t,
-                             TMCB_DESTROY,
-                             release_dlg_from_tm,
-                             (void*)dlg, NULL)<0 )
-    {
-        LM_ERR("failed to register unref tm for handling dialog-termination\n");
-    }
+	return 0;
 }
 
 /*!
@@ -771,7 +745,6 @@ int dlg_new_dialog(struct sip_msg *req, struct cell *t, const int run_initial_cb
     str ttag;
     str req_uri;
     unsigned int dir;
-    unsigned int del;
 
     if(current_dlg_pointer != NULL)
         return -1;
@@ -800,14 +773,7 @@ int dlg_new_dialog(struct sip_msg *req, struct cell *t, const int run_initial_cb
 
         dir = DLG_DIR_NONE;
 
-        dlg = get_dlg(&callid, &ftag, &ttag, &dir, &del);
-        if (del == 1)
-        {
-            LM_WARN("Failed to get dialog (callid: '%.*s') because it is marked for deletion!\n",
-                callid.len, callid.s);
-            unref_dlg(dlg, 1);
-            return 0;
-        }
+        dlg = get_dlg(&callid, &ftag, &ttag, &dir);
         if (dlg)
         {
             LM_DBG("Callid '%.*s' found, must be a spiraled request\n",
@@ -816,7 +782,7 @@ int dlg_new_dialog(struct sip_msg *req, struct cell *t, const int run_initial_cb
 
             if (run_initial_cbs)
                 run_dlg_callbacks( DLGCB_SPIRALED, dlg, req, NULL, DLG_DIR_DOWNSTREAM, 0);
-            // get_dlg with del==0 has incremented the ref count by 1
+            // get_dlg has incremented the ref count by 1
             unref_dlg(dlg, 1);
             goto finish;
         }
@@ -888,7 +854,10 @@ finish:
 	if (t) {
 		// transaction exists ==> keep ref counter large enough to
 		// avoid premature cleanup and ensure proper dialog referencing
-	    store_dlg_in_tm( req, t, dlg);
+	    if (store_dlg_in_tm( req, t, dlg) < 0) {
+			LM_ERR("failed to store dialog in transaction\n");
+			goto error;
+		}
 	}
 	else
 	{
@@ -896,14 +865,15 @@ finish:
 		// request being forwarded statefully
         if ( d_tmb.register_tmcb( req, NULL, TMCB_REQUEST_FWDED,
                 store_dlg_in_tm_cb, (void*)dlg, NULL)<0 ) {
-            LM_ERR("failed to store dialog in transaction during dialog creation for later reference\n");
+            LM_ERR("failed to register callback for storing dialog in transaction\n");
+			goto error;
         }
 	}
 
 	return 0;
 error:
-	unref_dlg(dlg,1);
-	profile_cleanup(req, 0, NULL);
+	unref_dlg(dlg,1);                   // undo ref regarding linking
+	profile_cleanup(req, 0, NULL);      // undo ref regarding setting current dialog
 	return -1;
 }
 
@@ -1001,7 +971,6 @@ void dlg_onroute(struct sip_msg* req, str *route_params, void *param)
 	str val, callid, ftag, ttag;
 	int h_entry, h_id, new_state, old_state, unref, event, timeout;
 	unsigned int dir;
-	unsigned int del;
 	int ret = 0;
 
 	if (current_dlg_pointer!=NULL)
@@ -1030,11 +999,7 @@ void dlg_onroute(struct sip_msg* req, str *route_params, void *param)
 			if ( parse_dlg_rr_param( val.s, val.s+val.len, &h_entry, &h_id)<0 )
 				return;
 
-			dlg = lookup_dlg( h_entry, h_id, &del);
-			if (del == 1) {
-				LM_DBG("dialog marked for deletion, ignoring\n");
-				return;
-			}
+			dlg = lookup_dlg( h_entry, h_id);
 			if (dlg==0) {
 				LM_WARN("unable to find dialog for %.*s "
 					"with route param '%.*s' [%u:%u]\n",
@@ -1044,8 +1009,8 @@ void dlg_onroute(struct sip_msg* req, str *route_params, void *param)
 				if (seq_match_mode==SEQ_MATCH_STRICT_ID )
 					return;
 			} else {
-				// lookup_dlg has incremented the ref count by 1
 				if (pre_match_parse( req, &callid, &ftag, &ttag, 1)<0) {
+					// lookup_dlg has incremented the ref count by 1
 					unref_dlg(dlg, 1);
 					return;
 				}
@@ -1064,6 +1029,7 @@ void dlg_onroute(struct sip_msg* req, str *route_params, void *param)
 							dlg->tag[DLG_CALLER_LEG].len,
 							dlg->tag[DLG_CALLEE_LEG].len, dlg->tag[DLG_CALLEE_LEG].s,
 							dlg->tag[DLG_CALLEE_LEG].len);
+					// lookup_dlg has incremented the ref count by 1
 					unref_dlg(dlg, 1);
 
 					// Reset variables in order to do a lookup based on SIP-Elements.
@@ -1082,11 +1048,7 @@ void dlg_onroute(struct sip_msg* req, str *route_params, void *param)
 			return;
 		/* TODO - try to use the RR dir detection to speed up here the
 		 * search -bogdan */
-		dlg = get_dlg(&callid, &ftag, &ttag, &dir, &del);
-		if (del == 1) {
-			LM_DBG("dialog marked for deletion, ignoring\n");
-			return;
-		}
+		dlg = get_dlg(&callid, &ftag, &ttag, &dir);
 		if (!dlg){
 			LM_DBG("Callid '%.*s' not found\n",
 				req->callid->body.len, req->callid->body.s);
@@ -1094,7 +1056,7 @@ void dlg_onroute(struct sip_msg* req, str *route_params, void *param)
 		}
 	}
 
-    /* set current dialog - it will keep a ref! */
+    /* set current dialog - re-use ref increment from dlg_get() above */
     set_current_dialog( req, dlg);
     _dlg_ctx.dlg = dlg;
 
@@ -1103,12 +1065,6 @@ void dlg_onroute(struct sip_msg* req, str *route_params, void *param)
         LM_ERR("failed to store dialog in transaction during dialog creation for later reference\n");
     }
 
-    if (del == 1) {
-        LM_DBG( "Use the dialog (callid: '%.*s') without further handling because it is marked for deletion\n",
-            callid.len, callid.s);
-        return;
-    }
-
 	/* run state machine */
 	switch ( req->first_line.u.request.method_value ) {
 		case METHOD_PRACK:

modules_k/dialog/dlg_hash.c

@@ -71,6 +71,49 @@
 struct dlg_table *d_table = 0;
 
 
+/*!
+ * \brief Reference a dialog without locking
+ * \param _dlg dialog
+ * \param _cnt increment for the reference counter
+ */
+#define ref_dlg_unsafe(_dlg,_cnt)     \
+	do { \
+		(_dlg)->ref += (_cnt); \
+		LM_DBG("ref dlg %p with %d -> %d\n", \
+			(_dlg),(_cnt),(_dlg)->ref); \
+	}while(0)
+
+
+/*!
+ * \brief Unreference a dialog without locking
+ * \param _dlg dialog
+ * \param _cnt decrement for the reference counter
+ * \param _d_entry dialog entry
+ */
+#define unref_dlg_unsafe(_dlg,_cnt,_d_entry)   \
+	do { \
+		(_dlg)->ref -= (_cnt); \
+		LM_DBG("unref dlg %p with %d -> %d\n",\
+			(_dlg),(_cnt),(_dlg)->ref);\
+		if ((_dlg)->ref<0) {\
+			LM_CRIT("bogus ref %d with cnt %d for dlg %p [%u:%u] "\
+				"with clid '%.*s' and tags '%.*s' '%.*s'\n",\
+				(_dlg)->ref, _cnt, _dlg,\
+				(_dlg)->h_entry, (_dlg)->h_id,\
+				(_dlg)->callid.len, (_dlg)->callid.s,\
+				(_dlg)->tag[DLG_CALLER_LEG].len,\
+				(_dlg)->tag[DLG_CALLER_LEG].s,\
+				(_dlg)->tag[DLG_CALLEE_LEG].len,\
+				(_dlg)->tag[DLG_CALLEE_LEG].s); \
+		}\
+		if ((_dlg)->ref<=0) { \
+			unlink_unsafe_dlg( _d_entry, _dlg);\
+			LM_DBG("ref <=0 for dialog %p\n",_dlg);\
+			destroy_dlg(_dlg);\
+		}\
+	}while(0)
+
+
 /*!
  * \brief Initialize the global dialog table
  * \param size size of the table
@@ -387,19 +430,18 @@ error:
 
 /*!
  * \brief Lookup a dialog in the global list
+ *
+ * Note that the caller is responsible for decrementing (or reusing)
+ * the reference counter by one again iff a dialog has been found.
  * \param h_entry number of the hash table entry
  * \param h_id id of the hash table entry
- * \param del will set to 1 if dialog is deleted
- * \return dialog structure on success, NULL on failure or if not found
+ * \return dialog structure on success, NULL on failure
  */
-struct dlg_cell* lookup_dlg( unsigned int h_entry, unsigned int h_id, unsigned int *del)
+struct dlg_cell* lookup_dlg( unsigned int h_entry, unsigned int h_id)
 {
 	struct dlg_cell *dlg;
 	struct dlg_entry *d_entry;
 
-	if (del != NULL)
-		*del = 0;
-
 	if (h_entry>=d_table->size)
 		goto not_found;
 
@@ -409,14 +451,7 @@ struct dlg_cell* lookup_dlg( unsigned int h_entry, unsigned int h_id, unsigned i
 
 	for( dlg=d_entry->first ; dlg ; dlg=dlg->next ) {
 		if (dlg->h_id == h_id) {
-			if (dlg->state==DLG_STATE_DELETED) {
-				if (del != NULL)
-					*del = 1;
-				dlg_unlock( d_table, d_entry);
-				goto not_found;
-			}
-			dlg->ref++;
-			LM_DBG("ref dlg %p with 1 -> %d\n", dlg, dlg->ref);
+			ref_dlg_unsafe(dlg, 1);
 			dlg_unlock( d_table, d_entry);
 			LM_DBG("dialog id=%u found on entry %u\n", h_id, h_entry);
 			return dlg;
@@ -438,18 +473,14 @@ not_found:
  * \param ttag to tag
  * \param dir direction
  * \param del will set to 1 if dialog is deleted
- * \return dialog structure on success, NULL on failure or if not found
+ * \return dialog structure on success, NULL on failure
  */
 static inline struct dlg_cell* internal_get_dlg(unsigned int h_entry,
-						str *callid, str *ftag, str *ttag, unsigned int *dir,
-						unsigned int *del)
+						str *callid, str *ftag, str *ttag, unsigned int *dir)
 {
 	struct dlg_cell *dlg;
 	struct dlg_entry *d_entry;
 
-	if (del != NULL)
-		*del = 0;
-
 	d_entry = &(d_table->entries[h_entry]);
 
 	dlg_lock( d_table, d_entry);
@@ -457,14 +488,7 @@ static inline struct dlg_cell* internal_get_dlg(unsigned int h_entry,
 	for( dlg = d_entry->first ; dlg ; dlg = dlg->next ) {
 		/* Check callid / fromtag / totag */
 		if (match_dialog( dlg, callid, ftag, ttag, dir)==1) {
-			if (dlg->state==DLG_STATE_DELETED) {
-				if (del != NULL)
-					*del = 1;
-				dlg_unlock( d_table, d_entry);
-				goto not_found;
-			}
-			dlg->ref++;
-			LM_DBG("ref dlg %p with 1 -> %d\n", dlg, dlg->ref);
+			ref_dlg_unsafe(dlg, 1);
 			dlg_unlock( d_table, d_entry);
 			LM_DBG("dialog callid='%.*s' found\n on entry %u, dir=%d\n",
 				callid->len, callid->s,h_entry,*dir);
@@ -473,8 +497,6 @@ static inline struct dlg_cell* internal_get_dlg(unsigned int h_entry,
 	}
 
 	dlg_unlock( d_table, d_entry);
-
-not_found:
 	LM_DBG("no dialog callid='%.*s' found\n", callid->len, callid->s);
 	return 0;
 }
@@ -489,22 +511,22 @@ not_found:
  * "The combination of the To tag, From tag, and Call-ID completely  
  * defines a peer-to-peer SIP relationship between [two UAs] and is 
  * referred to as a dialog."
+ * Note that the caller is responsible for decrementing (or reusing)
+ * the reference counter by one again iff a dialog has been found.
  * \param callid callid
  * \param ftag from tag
  * \param ttag to tag
  * \param dir direction
- * \param del deleted dialog information
  * \return dialog structure on success, NULL on failure
  */
-struct dlg_cell* get_dlg( str *callid, str *ftag, str *ttag, unsigned int *dir,
-		unsigned int *del)
+struct dlg_cell* get_dlg( str *callid, str *ftag, str *ttag, unsigned int *dir)
 {
 	struct dlg_cell *dlg;
 
 	if ((dlg = internal_get_dlg(core_hash(callid, 0,
-			d_table->size), callid, ftag, ttag, dir, del)) == 0 &&
+			d_table->size), callid, ftag, ttag, dir)) == 0 &&
 			(dlg = internal_get_dlg(core_hash(callid, ttag->len
-			?ttag:0, d_table->size), callid, ftag, ttag, dir, del)) == 0) {
+			?ttag:0, d_table->size), callid, ftag, ttag, dir)) == 0) {
 		LM_DBG("no dialog callid='%.*s' found\n", callid->len, callid->s);
 		return 0;
 	}
@@ -534,58 +556,13 @@ void link_dlg(struct dlg_cell *dlg, int n)
 		d_entry->last = dlg;
 	}
 
-	dlg->ref += 1 + n;
-
-	LM_DBG("ref dlg %p with %d -> %d\n", dlg, n+1, dlg->ref);
+	ref_dlg_unsafe(dlg, 1+n);
 
 	dlg_unlock( d_table, d_entry);
 	return;
 }
 
 
-/*!
- * \brief Reference a dialog without locking
- * \param _dlg dialog
- * \param _cnt increment for the reference counter
- */
-#define ref_dlg_unsafe(_dlg,_cnt)     \
-	do { \
-		(_dlg)->ref += (_cnt); \
-		LM_DBG("ref dlg %p with %d -> %d\n", \
-			(_dlg),(_cnt),(_dlg)->ref); \
-	}while(0)
-
-
-/*!
- * \brief Unreference a dialog without locking
- * \param _dlg dialog
- * \param _cnt decrement for the reference counter
- * \param _d_entry dialog entry
- */
-#define unref_dlg_unsafe(_dlg,_cnt,_d_entry)   \
-	do { \
-		(_dlg)->ref -= (_cnt); \
-		LM_DBG("unref dlg %p with %d -> %d\n",\
-			(_dlg),(_cnt),(_dlg)->ref);\
-		if ((_dlg)->ref<0) {\
-			LM_CRIT("bogus ref %d with cnt %d for dlg %p [%u:%u] "\
-				"with clid '%.*s' and tags '%.*s' '%.*s'\n",\
-				(_dlg)->ref, _cnt, _dlg,\
-				(_dlg)->h_entry, (_dlg)->h_id,\
-				(_dlg)->callid.len, (_dlg)->callid.s,\
-				(_dlg)->tag[DLG_CALLER_LEG].len,\
-				(_dlg)->tag[DLG_CALLER_LEG].s,\
-				(_dlg)->tag[DLG_CALLEE_LEG].len,\
-				(_dlg)->tag[DLG_CALLEE_LEG].s); \
-		}\
-		if ((_dlg)->ref<=0) { \
-			unlink_unsafe_dlg( _d_entry, _dlg);\
-			LM_DBG("ref <=0 for dialog %p\n",_dlg);\
-			destroy_dlg(_dlg);\
-		}\
-	}while(0)
-
-
 /*!
  * \brief Refefence a dialog with locking
  * \see ref_dlg_unsafe

modules_k/dialog/dlg_hash.h

@@ -253,12 +253,14 @@ int dlg_set_toroute(struct dlg_cell *dlg, str *route);
 
 /*!
  * \brief Lookup a dialog in the global list
+ *
+ * Note that the caller is responsible for decrementing (or reusing)
+ * the reference counter by one again iff a dialog has been found.
  * \param h_entry number of the hash table entry
  * \param h_id id of the hash table entry
- * \param del will set to 1 if dialog is deleted
- * \return dialog structure on success, NULL on failure or if not found
+ * \return dialog structure on success, NULL on failure
  */
-struct dlg_cell* lookup_dlg( unsigned int h_entry, unsigned int h_id, unsigned int *del);
+struct dlg_cell* lookup_dlg( unsigned int h_entry, unsigned int h_id);
 
 
 /*!
@@ -269,14 +271,15 @@ struct dlg_cell* lookup_dlg( unsigned int h_entry, unsigned int h_id, unsigned i
  * "The combination of the To tag, From tag, and Call-ID completely  
  * defines a peer-to-peer SIP relationship between [two UAs] and is 
  * referred to as a dialog."
+ * Note that the caller is responsible for decrementing (or reusing)
+ * the reference counter by one again iff a dialog has been found.
  * \param callid callid
  * \param ftag from tag
  * \param ttag to tag
  * \param dir direction
- * \param del unless null, flag that is set if dialog is in "deleted" state
  * \return dialog structure on success, NULL on failure
  */
-struct dlg_cell* get_dlg(str *callid, str *ftag, str *ttag, unsigned int *dir, unsigned int *del);
+struct dlg_cell* get_dlg(str *callid, str *ftag, str *ttag, unsigned int *dir);
 
 
 /*!

modules_k/dialog/dlg_profile.c

@@ -465,6 +465,9 @@ void set_current_dialog(struct sip_msg *msg, struct dlg_cell *dlg)
 	}
 	current_pending_linkers = NULL;
 	current_dlg_pointer = dlg;
+
+	/* do not increase reference counter here, let caller handle it
+	 * (yes, this is somewhat ugly) */
 }
 
 

modules_k/dialog/dlg_req_within.c

@@ -318,7 +318,7 @@ struct mi_root * mi_terminate_dlg(struct mi_root *cmd_tree, void *param ){
 
 	LM_DBG("h_entry %u h_id %u\n", h_entry, h_id);
 
-	dlg = lookup_dlg(h_entry, h_id, NULL);
+	dlg = lookup_dlg(h_entry, h_id);
 
 	// lookup_dlg has incremented the reference count