add support for sd-event as event loop #1

defaultbranch · 2020-11-25T09:06:49Z

Der grösste teil vom Code steckt in sdevent.c.

Als Vorlage habe ich libuv.c und libev.c verwendet.

Es gibt noch ein README.md zum Einbinden von Event-Loop-Bibliotheken.

Chrome has started being able to issue frame type 0x42, we drop the connection before we realize we wanted to ignore it. This explicitly ignores it a bit earlier.

Change-Id: I81b67cbb28591a20842fc7d6e09b78375845aae4

langchr86

I miss one block of: https://github.com/defaultbranch/libwebsockets/blob/main/lib/event-libs/README.md

Enabling event lib adoption

You need to add a LWS_SERVER_OPTION... flag as necessary in ./lib/libwebsockets.h enum lws_context_options, and follow the existing code in lws_create_context() to convert the flag into binding your ops struct to the context.

langchr86 · 2020-11-26T07:57:35Z

CMakeLists.txt

@@ -152,6 +152,7 @@ option(LWS_WITH_LIBEV "Compile with support for libev" OFF)
 option(LWS_WITH_LIBUV "Compile with support for libuv" OFF)
 option(LWS_WITH_LIBEVENT "Compile with support for libevent" OFF)
 option(LWS_WITH_GLIB "Compile with support for glib event loop" OFF)
+option(LWS_WITH_SDEVENT "Compile with support for systemd event loop" OFF)


Suggested change

option(LWS_WITH_SDEVENT "Compile with support for systemd event loop" OFF)

option(LWS_WITH_SDEVENT "Compile with support for systemd event loop: sd-event" OFF)

not sure, I wanted to keep the wording aligned with libuv/libevent/glib above.
Is "systemd event loop" ambiguous? I thought sd-event is the only systemd event loop?

Yes it is the only one. I only think that we could easily miss the d in systemd and then think about raw Linux event loop or something like that. sd-event ist much more specific. Maybe we should use only Compile with support for sd-event instead of systemd event loop

Changed text to "Compile with support for sd-event"

langchr86 · 2020-11-26T07:57:58Z

include/libwebsockets/lws-context-vhost.h

@@ -230,6 +230,9 @@
 	/**< (CTX) Disable lws_system state, eg, because we are a secure streams
 	 * proxy client that is not trying to track system state by itself. */

+#define LWS_SERVER_OPTION_SDEVENT			 (1ll << 36)
+    /**< (CTX) Use systemd event loop */


Suggested change

/**< (CTX) Use systemd event loop */

/**< (CTX) Use systemd event loop: sd-event */

(similar to the previous comment)

Changed to "Use sd-event loop"

langchr86 · 2020-11-26T08:16:48Z

lib/event-libs/sdevent/sdevent.c

+#endif
+const lws_plugin_evlib_t evlib_sd = {
+        .hdr = {
+                "systemd event loop",


Suggested change

"systemd event loop",

"systemd event loop: sd-event",

(similar to the previous comment)

CMakeLists-implied-options.txt

langchr86 · 2020-11-26T08:36:34Z

CMakeLists-implied-options.txt

+if (LWS_WITH_SDEVENT)
+	set(LWS_WITH_SDEVENT 1)
+endif()


The conditional and the set variable are both the exact same. What is this check intended for?

I copied this, not sure whether its a bug (could be), or it is there to ensure that anything "defined" is an integer 1 while anything undefined remains undefined (similar to what https://stackoverflow.com/questions/14751973/what-is-in-c does)

Need to check...

I forwarded the question to warmcat#2130, waiting for an answer.

Removed. (The maintainers found this pointless as well.)

langchr86

Looks not too bad. But I did not completely understand everything. Maybe I will with the next review after you added more tests. So that the code can mature. Most things I noticed is the missing error handling at many points.

langchr86 · 2020-11-26T08:39:25Z

lib/event-libs/sdevent/CMakeLists.txt

+set(LIB_SYSTEMD_LIBRARIES CACHE PATH "Path to the libsystemd library")
+if ("${LWS_SYSTEMD_LIBRARIES}" STREQUAL "")
+    if (NOT LIB_SYSTEMD_FOUND)
+        message("searching for systemd")


I would remove this line because the find_library command will already print something similar.

langchr86 · 2020-11-26T08:41:36Z

lib/event-libs/sdevent/CMakeLists.txt

+
+if (LWS_WITH_EVLIB_PLUGINS)
+
+    create_evlib_plugin(evlib_sd sdevent.c ${LIBSYSTEMD_LIBRARIES})


Other implementations do also mention their private header here.

Suggested change

create_evlib_plugin(evlib_sd sdevent.c ${LIBSYSTEMD_LIBRARIES})

create_evlib_plugin(evlib_sd sdevent.c private-lib-event-libs-sdevent.h ${LIBSYSTEMD_LIBRARIES})

langchr86 · 2020-11-26T08:46:47Z

lib/event-libs/sdevent/sdevent.c

+struct lws_context_eventlibs_sdevent {
+};
+
+struct lws_pt_eventlibs_sdevent {
+    struct lws_context_per_thread *pt;
+    struct sd_event *io_loop;
+    struct sd_event_source *sultimer;
+    struct sd_event_source *idletimer;
+};
+
+struct lws_vh_eventlibs_sdevent {
+};
+
+struct lws_wsi_watcher_sdevent {
+    struct sd_event_source *source;
+    uint32_t events;
+};


Those should go into private-lib-event-libs-sdevent.h

langchr86 · 2020-11-26T08:47:53Z

lib/event-libs/sdevent/private-lib-event-libs-sdevent.h

@@ -0,0 +1,3 @@
+#include <private-lib-core.h>


Do we need to add the licence/copyright header in each of these source files?

langchr86 · 2020-11-26T09:04:22Z

lib/event-libs/sdevent/sdevent.c

+            sd_event_source_unref(ptpriv->sultimer);
+            ptpriv->sultimer = NULL;


Suggested change

sd_event_source_unref(ptpriv->sultimer);

ptpriv->sultimer = NULL;

ptpriv->sultimer = sd_event_source_unref(ptpriv->sultimer);

langchr86 · 2020-11-26T09:23:21Z

lib/event-libs/sdevent/sdevent.c

+        if (!(wsi_to_priv_sd(wsi)->events & (EPOLLIN | EPOLLOUT)))
+            sd_event_source_set_enabled(wsi_to_priv_sd(wsi)->source, SD_EVENT_ONESHOT);
+        else
+            sd_event_source_set_enabled(wsi_to_priv_sd(wsi)->source, SD_EVENT_OFF);


All sd_event_ functions are missing error handling in this function.

langchr86 · 2020-11-26T09:30:51Z

lib/event-libs/sdevent/sdevent.c

+
+        sd_event_source_set_io_events(wsi_to_priv_sd(wsi)->source, wsi_to_priv_sd(wsi)->events);
+
+        if (!(wsi_to_priv_sd(wsi)->events & (EPOLLIN | EPOLLOUT)))


Switch if/else clause to make this more understandable ( no ! needed). Besides, is this even correct? If I understand correctly we do disable the event if at least one of the two event flags are set?

langchr86 · 2020-11-26T09:38:26Z

lib/event-libs/sdevent/sdevent.c

+    eventfd.events = 0;
+    eventfd.revents = 0;
+
+    // TODO handle revents error bits


langchr86 · 2020-11-26T09:40:05Z

lib/event-libs/sdevent/sdevent.c

+    }
+
+    lws_pt_unlock(pt);
+    lws_context_unlock(pt->context);


Is it correct to release the locks here? Because you still use pt later.

Copied from libuv/libev.

But I will check this again...

langchr86 · 2020-11-26T09:41:00Z

lib/event-libs/sdevent/sdevent.c

+    lws_context_unlock(pt->context);
+
+    return -1;
+}


All sd_event_ functions are missing error handling in this function.

When using a foreign libuv loop, context creation may fail after adding handles to the foreign loop... if so, it can no longer deal with the fatal error by unpicking the created context and returning NULL... it has to brazen it out with a half-baked context that has already started the destroy flow and allow the foreign loop to close out the handles the usual way for libuv. warmcat#2129

defaultbranch · 2020-11-28T09:32:08Z

@langchr86 wrote:

I miss one block of: https://github.com/defaultbranch/libwebsockets/blob/main/lib/event-libs/README.md

Enabling event lib adoption

You need to add a LWS_SERVER_OPTION... flag as necessary in ./lib/libwebsockets.h enum lws_context_options, and follow the existing code in lws_create_context() to convert the flag into binding your ops struct to the context.

This README is probably a bit out of date regarding the include file location. Today the enum and the new flag LWS_SERVER_OPTION_SDEVENT reside in include/libwebsockets/lws-context-vhost.h instead.

Generate the config file for the installation tree fix wrong LWS__INCLUDE_DIRS definition

Yucong Sun and others added 3 commits November 23, 2020 20:23

mingw: libevent build fixes

43f94f4

h2: ignore unknown frame types a bit earlier

8e94950

Chrome has started being able to issue frame type 0x42, we drop the connection before we realize we wanted to ignore it. This explicitly ignores it a bit earlier.

logging: reduce serving logs

d369a38

defaultbranch requested a review from langchr86 November 25, 2020 09:09

defaultbranch marked this pull request as draft November 25, 2020 09:09

sspc: double free + rx ret not checked

3e69956

Change-Id: I81b67cbb28591a20842fc7d6e09b78375845aae4

langchr86 reviewed Nov 26, 2020

View reviewed changes

CMakeLists-implied-options.txt Show resolved Hide resolved

langchr86 reviewed Nov 26, 2020

View reviewed changes

lws-team added 6 commits November 30, 2020 12:53

async-dns: fix error path

d1cbe7a

h2: fix ignoring unknown frame payloads

b329ea8

zip-fops: example: correct zipfile serving path

bebf0ef

zip-fops: example: add concompressed mount too

20592ce

zip-fops: uncompressed length fix

3e3a725

v4.1.6

736f073

defaultbranch force-pushed the sd-event branch 2 times, most recently from bb1d011 to 9239f11 Compare December 14, 2020 12:18

s-rogonov and others added 2 commits December 24, 2020 13:50

cmake: move LWS__INCLUDE_DIRS def to after REL_INCLIDE_DIR def

2775f92

Generate the config file for the installation tree fix wrong LWS__INCLUDE_DIRS definition

esp32: update build arrangements

382d918

defaultbranch force-pushed the sd-event branch 2 times, most recently from e6fc0d8 to 8bc15ae Compare January 6, 2021 13:10

zishfish added 2 commits January 6, 2021 14:15

add support for sd-event as event loop

a7b494d

add sd-event support to example «minimal-http-server-eventlib-foreign»

8667c29

defaultbranch force-pushed the sd-event branch from 8bc15ae to 8667c29 Compare January 6, 2021 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for sd-event as event loop #1

add support for sd-event as event loop #1

defaultbranch commented Nov 25, 2020

langchr86 left a comment

langchr86 Nov 26, 2020

defaultbranch Nov 26, 2020 •

edited

Loading

langchr86 Nov 26, 2020

defaultbranch Nov 28, 2020

langchr86 Nov 26, 2020

defaultbranch Nov 26, 2020

defaultbranch Nov 28, 2020

langchr86 Nov 26, 2020

defaultbranch Nov 26, 2020

langchr86 Nov 26, 2020 •

edited

Loading

defaultbranch Nov 26, 2020

defaultbranch Nov 28, 2020

defaultbranch Nov 28, 2020

langchr86 left a comment

langchr86 Nov 26, 2020

defaultbranch Nov 28, 2020

langchr86 Nov 26, 2020

defaultbranch Nov 28, 2020

langchr86 Nov 26, 2020

langchr86 Nov 26, 2020

langchr86 Nov 26, 2020

langchr86 Nov 26, 2020

langchr86 Nov 26, 2020

langchr86 Nov 26, 2020

langchr86 Nov 26, 2020

defaultbranch Nov 26, 2020

langchr86 Nov 26, 2020

defaultbranch commented Nov 28, 2020

	option(LWS_WITH_SDEVENT "Compile with support for systemd event loop" OFF)
	option(LWS_WITH_SDEVENT "Compile with support for systemd event loop: sd-event" OFF)

	/*< (CTX) Use systemd event loop /
	/*< (CTX) Use systemd event loop: sd-event /


		if (LWS_WITH_EVLIB_PLUGINS)

		create_evlib_plugin(evlib_sd sdevent.c ${LIBSYSTEMD_LIBRARIES})

	create_evlib_plugin(evlib_sd sdevent.c ${LIBSYSTEMD_LIBRARIES})
	create_evlib_plugin(evlib_sd sdevent.c private-lib-event-libs-sdevent.h ${LIBSYSTEMD_LIBRARIES})

		sd_event_source_unref(ptpriv->sultimer);
		ptpriv->sultimer = NULL;

	sd_event_source_unref(ptpriv->sultimer);
	ptpriv->sultimer = NULL;
	ptpriv->sultimer = sd_event_source_unref(ptpriv->sultimer);


		sd_event_source_set_io_events(wsi_to_priv_sd(wsi)->source, wsi_to_priv_sd(wsi)->events);

		if (!(wsi_to_priv_sd(wsi)->events & (EPOLLIN \| EPOLLOUT)))

add support for sd-event as event loop #1

Are you sure you want to change the base?

add support for sd-event as event loop #1

Conversation

defaultbranch commented Nov 25, 2020

langchr86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defaultbranch Nov 26, 2020 • edited Loading

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

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

langchr86 Nov 26, 2020 • edited Loading

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

langchr86 left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defaultbranch commented Nov 28, 2020

defaultbranch Nov 26, 2020 •

edited

Loading

langchr86 Nov 26, 2020 •

edited

Loading