@henningw commented on this pull request.

Thanks for the PR. This is a smaller patch, so I just added a few comments, mostly related to formatting. Before merging the module documentation should be extended as well with the new function, its return code and description from the PR.

In src/modules/ims_registrar_pcscf/ims_registrar_pcscf_mod.c:

> @@ -142,15 +143,15 @@ inline void pcscf_act_time()
  * Exported functions
  */
 static cmd_export_t cmds[] = {
-	{"pcscf_save",     		(cmd_function)w_save,                   1,  	save_fixup2,            0,ONREPLY_ROUTE },
+	{"pcscf_save",     		        (cmd_function)w_save,                   1,  	save_fixup2,            0,ONREPLY_ROUTE },

Please align the formatting to the existing code, there seems to be some whitespaces/tabs here and also two more places below.

In src/modules/ims_registrar_pcscf/ims_registrar_pcscf_mod.c:

> @@ -261,6 +262,8 @@ static int mod_init(void) {
 	if (bind_usrloc(&ul) < 0) {
 		return -1;
 	}
+	if (ul.db_mode == DB_ONLY)
+            ul.register_ulcb_method(NULL, PCSCF_CONTACT_UPDATE, callback_pcscf_contact_cb, NULL);

This can fail according to ims_usrloc_pcscf/ul_callback.c. So the return value should be checked and aborted on error, in a similar ways as the other cases above.

In src/modules/ims_registrar_pcscf/save.c:

> @@ -467,6 +484,20 @@ int save(struct sip_msg* _m, udomain_t* _d, int _cflags) {
 		goto error;
 	}
 
+        // skip subscribe, if all contacts have "expires=0" parameter
+	contact_t *ctct;

Move the contact and also expir parameter definition to the beginning of the function. Not sure why the cast to unsigned int * is needed here? The if (expir) condition also need to be aligned from the formatting.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.