[sr-dev] [kamailio/kamailio] async: added support for millisecond resolution sleep (#2016)

[Henning Westerholt]

henningw commented on this pull request.

Thank you for the pull request! Generally all looks fine. I've had a few questions regarding the memory management in the error cases (I only looked through the code, did not tested it).

> +			sizeof(struct async_ms_list));
+	if(_async_ms_list == NULL) {
+		LM_ERR("no more shm\n");
+		return -1;
+	}
+	memset(_async_ms_list, 0, sizeof(struct async_ms_list));
+	if(lock_init(&_async_ms_list->lock) == 0) {
+		LM_ERR("cannot init lock \n");
+		shm_free(_async_ms_list);
+		_async_ms_list = 0;
+		return -1;
+	}
+	return 0;
+}
+
+int async_destroy_ms_timer_list(void)

It is not necessary to shm_free the allocated _async_ms_list as well here?

> +		return -1;
+	}
+	dsize = sizeof(async_task_t) + sizeof(async_task_param_t) + sizeof(async_ms_item_t);
+
+	at = (async_task_t *)shm_malloc(dsize);
+	if(at == NULL) {
+		LM_ERR("no more shm memory\n");
+		return -1;
+	}
+	memset(at, 0, dsize);
+	at->param = (char *)at + sizeof(async_task_t);
+	atp = (async_task_param_t *)at->param;
+	ai = (async_ms_item_t *) ((char *)at +  sizeof(async_task_t) + sizeof(async_task_param_t));
+	ai->at = at;
+
+	if(cbname && cbname->len>=ASYNC_CBNAME_SIZE-1) {

Is here a shm_free for "at" missing?

> +		return -1;
+	}
+	memset(at, 0, dsize);
+	at->param = (char *)at + sizeof(async_task_t);
+	atp = (async_task_param_t *)at->param;
+	ai = (async_ms_item_t *) ((char *)at +  sizeof(async_task_t) + sizeof(async_task_param_t));
+	ai->at = at;
+
+	if(cbname && cbname->len>=ASYNC_CBNAME_SIZE-1) {
+		LM_ERR("callback name is too long: %.*s\n", cbname->len, cbname->s);
+		return -1;
+	}
+
+	t = tmb.t_gett();
+	if(t == NULL || t == T_UNDEFINED) {
+		if(tmb.t_newtran(msg) < 0) {

Same as above

> +	ai = (async_ms_item_t *) ((char *)at +  sizeof(async_task_t) + sizeof(async_task_param_t));
+	ai->at = at;
+
+	if(cbname && cbname->len>=ASYNC_CBNAME_SIZE-1) {
+		LM_ERR("callback name is too long: %.*s\n", cbname->len, cbname->s);
+		return -1;
+	}
+
+	t = tmb.t_gett();
+	if(t == NULL || t == T_UNDEFINED) {
+		if(tmb.t_newtran(msg) < 0) {
+			LM_ERR("cannot create the transaction\n");
+			return -1;
+		}
+		t = tmb.t_gett();
+		if(t == NULL || t == T_UNDEFINED) {

Same as above

> +	t = tmb.t_gett();
+	if(t == NULL || t == T_UNDEFINED) {
+		if(tmb.t_newtran(msg) < 0) {
+			LM_ERR("cannot create the transaction\n");
+			return -1;
+		}
+		t = tmb.t_gett();
+		if(t == NULL || t == T_UNDEFINED) {
+			LM_ERR("cannot lookup the transaction\n");
+			return -1;
+		}
+	}
+	
+	if(tmb.t_suspend(msg, &tindex, &tlabel) < 0) {
+		LM_ERR("failed to suspend the processing\n");
+		shm_free(ai);

You free here the "ai" structure - where is this memory actually allocated? And again, what about "at"?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2016#pullrequestreview-267499810
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20190728/0dc09319/attachment.html>