event refactoring #3018

cspiel1 · 2024-04-29T15:43:06Z

Idea got from:

core: move DnD feature to menu #3011 (comment)
Initially from core: move DnD feature to menu #3011 (comment)

This PR involved now to a backwards compatible solution. Old and new event API exist in parallel.

For now only updated three modules.

include/baresip.h

sreimers · 2024-04-29T18:55:44Z

src/event.c

@@ -333,29 +424,27 @@ void uag_event_unregister(ua_event_h *h)
 * @param fmt  Formatted arguments
 * @param ...  Variable arguments
 */
-void ua_event(struct ua *ua, enum ua_event ev, struct call *call,
-	      const char *fmt, ...)
+void ua_event(enum ua_event ev, const char *fmt, ...)


Unsure if mixing formatting args and optional args is a good idea. It's very confusing.

SInce this is ua_event, I would prefer to keep struct ua *ua param and add event_param (without struct ua *ua; in union).

Yes.

And maybe we should not directly drop the current API.

I'll try a multi function solution for emitting events:

/** * @deprecated */ void ua_event(struct ua *ua, enum ua_event ev, struct call *call, const char *fmt, ...); void ua_event_app(enum ua_event ev, const char *fmt, ...); void ua_event_ua(enum ua_event ev, struct ua *ua, const char *fmt, ...); void ua_event_call(enum ua_event ev, struct call *call, const char *fmt, ...); void ua_event_sip_msg(enum ua_event ev, struct sip_msg *msg, const char *fmt, ...);

This could be extended without breaking the API.

cspiel1 · 2024-04-30T11:03:44Z

I completely replaced the commits

cspiel1 · 2024-04-30T14:39:11Z

This commit shows how DnD could be solved: 881af62

juha-h · 2024-05-01T05:36:07Z

Still don't understand why new event (UA_EVENT_SIP_INCOMING) is needed. Also, better name would be UA_EVENT_INVITE_INCOMING.

Now when I read ua.c code, I noticed that call blocking based on contacts does not belong to core either and should be moved to menu.

sreimers · 2024-05-01T06:15:29Z

src/event.c

+	va_list ap;
+	int err;
+
+	if (event_class(ev) != EVENT_CLASS_CALL)


Is EVENT_CLASS_CALL right here?

Good point!

if (event_class(ev) != EVENT_CLASS_SIP) return EINVAL;

is correct.

Explanation

@juha-h
The event class selects the union field.

event class union field

EVENT_CLASS_UA event.u.ua

EVENT_CLASS_CALL event.u.call

EVENT_CLASS_APP event.nothing

EVENT_CLASS_SIP event.u.msg

Problem

The function ua_event_class_name() returns some incongruous class names

register, better would be ua

mwi --> better ua

UA_EVENT_CREATE returns application --> better ua

VU_REPORT --> better call

But maybe there are already applications which event parsing rely on the current texts. Thus I didn't touch this function. The function is used in event_encode_dict() which I would mark as deprecated.

The new function odict_encode_event() uses event_class_name() that returns logical fitting class names. This should also make parsing of events simpler for applications.

src/event.c

cspiel1 · 2024-05-02T07:11:32Z

Module events

The commit 9542c48 should be discussed, and maybe dropped.

A replacement for function module_event() could be event_module_emit().
Does an extra API function event_module_emit for Module events really have enough benefit for the extra code?

	/* new module event */
	event_module_emit("ctrl_dbus", modev->event, NULL, "%s", modev->txt);
	/* or we use the other event functions, like */
	event_app_emit(UA_EVENT_MODULE, NULL, "ctrl_dbus,%s,%s",
		       modev->event, modev->txt);

Note: event_module_emit() currently untested

cspiel1 · 2024-05-02T12:28:19Z

The tests are green now.

Now everything is backwards compatible, the old functions are marked as deprecated and a warning is printed if they are used.

The unit tests already use the new event handler.

In order to keep the PR as small as possible I will

remove the commit that adds the DnD example,
remove the new module event API mentioned here event refactoring #3018 (comment)

juha-h · 2024-05-02T12:43:41Z

Regarding this:

err = event_sip_msg_emit(UA_EVENT_SIP_INCOMING, msg, "");

I would prefer names event_sip_req_emit and UA_EVENT_SIP_REQ_INCOMING.

alfredh · 2024-05-02T12:53:17Z

include/baresip.h

+		struct call *call;
+		const struct sip_msg *msg;
+		void *arg;
+	} u;


union means that only one of the pointers are valid, for example ua OR call.
you cannot cast a ua pointer to a call pointer, or msg pointer etc.

is it better to use a struct here ?

most of the events have a valid UA pointer

I also thought about the choice between union and struct. And decided to try first a union. Reasons for that:

In the current UA event API we often pass a call and its ua which is redundant.

Then we had often UA events where ua was set, but call was NULL.

The new events which add a sip_msg pointer should be used before a call is allocated.

It seems to work with a union. An application would have no benefit to have multiple of these pointers.

alfredh · 2024-05-02T12:55:48Z

include/baresip.h

+int event_ua_emit(enum ua_event ev, struct ua *ua, const char *fmt, ...);
+int event_call_emit(enum ua_event ev, struct call *call, const char *fmt, ...);
+int event_sip_msg_emit(enum ua_event ev, const struct sip_msg *msg,
+		       const char *fmt, ...);


is the call valid in this case ?

Yes. Here the table from above extended with the function names:

function name event class union field

event_ua_emit EVENT_CLASS_UA event.u.ua

event_call_emit EVENT_CLASS_CALL event.u.call

event_app_emit EVENT_CLASS_APP event.u.arg

event_sip_msg_emit EVENT_CLASS_SIP event.u.msg

cspiel1 · 2024-05-02T12:56:22Z

Regarding this:
err = event_sip_msg_emit(UA_EVENT_SIP_INCOMING, msg, "");
I would prefer names event_sip_req_emit and UA_EVENT_SIP_REQ_INCOMING.

It is more the type of the second parameter we want to address with the name. If we had C++ it would be best like this:

int event_emit(enum ua_event ev, void *arg, const char *fmt, ...);
int event_emit(enum ua_event ev, struct ua *ua, const char *fmt, ...);
int event_emit(enum ua_event ev, struct call *call, const char *fmt, ...);
int event_emit(enum ua_event ev, const struct sip_msg *msg, const char *fmt, ...);

And it is not only for incoming SIP requests.
event_sip_msg_emit() could also be used for SIP replies, for every event for which we want to add a SIP message.

For the event I tried to keep it short with UA_EVENT_SIP_INCOMING. We could also name it UA_EVENT_SIPMSG_INCOMING.

alfredh · 2024-05-02T12:57:41Z

src/ua.c

-	if (uag_dnd()) {
-		(void)sip_treply(NULL, uag_sip(), msg,
-			480,"Temporarily Unavailable");
+	err = event_sip_msg_emit(UA_EVENT_SIP_INCOMING, msg, "");


UA_EVENT_SIPSESS_CONN

alfredh · 2024-05-02T13:01:09Z

include/baresip.h


 	UA_EVENT_MAX,
 };

+
+struct event {


perhaps add a one or two letter prefix to "event", like xxevent.

okay, this time it should more general than ua. Maybe bs for baresip? --> bsevent?

Or bevent?

cspiel1 · 2024-05-02T13:21:06Z

Currently there are a lot of deprecated warnings when the unit tests are ran. Thus before I'll set this PR green, I will remove the calls to the deprecated API in the baresip core.

juha-h · 2024-05-02T13:53:20Z

For the event I tried to keep it short with UA_EVENT_SIP_INCOMING. We could also name it UA_EVENT_SIPMSG_INCOMING.

OK, but I would prefer the same as in the event_sip_msg_emit, i.e., UA_EVENT_SIP_MSG_INCOMING.

cspiel1 · 2024-05-02T13:59:44Z

For the event I tried to keep it short with UA_EVENT_SIP_INCOMING. We could also name it UA_EVENT_SIPMSG_INCOMING.

OK, but I would prefer the same as in the event_sip_msg_emit, i.e., UA_EVENT_SIP_MSG_INCOMING.

The "emit" function names are related to event classes and to the union fields.
The event types (which still are enum ua_event) are grouped into event classes. @alfredh already suggested to name the new event UA_EVENT_SIPSESS_CONN which makes sense, because it is emitted in the sipsess connection handler. This name is both, perfectly specific and general enough. The event class is EVENT_CLASS_SIP.

juha-h · 2024-05-02T14:00:51Z

static bool event_handler(struct event *event, void *arg)
...
case UA_EVENT_CALL_INCOMING:

		if (call_state(call) != CALL_STATE_INCOMING)
			return false;

In my opinion event_handler should return false only in case of error. Otherwise it simply handles the event and returns true. As result of the handling, e.g., call_state, can change.

cspiel1 · 2024-05-02T14:11:05Z

Please compare with list_apply() implementation! It also stops the loop when the handler returns true.
true means stop processing.

We don't have a call object where event_sip_msg_emit(), event_ua_emit() or event_app_emit() is called. It should become a general purpose event API.

The err code of the "emit" function can be used to have influence on further processing after the event was emitted and processed by the handler list. As an example see Dnd commit: 881af62

juha-h · 2024-05-02T14:19:51Z

Christian Spielberger writes:

The err code of the "emit" function can be used to have influence on further processing after the event was emitted and processed by the handler list. As an example see Dnd commit: 881af62

My point is that the information regarding possible further processing should be carried by some other means than the boolean result, e.g., by call state change. So, for example, in case of DnD, after event processing, the call state could be either CALL_STATE_TERMINATED or CALL_STATE_RINGING.

juha-h · 2024-05-06T06:57:41Z

Why these are not done before bevent_sip_msg_emit(UA_EVENT_SIPSESS_CONN, msg, "") call?

        /* handle multiple calls */
	if (config->call.max_calls &&
	    uag_call_count() + 1 > config->call.max_calls) {

		info("ua: rejected call from %r (maximum %d calls)\n",
		     &msg->from.auri, config->call.max_calls);
		(void)sip_treply(NULL, uag_sip(), msg, 486, "Max Calls");
		return;
	}

	/* Handle Require: header, check for any required extensions */
	hdr = sip_msg_hdr_apply(msg, true, SIP_HDR_REQUIRE,
				require_handler, ua);
	if (hdr) {
		info("ua: call from %r rejected with 420"
			     " -- option-tag '%r' not supported\n",
			     &msg->from.auri, &hdr->val);

		(void)sip_treplyf(NULL, NULL, uag_sip(), msg, false,
				  420, "Bad Extension",
				  "Unsupported: %r\r\n"
				  "Content-Length: 0\r\n\r\n",
				  &hdr->val);
		return;
	}

	if (ua->acc->rel100_mode == REL100_REQUIRED &&
	    !(sip_msg_hdr_has_value(msg, SIP_HDR_SUPPORTED, "100rel") ||
	      sip_msg_hdr_has_value(msg, SIP_HDR_REQUIRE, "100rel"))) {

		info("ua: call from %r rejected with 421"
			     " -- option-tag '%s' not supported by remote\n",
			     &msg->from.auri, "100rel");
		(void)sip_treplyf(NULL, NULL, uag_sip(), msg, false,
				  421, "Extension required",
				  "Require: 100rel\r\n"
				  "Content-Length: 0\r\n\r\n");
		return;
	}

juha-h · 2024-05-06T07:10:05Z

Why can't there be ua_accept API function that would be called from the app? Then there would be no need to process anything in sipsess_conn_handler after bevent_sip_msg_emit(UA_EVENT_SIPSESS_CONN, msg, "");

juha-h · 2024-05-06T07:35:26Z

So when INVITE comes in, ipsess_conn_handler does the checks and (if they succees) sends UA_EVENT_SIPSESS_CONN event. When the app receives it, it calls ua_hangup, ua_answer (when auto answering), or ua_accept. That would be clean without any implicit actions.

juha-h · 2024-05-06T09:25:37Z

... but I'm ready to give up as long there API documentation is clear.

cspiel1 · 2024-05-06T10:04:56Z

I'll add Doxygen to the API

juha-h · 2024-05-06T13:19:50Z

Once you move these #3018 (comment), I can add new API function ua_accept and can always stop processing after UA_EVENT_SIPSESS_CONN event.

cspiel1 · 2024-05-06T13:27:00Z

Yes, this would be possible from App side.

cspiel1 · 2024-05-13T12:22:41Z

updated the description

juha-h · 2024-05-13T14:59:46Z

Could you move this

err = bevent_sip_msg_emit(UA_EVENT_SIPSESS_CONN, msg, "");
	if (err == ENOENT)
		return;

down after all checks?

cspiel1 · 2024-05-14T08:40:29Z

Yes, it can be moved to line 760.

Also in following PRs we could move the max_calls check to the application e.g. module menu. We could discuss this.

alfredh · 2024-05-17T07:48:25Z

include/baresip.h

@@ -881,6 +886,7 @@ enum answer_method {
 /** Defines the User-Agent event handler */
 typedef void (ua_event_h)(struct ua *ua, enum ua_event ev,
 			  struct call *call, const char *prm, void *arg);
+typedef void (bevent_h)(struct bevent *event, void *arg);


suggested signature:

typedef void (bevent_h)(struct ua_event ev, struct bevent_payload *event, void *arg);

All events must have a valid EV event set.

The payload is optional and type dependent.

I'll try this.

Do you mean?

typedef void (bevent_h)(enum ua_event ev, struct bevent_payload *event, void *arg);

Rename struct bevent -> struct bevent_payload?

alfredh · 2024-05-17T07:54:30Z

src/event.c

+	} u;
+};
+
+


perhaps rename this file to bevent.c ?

cspiel1 · 2024-05-21T08:09:23Z

@alfredh Now I changed to

typedef void (bevent_h)(enum ua_event ev, struct bevent *event, void *arg);

Can we keep the shorter name struct bevent?

All events must have a valid EV event set.
The payload is optional and type dependent.

The .._emit(enum ua_event ev, ...) functions ensure that a valid ev is given. Everything else is optional.

alfredh · 2024-05-24T08:19:30Z

I would like to suggest that we split this up into logical blocks (many PRs).

add core bevent
add backwards wrapper
update function calls
add new features like DND

For example a small PR can be to just rename event.c to bevent.c

Is there a need to update all the calls that are sending the event ?
It would be nice if the PR is small and easy to read.

alfredh · 2024-05-24T08:20:35Z

CMakeLists.txt

@@ -167,7 +167,7 @@ set(SRCS
  src/custom_hdrs.c
  src/descr.c
  src/dial_number.c
-  src/event.c
+  src/bevent.c


this can be a separate PR

alfredh · 2024-05-24T08:21:09Z

include/baresip.h

@@ -866,10 +866,15 @@ enum ua_event {
 	UA_EVENT_MODULE,
 	UA_EVENT_END_OF_FILE,
 	UA_EVENT_CUSTOM,
+	UA_EVENT_SIPSESS_CONN,


this change and related changes can be separate PR

cspiel1 · 2024-05-27T05:56:48Z

squashed the commits and pushed to https://github.com/cspiel1/baresip/tree/bevent_complete_refactoring as backup
will push force the first PR in order to keep the conversation here

This is the first commit of a series that introduce new baresip events.

cspiel1 mentioned this pull request Apr 29, 2024

core: move DnD feature to menu #3011

Closed

sreimers reviewed Apr 29, 2024

View reviewed changes

include/baresip.h Outdated Show resolved Hide resolved

sreimers reviewed Apr 29, 2024

View reviewed changes

cspiel1 force-pushed the event_refactoring branch from ee1fd6e to d355985 Compare April 30, 2024 11:02

cspiel1 force-pushed the event_refactoring branch 3 times, most recently from f129576 to 7915e5a Compare April 30, 2024 11:14

cspiel1 force-pushed the event_refactoring branch from 8b5c3b1 to 881af62 Compare April 30, 2024 14:40

sreimers reviewed May 1, 2024

View reviewed changes

src/event.c Outdated Show resolved Hide resolved

cspiel1 force-pushed the event_refactoring branch 2 times, most recently from 2affce8 to 9542c48 Compare May 2, 2024 07:21

cspiel1 force-pushed the event_refactoring branch from 7bdf35d to 2b04aa6 Compare May 2, 2024 12:29

alfredh reviewed May 2, 2024

View reviewed changes

cspiel1 force-pushed the event_refactoring branch 4 times, most recently from aae140c to 82049cd Compare May 6, 2024 10:26

cspiel1 added this to the v3.13.0 milestone May 13, 2024

alfredh reviewed May 17, 2024

View reviewed changes

src/event.c Outdated

} u;

};

Copy link

Collaborator

alfredh May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps rename this file to bevent.c ?

cspiel1 reacted with thumbs up emoji

alfredh reviewed May 24, 2024

View reviewed changes

cspiel1 marked this pull request as draft May 27, 2024 05:44

cspiel1 force-pushed the event_refactoring branch from 8ce80f0 to 6349946 Compare May 27, 2024 05:57

cspiel1 marked this pull request as ready for review May 27, 2024 05:58

event: rename event.c -> bevent.c

2c44d85

This is the first commit of a series that introduce new baresip events.

cspiel1 force-pushed the event_refactoring branch from 6349946 to 2c44d85 Compare May 27, 2024 06:02

alfredh removed this from the v3.13.0 milestone May 27, 2024

alfredh merged commit 5228c7b into baresip:main May 28, 2024
18 checks passed

cspiel1 deleted the event_refactoring branch May 28, 2024 12:49

event class	union field
EVENT_CLASS_UA	event.u.ua
EVENT_CLASS_CALL	event.u.call
EVENT_CLASS_APP	event.nothing
EVENT_CLASS_SIP	event.u.msg

function name	event class	union field
event_ua_emit	EVENT_CLASS_UA	event.u.ua
event_call_emit	EVENT_CLASS_CALL	event.u.call
event_app_emit	EVENT_CLASS_APP	event.u.arg
event_sip_msg_emit	EVENT_CLASS_SIP	event.u.msg

event refactoring #3018

event refactoring #3018

Conversation

cspiel1 commented Apr 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cspiel1 commented Apr 30, 2024

cspiel1 commented Apr 30, 2024 • edited

juha-h commented May 1, 2024

Choose a reason for hiding this comment

cspiel1 May 2, 2024 • edited

Choose a reason for hiding this comment

Explanation

Problem

cspiel1 commented May 2, 2024 • edited

Module events

cspiel1 commented May 2, 2024

juha-h commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cspiel1 commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cspiel1 commented May 2, 2024

juha-h commented May 2, 2024

cspiel1 commented May 2, 2024

juha-h commented May 2, 2024

cspiel1 commented May 2, 2024

juha-h commented May 2, 2024 via email

juha-h commented May 6, 2024

juha-h commented May 6, 2024 • edited

juha-h commented May 6, 2024 • edited

juha-h commented May 6, 2024

cspiel1 commented May 6, 2024

juha-h commented May 6, 2024

cspiel1 commented May 6, 2024

cspiel1 commented May 13, 2024

juha-h commented May 13, 2024

cspiel1 commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cspiel1 commented May 21, 2024

alfredh commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cspiel1 commented May 27, 2024 • edited

cspiel1 commented Apr 29, 2024 •

edited

cspiel1 commented Apr 30, 2024 •

edited

cspiel1 May 2, 2024 •

edited

cspiel1 commented May 2, 2024 •

edited

juha-h commented May 6, 2024 •

edited

juha-h commented May 6, 2024 •

edited

cspiel1 commented May 27, 2024 •

edited