1:15 a.m.
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
1:15 a.m.
New subject: [PATCH 2/2] unit: Add an fswatch unit test
---
.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
3:55 p.m.
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
4:13 p.m.
Hi Denis,
On 20 July 2018 at 17:55, Denis Kenzior <denkenz(a)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
4:21 p.m.
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
4:29 p.m.
On 20 July 2018 at 18:21, Denis Kenzior <denkenz(a)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
5:16 p.m.
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
5:37 p.m.
On 20 July 2018 at 19:16, Denis Kenzior <denkenz(a)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
5:41 p.m.
On 20 July 2018 at 19:37, Andrew Zaborowski <andrew.zaborowski(a)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
5:55 p.m.
Hi Andrew,
On 07/20/2018 12:41 PM, Andrew Zaborowski wrote:
On 20 July 2018 at 19:37, Andrew Zaborowski
<andrew.zaborowski(a)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
1194
days inactive
1194
days old
9 comments
2 participants
participants (2)
-
Andrew Zaborowski -
Denis Kenzior