[PATCH 1/2] fswatch: Add fswatch API

[PATCH 1/2] fswatch: Add fswatch API

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 9377 bytes --]

Add the header and the implementation for a filesystem watch API.  It's
a simple wrapper around the Linux inotify mechanism but not named inotify
so as not to preclude using other mechanisms in the implementation.
---
 Makefile.am   |   6 +-
 ell/ell.h     |   1 +
 ell/fswatch.c | 257 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/fswatch.h |  45 ++++++++++
 4 files changed, 307 insertions(+), 2 deletions(-)
 create mode 100644 ell/fswatch.c
 create mode 100644 ell/fswatch.h

diff --git a/Makefile.am b/Makefile.am
index 100e24c..8203fd4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -45,7 +45,8 @@ pkginclude_HEADERS = ell/ell.h \
 			ell/pkcs5.h \
 			ell/file.h \
 			ell/net.h \
-			ell/dhcp.h
+			ell/dhcp.h \
+			ell/fswatch.h
 
 lib_LTLIBRARIES = ell/libell.la
 
@@ -104,7 +105,8 @@ ell_libell_la_SOURCES = $(linux_headers) \
 			ell/dhcp-private.h \
 			ell/dhcp.c \
 			ell/dhcp-transport.c \
-			ell/dhcp-lease.c
+			ell/dhcp-lease.c \
+			ell/fswatch.c
 
 ell_libell_la_LDFLAGS = -no-undefined \
 			-version-info $(ELL_CURRENT):$(ELL_REVISION):$(ELL_AGE)
diff --git a/ell/ell.h b/ell/ell.h
index 03bb61d..c03faf2 100644
--- a/ell/ell.h
+++ b/ell/ell.h
@@ -53,3 +53,4 @@
 #include <ell/dbus-service.h>
 #include <ell/dbus-client.h>
 #include <ell/dhcp.h>
+#include <ell/fswatch.h>
diff --git a/ell/fswatch.c b/ell/fswatch.c
new file mode 100644
index 0000000..638bdf0
--- /dev/null
+++ b/ell/fswatch.c
@@ -0,0 +1,257 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2018  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <dirent.h>
+#include <unistd.h>
+#include <sys/inotify.h>
+
+#include "private.h"
+#include "util.h"
+#include "io.h"
+#include "queue.h"
+#include "fswatch.h"
+
+static struct l_io *inotify_io;
+static struct l_queue *watches;
+static bool in_notify;
+static bool stale_items;
+
+struct l_fswatch {
+	int id;
+	l_fswatch_cb_t cb;
+	void *user_data;
+	l_fswatch_destroy_cb_t destroy;
+};
+
+#define EVENT_MASK	(IN_CREATE | IN_DELETE | IN_DELETE_SELF | \
+				IN_MODIFY | IN_MOVE | IN_MOVE_SELF)
+
+static void l_fswatch_free(void *data)
+{
+	struct l_fswatch *watch = data;
+
+	if (watch->destroy)
+		watch->destroy(watch, watch->user_data);
+
+	l_free(watch);
+}
+
+static void l_fswatch_shutdown(void)
+{
+	if (!inotify_io)
+		return;
+
+	l_io_destroy(inotify_io);
+	inotify_io = NULL;
+}
+
+static bool l_fswatch_remove_func(const void *a, const void *b)
+{
+	const struct l_fswatch *watch = a;
+
+	return !watch->cb;
+}
+
+static bool inotify_read_cb(struct l_io *io, void *user_data)
+{
+	uint8_t buf[sizeof(struct inotify_event) + NAME_MAX + 1];
+	const struct inotify_event *event;
+	ssize_t len;
+	enum l_fswatch_event event_type;
+
+	len = read(l_io_get_fd(io), buf, sizeof(buf));
+	if (unlikely(len <= 0))
+		return true;
+
+	event = (struct inotify_event *) buf;
+
+	if (event->mask & IN_CREATE)
+		event_type = L_FSWATCH_EVENT_CREATE;
+	else if (event->mask & (IN_DELETE | IN_DELETE_SELF))
+		event_type = L_FSWATCH_EVENT_DELETE;
+	else if (event->mask & (IN_MOVE | IN_MOVE_SELF))
+		event_type = L_FSWATCH_EVENT_MOVE;
+	else
+		event_type = L_FSWATCH_EVENT_MODIFY;
+
+	in_notify = true;
+
+	while ((size_t) len >= sizeof(struct inotify_event) + event->len) {
+		const struct l_queue_entry *entry;
+		const uint8_t *next_ptr;
+		const char *name = NULL;
+
+		if (event->len)
+			name = event->name;
+
+		for (entry = l_queue_get_entries(watches); entry;
+				entry = entry->next) {
+			struct l_fswatch *watch = entry->data;
+
+			if (watch->id != event->wd)
+				continue;
+
+			if ((event->mask & EVENT_MASK) && watch->cb)
+				watch->cb(watch, name, event_type,
+						watch->user_data);
+
+			if (event->mask & IN_IGNORED) {
+				stale_items = true;
+				watch->cb = NULL;
+			}
+		}
+
+		len -= sizeof(struct inotify_event) + event->len;
+		/* event->len is supposed to include alignment */
+		next_ptr = (const uint8_t *) (event + 1) + event->len;
+		event = (struct inotify_event *) next_ptr;
+	}
+
+	in_notify = false;
+
+	if (stale_items) {
+		struct l_fswatch *watch;
+
+		while ((watch = l_queue_remove_if(watches,
+						l_fswatch_remove_func, NULL)))
+			l_fswatch_free(watch);
+
+		stale_items = false;
+
+		if (l_queue_isempty(watches))
+			l_fswatch_shutdown();
+	}
+
+	return true;
+}
+
+static void l_fswatch_io_destroy(void *user_data)
+{
+	l_queue_destroy(watches, l_fswatch_free);
+	watches = NULL;
+}
+
+static bool l_fswatch_init(void)
+{
+	int inotify_fd = inotify_init1(IN_CLOEXEC);
+
+	if (unlikely(inotify_fd < 0))
+		return false;
+
+	inotify_io = l_io_new(inotify_fd);
+	if (unlikely(!inotify_io)) {
+		close(inotify_fd);
+		return false;
+	}
+
+	l_io_set_close_on_destroy(inotify_io, true);
+
+	if (unlikely(!l_io_set_read_handler(inotify_io, inotify_read_cb,
+						NULL, l_fswatch_io_destroy))) {
+		l_io_destroy(inotify_io);
+		return false;
+	}
+
+	return true;
+}
+
+LIB_EXPORT struct l_fswatch *l_fswatch_new(const char *path, l_fswatch_cb_t cb,
+						void *user_data,
+						l_fswatch_destroy_cb_t destroy)
+{
+	struct l_fswatch *watch;
+	int id;
+
+	if (unlikely(!cb))
+		return NULL;
+
+	if (!inotify_io && !l_fswatch_init())
+		return NULL;
+
+	/*
+	 * inotify_watch_add already checks if the path is already being
+	 * watched and will return the old watch ID so we're fine.
+	 */
+	id = inotify_add_watch(l_io_get_fd(inotify_io), path,
+						EVENT_MASK | IN_EXCL_UNLINK);
+	if (unlikely(id < 0)) {
+		if (l_queue_isempty(watches))
+			l_fswatch_shutdown();
+
+		return NULL;
+	}
+
+	watch = l_new(struct l_fswatch, 1);
+	watch->id = id;
+	watch->cb = cb;
+	watch->user_data = user_data;
+	watch->destroy = destroy;
+
+	if (!watches)
+		watches = l_queue_new();
+
+	l_queue_push_tail(watches, watch);
+
+	return watch;
+}
+
+LIB_EXPORT bool l_fswatch_destroy(struct l_fswatch *watch)
+{
+	const struct l_queue_entry *entry;
+	int id;
+
+	if (unlikely(!inotify_io || !watch))
+		return false;
+
+	id = watch->id;
+
+	/* Check if we have any other watch with the same inotify ID */
+	for (entry = l_queue_get_entries(watches); entry;
+			entry = entry->next) {
+		struct l_fswatch *watch2 = entry->data;
+
+		if (watch2 != watch && watch2->id == id)
+			break;
+	}
+
+	if (!entry && id != -1)
+		if (inotify_rm_watch(l_io_get_fd(inotify_io), id) < 0)
+			return false;
+
+	if (in_notify) {
+		watch->cb = NULL;
+		stale_items = true;
+		return true;
+	}
+
+	l_queue_remove(watches, watch);
+	l_fswatch_free(watch);
+
+	if (l_queue_isempty(watches))
+		l_fswatch_shutdown();
+
+	return true;
+}
diff --git a/ell/fswatch.h b/ell/fswatch.h
new file mode 100644
index 0000000..79b379c
--- /dev/null
+++ b/ell/fswatch.h
@@ -0,0 +1,45 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2018  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __ELL_FSWATCH_H
+#define __ELL_FSWATCH_H
+
+struct l_fswatch;
+
+enum l_fswatch_event {
+	L_FSWATCH_EVENT_CREATE,
+	L_FSWATCH_EVENT_MOVE,
+	L_FSWATCH_EVENT_MODIFY,
+	L_FSWATCH_EVENT_DELETE,
+};
+
+typedef void (*l_fswatch_cb_t) (struct l_fswatch *watch, const char *filename,
+				enum l_fswatch_event event, void *user_data);
+typedef void (*l_fswatch_destroy_cb_t) (struct l_fswatch *watch,
+					void *user_data);
+
+struct l_fswatch *l_fswatch_new(const char *path, l_fswatch_cb_t cb,
+				void *user_data,
+				l_fswatch_destroy_cb_t destroy);
+bool l_fswatch_destroy(struct l_fswatch *watch);
+
+#endif /* __ELL_FSWATCH_H */
-- 
2.14.1

[PATCH 2/2] unit: Add an fswatch unit test

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 8512 bytes --]

---
 .gitignore          |   1 +
 Makefile.am         |   5 +-
 unit/test-fswatch.c | 290 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 295 insertions(+), 1 deletion(-)
 create mode 100644 unit/test-fswatch.c

diff --git a/.gitignore b/.gitignore
index 4dc3777..6c9ffb0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -55,6 +55,7 @@ unit/test-uuid
 unit/test-key
 unit/test-pbkdf2
 unit/test-dhcp
+unit/test-fswatch
 unit/cert-*.pem
 unit/cert-*.csr
 unit/cert-*.srl
diff --git a/Makefile.am b/Makefile.am
index 8203fd4..9fe00fb 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -147,7 +147,8 @@ unit_tests = unit/test-unit \
 			unit/test-base64 \
 			unit/test-uuid \
 			unit/test-pbkdf2 \
-			unit/test-dhcp
+			unit/test-dhcp \
+			unit/test-fswatch
 
 dbus_tests = unit/test-hwdb \
 			unit/test-dbus \
@@ -264,6 +265,8 @@ unit_test_uuid_LDADD = ell/libell-private.la
 
 unit_test_dhcp_LDADD = ell/libell-private.la
 
+unit_test_fswatch_LDADD = ell/libell-private.la
+
 if MAINTAINER_MODE
 noinst_LTLIBRARIES += unit/example-plugin.la
 endif
diff --git a/unit/test-fswatch.c b/unit/test-fswatch.c
new file mode 100644
index 0000000..3042fbb
--- /dev/null
+++ b/unit/test-fswatch.c
@@ -0,0 +1,290 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2011-2014  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <assert.h>
+
+#include <ell/ell.h>
+
+#define TEST_DIR "/tmp/ell-test-dir"
+#define TEST_FILE1 "/tmp/ell-test-dir/file1"
+#define TEST_FILE2 "/tmp/ell-test-dir/file2"
+
+static struct l_fswatch *dir_watch;
+static struct l_fswatch *file_watch;
+static int data;
+
+static enum {
+	TEST_DIR_DESTROY_CB_1,
+	TEST_FILE_CREATE,
+	TEST_FILE_MOVE,
+	TEST_FILE_MODIFY,
+	TEST_FILE_REMOVE,
+	TEST_DIR_REMOVE,
+	TEST_DIR_DESTROY_CB_2,
+} state;
+
+static int substate;
+
+enum {
+	TEST_FILE_MOVE_DIR_FROM_EVENT = 1 << 0,
+	TEST_FILE_MOVE_DIR_TO_EVENT = 1 << 1,
+	TEST_FILE_MOVE_FILE_EVENT = 1 << 2,
+	TEST_FILE_MOVE_ALL = 7,
+};
+
+enum {
+	TEST_FILE_MODIFY_DIR_EVENT = 1 << 0,
+	TEST_FILE_MODIFY_FILE_EVENT = 1 << 1,
+	TEST_FILE_MODIFY_ALL = 3,
+};
+
+enum {
+	TEST_FILE_REMOVE_DIR_EVENT = 1 << 0,
+	TEST_FILE_REMOVE_FILE_EVENT = 1 << 1,
+	TEST_FILE_REMOVE_DESTROY_CB = 1 << 2,
+	TEST_FILE_REMOVE_ALL = 7,
+};
+
+static void test_file_move_check(void)
+{
+	if (substate == TEST_FILE_MOVE_ALL) {
+		l_info("TEST_FILE_MODIFY");
+		state = TEST_FILE_MODIFY;
+		substate = 0;
+		assert(close(creat(TEST_FILE2, 0600)) == 0);
+	}
+}
+
+static void test_file_modify_check(void)
+{
+	if (substate == TEST_FILE_MODIFY_ALL) {
+		l_info("TEST_FILE_REMOVE");
+		state = TEST_FILE_REMOVE;
+		substate = 0;
+		assert(unlink(TEST_FILE2) == 0);
+	}
+}
+
+static void test_file_remove_check(void)
+{
+	if (substate == TEST_FILE_REMOVE_ALL) {
+		l_info("TEST_DIR_REMOVE");
+		state = TEST_DIR_REMOVE;
+		substate = 0;
+		assert(rmdir(TEST_DIR) == 0);
+	}
+}
+
+static void file_watch_destroy(struct l_fswatch *watch, void *user_data)
+{
+	assert(state == TEST_FILE_REMOVE);
+	assert(!(substate & TEST_FILE_REMOVE_DESTROY_CB));
+	assert(substate & TEST_FILE_REMOVE_FILE_EVENT);
+
+	substate |= TEST_FILE_REMOVE_DESTROY_CB;
+
+	test_file_remove_check();
+}
+
+static void file_watch_cb(struct l_fswatch *watch, const char *filename,
+				enum l_fswatch_event event, void *user_data)
+{
+	assert(watch == file_watch);
+	assert(user_data == &data);
+	assert(filename == NULL);
+
+	switch (state) {
+	case TEST_FILE_MOVE:
+		assert(event == L_FSWATCH_EVENT_MOVE);
+		assert(!(substate & TEST_FILE_MOVE_FILE_EVENT));
+
+		substate |= TEST_FILE_MOVE_FILE_EVENT;
+
+		test_file_move_check();
+		break;
+	case TEST_FILE_MODIFY:
+		assert(event == L_FSWATCH_EVENT_MODIFY);
+		assert(!(substate & TEST_FILE_MODIFY_FILE_EVENT));
+
+		substate |= TEST_FILE_MODIFY_FILE_EVENT;
+
+		test_file_modify_check();
+		break;
+	case TEST_FILE_REMOVE:
+		assert(event == L_FSWATCH_EVENT_DELETE);
+		assert(!(substate & TEST_FILE_REMOVE_FILE_EVENT));
+
+		substate |= TEST_FILE_REMOVE_FILE_EVENT;
+
+		test_file_remove_check();
+		break;
+	default:
+		assert(false);
+	}
+}
+
+static void dir_watch_destroy_1(struct l_fswatch *watch, void *user_data)
+{
+	assert(state == TEST_DIR_DESTROY_CB_1);
+	assert(dir_watch);
+
+	dir_watch = NULL;
+}
+
+static void dir_watch_destroy_2(struct l_fswatch *watch, void *user_data)
+{
+	assert(state == TEST_DIR_DESTROY_CB_2);
+	assert(dir_watch);
+
+	l_main_quit();
+
+	dir_watch = NULL;
+}
+
+static void dir_watch_cb(struct l_fswatch *watch, const char *filename,
+				enum l_fswatch_event event, void *user_data)
+{
+	assert(watch == dir_watch);
+	assert(user_data == &data);
+
+	switch (state) {
+	case TEST_FILE_CREATE:
+		assert(event == L_FSWATCH_EVENT_CREATE);
+		assert(filename);
+		assert(strstr(TEST_FILE1, filename));
+
+		file_watch = l_fswatch_new(TEST_FILE1, file_watch_cb, &data,
+						file_watch_destroy);
+		assert(file_watch);
+
+		l_info("TEST_FILE_MOVE");
+		state = TEST_FILE_MOVE;
+		substate = 0;
+		assert(rename(TEST_FILE1, TEST_FILE2) == 0);
+		break;
+	case TEST_FILE_MOVE:
+		assert(event == L_FSWATCH_EVENT_MOVE);
+		assert(filename);
+
+		if (strstr(TEST_FILE1, filename)) {
+			assert(!(substate & TEST_FILE_MOVE_DIR_FROM_EVENT));
+
+			substate |= TEST_FILE_MOVE_DIR_FROM_EVENT;
+		} else if (strstr(TEST_FILE2, filename)) {
+			assert(!(substate & TEST_FILE_MOVE_DIR_TO_EVENT));
+
+			substate |= TEST_FILE_MOVE_DIR_TO_EVENT;
+		} else
+			assert(false);
+
+		test_file_move_check();
+		break;
+	case TEST_FILE_MODIFY:
+		assert(event == L_FSWATCH_EVENT_MODIFY);
+		assert(filename);
+		assert(strstr(TEST_FILE2, filename));
+		assert(!(substate & TEST_FILE_MODIFY_DIR_EVENT));
+
+		substate |= TEST_FILE_MODIFY_DIR_EVENT;
+
+		test_file_modify_check();
+		break;
+	case TEST_FILE_REMOVE:
+		assert(event == L_FSWATCH_EVENT_DELETE);
+		assert(filename);
+		assert(strstr(TEST_FILE2, filename));
+		assert(!(substate & TEST_FILE_REMOVE_DIR_EVENT));
+
+		substate |= TEST_FILE_REMOVE_DIR_EVENT;
+
+		test_file_remove_check();
+		break;
+	case TEST_DIR_REMOVE:
+		assert(event == L_FSWATCH_EVENT_DELETE);
+		assert(filename == NULL);
+
+		l_info("TEST_DIR_DESTROY_CB_2");
+		state = TEST_DIR_DESTROY_CB_2;
+		break;
+	default:
+		assert(false);
+	}
+}
+
+static void timeout_cb(struct l_timeout *timeout, void *user_data)
+{
+	assert(false);
+}
+
+int main(int argc, char *argv[])
+{
+	struct l_timeout *timeout;
+
+	assert(l_main_init());
+
+	l_log_set_stderr();
+
+	unlink(TEST_FILE1);
+	unlink(TEST_FILE2);
+	rmdir(TEST_DIR);
+	assert(mkdir(TEST_DIR, 0700) == 0);
+
+	timeout = l_timeout_create(1, timeout_cb, NULL, NULL);
+
+	l_info("TEST_DIR_DESTROY_CB_1");
+	state = TEST_DIR_DESTROY_CB_1;
+
+	dir_watch = l_fswatch_new(TEST_DIR, dir_watch_cb, &data,
+					dir_watch_destroy_1);
+
+	assert(dir_watch);
+	assert(l_fswatch_destroy(dir_watch));
+	assert(!dir_watch);
+
+	dir_watch = l_fswatch_new(TEST_DIR, dir_watch_cb, &data,
+					dir_watch_destroy_2);
+
+	assert(dir_watch);
+	assert(!l_fswatch_new(TEST_FILE1, file_watch_cb, &data,
+				file_watch_destroy));
+
+	l_info("TEST_FILE_CREATE");
+	state = TEST_FILE_CREATE;
+	assert(close(creat(TEST_FILE1, 0600)) == 0);
+
+	l_main_run();
+
+	l_timeout_remove(timeout);
+
+	l_main_exit();
+
+	return 0;
+}
-- 
2.14.1

Re: [PATCH 1/2] fswatch: Add fswatch API

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3744 bytes --]

Hi Andrew,

> +static bool inotify_read_cb(struct l_io *io, void *user_data)
> +{
> +	uint8_t buf[sizeof(struct inotify_event) + NAME_MAX + 1];
> +	const struct inotify_event *event;
> +	ssize_t len;
> +	enum l_fswatch_event event_type;
> +
> +	len = read(l_io_get_fd(io), buf, sizeof(buf));

This might need a TEMP_FAILURE_RETRY()

> +	if (unlikely(len <= 0))
> +		return true;
> +
> +	event = (struct inotify_event *) buf;
> +
> +	if (event->mask & IN_CREATE)
> +		event_type = L_FSWATCH_EVENT_CREATE;
> +	else if (event->mask & (IN_DELETE | IN_DELETE_SELF))
> +		event_type = L_FSWATCH_EVENT_DELETE;
> +	else if (event->mask & (IN_MOVE | IN_MOVE_SELF))
> +		event_type = L_FSWATCH_EVENT_MOVE;
> +	else
> +		event_type = L_FSWATCH_EVENT_MODIFY;

Why is this outside the while loop below?  It sounds like there may be 
multiple inotify events during a single read and each one would have a 
different mask, no?

In fact, this might be better written as a for loop, similar to how the 
manpage does it.

> +
> +	in_notify = true;
> +
> +	while ((size_t) len >= sizeof(struct inotify_event) + event->len) {
> +		const struct l_queue_entry *entry;
> +		const uint8_t *next_ptr;
> +		const char *name = NULL;
> +
> +		if (event->len)
> +			name = event->name;
> +
> +		for (entry = l_queue_get_entries(watches); entry;
> +				entry = entry->next) {
> +			struct l_fswatch *watch = entry->data;
> +
> +			if (watch->id != event->wd)
> +				continue;
> +
> +			if ((event->mask & EVENT_MASK) && watch->cb)
> +				watch->cb(watch, name, event_type,
> +						watch->user_data);
> +
> +			if (event->mask & IN_IGNORED) {
> +				stale_items = true;
> +				watch->cb = NULL;
> +			}
> +		}
> +
> +		len -= sizeof(struct inotify_event) + event->len;
> +		/* event->len is supposed to include alignment */
> +		next_ptr = (const uint8_t *) (event + 1) + event->len;
> +		event = (struct inotify_event *) next_ptr;
> +	}
> +
> +	in_notify = false;
> +
> +	if (stale_items) {
> +		struct l_fswatch *watch;
> +
> +		while ((watch = l_queue_remove_if(watches,
> +						l_fswatch_remove_func, NULL)))
> +			l_fswatch_free(watch);
> +
> +		stale_items = false;
> +
> +		if (l_queue_isempty(watches))
> +			l_fswatch_shutdown();
> +	}
> +
> +	return true;
> +}
> +

<snip>

> +
> +LIB_EXPORT bool l_fswatch_destroy(struct l_fswatch *watch)

I still don't see why this returns bool, what can the caller do if the 
call fails?

> +{
> +	const struct l_queue_entry *entry;
> +	int id;
> +
> +	if (unlikely(!inotify_io || !watch))
> +		return false;

This can just as easily be void

> +
> +	id = watch->id;
> +
> +	/* Check if we have any other watch with the same inotify ID */
> +	for (entry = l_queue_get_entries(watches); entry;
> +			entry = entry->next) {
> +		struct l_fswatch *watch2 = entry->data;
> +
> +		if (watch2 != watch && watch2->id == id)
> +			break;
> +	}
> +
> +	if (!entry && id != -1)
> +		if (inotify_rm_watch(l_io_get_fd(inotify_io), id) < 0)
> +			return false;

So how do we delete the struct l_fswatch *watch memory here?

> +
> +	if (in_notify) {
> +		watch->cb = NULL;
> +		stale_items = true;
> +		return true;
> +	}
> +
> +	l_queue_remove(watches, watch);
> +	l_fswatch_free(watch);
> +
> +	if (l_queue_isempty(watches))
> +		l_fswatch_shutdown();
> +
> +	return true;
> +}

<snip>

> +typedef void (*l_fswatch_destroy_cb_t) (struct l_fswatch *watch,
> +					void *user_data);

Why would this destroy callback signature be different from all others? 
It also precludes us from doing something like:

l_fswatch_new("/foo/bar", cb, u, (l_fswatch_destroy_cb_t) l_free);

Regards,
-Denis

Re: [PATCH 1/2] fswatch: Add fswatch API

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 5416 bytes --]

Hi Denis,

On 20 July 2018 at 17:55, Denis Kenzior <denkenz@gmail.com> wrote:
>> +static bool inotify_read_cb(struct l_io *io, void *user_data)
>> +{
>> +       uint8_t buf[sizeof(struct inotify_event) + NAME_MAX + 1];
>> +       const struct inotify_event *event;
>> +       ssize_t len;
>> +       enum l_fswatch_event event_type;
>> +
>> +       len = read(l_io_get_fd(io), buf, sizeof(buf));
>
>
> This might need a TEMP_FAILURE_RETRY()

In theory it shouldn't because if we don't manage to read the event
the main loop will keep calling us but let me do this anyway.

>
>> +       if (unlikely(len <= 0))
>> +               return true;
>> +
>> +       event = (struct inotify_event *) buf;
>> +
>> +       if (event->mask & IN_CREATE)
>> +               event_type = L_FSWATCH_EVENT_CREATE;
>> +       else if (event->mask & (IN_DELETE | IN_DELETE_SELF))
>> +               event_type = L_FSWATCH_EVENT_DELETE;
>> +       else if (event->mask & (IN_MOVE | IN_MOVE_SELF))
>> +               event_type = L_FSWATCH_EVENT_MOVE;
>> +       else
>> +               event_type = L_FSWATCH_EVENT_MODIFY;
>
>
> Why is this outside the while loop below?  It sounds like there may be
> multiple inotify events during a single read and each one would have a
> different mask, no?

Oops, yes, totally right.

>
> In fact, this might be better written as a for loop, similar to how the
> manpage does it.
>
>
>> +
>> +       in_notify = true;
>> +
>> +       while ((size_t) len >= sizeof(struct inotify_event) + event->len)
>> {
>> +               const struct l_queue_entry *entry;
>> +               const uint8_t *next_ptr;
>> +               const char *name = NULL;
>> +
>> +               if (event->len)
>> +                       name = event->name;
>> +
>> +               for (entry = l_queue_get_entries(watches); entry;
>> +                               entry = entry->next) {
>> +                       struct l_fswatch *watch = entry->data;
>> +
>> +                       if (watch->id != event->wd)
>> +                               continue;
>> +
>> +                       if ((event->mask & EVENT_MASK) && watch->cb)
>> +                               watch->cb(watch, name, event_type,
>> +                                               watch->user_data);
>> +
>> +                       if (event->mask & IN_IGNORED) {
>> +                               stale_items = true;
>> +                               watch->cb = NULL;
>> +                       }
>> +               }
>> +
>> +               len -= sizeof(struct inotify_event) + event->len;
>> +               /* event->len is supposed to include alignment */
>> +               next_ptr = (const uint8_t *) (event + 1) + event->len;
>> +               event = (struct inotify_event *) next_ptr;
>> +       }
>> +
>> +       in_notify = false;
>> +
>> +       if (stale_items) {
>> +               struct l_fswatch *watch;
>> +
>> +               while ((watch = l_queue_remove_if(watches,
>> +                                               l_fswatch_remove_func,
>> NULL)))
>> +                       l_fswatch_free(watch);
>> +
>> +               stale_items = false;
>> +
>> +               if (l_queue_isempty(watches))
>> +                       l_fswatch_shutdown();
>> +       }
>> +
>> +       return true;
>> +}
>> +
>
>
> <snip>
>
>> +
>> +LIB_EXPORT bool l_fswatch_destroy(struct l_fswatch *watch)
>
>
> I still don't see why this returns bool, what can the caller do if the call
> fails?

Not much, but we can't do much either.  We can free the memeory, yes,
but should we hide the error?

>
>> +{
>> +       const struct l_queue_entry *entry;
>> +       int id;
>> +
>> +       if (unlikely(!inotify_io || !watch))
>> +               return false;
>
>
> This can just as easily be void
>
>> +
>> +       id = watch->id;
>> +
>> +       /* Check if we have any other watch with the same inotify ID */
>> +       for (entry = l_queue_get_entries(watches); entry;
>> +                       entry = entry->next) {
>> +               struct l_fswatch *watch2 = entry->data;
>> +
>> +               if (watch2 != watch && watch2->id == id)
>> +                       break;
>> +       }
>> +
>> +       if (!entry && id != -1)
>> +               if (inotify_rm_watch(l_io_get_fd(inotify_io), id) < 0)
>> +                       return false;
>
>
> So how do we delete the struct l_fswatch *watch memory here?
>
>> +
>> +       if (in_notify) {
>> +               watch->cb = NULL;
>> +               stale_items = true;
>> +               return true;
>> +       }
>> +
>> +       l_queue_remove(watches, watch);
>> +       l_fswatch_free(watch);
>> +
>> +       if (l_queue_isempty(watches))
>> +               l_fswatch_shutdown();
>> +
>> +       return true;
>> +}
>
>
> <snip>
>
>> +typedef void (*l_fswatch_destroy_cb_t) (struct l_fswatch *watch,
>> +                                       void *user_data);
>
>
> Why would this destroy callback signature be different from all others? It
> also precludes us from doing something like:
>
> l_fswatch_new("/foo/bar", cb, u, (l_fswatch_destroy_cb_t) l_free);

Ok, thought it might be useful to pass the watch pointer, it doesn't
cost us anything and could simplify things for some users.

Best regards

Re: [PATCH 1/2] fswatch: Add fswatch API

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

Hi Andrew,

>>> +LIB_EXPORT bool l_fswatch_destroy(struct l_fswatch *watch)
>>
>>
>> I still don't see why this returns bool, what can the caller do if the call
>> fails?
> 
> Not much, but we can't do much either.  We can free the memeory, yes,
> but should we hide the error?

There's no point returning an error out to the user if they can't do 
anything about it, especially if it leaves them powerless from 
reclaiming memory allocated to the watch.

So yes, it is probably better to return void (and maybe print an 
l_error) than to return an error here.

>>> +typedef void (*l_fswatch_destroy_cb_t) (struct l_fswatch *watch,
>>> +                                       void *user_data);
>>
>>
>> Why would this destroy callback signature be different from all others? It
>> also precludes us from doing something like:
>>
>> l_fswatch_new("/foo/bar", cb, u, (l_fswatch_destroy_cb_t) l_free);
> 
> Ok, thought it might be useful to pass the watch pointer, it doesn't
> cost us anything and could simplify things for some users.
> 

This is useful for callbacks, but almost never happens for destroy 
callbacks.  In too many cases the destroy callback is just l_free or 
some_structure_destroy(struct some_structure).  So having to come up 
with another function just to have the correct signature is wasteful. 
For the 1-5% usecase where this might be useful, the caller can carry 
the fswatch pointer in the userdata structure itself.

Regards,
-Denis

Re: [PATCH 1/2] fswatch: Add fswatch API

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1983 bytes --]

On 20 July 2018 at 18:21, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> +LIB_EXPORT bool l_fswatch_destroy(struct l_fswatch *watch)
>>>
>>> I still don't see why this returns bool, what can the caller do if the
>>> call
>>> fails?
>>
>> Not much, but we can't do much either.  We can free the memeory, yes,
>> but should we hide the error?
>
>
> There's no point returning an error out to the user if they can't do
> anything about it, especially if it leaves them powerless from reclaiming
> memory allocated to the watch.
>
> So yes, it is probably better to return void (and maybe print an l_error)
> than to return an error here.

Ok.

>
>>>> +typedef void (*l_fswatch_destroy_cb_t) (struct l_fswatch *watch,
>>>> +                                       void *user_data);
>>>
>>>
>>>
>>> Why would this destroy callback signature be different from all others?
>>> It
>>> also precludes us from doing something like:
>>>
>>> l_fswatch_new("/foo/bar", cb, u, (l_fswatch_destroy_cb_t) l_free);
>>
>>
>> Ok, thought it might be useful to pass the watch pointer, it doesn't
>> cost us anything and could simplify things for some users.
>>
>
> This is useful for callbacks, but almost never happens for destroy
> callbacks.  In too many cases the destroy callback is just l_free or
> some_structure_destroy(struct some_structure).  So having to come up with
> another function just to have the correct signature is wasteful. For the
> 1-5% usecase where this might be useful, the caller can carry the fswatch
> pointer in the userdata structure itself.

Fair enough although I was specifically thinking about simplifying
cases where there's no userdata and the watch pointer is enough to
identify the watch... but then you can also just have multiple destroy
callbacks.

BTW I looked at the for loop in the man page and it seems it's
actually not initialising 'event' before first use so I won't be
following that pattern.

Cheers

Re: [PATCH 1/2] fswatch: Add fswatch API

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

Hi Andrew,

> BTW I looked at the for loop in the man page and it seems it's
> actually not initialising 'event' before first use so I won't be
> following that pattern.

for (ptr = buf; ptr < buf + len;
	ptr += sizeof(struct inotify_event) + event->len) {

	event = (const struct inotify_event *) ptr;

...
}

This looks fine to me?  The ptr += is not executed until the end of the 
iteration.

Regards,
-Denis

Re: [PATCH 1/2] fswatch: Add fswatch API

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

On 20 July 2018 at 19:16, Denis Kenzior <denkenz@gmail.com> wrote:
>> BTW I looked at the for loop in the man page and it seems it's
>> actually not initialising 'event' before first use so I won't be
>> following that pattern.
>
>
> for (ptr = buf; ptr < buf + len;
>         ptr += sizeof(struct inotify_event) + event->len) {
>
>         event = (const struct inotify_event *) ptr;
>
> ...
> }
>
> This looks fine to me?  The ptr += is not executed until the end of the
> iteration.

Oh true, it doesn't do the length check before initial iteration.  I
thought it's better to have a sanity check.

The loop is wrong though and would cause valgrind to complain because
after your last iteration ptr equals buf + len, so 'event' also points
at the end of buf and event->len is beyond the end of your buffer.

Cheers

Re: [PATCH 1/2] fswatch: Add fswatch API

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

On 20 July 2018 at 19:37, Andrew Zaborowski <andrew.zaborowski@intel.com> wrote:
> The loop is wrong though and would cause valgrind to complain because
> after your last iteration ptr equals buf + len, so 'event' also points
> at the end of buf and event->len is beyond the end of your buffer.

Uhh no, it's actually only checking that ptr < buf + len, I'm not
reading correctly today.

Best regards

Re: [PATCH 1/2] fswatch: Add fswatch API

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

Hi Andrew,

On 07/20/2018 12:41 PM, Andrew Zaborowski wrote:
> On 20 July 2018 at 19:37, Andrew Zaborowski <andrew.zaborowski@intel.com> wrote:
>> The loop is wrong though and would cause valgrind to complain because
>> after your last iteration ptr equals buf + len, so 'event' also points
>> at the end of buf and event->len is beyond the end of your buffer.
> 
> Uhh no, it's actually only checking that ptr < buf + len, I'm not
> reading correctly today.

Right.  The only thing it doesn't do is check that len >= sizeof(struct 
inotify_event).  But then I imagine you can trust the kernel to not 
screw this up.

I find the for loop version a bit more readable, but I'm okay applying 
your version as well.

Regards,
-Denis

Re: [PATCH 1/2] fswatch: Add fswatch API

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

Hi Andrew,

On 07/20/2018 01:12 PM, Andrew Zaborowski wrote:
> Add the header and the implementation for a filesystem watch API.  It's
> a simple wrapper around the Linux inotify mechanism but not named inotify
> so as not to preclude using other mechanisms in the implementation.
> ---
>   Makefile.am   |   6 +-
>   ell/ell.h     |   1 +
>   ell/fswatch.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   ell/fswatch.h |  44 ++++++++++
>   4 files changed, 304 insertions(+), 2 deletions(-)
>   create mode 100644 ell/fswatch.c
>   create mode 100644 ell/fswatch.h
> 

Both applied, thanks.

Regards,
-Denis

[PATCH 1/2] fswatch: Add fswatch API

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 9270 bytes --]

Add the header and the implementation for a filesystem watch API.  It's
a simple wrapper around the Linux inotify mechanism but not named inotify
so as not to preclude using other mechanisms in the implementation.
---
 Makefile.am   |   6 +-
 ell/ell.h     |   1 +
 ell/fswatch.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/fswatch.h |  44 ++++++++++
 4 files changed, 304 insertions(+), 2 deletions(-)
 create mode 100644 ell/fswatch.c
 create mode 100644 ell/fswatch.h

diff --git a/Makefile.am b/Makefile.am
index 100e24c..8203fd4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -45,7 +45,8 @@ pkginclude_HEADERS = ell/ell.h \
 			ell/pkcs5.h \
 			ell/file.h \
 			ell/net.h \
-			ell/dhcp.h
+			ell/dhcp.h \
+			ell/fswatch.h
 
 lib_LTLIBRARIES = ell/libell.la
 
@@ -104,7 +105,8 @@ ell_libell_la_SOURCES = $(linux_headers) \
 			ell/dhcp-private.h \
 			ell/dhcp.c \
 			ell/dhcp-transport.c \
-			ell/dhcp-lease.c
+			ell/dhcp-lease.c \
+			ell/fswatch.c
 
 ell_libell_la_LDFLAGS = -no-undefined \
 			-version-info $(ELL_CURRENT):$(ELL_REVISION):$(ELL_AGE)
diff --git a/ell/ell.h b/ell/ell.h
index 03bb61d..c03faf2 100644
--- a/ell/ell.h
+++ b/ell/ell.h
@@ -53,3 +53,4 @@
 #include <ell/dbus-service.h>
 #include <ell/dbus-client.h>
 #include <ell/dhcp.h>
+#include <ell/fswatch.h>
diff --git a/ell/fswatch.c b/ell/fswatch.c
new file mode 100644
index 0000000..c65a1b9
--- /dev/null
+++ b/ell/fswatch.c
@@ -0,0 +1,255 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2018  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <dirent.h>
+#include <unistd.h>
+#include <sys/inotify.h>
+
+#include "private.h"
+#include "util.h"
+#include "io.h"
+#include "queue.h"
+#include "fswatch.h"
+
+static struct l_io *inotify_io;
+static struct l_queue *watches;
+static bool in_notify;
+static bool stale_items;
+
+struct l_fswatch {
+	int id;
+	l_fswatch_cb_t cb;
+	void *user_data;
+	l_fswatch_destroy_cb_t destroy;
+};
+
+#define EVENT_MASK	(IN_CREATE | IN_DELETE | IN_DELETE_SELF | \
+				IN_MODIFY | IN_MOVE | IN_MOVE_SELF)
+
+static void l_fswatch_free(void *data)
+{
+	struct l_fswatch *watch = data;
+
+	if (watch->destroy)
+		watch->destroy(watch->user_data);
+
+	l_free(watch);
+}
+
+static void l_fswatch_shutdown(void)
+{
+	if (!inotify_io)
+		return;
+
+	l_io_destroy(inotify_io);
+	inotify_io = NULL;
+}
+
+static bool l_fswatch_remove_func(const void *a, const void *b)
+{
+	const struct l_fswatch *watch = a;
+
+	return !watch->cb;
+}
+
+static bool inotify_read_cb(struct l_io *io, void *user_data)
+{
+	uint8_t buf[sizeof(struct inotify_event) + NAME_MAX + 1]
+		__attribute__ ((aligned(__alignof__(struct inotify_event))));
+	const uint8_t *ptr;
+	const struct inotify_event *event;
+	ssize_t len;
+
+	len = TEMP_FAILURE_RETRY(read(l_io_get_fd(io), buf, sizeof(buf)));
+	if (unlikely(len <= 0))
+		return true;
+
+	in_notify = true;
+
+	for (ptr = buf; ptr < buf + len;
+			ptr += sizeof(struct inotify_event) + event->len) {
+		const struct l_queue_entry *entry;
+		const char *name = NULL;
+
+		event = (struct inotify_event *) ptr;
+
+		if (event->len)
+			name = event->name;
+
+		for (entry = l_queue_get_entries(watches); entry;
+				entry = entry->next) {
+			struct l_fswatch *watch = entry->data;
+
+			if (watch->id != event->wd)
+				continue;
+
+			if ((event->mask & IN_CREATE) && watch->cb)
+				watch->cb(watch, name, L_FSWATCH_EVENT_CREATE,
+						watch->user_data);
+
+			if ((event->mask & (IN_MOVE | IN_MOVE_SELF)) &&
+					watch->cb)
+				watch->cb(watch, name, L_FSWATCH_EVENT_MOVE,
+						watch->user_data);
+
+			if ((event->mask & IN_MODIFY) && watch->cb)
+				watch->cb(watch, name, L_FSWATCH_EVENT_MODIFY,
+						watch->user_data);
+
+			if ((event->mask & (IN_DELETE | IN_DELETE_SELF)) &&
+					watch->cb)
+				watch->cb(watch, name, L_FSWATCH_EVENT_DELETE,
+						watch->user_data);
+
+			if (event->mask & IN_IGNORED) {
+				stale_items = true;
+				watch->cb = NULL;
+			}
+		}
+	}
+
+	in_notify = false;
+
+	if (stale_items) {
+		struct l_fswatch *watch;
+
+		while ((watch = l_queue_remove_if(watches,
+						l_fswatch_remove_func, NULL)))
+			l_fswatch_free(watch);
+
+		stale_items = false;
+
+		if (l_queue_isempty(watches))
+			l_fswatch_shutdown();
+	}
+
+	return true;
+}
+
+static void l_fswatch_io_destroy(void *user_data)
+{
+	l_queue_destroy(watches, l_fswatch_free);
+	watches = NULL;
+}
+
+static bool l_fswatch_init(void)
+{
+	int inotify_fd = inotify_init1(IN_CLOEXEC);
+
+	if (unlikely(inotify_fd < 0))
+		return false;
+
+	inotify_io = l_io_new(inotify_fd);
+	if (unlikely(!inotify_io)) {
+		close(inotify_fd);
+		return false;
+	}
+
+	l_io_set_close_on_destroy(inotify_io, true);
+
+	if (unlikely(!l_io_set_read_handler(inotify_io, inotify_read_cb,
+						NULL, l_fswatch_io_destroy))) {
+		l_io_destroy(inotify_io);
+		return false;
+	}
+
+	return true;
+}
+
+LIB_EXPORT struct l_fswatch *l_fswatch_new(const char *path, l_fswatch_cb_t cb,
+						void *user_data,
+						l_fswatch_destroy_cb_t destroy)
+{
+	struct l_fswatch *watch;
+	int id;
+
+	if (unlikely(!cb))
+		return NULL;
+
+	if (!inotify_io && !l_fswatch_init())
+		return NULL;
+
+	/*
+	 * inotify_watch_add already checks if the path is already being
+	 * watched and will return the old watch ID so we're fine.
+	 */
+	id = inotify_add_watch(l_io_get_fd(inotify_io), path,
+						EVENT_MASK | IN_EXCL_UNLINK);
+	if (unlikely(id < 0)) {
+		if (l_queue_isempty(watches))
+			l_fswatch_shutdown();
+
+		return NULL;
+	}
+
+	watch = l_new(struct l_fswatch, 1);
+	watch->id = id;
+	watch->cb = cb;
+	watch->user_data = user_data;
+	watch->destroy = destroy;
+
+	if (!watches)
+		watches = l_queue_new();
+
+	l_queue_push_tail(watches, watch);
+
+	return watch;
+}
+
+LIB_EXPORT void l_fswatch_destroy(struct l_fswatch *watch)
+{
+	const struct l_queue_entry *entry;
+	int id;
+
+	if (unlikely(!inotify_io || !watch))
+		return;
+
+	id = watch->id;
+
+	/* Check if we have any other watch with the same inotify ID */
+	for (entry = l_queue_get_entries(watches); entry;
+			entry = entry->next) {
+		struct l_fswatch *watch2 = entry->data;
+
+		if (watch2 != watch && watch2->id == id)
+			break;
+	}
+
+	if (!entry && id != -1)
+		inotify_rm_watch(l_io_get_fd(inotify_io), id);
+
+	if (in_notify) {
+		watch->cb = NULL;
+		stale_items = true;
+		return;
+	}
+
+	l_queue_remove(watches, watch);
+	l_fswatch_free(watch);
+
+	if (l_queue_isempty(watches))
+		l_fswatch_shutdown();
+}
diff --git a/ell/fswatch.h b/ell/fswatch.h
new file mode 100644
index 0000000..419df09
--- /dev/null
+++ b/ell/fswatch.h
@@ -0,0 +1,44 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2018  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __ELL_FSWATCH_H
+#define __ELL_FSWATCH_H
+
+struct l_fswatch;
+
+enum l_fswatch_event {
+	L_FSWATCH_EVENT_CREATE,
+	L_FSWATCH_EVENT_MOVE,
+	L_FSWATCH_EVENT_MODIFY,
+	L_FSWATCH_EVENT_DELETE,
+};
+
+typedef void (*l_fswatch_cb_t) (struct l_fswatch *watch, const char *filename,
+				enum l_fswatch_event event, void *user_data);
+typedef void (*l_fswatch_destroy_cb_t) (void *user_data);
+
+struct l_fswatch *l_fswatch_new(const char *path, l_fswatch_cb_t cb,
+				void *user_data,
+				l_fswatch_destroy_cb_t destroy);
+void l_fswatch_destroy(struct l_fswatch *watch);
+
+#endif /* __ELL_FSWATCH_H */
-- 
2.14.1

Re: [PATCH 1/2] fswatch: Add fswatch API

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 8828 bytes --]

Hi Denis,

On 17 July 2018 at 21:33, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
> <snip>
>
>> +static struct l_io *inotify_io;
>> +static struct l_queue *watches;
>> +static bool in_notify;
>> +static bool stale_items;
>> +
>> +struct l_fswatch {
>> +       char *path;
>
>
> Why do we store the path?  It isn't used anywhere.

True, I was thinking about having an accessor similar to l_io_get_fd
but we would first need to figure out how to update the path on the
IN_MOVE_SELF event.

>
>> +       int id;
>> +       l_fswatch_cb_t cb;
>> +       void *user_data;
>> +};
>> +
>> +#define EVENT_MASK     (IN_CREATE | IN_DELETE | IN_DELETE_SELF | \
>> +                               IN_MODIFY | IN_MOVE | IN_MOVE_SELF)
>
>
> Should we expose some of this info?  Wouldn't it be useful to know what
> exactly changed for a given path?

It might be useful, I can add an enum with values such as
L_FSWATCH_EVENT_CREATE / MOVE / MODIFY / DELETE.

However relying on this info is seemingly discouraged by inotify(7)
because there's no race-free way to use it.  For our iwd use case we
probably want to always reload the affected files and having this
information isn't of any help.

>
>> +
>> +typedef void (*l_fswatch_cb_t) (struct l_fswatch *watch, const char
>> *filename,
>> +                               void *user_data);
>> +
>> +static void l_fswatch_destroy(void *data)
>> +{
>> +       struct l_fswatch *watch = data;
>> +
>> +       l_free(watch->path);
>> +       l_free(watch);
>> +}
>> +
>> +static void l_fswatch_shutdown(void)
>> +{
>> +       if (!inotify_io)
>> +               return;
>> +
>> +       l_io_destroy(inotify_io);
>> +       inotify_io = NULL;
>> +
>> +       l_queue_destroy(watches, l_fswatch_destroy);
>> +       watches = NULL;
>
>
> Every time this function is called the watches queue is empty.  It might be
> better to just hang the watches queue off of the inotify_io and provide a
> proper destroy callback.

We will still need a global variable for the queue because we need to
access it when adding or removing watches.

> That was the queue will be destroyed whenever the
> inotify_io is destroyed.  This probably also buys you auto-destruct at exit
> as well.

Ok, yep, it makes sense to move this part to the inotify_io's read
destroy callback.

>
>
>> +}
>> +
>> +static bool l_fswatch_remove_func(void *data, void *user_data)
>> +{
>> +       struct l_fswatch *watch = data;
>> +
>> +       if (watch->cb)
>> +               return false;
>> +
>> +       l_fswatch_destroy(watch);
>> +
>> +       return true;
>> +}
>> +
>> +static bool inotify_read_cb(struct l_io *io, void *user_data)
>> +{
>> +       uint8_t buf[sizeof(struct inotify_event) + NAME_MAX + 1];
>> +       const struct inotify_event *event;
>> +       ssize_t len;
>> +
>> +       len = read(l_io_get_fd(io), buf, sizeof(buf));
>> +       if (unlikely(len <= 0))
>> +               return true;
>> +
>> +       event = (struct inotify_event *) buf;
>> +       in_notify = true;
>> +
>> +       while ((size_t) len >= sizeof(struct inotify_event) + event->len)
>> {
>> +               const struct l_queue_entry *entry;
>> +               const uint8_t *next_ptr;
>> +               const char *name = NULL;
>> +
>> +               if (event->len)
>> +                       name = event->name;
>> +
>> +               for (entry = l_queue_get_entries(watches); entry;
>> +                               entry = entry->next) {
>> +                       struct l_fswatch *watch = entry->data;
>> +
>> +                       if (watch->id != event->wd)
>> +                               continue;
>> +
>> +                       if (event->mask & IN_IGNORED)
>> +                               watch->id = -1; > +
>> +                       if ((event->mask & EVENT_MASK) && watch->cb)
>> +                               watch->cb(watch, name, watch->user_data);
>
>
> This implies that watch->cb can be NULL?  See discussion below
>
>> +               }
>> +
>> +               len -= sizeof(struct inotify_event) + event->len;
>> +               /* event->len is supposed to include alignment */
>> +               next_ptr = (const uint8_t *) (event + 1) + event->len;
>> +               event = (struct inotify_event *) next_ptr;
>> +       }
>> +
>> +       in_notify = false;
>> +
>> +       if (stale_items) {
>> +               l_queue_foreach_remove(watches, l_fswatch_remove_func,
>> NULL);
>> +               stale_items = false;
>> +
>> +               if (l_queue_isempty(watches))
>> +                       l_fswatch_shutdown();
>> +       }
>> +
>> +       return true;
>> +}
>> +
>> +LIB_EXPORT struct l_fswatch *l_fswatch_new(const char *path,
>> l_fswatch_cb_t cb,
>> +                                               void *user_data)
>
>
> We really need a destroy callback here as well

Ok, then I guess I can actually free the watches for which the kernel
sends us IN_IGNORED events and call the destroy callbacks, rather than
just set watch->id to -1.

>
>> +{
>> +       struct l_fswatch *watch;
>> +       int id;
>> +
>> +       if (!inotify_io) {
>> +               int inotify_fd = inotify_init1(IN_CLOEXEC);
>> +
>> +               if (unlikely(inotify_fd < 0))
>> +                       return NULL;
>> +
>> +               inotify_io = l_io_new(inotify_fd);
>> +               if (unlikely(!inotify_io)) {
>> +                       close(inotify_fd);
>> +                       return NULL;
>> +               }
>> +
>> +               l_io_set_close_on_destroy(inotify_io, true);
>> +
>> +               if (unlikely(!l_io_set_read_handler(inotify_io,
>> inotify_read_cb,
>> +                                                       NULL, NULL))) {
>> +                       l_io_destroy(inotify_io);
>> +                       return NULL;
>> +               }
>
>
> Can we split this out into a separate function?  Most of these
> lazy-initialization details are irrelevant to l_fswatch_new, and if we ever
> move to a non-lazy initialization implementation, the changes will be
> compartmentalized better.

Ok.

>
>
>> +       }
>> +
>> +       /*
>> +        * inotify_watch_add already checks if the path is already being
>> +        * watched and will return the old watch ID so we're fine.
>> +        */
>> +       id = inotify_add_watch(l_io_get_fd(inotify_io), path,
>> +                                               EVENT_MASK |
>> IN_EXCL_UNLINK);
>> +       if (unlikely(id < 0)) {
>> +               if (l_queue_isempty(watches))
>> +                       l_fswatch_shutdown();
>> +
>> +               return NULL;
>> +       }
>> +
>> +       watch = l_new(struct l_fswatch, 1);
>> +       watch->path = l_strdup(path);
>> +       watch->id = id;
>> +       watch->cb = cb;
>> +       watch->user_data = user_data;
>> +
>> +       if (!watches)
>> +               watches = l_queue_new();
>> +
>> +       l_queue_push_tail(watches, watch);
>> +
>> +       return watch;
>> +}
>> +
>> +LIB_EXPORT bool l_fswatch_remove(struct l_fswatch *watch)
>
>
> Not sure why half of our APIs use _remove and the other half uses _destroy.
> But why would this return a bool?  That seems wrong, it should be void.

It's simliar to l_queue_remove and it may fail (in this case also
because of IO) although it's different from l_queue_remove in that it
also frees the object so I can rename it to _destroy.

>
>
>> +{
>> +       const struct l_queue_entry *entry;
>> +       int id;
>> +
>> +       if (unlikely(!inotify_io || !watch))
>> +               return false;
>> +
>> +       id = watch->id;
>> +
>> +       /* Check if we have any other watch with the same inotify ID */
>> +       for (entry = l_queue_get_entries(watches); entry;
>> +                       entry = entry->next) {
>> +               struct l_fswatch *watch2 = entry->data;
>> +
>> +               if (watch2 != watch && watch2->id == id)
>> +                       break;
>> +       }
>> +
>> +       if (!entry && id != -1)
>> +               if (inotify_rm_watch(l_io_get_fd(inotify_io), id) < 0)
>> +                       return false;
>> +
>> +       if (in_notify) {
>> +               watch->cb = NULL;
>> +               stale_items = false;
>
>
> should this be true actually?

Oops, yes it should.

I was actually testing this code path in the unit test but changed the
test before running it through valgrind.

> Also, you're not checking that cb is non-null
> in l_fswatch_new.  So we should either introduce a different guard variable
> or ensure that cb isn't NULL at first.

Ok.

Best regards

Re: [PATCH 1/2] fswatch: Add fswatch API

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 6036 bytes --]

Hi Andrew,

<snip>

> +static struct l_io *inotify_io;
> +static struct l_queue *watches;
> +static bool in_notify;
> +static bool stale_items;
> +
> +struct l_fswatch {
> +	char *path;

Why do we store the path?  It isn't used anywhere

> +	int id;
> +	l_fswatch_cb_t cb;
> +	void *user_data;
> +};
> +
> +#define EVENT_MASK	(IN_CREATE | IN_DELETE | IN_DELETE_SELF | \
> +				IN_MODIFY | IN_MOVE | IN_MOVE_SELF)

Should we expose some of this info?  Wouldn't it be useful to know what 
exactly changed for a given path?

> +
> +typedef void (*l_fswatch_cb_t) (struct l_fswatch *watch, const char *filename,
> +				void *user_data);
> +
> +static void l_fswatch_destroy(void *data)
> +{
> +	struct l_fswatch *watch = data;
> +
> +	l_free(watch->path);
> +	l_free(watch);
> +}
> +
> +static void l_fswatch_shutdown(void)
> +{
> +	if (!inotify_io)
> +		return;
> +
> +	l_io_destroy(inotify_io);
> +	inotify_io = NULL;
> +
> +	l_queue_destroy(watches, l_fswatch_destroy);
> +	watches = NULL;

Every time this function is called the watches queue is empty.  It might 
be better to just hang the watches queue off of the inotify_io and 
provide a proper destroy callback.  That was the queue will be destroyed 
whenever the inotify_io is destroyed.  This probably also buys you 
auto-destruct at exit as well.

> +}
> +
> +static bool l_fswatch_remove_func(void *data, void *user_data)
> +{
> +	struct l_fswatch *watch = data;
> +
> +	if (watch->cb)
> +		return false;
> +
> +	l_fswatch_destroy(watch);
> +
> +	return true;
> +}
> +
> +static bool inotify_read_cb(struct l_io *io, void *user_data)
> +{
> +	uint8_t buf[sizeof(struct inotify_event) + NAME_MAX + 1];
> +	const struct inotify_event *event;
> +	ssize_t len;
> +
> +	len = read(l_io_get_fd(io), buf, sizeof(buf));
> +	if (unlikely(len <= 0))
> +		return true;
> +
> +	event = (struct inotify_event *) buf;
> +	in_notify = true;
> +
> +	while ((size_t) len >= sizeof(struct inotify_event) + event->len) {
> +		const struct l_queue_entry *entry;
> +		const uint8_t *next_ptr;
> +		const char *name = NULL;
> +
> +		if (event->len)
> +			name = event->name;
> +
> +		for (entry = l_queue_get_entries(watches); entry;
> +				entry = entry->next) {
> +			struct l_fswatch *watch = entry->data;
> +
> +			if (watch->id != event->wd)
> +				continue;
> +
> +			if (event->mask & IN_IGNORED)
> +				watch->id = -1; > +
> +			if ((event->mask & EVENT_MASK) && watch->cb)
> +				watch->cb(watch, name, watch->user_data);

This implies that watch->cb can be NULL?  See discussion below

> +		}
> +
> +		len -= sizeof(struct inotify_event) + event->len;
> +		/* event->len is supposed to include alignment */
> +		next_ptr = (const uint8_t *) (event + 1) + event->len;
> +		event = (struct inotify_event *) next_ptr;
> +	}
> +
> +	in_notify = false;
> +
> +	if (stale_items) {
> +		l_queue_foreach_remove(watches, l_fswatch_remove_func, NULL);
> +		stale_items = false;
> +
> +		if (l_queue_isempty(watches))
> +			l_fswatch_shutdown();
> +	}
> +
> +	return true;
> +}
> +
> +LIB_EXPORT struct l_fswatch *l_fswatch_new(const char *path, l_fswatch_cb_t cb,
> +						void *user_data)

We really need a destroy callback here as well

> +{
> +	struct l_fswatch *watch;
> +	int id;
> +
> +	if (!inotify_io) {
> +		int inotify_fd = inotify_init1(IN_CLOEXEC);
> +
> +		if (unlikely(inotify_fd < 0))
> +			return NULL;
> +
> +		inotify_io = l_io_new(inotify_fd);
> +		if (unlikely(!inotify_io)) {
> +			close(inotify_fd);
> +			return NULL;
> +		}
> +
> +		l_io_set_close_on_destroy(inotify_io, true);
> +
> +		if (unlikely(!l_io_set_read_handler(inotify_io, inotify_read_cb,
> +							NULL, NULL))) {
> +			l_io_destroy(inotify_io);
> +			return NULL;
> +		}

Can we split this out into a separate function?  Most of these 
lazy-initialization details are irrelevant to l_fswatch_new, and if we 
ever move to a non-lazy initialization implementation, the changes will 
be compartmentalized better.

> +	}
> +
> +	/*
> +	 * inotify_watch_add already checks if the path is already being
> +	 * watched and will return the old watch ID so we're fine.
> +	 */
> +	id = inotify_add_watch(l_io_get_fd(inotify_io), path,
> +						EVENT_MASK | IN_EXCL_UNLINK);
> +	if (unlikely(id < 0)) {
> +		if (l_queue_isempty(watches))
> +			l_fswatch_shutdown();
> +
> +		return NULL;
> +	}
> +
> +	watch = l_new(struct l_fswatch, 1);
> +	watch->path = l_strdup(path);
> +	watch->id = id;
> +	watch->cb = cb;
> +	watch->user_data = user_data;
> +
> +	if (!watches)
> +		watches = l_queue_new();
> +
> +	l_queue_push_tail(watches, watch);
> +
> +	return watch;
> +}
> +
> +LIB_EXPORT bool l_fswatch_remove(struct l_fswatch *watch)

Not sure why half of our APIs use _remove and the other half uses 
_destroy.  But why would this return a bool?  That seems wrong, it 
should be void.

> +{
> +	const struct l_queue_entry *entry;
> +	int id;
> +
> +	if (unlikely(!inotify_io || !watch))
> +		return false;
> +
> +	id = watch->id;
> +
> +	/* Check if we have any other watch with the same inotify ID */
> +	for (entry = l_queue_get_entries(watches); entry;
> +			entry = entry->next) {
> +		struct l_fswatch *watch2 = entry->data;
> +
> +		if (watch2 != watch && watch2->id == id)
> +			break;
> +	}
> +
> +	if (!entry && id != -1)
> +		if (inotify_rm_watch(l_io_get_fd(inotify_io), id) < 0)
> +			return false;
> +
> +	if (in_notify) {
> +		watch->cb = NULL;
> +		stale_items = false;

should this be true actually?  Also, you're not checking that cb is 
non-null in l_fswatch_new.  So we should either introduce a different 
guard variable or ensure that cb isn't NULL at first.

> +		return true;
> +	}
> +
> +	l_queue_remove(watches, watch);
> +	l_fswatch_destroy(watch);
> +
> +	if (l_queue_isempty(watches))
> +		l_fswatch_shutdown();
> +
> +	return true;
> +}

Regards,
-Denis

[PATCH 1/2] fswatch: Add fswatch API

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 8457 bytes --]

Add the header and the implementation for a filesystem watch API.  It's
a simple wrapper around the Linux inotify mechanism but not named inotify
so as not to preclude using other mechanisms in the implemntation.
---
 Makefile.am   |   6 +-
 ell/ell.h     |   1 +
 ell/fswatch.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/fswatch.h |  35 +++++++++
 4 files changed, 271 insertions(+), 2 deletions(-)
 create mode 100644 ell/fswatch.c
 create mode 100644 ell/fswatch.h

diff --git a/Makefile.am b/Makefile.am
index 100e24c..8203fd4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -45,7 +45,8 @@ pkginclude_HEADERS = ell/ell.h \
 			ell/pkcs5.h \
 			ell/file.h \
 			ell/net.h \
-			ell/dhcp.h
+			ell/dhcp.h \
+			ell/fswatch.h
 
 lib_LTLIBRARIES = ell/libell.la
 
@@ -104,7 +105,8 @@ ell_libell_la_SOURCES = $(linux_headers) \
 			ell/dhcp-private.h \
 			ell/dhcp.c \
 			ell/dhcp-transport.c \
-			ell/dhcp-lease.c
+			ell/dhcp-lease.c \
+			ell/fswatch.c
 
 ell_libell_la_LDFLAGS = -no-undefined \
 			-version-info $(ELL_CURRENT):$(ELL_REVISION):$(ELL_AGE)
diff --git a/ell/ell.h b/ell/ell.h
index 03bb61d..c03faf2 100644
--- a/ell/ell.h
+++ b/ell/ell.h
@@ -53,3 +53,4 @@
 #include <ell/dbus-service.h>
 #include <ell/dbus-client.h>
 #include <ell/dhcp.h>
+#include <ell/fswatch.h>
diff --git a/ell/fswatch.c b/ell/fswatch.c
new file mode 100644
index 0000000..1ac80e4
--- /dev/null
+++ b/ell/fswatch.c
@@ -0,0 +1,231 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2018  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <dirent.h>
+#include <unistd.h>
+#include <sys/inotify.h>
+
+#include "private.h"
+#include "util.h"
+#include "io.h"
+#include "queue.h"
+#include "fswatch.h"
+
+static struct l_io *inotify_io;
+static struct l_queue *watches;
+static bool in_notify;
+static bool stale_items;
+
+struct l_fswatch {
+	char *path;
+	int id;
+	l_fswatch_cb_t cb;
+	void *user_data;
+};
+
+#define EVENT_MASK	(IN_CREATE | IN_DELETE | IN_DELETE_SELF | \
+				IN_MODIFY | IN_MOVE | IN_MOVE_SELF)
+
+typedef void (*l_fswatch_cb_t) (struct l_fswatch *watch, const char *filename,
+				void *user_data);
+
+static void l_fswatch_destroy(void *data)
+{
+	struct l_fswatch *watch = data;
+
+	l_free(watch->path);
+	l_free(watch);
+}
+
+static void l_fswatch_shutdown(void)
+{
+	if (!inotify_io)
+		return;
+
+	l_io_destroy(inotify_io);
+	inotify_io = NULL;
+
+	l_queue_destroy(watches, l_fswatch_destroy);
+	watches = NULL;
+}
+
+static bool l_fswatch_remove_func(void *data, void *user_data)
+{
+	struct l_fswatch *watch = data;
+
+	if (watch->cb)
+		return false;
+
+	l_fswatch_destroy(watch);
+
+	return true;
+}
+
+static bool inotify_read_cb(struct l_io *io, void *user_data)
+{
+	uint8_t buf[sizeof(struct inotify_event) + NAME_MAX + 1];
+	const struct inotify_event *event;
+	ssize_t len;
+
+	len = read(l_io_get_fd(io), buf, sizeof(buf));
+	if (unlikely(len <= 0))
+		return true;
+
+	event = (struct inotify_event *) buf;
+	in_notify = true;
+
+	while ((size_t) len >= sizeof(struct inotify_event) + event->len) {
+		const struct l_queue_entry *entry;
+		const uint8_t *next_ptr;
+		const char *name = NULL;
+
+		if (event->len)
+			name = event->name;
+
+		for (entry = l_queue_get_entries(watches); entry;
+				entry = entry->next) {
+			struct l_fswatch *watch = entry->data;
+
+			if (watch->id != event->wd)
+				continue;
+
+			if (event->mask & IN_IGNORED)
+				watch->id = -1;
+
+			if ((event->mask & EVENT_MASK) && watch->cb)
+				watch->cb(watch, name, watch->user_data);
+		}
+
+		len -= sizeof(struct inotify_event) + event->len;
+		/* event->len is supposed to include alignment */
+		next_ptr = (const uint8_t *) (event + 1) + event->len;
+		event = (struct inotify_event *) next_ptr;
+	}
+
+	in_notify = false;
+
+	if (stale_items) {
+		l_queue_foreach_remove(watches, l_fswatch_remove_func, NULL);
+		stale_items = false;
+
+		if (l_queue_isempty(watches))
+			l_fswatch_shutdown();
+	}
+
+	return true;
+}
+
+LIB_EXPORT struct l_fswatch *l_fswatch_new(const char *path, l_fswatch_cb_t cb,
+						void *user_data)
+{
+	struct l_fswatch *watch;
+	int id;
+
+	if (!inotify_io) {
+		int inotify_fd = inotify_init1(IN_CLOEXEC);
+
+		if (unlikely(inotify_fd < 0))
+			return NULL;
+
+		inotify_io = l_io_new(inotify_fd);
+		if (unlikely(!inotify_io)) {
+			close(inotify_fd);
+			return NULL;
+		}
+
+		l_io_set_close_on_destroy(inotify_io, true);
+
+		if (unlikely(!l_io_set_read_handler(inotify_io, inotify_read_cb,
+							NULL, NULL))) {
+			l_io_destroy(inotify_io);
+			return NULL;
+		}
+	}
+
+	/*
+	 * inotify_watch_add already checks if the path is already being
+	 * watched and will return the old watch ID so we're fine.
+	 */
+	id = inotify_add_watch(l_io_get_fd(inotify_io), path,
+						EVENT_MASK | IN_EXCL_UNLINK);
+	if (unlikely(id < 0)) {
+		if (l_queue_isempty(watches))
+			l_fswatch_shutdown();
+
+		return NULL;
+	}
+
+	watch = l_new(struct l_fswatch, 1);
+	watch->path = l_strdup(path);
+	watch->id = id;
+	watch->cb = cb;
+	watch->user_data = user_data;
+
+	if (!watches)
+		watches = l_queue_new();
+
+	l_queue_push_tail(watches, watch);
+
+	return watch;
+}
+
+LIB_EXPORT bool l_fswatch_remove(struct l_fswatch *watch)
+{
+	const struct l_queue_entry *entry;
+	int id;
+
+	if (unlikely(!inotify_io || !watch))
+		return false;
+
+	id = watch->id;
+
+	/* Check if we have any other watch with the same inotify ID */
+	for (entry = l_queue_get_entries(watches); entry;
+			entry = entry->next) {
+		struct l_fswatch *watch2 = entry->data;
+
+		if (watch2 != watch && watch2->id == id)
+			break;
+	}
+
+	if (!entry && id != -1)
+		if (inotify_rm_watch(l_io_get_fd(inotify_io), id) < 0)
+			return false;
+
+	if (in_notify) {
+		watch->cb = NULL;
+		stale_items = false;
+		return true;
+	}
+
+	l_queue_remove(watches, watch);
+	l_fswatch_destroy(watch);
+
+	if (l_queue_isempty(watches))
+		l_fswatch_shutdown();
+
+	return true;
+}
diff --git a/ell/fswatch.h b/ell/fswatch.h
new file mode 100644
index 0000000..1aa18ad
--- /dev/null
+++ b/ell/fswatch.h
@@ -0,0 +1,35 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2018  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __ELL_FSWATCH_H
+#define __ELL_FSWATCH_H
+
+struct l_fswatch;
+
+typedef void (*l_fswatch_cb_t) (struct l_fswatch *watch, const char *filename,
+				void *user_data);
+
+struct l_fswatch *l_fswatch_new(const char *path, l_fswatch_cb_t cb,
+				void *user_data);
+bool l_fswatch_remove(struct l_fswatch *watch);
+
+#endif /* __ELL_FSWATCH_H */
-- 
2.14.1