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-06T06:01:59Z

If an incoming INVITE is rejected by an event handler, then no further event handler should process the event. My proposition is to return true if the event processing should be stopped. We could also add a stop flag to struct bevent.

Isn't it possible to return a bool from a Kotlin callback function?

cspiel1 · 2024-05-06T06:27:11Z

Instead of the return value I put the stop flag to struct bevent. Now there are the options:

setting bevent.err to an error code -> stops event processing with an error,
setting bevent.stop=true -> stop event processing without an error,
by default the next event handler is called. This removes also the need for return false; at multiple locations.

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

Christian Spielberger writes:

If an incoming INVITE is rejected by an event handler, then no further event handler should process the event. My proposition is to return true if the event processing should be stopped. We could also add a stop flag to `struct bevent`.

That doesn't change the situation. If event processing should be stopped (for example when the handler rejects the INVITE due to dnd), the state will be CALL_STATE_TERMINATED. Why checking of the state is not enough? Why do you need a return value or stopped flag, which in my opinion is bad design and breaks the idea of handler? After short search I have not found in baresip any other handler that returns a value or has stopped flag.

Isn't it possible to return a bool from a Kotlin callback function?

Yes it could be possible, but see above.

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 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 2 times, most recently from 559faa7 to f129576 Compare April 30, 2024 11:09

core: add UA info API

7915e5a

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

cspiel1 added 7 commits April 30, 2024 13:23

event: backward compatible event class name

34101b9

event: add info_encode_dict()

5cbf8bd

event: avoid dangling-pointer warning

e8d7fd7

call,event: send first call info/event

4e56c88

core: rename info -> event

c5e91eb

ua, menu: some replacements UA event --> new event API

857e2d2

event: add UA_EVENT_SIP_INCOMING

3626601

ua,menu: add Dnd as example for new event API

881af62

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

cspiel1 added 3 commits April 30, 2024 16:49

api: event_get_msg returns const

76b7cf8

event: fix emit event

b0c5436

test: use new event API

1a22bf6

sreimers reviewed May 1, 2024

View reviewed changes

src/event.c Outdated Show resolved Hide resolved

event: add void *arg to application event

bad55c4

cspiel1 force-pushed the event_refactoring branch from ef92d04 to 2affce8 Compare May 2, 2024 07:18

cspiel1 added 2 commits May 2, 2024 09:20

event: remove redudant parameter of static event_emit()

e6c7e71

event: add event_module_emit() for discussion

9542c48

event: put stop flag to bevent instead of return value

a363273

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

event: add doxygen

82049cd

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

event: lint fix

5fd2087

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

ua: move emit of SIPSESS_CONN event after SIP checks

722e7ea

alfredh reviewed May 17, 2024

View reviewed changes

src/event.c

} 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

cspiel1 added 2 commits May 17, 2024 11:47

event: rename file event.c to bevent.c

53d9c1b

bevent: add enum ev to event handler

8ce80f0

alfredh reviewed May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

event refactoring #3018

event refactoring #3018

cspiel1 commented Apr 29, 2024 •

edited

sreimers Apr 29, 2024

juha-h Apr 30, 2024

cspiel1 Apr 30, 2024

cspiel1 commented Apr 30, 2024

cspiel1 commented Apr 30, 2024 •

edited

juha-h commented May 1, 2024

sreimers May 1, 2024

cspiel1 May 2, 2024 •

edited

cspiel1 commented May 2, 2024 •

edited

cspiel1 commented May 6, 2024

cspiel1 commented May 6, 2024 •

edited

juha-h commented May 6, 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

alfredh May 17, 2024

cspiel1 May 17, 2024

cspiel1 May 17, 2024

alfredh May 17, 2024

cspiel1 commented May 21, 2024

alfredh commented May 24, 2024

alfredh May 24, 2024

alfredh May 24, 2024

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

event refactoring #3018

Are you sure you want to change the base?

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 6, 2024

cspiel1 commented May 6, 2024 • edited

juha-h commented May 6, 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 Apr 29, 2024 •

edited

cspiel1 commented Apr 30, 2024 •

edited

cspiel1 May 2, 2024 •

edited

cspiel1 commented May 2, 2024 •

edited

cspiel1 commented May 6, 2024 •

edited

juha-h commented May 6, 2024 •

edited

juha-h commented May 6, 2024 •

edited