* [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-04 13:42 [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
@ 2023-05-04 13:42 ` Alexey Gladkov
2023-05-05 3:08 ` Leonid Krivoshein
` (8 more replies)
2023-05-04 13:42 ` [make-initrd] [PATCH 2/3] Replace polld by uevent queue Alexey Gladkov
` (7 subsequent siblings)
8 siblings, 9 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-04 13:42 UTC (permalink / raw)
To: make-initrd
The new C implementation of ueventd makes the logic more understandable.
It also allowed us to add more optimizations. For now, we are still
using the filesystem as the database because it allows us to accumulate
events before ueventd is started.
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
data/bin/uevent-sh-functions | 3 +
data/etc/rc.d/init.d/uevent | 9 +-
data/sbin/ueventd | 180 ----------
data/sbin/ueventd-queue | 65 ++++
datasrc/ueventd/Makefile.mk | 13 +
datasrc/ueventd/logging.c | 63 ++++
datasrc/ueventd/memory.c | 32 ++
datasrc/ueventd/path.c | 80 +++++
datasrc/ueventd/process.c | 17 +
datasrc/ueventd/queue-processor.c | 116 +++++++
datasrc/ueventd/ueventd.c | 551 ++++++++++++++++++++++++++++++
datasrc/ueventd/ueventd.h | 76 +++++
12 files changed, 1024 insertions(+), 181 deletions(-)
delete mode 100755 data/sbin/ueventd
create mode 100755 data/sbin/ueventd-queue
create mode 100644 datasrc/ueventd/Makefile.mk
create mode 100644 datasrc/ueventd/logging.c
create mode 100644 datasrc/ueventd/memory.c
create mode 100644 datasrc/ueventd/path.c
create mode 100644 datasrc/ueventd/process.c
create mode 100644 datasrc/ueventd/queue-processor.c
create mode 100644 datasrc/ueventd/ueventd.c
create mode 100644 datasrc/ueventd/ueventd.h
diff --git a/data/bin/uevent-sh-functions b/data/bin/uevent-sh-functions
index ff86d645..7f3947ae 100644
--- a/data/bin/uevent-sh-functions
+++ b/data/bin/uevent-sh-functions
@@ -33,6 +33,9 @@ release_event() {
done_event() {
rm -f -- "$1"
+
+ local message_time=1 queue="${QUEUE:--}" session="${SESSION:-0}"
+ message "$queue: session=$session: finish event: $1" ||:
}
fi
diff --git a/data/etc/rc.d/init.d/uevent b/data/etc/rc.d/init.d/uevent
index 8997ff8f..a8e6a8e6 100755
--- a/data/etc/rc.d/init.d/uevent
+++ b/data/etc/rc.d/init.d/uevent
@@ -20,7 +20,14 @@ PIDFILE=/var/run/$NAME.pid
ARGS="--lockfile $LOCKFILE --pidfile $PIDFILE --displayname $NAME --name $NAME"
start() {
- start_daemon --background $ARGS -- "$NAME"
+ export filterdir ueventdir uevent_confdb
+
+ mkdir -p -- \
+ "$filterdir" \
+ "$ueventdir" \
+ "$uevent_confdb/queue/pause"
+
+ start_daemon --background $ARGS -- "$NAME" /sbin/ueventd-queue
RETVAL=$?
return $RETVAL
}
diff --git a/data/sbin/ueventd b/data/sbin/ueventd
deleted file mode 100755
index d891016b..00000000
--- a/data/sbin/ueventd
+++ /dev/null
@@ -1,180 +0,0 @@
-#!/bin/bash -eu
-
-. shell-error
-. shell-signal
-
-. /.initrd/initenv
-. uevent-sh-functions
-
-message_time=1
-
-pidfile="/var/run/$PROG.pid"
-logfile="/var/log/$PROG.log"
-inotifyd='/sbin/inotifyd'
-
-[ "${RDLOG-}" != 'console' ] ||
- logfile=/dev/console
-
-UEVENT_MODE="${UEVENT_MODE:-server}"
-
-if [ "$UEVENT_MODE" = 'server' ]; then
- exec >"$logfile" 2>&1
-
- message "starting server ..."
-
- [ -d "$filterdir" ] ||
- fatal "$filterdir: bad directory"
-
- echo "$$" > "$pidfile"
-
- exit_handler()
- {
- local d rc="$?"
- trap - EXIT
- for d in "$filterdir"/*; do
- [ ! -d "$d" ] || [ ! -f "$d.pid" ] ||
- rm -f -- "$d.pid"
- done
- [ ! -f "$pidfile" ] ||
- rm -f -- "$pidfile"
- exit $rc
- }
- set_cleanup_handler exit_handler
-
- mkdir -p -- "$uevent_confdb/queue/pause"
-
- export UEVENT_MODE='queue-processor'
-
- for d in "$filterdir"/*; do
- [ ! -d "$d" ] || "$0" "n" "${d%/*}" "${d##*/}"
- done
-
- "$inotifyd" "$0" "$pidfile:x" "$filterdir:nd" &
- wait
-
- exit 0
-fi
-
-evtype="$1"
-name="${3-}"
-event="$2${name:+/$name}"
-
-if [ "$UEVENT_MODE" = 'queue-processor' ]; then
- case "$evtype" in
- n)
- [ -d "$event" ] && [ -n "${name##.*}" ] ||
- exit 0
-
- export UEVENT_MODE='queue-handler'
- export QUEUE="$name"
- export INDIR="$event"
-
- message "starting sub-process for '$QUEUE' queue ..."
-
- mkdir -p -- "$ueventdir/$QUEUE"
-
- :> "$event.startup"
- :> "$event.pid"
- :> "$event.timer"
-
- "$inotifyd" "$0" "$pidfile:x" "$event.pid:x" "$event.startup:0" "$event.timer:0" "$event:ny" &
-
- while [ -e "$event.startup" ]; do
- :<"$event.startup"
- sleep 0.1
- done
-
- echo "$!" >"$event.pid"
- ;;
- d)
- [ ! -d "$event" ] || [ ! -f "$event.pid" ] ||
- rm -f -- "$event.pid"
- ;;
- x)
- kill "$PPID"
- ;;
- esac
- exit 0
-fi
-
-[ "$UEVENT_MODE" = 'queue-handler' ] ||
- fatal "unexpected mode: $UEVENT_MODE"
-
-if [ "$2" = "$INDIR.startup" ]; then
- [ "$evtype" = '0' ] ||
- exit 0
- rm -f "$INDIR.startup"
-fi
-
-if [ "$evtype" = 'x' ]; then
- kill "$PPID"
- exit 0
-fi
-
-if [ -e "$uevent_confdb/queue/pause/.all" ] || [ -e "$uevent_confdb/queue/pause/$QUEUE" ]; then
- message "$QUEUE: queue paused"
- exit 0
-fi
-
-udevadm settle --timeout=3 ||:
-
-glob()
-{
- [ -e "$1" ]
-}
-
-queuedir="$ueventdir/$QUEUE"
-
-mv -f -- "$INDIR"/* "$queuedir" 2>/dev/null ||:
-glob "$queuedir"/* || exit 0
-
-[ "$evtype" != '0' ] || [ "$2" = "$INDIR.startup" ] ||
- message "$QUEUE: retrying with failed events ..."
-
-for ev in "$queuedir"/*; do
- message "$QUEUE: event $ev"
-done
-
-get_name()
-{
- [ ! -d "$fn" ] && [ -x "$fn" ] || return 1
- name="${fn##*/}"
- name="${name#[0-9][0-9][0-9]-}"
-}
-
-run_scripts()
-{
- local exe
-
- for exe in "/lib/uevent/each/$1"/*; do
- [ -x "$exe" ] || continue
- "$exe" "$2" ||:
- done
-}
-
-for fn in "$handlerdir/$QUEUE"/*; do
- get_name || continue
-
- run_scripts pre "$QUEUE"
-
- message "$QUEUE: running queue-specific handler: $fn"
- "$fn" "$queuedir" || message "$QUEUE: event handler failed: $fn"
-
- run_scripts post "$QUEUE"
-done
-
-for fn in "$handlerdir"/*; do
- get_name && glob "$queuedir/$name".* || continue
-
- run_scripts pre "$QUEUE"
-
- message "$QUEUE: running handler: $fn"
- "$fn" || message "$QUEUE: event handler failed: $fn"
-
- run_scripts post "$QUEUE"
-done
-
-if glob "$queuedir"/*; then
- sleep ${RDUEVENT_TIMEOUT:-1}
- :<"$filterdir/$QUEUE.timer"
-fi
diff --git a/data/sbin/ueventd-queue b/data/sbin/ueventd-queue
new file mode 100755
index 00000000..c106dc60
--- /dev/null
+++ b/data/sbin/ueventd-queue
@@ -0,0 +1,65 @@
+#!/bin/bash -eu
+
+. /.initrd/initenv
+
+[ "${RDLOG-}" != 'console' ] &&
+ logfile=/var/log/ueventd.log ||
+ logfile=/dev/console
+
+exec >"$logfile" 2>&1
+
+. shell-error
+. uevent-sh-functions
+
+message_time=1
+
+export queuedir="$1"
+export QUEUE="${queuedir##*/}"
+export SESSION="${SESSION:-0}"
+
+glob()
+{
+ [ -e "$1" ]
+}
+
+get_name()
+{
+ [ ! -d "$fn" ] && [ -x "$fn" ] || return 1
+ name="${fn##*/}"
+ name="${name#[0-9][0-9][0-9]-}"
+}
+
+run_scripts()
+{
+ local exe
+
+ for exe in "/lib/uevent/each/$1"/*; do
+ [ ! -x "$exe" ] || "$exe" "$2" ||:
+ done
+}
+
+for ev in "$queuedir"/*; do
+ message "$QUEUE: session=$SESSION: event $ev"
+done
+
+for fn in "$handlerdir/$QUEUE"/*; do
+ get_name || continue
+
+ run_scripts pre "$QUEUE"
+
+ message "$QUEUE: session=$SESSION: running queue-specific handler: $fn"
+ "$fn" "$queuedir" || message "$QUEUE: session=$SESSION: event handler failed: $fn"
+
+ run_scripts post "$QUEUE"
+done
+
+for fn in "$handlerdir"/*; do
+ get_name && glob "$queuedir/$name".* || continue
+
+ run_scripts pre "$QUEUE"
+
+ message "$QUEUE: session=$SESSION: running handler: $fn"
+ "$fn" || message "$QUEUE: session=$SESSION: event handler failed: $fn"
+
+ run_scripts post "$QUEUE"
+done
diff --git a/datasrc/ueventd/Makefile.mk b/datasrc/ueventd/Makefile.mk
new file mode 100644
index 00000000..887cc8d1
--- /dev/null
+++ b/datasrc/ueventd/Makefile.mk
@@ -0,0 +1,13 @@
+ueventd_DEST = $(dest_data_sbindir)/ueventd
+ueventd_SRCS = \
+ datasrc/ueventd/logging.c \
+ datasrc/ueventd/memory.c \
+ datasrc/ueventd/path.c \
+ datasrc/ueventd/process.c \
+ datasrc/ueventd/queue-processor.c \
+ datasrc/ueventd/ueventd.c \
+ datasrc/ueventd/ueventd.h
+
+ueventd_CFLAGS = -D_GNU_SOURCE
+
+PROGS += ueventd
diff --git a/datasrc/ueventd/logging.c b/datasrc/ueventd/logging.c
new file mode 100644
index 00000000..671f6814
--- /dev/null
+++ b/datasrc/ueventd/logging.c
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <stdio.h>
+#include <stdarg.h>
+#include <strings.h>
+#include <errno.h>
+#include <time.h>
+
+#include "ueventd.h"
+
+#define default_logfile "/var/log/ueventd.log"
+
+int log_priority = LOG_INFO;
+
+int logging_level(const char *name)
+{
+ if (!strcasecmp(name, "debug")) return LOG_DEBUG;
+ if (!strcasecmp(name, "info")) return LOG_INFO;
+ if (!strcasecmp(name, "warning")) return LOG_WARNING;
+ if (!strcasecmp(name, "error")) return LOG_ERR;
+ return log_priority;
+}
+
+void logging_init(int loglevel)
+{
+ if (!getenv("UEVENTD_USE_STDERR")) {
+ char *rdlog = getenv("RDLOG");
+ const char *logfile = default_logfile;
+
+ if (rdlog && !strcasecmp(rdlog, "console"))
+ logfile = "/dev/console";
+
+ FILE *cons = fopen(logfile, "w+");
+ if (!cons)
+ fatal("open(%s): %m", logfile);
+
+ fclose(stderr);
+ stderr = cons;
+ }
+
+ log_priority = loglevel;
+}
+
+void logging_close(void)
+{
+}
+
+void message(int priority, const char *fmt, ...)
+{
+ va_list ap;
+ va_start(ap, fmt);
+ if (priority <= log_priority) {
+ time_t ts = time(NULL);
+ struct tm *t = localtime(&ts);
+ fprintf(stderr, "[%04d-%02d-%02d %02d:%02d:%02d] %s: ",
+ t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
+ t->tm_hour, t->tm_min, t->tm_sec,
+ program_invocation_short_name);
+ vfprintf(stderr, fmt, ap);
+ fprintf(stderr, "\n");
+ }
+ va_end(ap);
+}
diff --git a/datasrc/ueventd/memory.c b/datasrc/ueventd/memory.c
new file mode 100644
index 00000000..cb911d58
--- /dev/null
+++ b/datasrc/ueventd/memory.c
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <string.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <stdio.h>
+#include <errno.h>
+#include <error.h>
+#include <limits.h>
+
+#include "ueventd.h"
+
+void *xcalloc(size_t nmemb, size_t size)
+{
+ void *r = calloc(nmemb, size);
+ if (!r)
+ fatal("calloc: allocating %lu*%lu bytes: %m",
+ (unsigned long) nmemb, (unsigned long) size);
+ return r;
+}
+
+char *xasprintf(char **ptr, const char *fmt, ...)
+{
+ va_list arg;
+
+ va_start(arg, fmt);
+ if (vasprintf(ptr, fmt, arg) < 0)
+ fatal("vasprintf: %m");
+ va_end(arg);
+
+ return *ptr;
+}
diff --git a/datasrc/ueventd/path.c b/datasrc/ueventd/path.c
new file mode 100644
index 00000000..7880ad5c
--- /dev/null
+++ b/datasrc/ueventd/path.c
@@ -0,0 +1,80 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <stdlib.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include "ueventd.h"
+
+int is_dot_dir(struct dirent *ent)
+{
+ return (ent->d_type == DT_DIR &&
+ (ent->d_name[0] == '.' && (ent->d_name[1] == '\0' ||
+ (ent->d_name[1] == '.' && ent->d_name[2] == '\0') )));
+}
+
+DIR *xopendir(const char *path)
+{
+ DIR *dir = opendir(path);
+ if (!dir)
+ fatal("opendir: %s: %m", path);
+ return dir;
+}
+
+
+struct dirent *xreaddir(DIR *d, const char *path)
+{
+ struct dirent *ent;
+
+ errno = 0;
+ ent = readdir(d);
+ if (!ent) {
+ if (!errno)
+ return NULL;
+ fatal("readdir: %s: %m", path);
+ }
+ return ent;
+}
+
+int empty_directory(const char *path)
+{
+ struct dirent *ent;
+ int is_empty = 1;
+ DIR *d = xopendir(path);
+
+ while ((ent = xreaddir(d, path)) != NULL) {
+ if (ent->d_name[0] != '.') {
+ is_empty = 0;
+ break;
+ }
+ }
+ closedir(d);
+
+ return is_empty;
+}
+
+ssize_t read_retry(int fd, void *buf, size_t count)
+{
+ return TEMP_FAILURE_RETRY(read(fd, buf, count));
+}
+
+ssize_t write_retry(int fd, const void *buf, size_t count)
+{
+ return TEMP_FAILURE_RETRY(write(fd, buf, count));
+}
+
+ssize_t write_loop(int fd, const char *buffer, size_t count)
+{
+ ssize_t offset = 0;
+
+ while (count > 0) {
+ ssize_t block = write_retry(fd, &buffer[offset], count);
+
+ if (block <= 0)
+ return offset ? : block;
+ offset += block;
+ count -= (size_t) block;
+ }
+ return offset;
+}
diff --git a/datasrc/ueventd/process.c b/datasrc/ueventd/process.c
new file mode 100644
index 00000000..9ff4ec4f
--- /dev/null
+++ b/datasrc/ueventd/process.c
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2022 Arseny Maslennikov <arseny@altlinux.org>
+ * All rights reserved.
+ */
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include <unistd.h>
+#include <errno.h>
+
+#include "ueventd.h"
+
+pid_t waitpid_retry(pid_t pid, int *wstatus, int options)
+{
+ return (pid_t) TEMP_FAILURE_RETRY(waitpid(pid, wstatus, options));
+}
diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
new file mode 100644
index 00000000..ab5e03e4
--- /dev/null
+++ b/datasrc/ueventd/queue-processor.c
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <signal.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include "ueventd.h"
+
+static void event_handler(struct watch *queue, char *path) __attribute__((nonnull(1, 2)));
+static void move_files(char *srcdir, char *dstdir) __attribute__((nonnull(1, 2)));
+static void signal_unhandled_events(struct watch *queue) __attribute__((nonnull(1)));
+
+void event_handler(struct watch *queue, char *path)
+{
+ char *argv[] = { handler_file, path, NULL };
+ pid_t pid = vfork();
+
+ if (pid < 0) {
+ err("fork: %m");
+
+ } else if (!pid) {
+ execve(argv[0], argv, environ);
+ fatal("exec: %s: %m", argv[0]);
+ } else {
+ int status = 0;
+
+ if (waitpid_retry(pid, &status, 0) != pid || !WIFEXITED(status))
+ info("%s: session=%lu: %s failed",
+ queue->q_name, session, handler_file);
+ else
+ info("%s: session=%lu: %s finished with return code %d",
+ queue->q_name, session, handler_file, WEXITSTATUS(status));
+ }
+}
+
+void move_files(char *srcdir, char *dstdir)
+{
+ struct dirent *ent;
+ int srcfd, dstfd;
+
+ if ((srcfd = open(srcdir, O_RDONLY | O_CLOEXEC)) < 0)
+ fatal("open: %s: %m", srcdir);
+
+ errno = 0;
+ if ((dstfd = open(dstdir, O_RDONLY | O_CLOEXEC)) < 0) {
+ if (errno == ENOENT) {
+ if (mkdir(dstdir, 0755) < 0)
+ fatal("mkdir: %s: %m", dstdir);
+ dstfd = open(dstdir, O_RDONLY | O_CLOEXEC);
+ }
+ if (dstfd < 0)
+ fatal("open: %s: %m", dstdir);
+ }
+
+ DIR *d = fdopendir(srcfd);
+ if (!d)
+ fatal("fdopendir: %m");
+
+ while ((ent = xreaddir(d, srcdir)) != NULL) {
+ if (ent->d_name[0] != '.' && ent->d_type == DT_REG &&
+ renameat(srcfd, ent->d_name, dstfd, ent->d_name) < 0)
+ fatal("rename `%s/%s' -> `%s/%s': %m", srcdir, ent->d_name, dstdir, ent->d_name);
+ }
+
+ closedir(d);
+ close(dstfd);
+}
+
+void signal_unhandled_events(struct watch *queue)
+{
+ ssize_t len;
+
+ len = write_loop(queue->q_parentfd,
+ (char *) &queue->q_watchfd, sizeof(queue->q_watchfd));
+ if (len < 0)
+ err("write(pipe): %m");
+
+ info("%s: session=%lu: retry with the events remaining in the queue", queue->q_name, session);
+}
+
+void process_events(struct watch *queue)
+{
+ info("%s: session=%lu: processing events", queue->q_name, session);
+
+ char *numenv = NULL;
+
+ xasprintf(&numenv, "SESSION=%lu", session);
+ putenv(numenv);
+
+ char *srcdir, *dstdir;
+
+ xasprintf(&srcdir, "%s/%s", filter_dir, queue->q_name);
+ xasprintf(&dstdir, "%s/%s", uevent_dir, queue->q_name);
+
+ move_files(srcdir, dstdir);
+
+ if (!empty_directory(dstdir)) {
+ event_handler(queue, dstdir);
+
+ if (!empty_directory(dstdir))
+ signal_unhandled_events(queue);
+ }
+
+ free(srcdir);
+ free(dstdir);
+
+ exit(0);
+}
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
new file mode 100644
index 00000000..f5923367
--- /dev/null
+++ b/datasrc/ueventd/ueventd.c
@@ -0,0 +1,551 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <sys/types.h>
+#include <sys/epoll.h>
+#include <sys/inotify.h>
+#include <sys/signalfd.h>
+#include <sys/prctl.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <getopt.h>
+#include <errno.h>
+
+#include "ueventd.h"
+
+char *uevent_confdb;
+char *filter_dir;
+char *uevent_dir;
+char *handler_file;
+uint64_t session = 0;
+
+static struct watch *watch_list = NULL;
+static int pause_all = 0;
+
+enum {
+ FD_INOTIFY,
+ FD_SIGNAL,
+ FD_PIPE,
+ FD_MAX,
+};
+
+typedef int (*fdhandler_t)(int);
+
+struct fd_handler {
+ int fd;
+ fdhandler_t fd_handler;
+};
+
+struct fd_handler fd_list[FD_MAX];
+
+enum {
+ PIPE_READ,
+ PIPE_WRITE,
+ PIPE_MAX,
+};
+
+int pipefd[PIPE_MAX];
+
+#define EV_PAUSE_MASK (IN_CREATE | IN_DELETE | IN_ONLYDIR)
+#define EV_ROOT_MASK (IN_CREATE | IN_ONLYDIR)
+#define EV_QUEUE_MASK (IN_CLOSE_WRITE | IN_MOVED_TO | IN_DELETE_SELF)
+#define EV_MAX (FD_MAX * 32)
+
+static int add_queue_dir(int inotifyfd, const char *path, uint32_t mask) __attribute__((nonnull(2)));
+static void watch_pauses(int inotifyfd);
+static void watch_queues(int inotifyfd);
+static int watch_path(int inotifyfd, const char *dir, const char *name, uint32_t mask, int flags) __attribute__((nonnull(2, 3)));
+static int unwatch_path(int inotifyfd, ino_t ino);
+static void apply_pause(void);
+static int process_signal_events(int signfd);
+static int process_inotify_events(int inotifyfd);
+static int process_pipefd_events(int readfd);
+static void setup_signal_fd(struct fd_handler *el) __attribute__((nonnull(1)));
+static void setup_inotify_fd(struct fd_handler *el) __attribute__((nonnull(1)));
+static void setup_pipe_fd(struct fd_handler *el) __attribute__((nonnull(1)));
+static int setup_epoll_fd(struct fd_handler list[FD_MAX]);
+static pid_t spawn_worker(int epollfd, struct watch *queue) __attribute__((nonnull(2)));
+static inline char *get_param_dir(const char *name) __attribute__((nonnull(1)));
+
+int add_queue_dir(int inotifyfd, const char *path, uint32_t mask)
+{
+ int ret;
+
+ errno = 0;
+ ret = inotify_add_watch(inotifyfd, path, mask | IN_MASK_CREATE);
+ if (ret < 0) {
+ if (errno == EEXIST) {
+ return -128;
+ }
+ err("inotify failed to watch: %s: %m", path);
+ }
+ return ret;
+}
+
+int watch_path(int inotifyfd, const char *dir, const char *name, uint32_t mask, int flags)
+{
+ struct stat st;
+ struct watch *new = NULL;
+ char *path = NULL;
+
+ xasprintf(&path, "%s/%s", dir, name);
+
+ int wfd = add_queue_dir(inotifyfd, path, mask);
+ if (wfd < 0)
+ return (wfd == -128 ? 0 : wfd);
+
+ if (stat(path, &st) < 0) {
+ err("stat: %s: %m", path);
+ goto fail;
+ }
+
+ new = xcalloc(1, sizeof(*new) + sizeof(new->q_name[0]) * (strlen(name) + 1));
+
+ strcpy(new->q_name, name);
+
+ new->q_watchfd = wfd;
+ new->q_ino = st.st_ino;
+ new->q_flags = flags;
+ new->next = watch_list;
+
+ if (flags & F_QUEUE_DIR) {
+ if (!empty_directory(path))
+ new->q_flags |= F_DIRTY;
+ new->q_parentfd = pipefd[PIPE_WRITE];
+ }
+
+ if (pause_all)
+ new->q_flags |= F_PAUSED;
+
+ watch_list = new;
+
+ info("watch path: %s", path);
+ free(path);
+ return 0;
+fail:
+ inotify_rm_watch(inotifyfd, wfd);
+ free(new);
+ free(path);
+ return -1;
+}
+
+int unwatch_path(int inotifyfd, ino_t ino)
+{
+ struct watch *prev, *elem;
+
+ prev = NULL;
+ elem = watch_list;
+
+ while (elem) {
+ if (elem->q_ino == ino) {
+ if (prev)
+ prev->next = elem->next;
+ if (elem == watch_list)
+ watch_list = elem->next;
+ inotify_rm_watch(inotifyfd, elem->q_watchfd);
+ free(elem);
+ break;
+ }
+
+ prev = elem;
+ elem = elem->next;
+ }
+
+ return 0;
+}
+
+void watch_pauses(int inotifyfd)
+{
+ char *path = NULL;
+
+ xasprintf(&path, "%s/queue/pause", uevent_confdb);
+
+ if (watch_path(inotifyfd, path, ".", EV_PAUSE_MASK, F_PAUSE_DIR) < 0)
+ exit(EXIT_FAILURE);
+
+ free(path);
+}
+
+void apply_pause(void)
+{
+ struct watch *p;
+ DIR *dir;
+ char *path = NULL;
+
+ xasprintf(&path, "%s/queue/pause", uevent_confdb);
+
+ dir = xopendir(path);
+ pause_all = 0;
+
+ for (p = watch_list; p; p = p->next) {
+ if (p->q_flags & F_PAUSED)
+ p->q_flags &= ~F_PAUSED;
+ }
+
+ struct dirent *ent;
+
+ while ((ent = xreaddir(dir, path)) != NULL) {
+ if (ent->d_type != DT_DIR || is_dot_dir(ent))
+ continue;
+
+ if (!strcmp(ent->d_name, ".all"))
+ pause_all = 1;
+
+ for (p = watch_list; p; p = p->next) {
+ if ((p->q_flags & F_QUEUE_DIR) &&
+ (pause_all || !strcmp(ent->d_name, p->q_name)))
+ p->q_flags |= F_PAUSED;
+ }
+
+ if (pause_all)
+ break;
+ }
+ closedir(dir);
+ free(path);
+}
+
+void watch_queues(int inotifyfd)
+{
+ DIR *dir = opendir(filter_dir);
+ if (!dir)
+ fatal("opendir: %s: %m", filter_dir);
+
+ struct dirent *ent;
+
+ while ((ent = xreaddir(dir, filter_dir)) != NULL) {
+ if (ent->d_type != DT_DIR || is_dot_dir(ent))
+ continue;
+
+ if (watch_path(inotifyfd, filter_dir, ent->d_name, EV_QUEUE_MASK, F_QUEUE_DIR) < 0)
+ fatal("unable to start watching: %s/%s", filter_dir, ent->d_name);
+ }
+ closedir(dir);
+}
+
+int process_signal_events(int signfd)
+{
+ struct watch *p;
+ struct signalfd_siginfo fdsi = { 0 };
+ ssize_t size;
+
+ size = read_retry(signfd, &fdsi, sizeof(fdsi));
+ if (size != sizeof(fdsi)) {
+ err("unable to read signal info");
+ return 0;
+ }
+
+ if (fdsi.ssi_signo == SIGCHLD) {
+ while (1) {
+ pid_t pid = waitpid_retry(-1, NULL, WNOHANG);
+
+ if (pid <= 0) {
+ if (pid < 0 && errno != ECHILD) {
+ err("waitpid: %m");
+ return -1;
+ }
+ break;
+ }
+
+ for (p = watch_list; p; p = p->next) {
+ if (p->q_pid == pid)
+ p->q_pid = 0;
+ }
+ }
+ return 0;
+ }
+
+ info("got signal %d, exit", fdsi.ssi_signo);
+ return -1;
+}
+
+void setup_signal_fd(struct fd_handler *el)
+{
+ sigset_t mask;
+
+ sigemptyset(&mask);
+
+ sigaddset(&mask, SIGHUP);
+ sigaddset(&mask, SIGINT);
+ sigaddset(&mask, SIGQUIT);
+ sigaddset(&mask, SIGTERM);
+ sigaddset(&mask, SIGCHLD);
+
+ if ((el->fd = signalfd(-1, &mask, SFD_NONBLOCK | SFD_CLOEXEC)) < 0)
+ fatal("signalfd: %m");
+
+ if (sigprocmask(SIG_BLOCK, &mask, NULL) < 0)
+ fatal("sigprocmask: %m");
+
+ el->fd_handler = process_signal_events;
+}
+
+int process_inotify_events(int inotifyfd)
+{
+ char buf[4096] __attribute__ ((aligned(__alignof__(struct inotify_event))));
+
+ while (1) {
+ ssize_t len = read_retry(inotifyfd, buf, sizeof(buf));
+
+ if (len < 0) {
+ if (errno == EAGAIN)
+ break;
+ fatal("read: %m");
+ } else if (!len) {
+ break;
+ }
+
+ struct inotify_event *event;
+ char *ptr;
+
+ for (ptr = buf; ptr < buf + len; ptr += sizeof(*event) + event->len) {
+ struct watch *p;
+
+ event = (struct inotify_event *) ptr;
+
+ for (p = watch_list; p && (p->q_watchfd != event->wd); p = p->next);
+ if (!p)
+ continue;
+
+ if (event->mask & IN_DELETE_SELF) {
+ info("unwatch path: %s", p->q_name);
+ unwatch_path(inotifyfd, p->q_ino);
+ continue;
+ }
+
+ if (p->q_flags & F_ROOT_DIR) {
+ if (event->mask & IN_CREATE)
+ watch_queues(inotifyfd);
+ continue;
+ }
+
+ if (p->q_flags & F_PAUSE_DIR) {
+ if (event->mask & IN_CREATE)
+ info("%s: queue paused", event->name);
+ else if (event->mask & IN_DELETE)
+ info("%s: queue unpaused", event->name);
+ apply_pause();
+ continue;
+ }
+
+ if (!(p->q_flags & F_DIRTY))
+ p->q_flags |= F_DIRTY;
+ }
+ }
+ return 0;
+}
+
+void setup_inotify_fd(struct fd_handler *el)
+{
+ if ((el->fd = inotify_init1(IN_NONBLOCK | IN_CLOEXEC)) < 0)
+ fatal("inotify_init1: %m");
+
+ el->fd_handler = process_inotify_events;
+}
+
+int process_pipefd_events(int readfd)
+{
+ while (1) {
+ struct watch *p;
+ ssize_t len;
+ int value = 0;
+
+ errno = 0;
+ len = read_retry(readfd, &value, sizeof(value));
+
+ if (len < 0) {
+ if (errno == EAGAIN)
+ break;
+ fatal("read(pipe): %m");
+ } else if (!len) {
+ break;
+ }
+
+ for (p = watch_list; p; p = p->next) {
+ if (!(p->q_flags & F_QUEUE_DIR) || p->q_watchfd != value)
+ continue;
+ if (!(p->q_flags & F_DIRTY))
+ p->q_flags |= F_DIRTY;
+ break;
+ }
+ }
+
+ return 0;
+}
+
+void setup_pipe_fd(struct fd_handler *el)
+{
+ if (pipe2(pipefd, O_NONBLOCK | O_CLOEXEC) < 0)
+ fatal("pipe2: %m");
+
+ el->fd = pipefd[PIPE_READ];
+ el->fd_handler = process_pipefd_events;
+}
+
+int setup_epoll_fd(struct fd_handler list[FD_MAX])
+{
+ int epollfd;
+ struct epoll_event ev = { .events = EPOLLIN };
+
+ if ((epollfd = epoll_create1(EPOLL_CLOEXEC)) < 0)
+ fatal("epoll_create1: %m");
+
+ for (int i = 0; i < FD_MAX; i++) {
+ ev.data.fd = list[i].fd;
+
+ if (epoll_ctl(epollfd, EPOLL_CTL_ADD, list[i].fd, &ev) < 0)
+ fatal("epoll_ctl: %m");
+ }
+
+ return epollfd;
+}
+
+pid_t spawn_worker(int epollfd, struct watch *queue)
+{
+ pid_t pid;
+
+ session++;
+
+ pid = fork();
+ if (pid < 0) {
+ err("fork: %m");
+ return 0;
+ } else if (pid > 0) {
+ return pid;
+ }
+
+ if (prctl(PR_SET_PDEATHSIG, SIGKILL) < 0)
+ fatal("prctl(PR_SET_PDEATHSIG): %m");
+
+ for (int i = 0; i < FD_MAX; i++) {
+ if (fd_list[i].fd >= 0)
+ close(fd_list[i].fd);
+ }
+ close(epollfd);
+
+ sigset_t mask;
+ sigemptyset(&mask);
+
+ if (sigprocmask(SIG_SETMASK, &mask, NULL) < 0)
+ fatal("sigprocmask: %m");
+
+ process_events(queue);
+
+ return 0;
+}
+
+char *get_param_dir(const char *name)
+{
+ char *value = getenv(name);
+ if (!value)
+ fatal("please set `%s' env variable", name);
+ return value;
+}
+
+int main(int argc, char **argv)
+{
+ struct watch *e, *n;
+ int i, epollfd;
+ int loglevel = LOG_INFO;
+
+ while ((i = getopt(argc, argv, "hl:")) != -1) {
+ switch (i) {
+ case 'h':
+ fprintf(stderr, "Usage: %s [-l loglevel] handler-prog\n",
+ program_invocation_short_name);
+ exit(0);
+ break;
+ case 'l':
+ loglevel = logging_level(optarg);
+ break;
+ default:
+ exit(1);
+ break;
+ }
+ }
+
+ if (optind == argc)
+ fatal("specify handler program");
+
+ logging_init(loglevel);
+
+ info("starting server ...");
+
+ filter_dir = get_param_dir("filterdir");
+ uevent_dir = get_param_dir("ueventdir");
+ uevent_confdb = get_param_dir("uevent_confdb");
+ handler_file = argv[optind++];
+
+ setup_inotify_fd(&fd_list[FD_INOTIFY]);
+ setup_signal_fd(&fd_list[FD_SIGNAL]);
+ setup_pipe_fd(&fd_list[FD_PIPE]);
+
+ epollfd = setup_epoll_fd(fd_list);
+
+ watch_pauses(fd_list[FD_INOTIFY].fd);
+
+ if (watch_path(fd_list[FD_INOTIFY].fd, filter_dir, ".", EV_ROOT_MASK, F_ROOT_DIR) < 0)
+ exit(EXIT_FAILURE);
+
+ watch_queues(fd_list[FD_INOTIFY].fd);
+ apply_pause();
+
+ while (1) {
+ struct epoll_event ev[EV_MAX];
+ int fdcount;
+
+ errno = 0;
+ fdcount = epoll_wait(epollfd, ev, EV_MAX, 250);
+
+ if (fdcount < 0) {
+ if (errno == EINTR)
+ continue;
+ fatal("epoll_wait: %m");
+ }
+
+ for (i = 0; i < fdcount; i++) {
+ if (!(ev[i].events & EPOLLIN))
+ continue;
+ for (int k = 0; k < FD_MAX; k++) {
+ if (ev[i].data.fd != fd_list[k].fd)
+ continue;
+ if (fd_list[k].fd_handler(fd_list[k].fd) != 0)
+ goto done;
+ }
+ }
+
+ for (e = watch_list; e; e = e->next) {
+ if (!(e->q_flags & F_QUEUE_DIR) || !(e->q_flags & F_DIRTY) || (e->q_pid != 0) || (e->q_flags & F_PAUSED))
+ continue;
+ e->q_flags &= ~F_DIRTY;
+ e->q_pid = spawn_worker(epollfd, e);
+ }
+ }
+done:
+ for (e = watch_list, n = NULL; e; e = n) {
+ n = e->next;
+ if (e->q_pid)
+ kill(e->q_pid, SIGKILL);
+ inotify_rm_watch(fd_list[FD_INOTIFY].fd, e->q_watchfd);
+ free(e);
+ }
+
+ for (i = 0; i < FD_MAX; i++) {
+ if (fd_list[i].fd >= 0) {
+ if (epoll_ctl(epollfd, EPOLL_CTL_DEL, fd_list[i].fd, NULL) < 0)
+ err("epoll_ctl: %m");
+ close(fd_list[i].fd);
+ }
+ }
+ close(epollfd);
+ logging_close();
+
+ return EXIT_SUCCESS;
+}
diff --git a/datasrc/ueventd/ueventd.h b/datasrc/ueventd/ueventd.h
new file mode 100644
index 00000000..a5e50379
--- /dev/null
+++ b/datasrc/ueventd/ueventd.h
@@ -0,0 +1,76 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef __UEVENTD_H__
+#define __UEVENTD_H__
+
+#include <sys/types.h>
+#include <stdint.h>
+
+#define F_ROOT_DIR (1 << 0)
+#define F_QUEUE_DIR (1 << 1)
+#define F_PAUSE_DIR (1 << 2)
+#define F_DIRTY (1 << 3)
+#define F_PAUSED (1 << 4)
+
+struct watch {
+ struct watch *next;
+ int q_flags;
+ int q_watchfd;
+ int q_parentfd;
+ ino_t q_ino;
+ pid_t q_pid;
+ char q_name[];
+};
+
+extern char *filter_dir;
+extern char *uevent_dir;
+extern char *handler_file;
+extern uint64_t session;
+
+/* memory.c */
+extern void *xcalloc(size_t nmemb, size_t size) __attribute__((alloc_size(1, 2)));
+extern char *xasprintf(char **ptr, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
+
+/* queue-processor.c */
+extern void process_events(struct watch *queue) __attribute__((nonnull(1), noreturn));
+
+/* path.c */
+#include <dirent.h>
+
+extern DIR *xopendir(const char *path) __attribute__((nonnull(1)));
+extern struct dirent *xreaddir(DIR *d, const char *path) __attribute__((nonnull(1, 2)));
+extern int empty_directory(const char *path) __attribute__((nonnull(1)));
+extern ssize_t read_retry(int fd, void *buf, size_t count) __attribute__((nonnull(2)));
+extern ssize_t write_retry(int fd, const void *buf, size_t count) __attribute__((nonnull(2)));
+extern ssize_t write_loop(int fd, const char *buffer, size_t count) __attribute__((nonnull(2)));
+
+/* process.c */
+extern pid_t waitpid_retry(pid_t pid, int *wstatus, int options);
+
+#include <dirent.h>
+
+extern int is_dot_dir(struct dirent *ent) __attribute__((nonnull(1)));
+
+/* logging.c */
+#include <unistd.h>
+#include <syslog.h>
+#include <stdlib.h>
+
+extern void logging_init(int level);
+extern void logging_close(void);
+extern int logging_level(const char *lvl) __attribute__((nonnull(1)));
+extern void message(int priority, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
+
+#define __message(level, format, arg...) message(level, format, ##arg)
+
+#define fatal(format, arg...) \
+ do { \
+ __message(LOG_CRIT, "%s:%d: " format, __FILE__, __LINE__, ##arg); \
+ _exit(EXIT_FAILURE); \
+ } while (0)
+
+#define err(format, arg...) __message(LOG_ERR, format, ##arg)
+#define info(format, arg...) __message(LOG_INFO, format, ##arg)
+#define dbg(format, arg...) __message(LOG_DEBUG, format, ##arg)
+
+#endif /* __UEVENTD_H__ */
--
2.33.7
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
@ 2023-05-05 3:08 ` Leonid Krivoshein
2023-05-05 17:02 ` Alexey Gladkov
2023-05-05 4:03 ` Leonid Krivoshein
` (7 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-05 3:08 UTC (permalink / raw)
To: make-initrd
On 5/4/23 16:42, Alexey Gladkov wrote:
> [...]
> diff --git a/datasrc/ueventd/logging.c b/datasrc/ueventd/logging.c
> new file mode 100644
> index 00000000..671f6814
> --- /dev/null
> +++ b/datasrc/ueventd/logging.c
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <strings.h>
> +#include <errno.h>
> +#include <time.h>
> +
> +#include "ueventd.h"
> +
> +#define default_logfile "/var/log/ueventd.log"
> +
> +int log_priority = LOG_INFO;
> +
> [...]
> +void message(int priority, const char *fmt, ...)
> +{
> + va_list ap;
> + va_start(ap, fmt);
> + if (priority <= log_priority) {
> + time_t ts = time(NULL);
> + struct tm *t = localtime(&ts);
> + fprintf(stderr, "[%04d-%02d-%02d %02d:%02d:%02d] %s: ",
> + t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
> + t->tm_hour, t->tm_min, t->tm_sec,
> + program_invocation_short_name);
Кажется, есть риск спутать сообщения от разных потоков, тут должен быть
целостный буферизированный вывод через snprintf(). Мой комментарий как
раз в месте потенциального разрыва. Если журналирование выстраивается в
один поток выполнения, проблемы нет.
> + vfprintf(stderr, fmt, ap);
> + fprintf(stderr, "\n");
> + }
> + va_end(ap);
> +}
> [...]
--
WBR, Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-05 3:08 ` Leonid Krivoshein
@ 2023-05-05 17:02 ` Alexey Gladkov
0 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-05 17:02 UTC (permalink / raw)
To: make-initrd
On Fri, May 05, 2023 at 06:08:25AM +0300, Leonid Krivoshein wrote:
>
> On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > diff --git a/datasrc/ueventd/logging.c b/datasrc/ueventd/logging.c
> > new file mode 100644
> > index 00000000..671f6814
> > --- /dev/null
> > +++ b/datasrc/ueventd/logging.c
> > @@ -0,0 +1,63 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +#include <strings.h>
> > +#include <errno.h>
> > +#include <time.h>
> > +
> > +#include "ueventd.h"
> > +
> > +#define default_logfile "/var/log/ueventd.log"
> > +
> > +int log_priority = LOG_INFO;
> > +
> > [...]
> > +void message(int priority, const char *fmt, ...)
> > +{
> > + va_list ap;
> > + va_start(ap, fmt);
> > + if (priority <= log_priority) {
> > + time_t ts = time(NULL);
> > + struct tm *t = localtime(&ts);
> > + fprintf(stderr, "[%04d-%02d-%02d %02d:%02d:%02d] %s: ",
> > + t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
> > + t->tm_hour, t->tm_min, t->tm_sec,
> > + program_invocation_short_name);
>
> Кажется, есть риск спутать сообщения от разных потоков, тут должен быть
> целостный буферизированный вывод через snprintf(). Мой комментарий как
> раз в месте потенциального разрыва. Если журналирование выстраивается в
> один поток выполнения, проблемы нет.
Ты абсолютно прав. Тут вообще всё нужно делать иначе. Это нужно делать
через writev например.
> > + vfprintf(stderr, fmt, ap);
> > + fprintf(stderr, "\n");
> > + }
> > + va_end(ap);
> > +}
> > [...]
>
>
> --
> WBR, Leonid Krivoshein.
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
2023-05-05 3:08 ` Leonid Krivoshein
@ 2023-05-05 4:03 ` Leonid Krivoshein
2023-05-05 17:16 ` Alexey Gladkov
2023-05-05 5:21 ` Leonid Krivoshein
` (6 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-05 4:03 UTC (permalink / raw)
To: make-initrd
On 5/4/23 16:42, Alexey Gladkov wrote:
> [...]
> diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
> new file mode 100644
> index 00000000..ab5e03e4
> --- /dev/null
> +++ b/datasrc/ueventd/queue-processor.c
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <dirent.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +#include "ueventd.h"
> +
> +static void event_handler(struct watch *queue, char *path) __attribute__((nonnull(1, 2)));
> +static void move_files(char *srcdir, char *dstdir) __attribute__((nonnull(1, 2)));
> +static void signal_unhandled_events(struct watch *queue) __attribute__((nonnull(1)));
> +
> +void event_handler(struct watch *queue, char *path)
> +{
> + char *argv[] = { handler_file, path, NULL };
> + pid_t pid = vfork();
> +
> + if (pid < 0) {
> + err("fork: %m");
Сначала я подумал, что тут пропущен первый аргумент и таких мест
оказалось много. err() -- и твой макрос из ueventd.h, и стандартный
вызов из <err.h> с отличающейся семантикой. В новом коде ueventd
используется твой макрос, но в остальном коде make-initrd используется
стандартный вызов. Так недолго самому запутаться, см.: git grep 'err('.
> +
> + } else if (!pid) {
> + execve(argv[0], argv, environ);
> + fatal("exec: %s: %m", argv[0]);
> + } else {
> + int status = 0;
> +
> + if (waitpid_retry(pid, &status, 0) != pid || !WIFEXITED(status))
> + info("%s: session=%lu: %s failed",
> + queue->q_name, session, handler_file);
> + else
> + info("%s: session=%lu: %s finished with return code %d",
> + queue->q_name, session, handler_file, WEXITSTATUS(status));
> + }
> +}
> [...]
--
WBR, Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-05 4:03 ` Leonid Krivoshein
@ 2023-05-05 17:16 ` Alexey Gladkov
0 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-05 17:16 UTC (permalink / raw)
To: make-initrd
On Fri, May 05, 2023 at 07:03:16AM +0300, Leonid Krivoshein wrote:
>
> On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
> > new file mode 100644
> > index 00000000..ab5e03e4
> > --- /dev/null
> > +++ b/datasrc/ueventd/queue-processor.c
> > @@ -0,0 +1,116 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <sys/wait.h>
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <signal.h>
> > +#include <dirent.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +
> > +#include "ueventd.h"
> > +
> > +static void event_handler(struct watch *queue, char *path) __attribute__((nonnull(1, 2)));
> > +static void move_files(char *srcdir, char *dstdir) __attribute__((nonnull(1, 2)));
> > +static void signal_unhandled_events(struct watch *queue) __attribute__((nonnull(1)));
> > +
> > +void event_handler(struct watch *queue, char *path)
> > +{
> > + char *argv[] = { handler_file, path, NULL };
> > + pid_t pid = vfork();
> > +
> > + if (pid < 0) {
> > + err("fork: %m");
>
> Сначала я подумал, что тут пропущен первый аргумент и таких мест
> оказалось много. err() -- и твой макрос из ueventd.h, и стандартный
> вызов из <err.h> с отличающейся семантикой. В новом коде ueventd
> используется твой макрос, но в остальном коде make-initrd используется
> стандартный вызов. Так недолго самому запутаться, см.: git grep 'err('.
Да, ты прав. Название неудачное. Сначала сделал со стандартными, но потом
пришлось делать свои так как ни err.h, ни syslog не умеют время выводить.
> > +
> > + } else if (!pid) {
> > + execve(argv[0], argv, environ);
> > + fatal("exec: %s: %m", argv[0]);
> > + } else {
> > + int status = 0;
> > +
> > + if (waitpid_retry(pid, &status, 0) != pid || !WIFEXITED(status))
> > + info("%s: session=%lu: %s failed",
> > + queue->q_name, session, handler_file);
> > + else
> > + info("%s: session=%lu: %s finished with return code %d",
> > + queue->q_name, session, handler_file, WEXITSTATUS(status));
> > + }
> > +}
> > [...]
>
>
> --
> WBR, Leonid Krivoshein.
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
2023-05-05 3:08 ` Leonid Krivoshein
2023-05-05 4:03 ` Leonid Krivoshein
@ 2023-05-05 5:21 ` Leonid Krivoshein
2023-05-05 17:24 ` Alexey Gladkov
2023-05-14 20:12 ` Leonid Krivoshein
` (5 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-05 5:21 UTC (permalink / raw)
To: make-initrd
On 5/4/23 16:42, Alexey Gladkov wrote:
> [...]
> diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
> new file mode 100644
> index 00000000..ab5e03e4
> --- /dev/null
> +++ b/datasrc/ueventd/queue-processor.c
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <dirent.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +#include "ueventd.h"
> +
> [...]
> +void move_files(char *srcdir, char *dstdir)
> +{
> + struct dirent *ent;
> + int srcfd, dstfd;
> +
> + if ((srcfd = open(srcdir, O_RDONLY | O_CLOEXEC)) < 0)
> + fatal("open: %s: %m", srcdir);
> +
> + errno = 0;
> + if ((dstfd = open(dstdir, O_RDONLY | O_CLOEXEC)) < 0) {
> + if (errno == ENOENT) {
open() тоже прерываемый примитив, но его ты оборачивать не стал. Так
задумано?
> + if (mkdir(dstdir, 0755) < 0)
> + fatal("mkdir: %s: %m", dstdir);
> + dstfd = open(dstdir, O_RDONLY | O_CLOEXEC);
> + }
> + if (dstfd < 0)
> + fatal("open: %s: %m", dstdir);
> + }
> +
> + DIR *d = fdopendir(srcfd);
> + if (!d)
> + fatal("fdopendir: %m");
> +
> + while ((ent = xreaddir(d, srcdir)) != NULL) {
> + if (ent->d_name[0] != '.' && ent->d_type == DT_REG &&
> + renameat(srcfd, ent->d_name, dstfd, ent->d_name) < 0)
> + fatal("rename `%s/%s' -> `%s/%s': %m", srcdir, ent->d_name, dstdir, ent->d_name);
> + }
> +
> + closedir(d);
> + close(dstfd);
Возможно тут и нет ошибки, но srcfd будет закрыт только при завершении
программы, а что насчёт повторного вызова функции с такой же srcdir или
такое невозможно?
> +}
> [...]
--
WBR, Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-05 5:21 ` Leonid Krivoshein
@ 2023-05-05 17:24 ` Alexey Gladkov
0 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-05 17:24 UTC (permalink / raw)
To: make-initrd
On Fri, May 05, 2023 at 08:21:25AM +0300, Leonid Krivoshein wrote:
>
> On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
> > new file mode 100644
> > index 00000000..ab5e03e4
> > --- /dev/null
> > +++ b/datasrc/ueventd/queue-processor.c
> > @@ -0,0 +1,116 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <sys/wait.h>
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <signal.h>
> > +#include <dirent.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +
> > +#include "ueventd.h"
> > +
> > [...]
> > +void move_files(char *srcdir, char *dstdir)
> > +{
> > + struct dirent *ent;
> > + int srcfd, dstfd;
> > +
> > + if ((srcfd = open(srcdir, O_RDONLY | O_CLOEXEC)) < 0)
> > + fatal("open: %s: %m", srcdir);
> > +
> > + errno = 0;
> > + if ((dstfd = open(dstdir, O_RDONLY | O_CLOEXEC)) < 0) {
> > + if (errno == ENOENT) {
>
> open() тоже прерываемый примитив, но его ты оборачивать не стал. Так
> задумано?
Нет, это я пропустил просто.
> > + if (mkdir(dstdir, 0755) < 0)
> > + fatal("mkdir: %s: %m", dstdir);
> > + dstfd = open(dstdir, O_RDONLY | O_CLOEXEC);
> > + }
> > + if (dstfd < 0)
> > + fatal("open: %s: %m", dstdir);
> > + }
> > +
> > + DIR *d = fdopendir(srcfd);
> > + if (!d)
> > + fatal("fdopendir: %m");
> > +
> > + while ((ent = xreaddir(d, srcdir)) != NULL) {
> > + if (ent->d_name[0] != '.' && ent->d_type == DT_REG &&
> > + renameat(srcfd, ent->d_name, dstfd, ent->d_name) < 0)
> > + fatal("rename `%s/%s' -> `%s/%s': %m", srcdir, ent->d_name, dstdir, ent->d_name);
> > + }
> > +
> > + closedir(d);
> > + close(dstfd);
>
> Возможно тут и нет ошибки, но srcfd будет закрыт только при завершении
> программы, а что насчёт повторного вызова функции с такой же srcdir или
> такое невозможно?
Нет, srcfd будет закрыт в closedir.
https://git.altlinux.org/gears/g/glibc.git?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/closedir.c#l30
> > +}
> > [...]
>
>
> --
> WBR, Leonid Krivoshein.
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
` (2 preceding siblings ...)
2023-05-05 5:21 ` Leonid Krivoshein
@ 2023-05-14 20:12 ` Leonid Krivoshein
2023-05-14 20:45 ` Alexey Gladkov
2023-05-14 20:57 ` Leonid Krivoshein
` (4 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-14 20:12 UTC (permalink / raw)
To: make-initrd
Привет!
On 5/4/23 16:42, Alexey Gladkov wrote:
> [...]
> diff --git a/datasrc/ueventd/ueventd.h b/datasrc/ueventd/ueventd.h
> new file mode 100644
> index 00000000..a5e50379
> --- /dev/null
> +++ b/datasrc/ueventd/ueventd.h
К этому заголовочному файлу возникла пара вопросов.
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __UEVENTD_H__
> +#define __UEVENTD_H__
> +
> +#include <sys/types.h>
> +#include <stdint.h>
> +
> [...]
> +
> +/* path.c */
> +#include <dirent.h>
> +
> [...]
> +
> +/* process.c */
> +extern pid_t waitpid_retry(pid_t pid, int *wstatus, int options);
> +
> +#include <dirent.h>
1) Что <dirent.h> повторяется дважды, конечно не проблема. Но вообще это
необычно выглядит, когда заголовочные файлы упоминаются в чередовании с
остальным кодом. Обычно они все выносятся наверх, потому и заголовочные.
Такой подход убережёт от понимания ошибок компилятора при появлении в
дальнейшем конфликтов в именах между твоим и библиотечным кодом.
> +
> +extern int is_dot_dir(struct dirent *ent) __attribute__((nonnull(1)));
> +
> +/* logging.c */
> +#include <unistd.h>
> +#include <syslog.h>
> +#include <stdlib.h>
> +
> [...]
> +
> +#endif /* __UEVENTD_H__ */
2) Понятно, что код чисто Си-шный и никому пока "снаружи" (ueventd) не
требуется. Просто, опять же, привычно видеть тут совместимость с
"плюсовой обёрткой". Возможно, так задумано, чтобы слинковаться с твоим
кодом можно было только для Си-шных программ.
--
WBR, Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-14 20:12 ` Leonid Krivoshein
@ 2023-05-14 20:45 ` Alexey Gladkov
0 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-14 20:45 UTC (permalink / raw)
To: make-initrd
On Sun, May 14, 2023 at 11:12:22PM +0300, Leonid Krivoshein wrote:
> Привет!
>
>
> On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > diff --git a/datasrc/ueventd/ueventd.h b/datasrc/ueventd/ueventd.h
> > new file mode 100644
> > index 00000000..a5e50379
> > --- /dev/null
> > +++ b/datasrc/ueventd/ueventd.h
>
> К этому заголовочному файлу возникла пара вопросов.
>
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#ifndef __UEVENTD_H__
> > +#define __UEVENTD_H__
> > +
> > +#include <sys/types.h>
> > +#include <stdint.h>
> > +
> > [...]
> > +
> > +/* path.c */
> > +#include <dirent.h>
> > +
> > [...]
> > +
> > +/* process.c */
> > +extern pid_t waitpid_retry(pid_t pid, int *wstatus, int options);
> > +
> > +#include <dirent.h>
>
> 1) Что <dirent.h> повторяется дважды, конечно не проблема. Но вообще это
> необычно выглядит, когда заголовочные файлы упоминаются в чередовании с
> остальным кодом. Обычно они все выносятся наверх, потому и заголовочные.
> Такой подход убережёт от понимания ошибок компилятора при появлении в
> дальнейшем конфликтов в именах между твоим и библиотечным кодом.
Я так делаю чисто для удобства чтобы понимать, что include не нужен
определениям выше. Когда портянка include собрана сверху, то не всегда
ясно для чего include добавлен. Но для меня это совершенно не критично.
>
> > +
> > +extern int is_dot_dir(struct dirent *ent) __attribute__((nonnull(1)));
> > +
> > +/* logging.c */
> > +#include <unistd.h>
> > +#include <syslog.h>
> > +#include <stdlib.h>
> > +
> > [...]
> > +
> > +#endif /* __UEVENTD_H__ */
>
> 2) Понятно, что код чисто Си-шный и никому пока "снаружи" (ueventd) не
> требуется. Просто, опять же, привычно видеть тут совместимость с
> "плюсовой обёрткой". Возможно, так задумано, чтобы слинковаться с твоим
> кодом можно было только для Си-шных программ.
В make-initrd у меня нет ни одной программы на с++ и я на нём не пишу.
Когда будет хоть один запрос на использование с++ и когда пользователь
сможет это обосновать, то обязательно добавлю всё необходимое.
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
` (3 preceding siblings ...)
2023-05-14 20:12 ` Leonid Krivoshein
@ 2023-05-14 20:57 ` Leonid Krivoshein
2023-05-15 8:52 ` Alexey Gladkov
2023-05-14 23:08 ` Leonid Krivoshein
` (3 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-14 20:57 UTC (permalink / raw)
To: make-initrd
On 5/4/23 16:42, Alexey Gladkov wrote:
> [...]
> diff --git a/datasrc/ueventd/memory.c b/datasrc/ueventd/memory.c
> new file mode 100644
> index 00000000..cb911d58
> --- /dev/null
> +++ b/datasrc/ueventd/memory.c
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <error.h>
> +#include <limits.h>
> +
> +#include "ueventd.h"
> +
> +void *xcalloc(size_t nmemb, size_t size)
> +{
> + void *r = calloc(nmemb, size);
> + if (!r)
> + fatal("calloc: allocating %lu*%lu bytes: %m",
> + (unsigned long) nmemb, (unsigned long) size);
Есть "%zu".
> + return r;
> +}
> +
> +char *xasprintf(char **ptr, const char *fmt, ...)
> +{
> + va_list arg;
> +
> + va_start(arg, fmt);
> + if (vasprintf(ptr, fmt, arg) < 0)
Здесь и
> + fatal("vasprintf: %m");
> + va_end(arg);
> +
> + return *ptr;
и тут возможно разыменование ссылки на NULL, поскольку ptr в коде не
проверяется и в заголовочном файле xasprintf() не объявлен как nonnull(1).
> +}
> [...]
--
WBR, Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-14 20:57 ` Leonid Krivoshein
@ 2023-05-15 8:52 ` Alexey Gladkov
0 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-15 8:52 UTC (permalink / raw)
To: make-initrd
On Sun, May 14, 2023 at 11:57:53PM +0300, Leonid Krivoshein wrote:
>
> On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > diff --git a/datasrc/ueventd/memory.c b/datasrc/ueventd/memory.c
> > new file mode 100644
> > index 00000000..cb911d58
> > --- /dev/null
> > +++ b/datasrc/ueventd/memory.c
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <string.h>
> > +#include <stdlib.h>
> > +#include <stdarg.h>
> > +#include <stdio.h>
> > +#include <errno.h>
> > +#include <error.h>
> > +#include <limits.h>
> > +
> > +#include "ueventd.h"
> > +
> > +void *xcalloc(size_t nmemb, size_t size)
> > +{
> > + void *r = calloc(nmemb, size);
> > + if (!r)
> > + fatal("calloc: allocating %lu*%lu bytes: %m",
> > + (unsigned long) nmemb, (unsigned long) size);
>
> Есть "%zu".
Да, согласен.
>
> > + return r;
> > +}
> > +
> > +char *xasprintf(char **ptr, const char *fmt, ...)
> > +{
> > + va_list arg;
> > +
> > + va_start(arg, fmt);
> > + if (vasprintf(ptr, fmt, arg) < 0)
>
> Здесь и
>
> > + fatal("vasprintf: %m");
> > + va_end(arg);
> > +
> > + return *ptr;
>
> и тут возможно разыменование ссылки на NULL, поскольку ptr в коде не
> проверяется и в заголовочном файле xasprintf() не объявлен как nonnull(1).
Согласен. Нужно добавить аттрибут nonnull(1,2).
>
> > +}
> > [...]
>
>
> --
> WBR, Leonid Krivoshein.
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
` (4 preceding siblings ...)
2023-05-14 20:57 ` Leonid Krivoshein
@ 2023-05-14 23:08 ` Leonid Krivoshein
2023-05-15 9:05 ` Alexey Gladkov
2023-05-15 0:47 ` Leonid Krivoshein
` (2 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-14 23:08 UTC (permalink / raw)
To: make-initrd
Тут ошибок нет, но есть над чем позудеть.))
On 5/4/23 16:42, Alexey Gladkov wrote:
> [...]
> diff --git a/datasrc/ueventd/path.c b/datasrc/ueventd/path.c
> new file mode 100644
> index 00000000..7880ad5c
> --- /dev/null
> +++ b/datasrc/ueventd/path.c
> @@ -0,0 +1,80 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <stdlib.h>
> +#include <dirent.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +#include "ueventd.h"
> +
> +int is_dot_dir(struct dirent *ent)
> +{
> + return (ent->d_type == DT_DIR &&
> + (ent->d_name[0] == '.' && (ent->d_name[1] == '\0' ||
> + (ent->d_name[1] == '.' && ent->d_name[2] == '\0') )));
> +}
> +
> +DIR *xopendir(const char *path)
> +{
> + DIR *dir = opendir(path);
> + if (!dir)
> + fatal("opendir: %s: %m", path);
> + return dir;
> +}
> +
> +
> +struct dirent *xreaddir(DIR *d, const char *path)
> +{
> + struct dirent *ent;
> +
> + errno = 0;
> + ent = readdir(d);
> + if (!ent) {
> + if (!errno)
> + return NULL;
Создавать этот NULL дольше, чем вернуть готовый ent. Я бы поменял
условие на if (!ent && errno) fatal(...); Получится даже короче и
читабельней.
> + fatal("readdir: %s: %m", path);
> + }
> + return ent;
> +}
> +
> +int empty_directory(const char *path)
> +{
> + struct dirent *ent;
> + int is_empty = 1;
> + DIR *d = xopendir(path);
> +
> + while ((ent = xreaddir(d, path)) != NULL) {
> + if (ent->d_name[0] != '.') {
> + is_empty = 0;
> + break;
> + }
> + }
> + closedir(d);
> +
> + return is_empty;
> +}
Назначение функции по имени кажется двусмысленным и можно обойтись без
этой переменной в стеке, даже немного сократив код:
int is_dir_empty(const char *path)
{
struct dirent *ent;
DIR *d = xopendir(path);
while ((ent = xreaddir(d, path)) != NULL) {
if (ent->d_name[0] != '.') {
closedir(d);
return 0;
}
}
closedir(d);
return -1;
}
Не помню, какую задачу решает этот минус, но кажется правильней для
логического значения.
> +
> +ssize_t read_retry(int fd, void *buf, size_t count)
> +{
> + return TEMP_FAILURE_RETRY(read(fd, buf, count));
> +}
> +
> +ssize_t write_retry(int fd, const void *buf, size_t count)
> +{
> + return TEMP_FAILURE_RETRY(write(fd, buf, count));
> +}
> +
> +ssize_t write_loop(int fd, const char *buffer, size_t count)
> +{
> + ssize_t offset = 0;
> +
> + while (count > 0) {
> + ssize_t block = write_retry(fd, &buffer[offset], count);
> +
> + if (block <= 0)
> + return offset ? : block;
Первый раз встречаю в такой нотации. Перед двоеточием имеется ввиду тоже
offset?
> + offset += block;
> + count -= (size_t) block;
> + }
> + return offset;
> +}
> [...]
--
WBR, Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-14 23:08 ` Leonid Krivoshein
@ 2023-05-15 9:05 ` Alexey Gladkov
0 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-15 9:05 UTC (permalink / raw)
To: make-initrd
On Mon, May 15, 2023 at 02:08:09AM +0300, Leonid Krivoshein wrote:
> Тут ошибок нет, но есть над чем позудеть.))
>
>
> On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > diff --git a/datasrc/ueventd/path.c b/datasrc/ueventd/path.c
> > new file mode 100644
> > index 00000000..7880ad5c
> > --- /dev/null
> > +++ b/datasrc/ueventd/path.c
> > @@ -0,0 +1,80 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <stdlib.h>
> > +#include <dirent.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +
> > +#include "ueventd.h"
> > +
> > +int is_dot_dir(struct dirent *ent)
> > +{
> > + return (ent->d_type == DT_DIR &&
> > + (ent->d_name[0] == '.' && (ent->d_name[1] == '\0' ||
> > + (ent->d_name[1] == '.' && ent->d_name[2] == '\0') )));
> > +}
> > +
> > +DIR *xopendir(const char *path)
> > +{
> > + DIR *dir = opendir(path);
> > + if (!dir)
> > + fatal("opendir: %s: %m", path);
> > + return dir;
> > +}
> > +
> > +
> > +struct dirent *xreaddir(DIR *d, const char *path)
> > +{
> > + struct dirent *ent;
> > +
> > + errno = 0;
> > + ent = readdir(d);
> > + if (!ent) {
> > + if (!errno)
> > + return NULL;
>
> Создавать этот NULL дольше, чем вернуть готовый ent. Я бы поменял
> условие на if (!ent && errno) fatal(...); Получится даже короче и
> читабельней.
Такие оптимизации я оставлю до момента, когда буду трогать эту функцию
снова.
>
> > + fatal("readdir: %s: %m", path);
> > + }
> > + return ent;
> > +}
> > +
> > +int empty_directory(const char *path)
> > +{
> > + struct dirent *ent;
> > + int is_empty = 1;
> > + DIR *d = xopendir(path);
> > +
> > + while ((ent = xreaddir(d, path)) != NULL) {
> > + if (ent->d_name[0] != '.') {
> > + is_empty = 0;
> > + break;
> > + }
> > + }
> > + closedir(d);
> > +
> > + return is_empty;
> > +}
>
> Назначение функции по имени кажется двусмысленным и можно обойтись без
> этой переменной в стеке, даже немного сократив код:
>
> int is_dir_empty(const char *path)
> {
> struct dirent *ent;
> DIR *d = xopendir(path);
>
> while ((ent = xreaddir(d, path)) != NULL) {
> if (ent->d_name[0] != '.') {
> closedir(d);
> return 0;
> }
> }
> closedir(d);
>
> return -1;
> }
В моём коде лишняя переменная. В твоём два вызова closedir.
> Не помню, какую задачу решает этот минус, но кажется правильней для
> логического значения.
>
>
> > +
> > +ssize_t read_retry(int fd, void *buf, size_t count)
> > +{
> > + return TEMP_FAILURE_RETRY(read(fd, buf, count));
> > +}
> > +
> > +ssize_t write_retry(int fd, const void *buf, size_t count)
> > +{
> > + return TEMP_FAILURE_RETRY(write(fd, buf, count));
> > +}
> > +
> > +ssize_t write_loop(int fd, const char *buffer, size_t count)
> > +{
> > + ssize_t offset = 0;
> > +
> > + while (count > 0) {
> > + ssize_t block = write_retry(fd, &buffer[offset], count);
> > +
> > + if (block <= 0)
> > + return offset ? : block;
>
> Первый раз встречаю в такой нотации. Перед двоеточием имеется ввиду тоже
> offset?
Да.
>
> > + offset += block;
> > + count -= (size_t) block;
> > + }
> > + return offset;
> > +}
> > [...]
>
>
> --
> WBR, Leonid Krivoshein.
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
` (5 preceding siblings ...)
2023-05-14 23:08 ` Leonid Krivoshein
@ 2023-05-15 0:47 ` Leonid Krivoshein
2023-05-15 9:12 ` Alexey Gladkov
2023-05-15 4:38 ` Leonid Krivoshein
2023-05-20 9:00 ` Leonid Krivoshein
8 siblings, 1 reply; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-15 0:47 UTC (permalink / raw)
To: make-initrd
On 5/4/23 16:42, Alexey Gladkov wrote:
> [...]
> diff --git a/datasrc/ueventd/logging.c b/datasrc/ueventd/logging.c
> new file mode 100644
> index 00000000..671f6814
> --- /dev/null
> +++ b/datasrc/ueventd/logging.c
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <strings.h>
> +#include <errno.h>
> +#include <time.h>
> +
> +#include "ueventd.h"
> +
> +#define default_logfile "/var/log/ueventd.log"
> +
> +int log_priority = LOG_INFO;
Переменная не объявлена здесь как static и она никак не объявлена в
заголовочном файле. Просто, на всякий случай.
> +
> +int logging_level(const char *name)
> +{
> + if (!strcasecmp(name, "debug")) return LOG_DEBUG;
> + if (!strcasecmp(name, "info")) return LOG_INFO;
> + if (!strcasecmp(name, "warning")) return LOG_WARNING;
> + if (!strcasecmp(name, "error")) return LOG_ERR;
> + return log_priority;
Понятно, что такая обработка, видимо писавшаяся под парсер в ueventd.c:
loglevel = logging_level(optarg);
упрощает код. Но тогда его правильней сделать локальной частью
ueventd.c, как мне кажется. Потому что сейчас любое невалидное значение
name просто не меняет текущего значения log_priority, а я не уверен, что
это правильно. Если же функция остаётся в logging.c с таким дефолтом, то
вроде как анализ "info" кажется лишним.
> +}
> +
> [...]
--
WBR, Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-15 0:47 ` Leonid Krivoshein
@ 2023-05-15 9:12 ` Alexey Gladkov
0 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-15 9:12 UTC (permalink / raw)
To: make-initrd
On Mon, May 15, 2023 at 03:47:03AM +0300, Leonid Krivoshein wrote:
>
> On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > diff --git a/datasrc/ueventd/logging.c b/datasrc/ueventd/logging.c
> > new file mode 100644
> > index 00000000..671f6814
> > --- /dev/null
> > +++ b/datasrc/ueventd/logging.c
> > @@ -0,0 +1,63 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +#include <strings.h>
> > +#include <errno.h>
> > +#include <time.h>
> > +
> > +#include "ueventd.h"
> > +
> > +#define default_logfile "/var/log/ueventd.log"
> > +
> > +int log_priority = LOG_INFO;
>
> Переменная не объявлена здесь как static и она никак не объявлена в
> заголовочном файле. Просто, на всякий случай.
Я и не хотел её объявлять в хэдере. Для её изменения есть logging_init.
>
> > +
> > +int logging_level(const char *name)
> > +{
> > + if (!strcasecmp(name, "debug")) return LOG_DEBUG;
> > + if (!strcasecmp(name, "info")) return LOG_INFO;
> > + if (!strcasecmp(name, "warning")) return LOG_WARNING;
> > + if (!strcasecmp(name, "error")) return LOG_ERR;
> > + return log_priority;
>
> Понятно, что такая обработка, видимо писавшаяся под парсер в ueventd.c:
>
> loglevel = logging_level(optarg);
>
> упрощает код. Но тогда его правильней сделать локальной частью
> ueventd.c, как мне кажется. Потому что сейчас любое невалидное значение
> name просто не меняет текущего значения log_priority, а я не уверен, что
> это правильно. Если же функция остаётся в logging.c с таким дефолтом, то
> вроде как анализ "info" кажется лишним.
В новом коде весь logging вынесен в библиотеку для переиспользования.
Наличие общей функции преобразующей текстовое представление в loglevel
кажется мне обоснованным.
>
> > +}
> > +
> > [...]
>
>
> --
> WBR, Leonid Krivoshein.
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
` (6 preceding siblings ...)
2023-05-15 0:47 ` Leonid Krivoshein
@ 2023-05-15 4:38 ` Leonid Krivoshein
2023-05-15 10:43 ` Alexey Gladkov
2023-05-20 9:00 ` Leonid Krivoshein
8 siblings, 1 reply; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-15 4:38 UTC (permalink / raw)
To: make-initrd
On 5/4/23 16:42, Alexey Gladkov wrote:
> [...]
> diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
> new file mode 100644
> index 00000000..ab5e03e4
> --- /dev/null
> +++ b/datasrc/ueventd/queue-processor.c
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <dirent.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +#include "ueventd.h"
> +
> +static void event_handler(struct watch *queue, char *path) __attribute__((nonnull(1, 2)));
> +static void move_files(char *srcdir, char *dstdir) __attribute__((nonnull(1, 2)));
> +static void signal_unhandled_events(struct watch *queue) __attribute__((nonnull(1)));
> +
> +void event_handler(struct watch *queue, char *path)
Здесь оба указателя могут быть константными, т.к. данные не меняются и
дальше не передаются.
> +{
> + char *argv[] = { handler_file, path, NULL };
> + pid_t pid = vfork();
> +
> + if (pid < 0) {
> + err("fork: %m");
> +
> + } else if (!pid) {
> + execve(argv[0], argv, environ);
Компилятор не ругается здесь только потому, что подключен <unistd.h>
через "ueventd.h", а ругаться у него целых два повода (execve и
environ). Это я к тому, что в твоём заголовочном файле он подключен для
logging.c, а не для queue-processor.c. Согласно man 7 environ можно не
объявлять эту переменную в коде, если <unistd.h> подключается с
_GNU_SOURCE, т.е. как раз твой случай.
> + fatal("exec: %s: %m", argv[0]);
> + } else {
> + int status = 0;
> +
> + if (waitpid_retry(pid, &status, 0) != pid || !WIFEXITED(status))
> + info("%s: session=%lu: %s failed",
> + queue->q_name, session, handler_file);
Насчёт "session=%lu..." только лишь глядя в этот код понял, что и сам в
не публичном коде опрометчиво использую uint64_t с таким
форматированием, хотя везде по коду должно быть "session=" PRIu64 "...".
Нужно подключить заголовочный файл <inttypes.h> либо использовать
какое-то приведение типов. Пренебречь этим, как сейчас, можно только
полагаясь на конкретные платформы.
> + else
> + info("%s: session=%lu: %s finished with return code %d",
> + queue->q_name, session, handler_file, WEXITSTATUS(status));
> + }
> +}
> +
> +void move_files(char *srcdir, char *dstdir)
Здесь оба указателя тоже могут быть константными.
> +{
> + struct dirent *ent;
> + int srcfd, dstfd;
> +
> + if ((srcfd = open(srcdir, O_RDONLY | O_CLOEXEC)) < 0)
> + fatal("open: %s: %m", srcdir);
> +
> + errno = 0;
> + if ((dstfd = open(dstdir, O_RDONLY | O_CLOEXEC)) < 0) {
> + if (errno == ENOENT) {
> + if (mkdir(dstdir, 0755) < 0)
> + fatal("mkdir: %s: %m", dstdir);
> + dstfd = open(dstdir, O_RDONLY | O_CLOEXEC);
> + }
> + if (dstfd < 0)
> + fatal("open: %s: %m", dstdir);
> + }
> +
> + DIR *d = fdopendir(srcfd);
> + if (!d)
> + fatal("fdopendir: %m");
> +
> + while ((ent = xreaddir(d, srcdir)) != NULL) {
> + if (ent->d_name[0] != '.' && ent->d_type == DT_REG &&
Поле d_type заполняется не на всех системах (не на всех файловых
системах), доверять можно только d_name и d_ino. Если всё это происходит
на ramfs и есть уверенность, что в ней это поддерживается, то и нет
проблемы... пока в этот ramfs не смонтировать что-то ещё и не начать
использовать этот же код для другой файловой системы.
> + renameat(srcfd, ent->d_name, dstfd, ent->d_name) < 0)
> + fatal("rename `%s/%s' -> `%s/%s': %m", srcdir, ent->d_name, dstdir, ent->d_name);
> + }
> +
> + closedir(d);
> + close(dstfd);
> +}
> +
> +void signal_unhandled_events(struct watch *queue)
И здесь указатель может быть константным.
> +{
> + ssize_t len;
> +
> + len = write_loop(queue->q_parentfd,
> + (char *) &queue->q_watchfd, sizeof(queue->q_watchfd));
> + if (len < 0)
> + err("write(pipe): %m");
> +
> + info("%s: session=%lu: retry with the events remaining in the queue", queue->q_name, session);
> +}
Не углубляясь в код ueventd.c (только по названию и содержимому функции)
вообще нельзя понять, что она делает и причём тут PIPE. А всё потому,
что функция утилитарная и в "ueventd.h" поля структуры watch не имеют
комментариев.
Поэтому спрошу. Она сообщает о необработанных эвентах? Похоже, что через
PIPE родительскому процессу передаётся целое число (q_watchfd). Но
почему для передачи через PIPE тут используется write_loop()? Ведь в/в
через PIPE (даже неблокирующий) всегда транзакционен, если не превышает
PIPE_BUF.
> +
> +void process_events(struct watch *queue)
Похоже, тут тоже указатель может быть константным.
> +{
> + info("%s: session=%lu: processing events", queue->q_name, session);
> +
> + char *numenv = NULL;
> +
> + xasprintf(&numenv, "SESSION=%lu", session);
> + putenv(numenv);
Здесь возможна утечка: пропущен free(numenv), а когда numenv будет
освобождена?
> +
> + char *srcdir, *dstdir;
Для numenv есть инициализация, а тут нет, хотя использование одинаковое.
Нет предупреждений?
> +
> + xasprintf(&srcdir, "%s/%s", filter_dir, queue->q_name);
> + xasprintf(&dstdir, "%s/%s", uevent_dir, queue->q_name);
> +
> + move_files(srcdir, dstdir);
> +
> + if (!empty_directory(dstdir)) {
> + event_handler(queue, dstdir);
> +
> + if (!empty_directory(dstdir))
> + signal_unhandled_events(queue);
> + }
> +
> + free(srcdir);
> + free(dstdir);
> +
> + exit(0);
> +}
> [...]
--
WBR, Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-15 4:38 ` Leonid Krivoshein
@ 2023-05-15 10:43 ` Alexey Gladkov
0 siblings, 1 reply; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-15 10:43 UTC (permalink / raw)
To: make-initrd
On Mon, May 15, 2023 at 07:38:30AM +0300, Leonid Krivoshein wrote:
>
> On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
> > new file mode 100644
> > index 00000000..ab5e03e4
> > --- /dev/null
> > +++ b/datasrc/ueventd/queue-processor.c
> > @@ -0,0 +1,116 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <sys/wait.h>
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <signal.h>
> > +#include <dirent.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +
> > +#include "ueventd.h"
> > +
> > +static void event_handler(struct watch *queue, char *path) __attribute__((nonnull(1, 2)));
> > +static void move_files(char *srcdir, char *dstdir) __attribute__((nonnull(1, 2)));
> > +static void signal_unhandled_events(struct watch *queue) __attribute__((nonnull(1)));
> > +
> > +void event_handler(struct watch *queue, char *path)
>
> Здесь оба указателя могут быть константными, т.к. данные не меняются и
> дальше не передаются.
>
>
> > +{
> > + char *argv[] = { handler_file, path, NULL };
К сожалению path быть const не может так как иначе вот тут вылезет
предупреждение.
А если тут сделать argv константным, то ругань будет на execve, потому что
execve принимает char *const argv[].
> > + pid_t pid = vfork();
> > +
> > + if (pid < 0) {
> > + err("fork: %m");
> > +
> > + } else if (!pid) {
> > + execve(argv[0], argv, environ);
>
> Компилятор не ругается здесь только потому, что подключен <unistd.h>
> через "ueventd.h", а ругаться у него целых два повода (execve и
> environ).
Если ты предполагаешь, что тут должна быть ругань про отсутствие
определения execve и environ, то ты ошибаешься. Ругани тут нет так как
определения приезжают через ueventd.h.
Или ты про какие поводы для ругани ?
> Это я к тому, что в твоём заголовочном файле он подключен для
> logging.c, а не для queue-processor.c. Согласно man 7 environ можно не
> объявлять эту переменную в коде, если <unistd.h> подключается с
> _GNU_SOURCE, т.е. как раз твой случай.
Что значит подключён для logging.c ? unistd.h просто подключён в
ueventd.h.
Весь этот код компилируется с глобальным _GNU_SOURCE.
>
>
> > + fatal("exec: %s: %m", argv[0]);
> > + } else {
> > + int status = 0;
> > +
> > + if (waitpid_retry(pid, &status, 0) != pid || !WIFEXITED(status))
> > + info("%s: session=%lu: %s failed",
> > + queue->q_name, session, handler_file);
>
> Насчёт "session=%lu..." только лишь глядя в этот код понял, что и сам в
> не публичном коде опрометчиво использую uint64_t с таким
> форматированием, хотя везде по коду должно быть "session=" PRIu64 "...".
> Нужно подключить заголовочный файл <inttypes.h> либо использовать
> какое-то приведение типов. Пренебречь этим, как сейчас, можно только
> полагаясь на конкретные платформы.
Я знаю про PRIu64, но его использование не понимает clang и получается вот
так:
datasrc/ueventd/queue-processor.c:40:27: warning: format specifies type 'char *' but the argument has type 'uint64_t' (aka 'unsigned long') [-Wformat]
queue->q_name, session, handler_file);
^~~~~~~
datasrc/ueventd/ueventd.h:70:69: note: expanded from macro 'rd_info'
#define rd_info(format, arg...) rd_log_message(LOG_INFO, format, ##arg)
~~~~~~ ^~~
datasrc/ueventd/queue-processor.c:40:36: warning: data argument not used by format string [-Wformat-extra-args]
queue->q_name, session, handler_file);
~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
datasrc/ueventd/ueventd.h:70:69: note: expanded from macro 'rd_info'
#define rd_info(format, arg...) rd_log_message(LOG_INFO, format, ##arg)
~~~~~~ ^
2 warnings generated.
то есть как видишь у clang PRIu64 превратилось в пустую строку и аргументы
съехали.
Таким образом использование PRIu64 сразу делает код gcc-specific.
>
> > + else
> > + info("%s: session=%lu: %s finished with return code %d",
> > + queue->q_name, session, handler_file, WEXITSTATUS(status));
> > + }
> > +}
> > +
> > +void move_files(char *srcdir, char *dstdir)
>
> Здесь оба указателя тоже могут быть константными.
В принципе да.
> > +{
> > + struct dirent *ent;
> > + int srcfd, dstfd;
> > +
> > + if ((srcfd = open(srcdir, O_RDONLY | O_CLOEXEC)) < 0)
> > + fatal("open: %s: %m", srcdir);
> > +
> > + errno = 0;
> > + if ((dstfd = open(dstdir, O_RDONLY | O_CLOEXEC)) < 0) {
> > + if (errno == ENOENT) {
> > + if (mkdir(dstdir, 0755) < 0)
> > + fatal("mkdir: %s: %m", dstdir);
> > + dstfd = open(dstdir, O_RDONLY | O_CLOEXEC);
> > + }
> > + if (dstfd < 0)
> > + fatal("open: %s: %m", dstdir);
> > + }
> > +
> > + DIR *d = fdopendir(srcfd);
> > + if (!d)
> > + fatal("fdopendir: %m");
> > +
> > + while ((ent = xreaddir(d, srcdir)) != NULL) {
> > + if (ent->d_name[0] != '.' && ent->d_type == DT_REG &&
>
> Поле d_type заполняется не на всех системах (не на всех файловых
> системах), доверять можно только d_name и d_ino. Если всё это происходит
> на ramfs и есть уверенность, что в ней это поддерживается, то и нет
> проблемы... пока в этот ramfs не смонтировать что-то ещё и не начать
> использовать этот же код для другой файловой системы.
С одной стороны ты прав, но с другой стороны этот код не предназначен для
выполнения нигде кроме initramfs, где этой проблемы нет. А для отладки
d_type поддерживается всеми основными файловыми системами.
Если бы это был портабильный код, то я согласен, что нужно было бы делать
иначе и городить отдельный stat.
> > + renameat(srcfd, ent->d_name, dstfd, ent->d_name) < 0)
> > + fatal("rename `%s/%s' -> `%s/%s': %m", srcdir, ent->d_name, dstdir, ent->d_name);
> > + }
> > +
> > + closedir(d);
> > + close(dstfd);
> > +}
> > +
> > +void signal_unhandled_events(struct watch *queue)
>
> И здесь указатель может быть константным.
>
>
> > +{
> > + ssize_t len;
> > +
> > + len = write_loop(queue->q_parentfd,
> > + (char *) &queue->q_watchfd, sizeof(queue->q_watchfd));
> > + if (len < 0)
> > + err("write(pipe): %m");
> > +
> > + info("%s: session=%lu: retry with the events remaining in the queue", queue->q_name, session);
> > +}
>
> Не углубляясь в код ueventd.c (только по названию и содержимому функции)
> вообще нельзя понять, что она делает и причём тут PIPE. А всё потому,
> что функция утилитарная и в "ueventd.h" поля структуры watch не имеют
> комментариев.
Я не писал комментарий тут, потому что мне показалось поведение очень
тривиальным. Классический ipc. Мы просто отсылаем единственное сообщение
основному процессу если не обработали все эвенты в директории.
> Поэтому спрошу. Она сообщает о необработанных эвентах?
Да. Передаётся уникальный признак очереди, чтобы пометить эту очередь как
"грязную" и попробовать снова её обработать спустя timeout не дожидаясь
новых эвентов.
> Похоже, что через PIPE родительскому процессу передаётся целое число
> (q_watchfd). Но почему для передачи через PIPE тут используется
> write_loop()? Ведь в/в через PIPE (даже неблокирующий) всегда
> транзакционен, если не превышает PIPE_BUF.
Я писал этот код с многими допущениями. Если включать паранойю, то этот
кусок нужно делать совсем иначе. Я тут полагаюсь на то, что PIPE_BUF
достаточно большой.
> > +
> > +void process_events(struct watch *queue)
>
> Похоже, тут тоже указатель может быть константным.
>
>
> > +{
> > + info("%s: session=%lu: processing events", queue->q_name, session);
> > +
> > + char *numenv = NULL;
> > +
> > + xasprintf(&numenv, "SESSION=%lu", session);
> > + putenv(numenv);
>
> Здесь возможна утечка: пропущен free(numenv), а когда numenv будет
> освобождена?
Нет. Аргумент не копируется в environ, а вставляется как есть. Это
позволяет не создавать излишние копии как setenv.
putenv(3): The string pointed to by string becomes part of the
environment, so altering the string changes the environment.
> > +
> > + char *srcdir, *dstdir;
>
> Для numenv есть инициализация, а тут нет, хотя использование одинаковое.
> Нет предупреждений?
Инициализация numenv излишняя, как и в других местах, где используется
xasprintf. Эта функция не может вернуть ошибку и результат всегда будет в
первом аргументе.
> > +
> > + xasprintf(&srcdir, "%s/%s", filter_dir, queue->q_name);
> > + xasprintf(&dstdir, "%s/%s", uevent_dir, queue->q_name);
> > +
> > + move_files(srcdir, dstdir);
> > +
> > + if (!empty_directory(dstdir)) {
> > + event_handler(queue, dstdir);
> > +
> > + if (!empty_directory(dstdir))
> > + signal_unhandled_events(queue);
> > + }
> > +
> > + free(srcdir);
> > + free(dstdir);
> > +
> > + exit(0);
> > +}
> > [...]
>
>
> --
> WBR, Leonid Krivoshein.
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
` (7 preceding siblings ...)
2023-05-15 4:38 ` Leonid Krivoshein
@ 2023-05-20 9:00 ` Leonid Krivoshein
2023-05-20 13:18 ` Alexey Gladkov
` (3 more replies)
8 siblings, 4 replies; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-20 9:00 UTC (permalink / raw)
To: make-initrd
Привет!
On 5/4/23 16:42, Alexey Gladkov wrote:
> [...]
> diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
> new file mode 100644
> index 00000000..f5923367
> --- /dev/null
> +++ b/datasrc/ueventd/ueventd.c
> @@ -0,0 +1,551 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
Добрался до самого толстого партизана в этом лесу, неделю уже на него
смотрю, но ничего такого страшного в нём пока не увидел, вроде бы вполне
рабочий код. Eсть вопросы/предложения по упрощению, читабельности и
оптимизации...
> [...]
> +
> +typedef int (*fdhandler_t)(int);
> +
> +struct fd_handler {
> + int fd;
> + fdhandler_t fd_handler;
> +};
> +
> +struct fd_handler fd_list[FD_MAX];
Имена прототипа функции-обработчика (1), структуры (2) и её члена (3)
можно оставить как есть, назвав почти одним именем, но только, если
очень хочется кого-то запутать. :-) Имя fd_list[] тоже не очень
подходящее, поскольку это не только список именно fd. Константа FD_MAX,
хоть и не определена в использованных заголовочных файлах, это имя
иногда встречается рядом с FD_SET(), FD_CLR() и им подобным макросам в
других исходниках как функция FD_MAX(x). На всякий случай...
> +
> +enum {
> + PIPE_READ,
> + PIPE_WRITE,
> + PIPE_MAX,
> +};
> +
> +int pipefd[PIPE_MAX];
Данным фрагментом ты как бы говоришь: "я точно знаю, во что это
превратит компилятор", слишком безапелляционно. Тут define's или хотя бы
явное указание значений вполне уместно, с учётом дальнейшего
использования в строках 122 и 388. Кроме того, pipefd не объявлен как
static, можно не засорять глобальное пространство имён.
> [...]
> +
> +int main(int argc, char **argv)
> +{
> [...]
> + while (1) {
Не любишь ты использовать for (;;)....
> + struct epoll_event ev[EV_MAX];
> + int fdcount;
> +
> + errno = 0;
> + fdcount = epoll_wait(epollfd, ev, EV_MAX, 250);
Почему 250, а не -1? Напоминает не нужный в этом месте "sleep .25". Если
бы демон перезапускался для дефрагментации через какое-то число событий
или спустя сколько-то минут, а тут совсем холостой ход получается.
> +
> + if (fdcount < 0) {
> + if (errno == EINTR)
> + continue;
> + fatal("epoll_wait: %m");
> + }
> +
> + for (i = 0; i < fdcount; i++) {
> + if (!(ev[i].events & EPOLLIN))
> + continue;
> + for (int k = 0; k < FD_MAX; k++) {
> + if (ev[i].data.fd != fd_list[k].fd)
> + continue;
> + if (fd_list[k].fd_handler(fd_list[k].fd) != 0)
Непонятно, почему из всей структуры Userdata используется только
ev->data->fd и потом элементы перебираются в циклах для поиска и
сопоставления, хотя в ev->data при epoll_ctl(..., EPOL_CTL_ADD, ...)
вполне можно было бы разместить всё самое интересное для ускорения доступа.
> + goto done;
> + }
> + }
> +
> + for (e = watch_list; e; e = e->next) {
> + if (!(e->q_flags & F_QUEUE_DIR) || !(e->q_flags & F_DIRTY) || (e->q_pid != 0) || (e->q_flags & F_PAUSED))
> + continue;
> + e->q_flags &= ~F_DIRTY;
> + e->q_pid = spawn_worker(epollfd, e);
> + }
> + }
> +done:
> + for (e = watch_list, n = NULL; e; e = n) {
> + n = e->next;
> + if (e->q_pid)
> + kill(e->q_pid, SIGKILL);
> + inotify_rm_watch(fd_list[FD_INOTIFY].fd, e->q_watchfd);
> + free(e);
> + }
> +
> + for (i = 0; i < FD_MAX; i++) {
> + if (fd_list[i].fd >= 0) {
> + if (epoll_ctl(epollfd, EPOLL_CTL_DEL, fd_list[i].fd, NULL) < 0)
> + err("epoll_ctl: %m");
> + close(fd_list[i].fd);
> + }
> + }
> + close(epollfd);
> + logging_close();
> +
> + return EXIT_SUCCESS;
> +}
> [...]
--
WBR, Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-20 9:00 ` Leonid Krivoshein
@ 2023-05-20 13:18 ` Alexey Gladkov
2023-05-20 15:17 ` Vladimir D. Seleznev
2023-05-20 17:23 ` Leonid Krivoshein
2023-05-20 16:37 ` [make-initrd] [PATCH 1/3] ueventd: Simplify call of the queue handler Alexey Gladkov
` (2 subsequent siblings)
3 siblings, 2 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-20 13:18 UTC (permalink / raw)
To: make-initrd
On Sat, May 20, 2023 at 12:00:14PM +0300, Leonid Krivoshein wrote:
> Привет!
>
>
> On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
> > new file mode 100644
> > index 00000000..f5923367
> > --- /dev/null
> > +++ b/datasrc/ueventd/ueventd.c
> > @@ -0,0 +1,551 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
>
> Добрался до самого толстого партизана в этом лесу, неделю уже на него
> смотрю, но ничего такого страшного в нём пока не увидел, вроде бы вполне
> рабочий код. Eсть вопросы/предложения по упрощению, читабельности и
> оптимизации...
>
>
> > [...]
> > +
> > +typedef int (*fdhandler_t)(int);
> > +
> > +struct fd_handler {
> > + int fd;
> > + fdhandler_t fd_handler;
> > +};
> > +
> > +struct fd_handler fd_list[FD_MAX];
>
> Имена прототипа функции-обработчика (1), структуры (2) и её члена (3)
> можно оставить как есть, назвав почти одним именем, но только, если
> очень хочется кого-то запутать. :-)
Тип хэндлера (fdhandler_t), структура для хэндлера дескриптора (fd_handler),
ну и собственно список структур (fd_list). Мне кажется всё понятно и
читабильно.
> Имя fd_list[] тоже не очень подходящее, поскольку это не только список
> именно fd.
Принимаю предложения на более удачное название ))
> Константа FD_MAX, хоть и не определена в использованных
> заголовочных файлах, это имя иногда встречается рядом с FD_SET(),
> FD_CLR() и им подобным макросам в других исходниках как функция
> FD_MAX(x). На всякий случай...
Это имя ни с чем не конфликтует.
$ grep -re '\<FD_MAX\>' /usr/include | wc -l
0
> > +
> > +enum {
> > + PIPE_READ,
> > + PIPE_WRITE,
> > + PIPE_MAX,
> > +};
> > +
> > +int pipefd[PIPE_MAX];
>
> Данным фрагментом ты как бы говоришь: "я точно знаю, во что это
> превратит компилятор", слишком безапелляционно.
Стандарт си говорит:
If the first enumerator has no =, the value of its enumeration constant is
0. Each subsequent enumerator with no = defines its enumeration constant
as the value of the constant expression obtained by adding 1 to the value
of the previous enumeration constant.
Если какой-то компилятор этому не следует, то я не уверен, что мне есть
него дело.
Так что тут я могу быть уверен, что PIPE_READ=0, PIPE_WRITE=1, PIPE_MAX=3.
Это просто способ именования констант.
> Тут define's или хотя бы явное указание значений вполне уместно, с
> учётом дальнейшего использования в строках 122 и 388. Кроме того, pipefd
> не объявлен как static, можно не засорять глобальное пространство имён.
static я действительно забыл.
> > [...]
> > +
> > +int main(int argc, char **argv)
> > +{
> > [...]
> > + while (1) {
>
> Не любишь ты использовать for (;;)....
Да. Не люблю ))) Субъективно мне for(;;) кажется менее читабильным.
> > + struct epoll_event ev[EV_MAX];
> > + int fdcount;
> > +
> > + errno = 0;
> > + fdcount = epoll_wait(epollfd, ev, EV_MAX, 250);
>
> Почему 250, а не -1? Напоминает не нужный в этом месте "sleep .25". Если
> бы демон перезапускался для дефрагментации через какое-то число событий
> или спустя сколько-то минут, а тут совсем холостой ход получается.
Это сделано намеренно. Раньше ты спрашивал зачем отправлять wfd родителю
через пайп. Родитель помечает очередь как F_DIRTY и на следующей итерации
эта очередь попробует обработаться даже если новых эвентов нет.
> > +
> > + if (fdcount < 0) {
> > + if (errno == EINTR)
> > + continue;
> > + fatal("epoll_wait: %m");
> > + }
> > +
> > + for (i = 0; i < fdcount; i++) {
> > + if (!(ev[i].events & EPOLLIN))
> > + continue;
> > + for (int k = 0; k < FD_MAX; k++) {
> > + if (ev[i].data.fd != fd_list[k].fd)
> > + continue;
> > + if (fd_list[k].fd_handler(fd_list[k].fd) != 0)
>
> Непонятно, почему из всей структуры Userdata используется только
> ev->data->fd и потом элементы перебираются в циклах для поиска и
> сопоставления, хотя в ev->data при epoll_ctl(..., EPOL_CTL_ADD, ...)
> вполне можно было бы разместить всё самое интересное для ускорения доступа.
Всё? Это union. Я мог бы положить туда либо fd, либо указатель на
fd_list[k]. В целом так и стоит сделать. Раньше fd_list не было и остался
этот атавизм. Но это позволит избавиться от цикла только в этом месте.
> > + goto done;
> > + }
> > + }
> > +
> > + for (e = watch_list; e; e = e->next) {
> > + if (!(e->q_flags & F_QUEUE_DIR) || !(e->q_flags & F_DIRTY) || (e->q_pid != 0) || (e->q_flags & F_PAUSED))
> > + continue;
> > + e->q_flags &= ~F_DIRTY;
> > + e->q_pid = spawn_worker(epollfd, e);
> > + }
> > + }
> > +done:
> > + for (e = watch_list, n = NULL; e; e = n) {
> > + n = e->next;
> > + if (e->q_pid)
> > + kill(e->q_pid, SIGKILL);
> > + inotify_rm_watch(fd_list[FD_INOTIFY].fd, e->q_watchfd);
> > + free(e);
> > + }
> > +
> > + for (i = 0; i < FD_MAX; i++) {
> > + if (fd_list[i].fd >= 0) {
> > + if (epoll_ctl(epollfd, EPOLL_CTL_DEL, fd_list[i].fd, NULL) < 0)
> > + err("epoll_ctl: %m");
> > + close(fd_list[i].fd);
> > + }
> > + }
> > + close(epollfd);
> > + logging_close();
> > +
> > + return EXIT_SUCCESS;
> > +}
> > [...]
>
>
> --
> WBR, Leonid Krivoshein.
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-20 13:18 ` Alexey Gladkov
@ 2023-05-20 15:17 ` Vladimir D. Seleznev
2023-05-20 17:23 ` Leonid Krivoshein
1 sibling, 0 replies; 45+ messages in thread
From: Vladimir D. Seleznev @ 2023-05-20 15:17 UTC (permalink / raw)
To: make-initrd
On Sat, May 20, 2023 at 03:18:35PM +0200, Alexey Gladkov wrote:
> On Sat, May 20, 2023 at 12:00:14PM +0300, Leonid Krivoshein wrote:
> > Привет!
> >
> >
> > On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > > +typedef int (*fdhandler_t)(int);
> > > +
> > > +struct fd_handler {
> > > + int fd;
> > > + fdhandler_t fd_handler;
> > > +};
> > > +
> > > +struct fd_handler fd_list[FD_MAX];
> >
> > Имена прототипа функции-обработчика (1), структуры (2) и её члена (3)
> > можно оставить как есть, назвав почти одним именем, но только, если
> > очень хочется кого-то запутать. :-)
>
> Тип хэндлера (fdhandler_t), структура для хэндлера дескриптора (fd_handler),
> ну и собственно список структур (fd_list). Мне кажется всё понятно и
> читабильно.
>
> > Имя fd_list[] тоже не очень подходящее, поскольку это не только список
> > именно fd.
>
> Принимаю предложения на более удачное название ))
fd_handler_list[]
--
WBR,
Vladimir D. Seleznev
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-20 13:18 ` Alexey Gladkov
2023-05-20 15:17 ` Vladimir D. Seleznev
@ 2023-05-20 17:23 ` Leonid Krivoshein
2023-05-20 18:51 ` Alexey Gladkov
2023-05-21 8:53 ` [make-initrd] [PATCH] ueventd: Don't use a epoll timeout when it's not needed Alexey Gladkov
1 sibling, 2 replies; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-20 17:23 UTC (permalink / raw)
To: make-initrd
On 5/20/23 16:18, Alexey Gladkov wrote:
> On Sat, May 20, 2023 at 12:00:14PM +0300, Leonid Krivoshein wrote:
>> [...]
>>> [...]
>>> +
>>> +typedef int (*fdhandler_t)(int);
>>> +
>>> +struct fd_handler {
>>> + int fd;
>>> + fdhandler_t fd_handler;
>>> +};
>>> +
>>> +struct fd_handler fd_list[FD_MAX];
>> [...]
>> Имя fd_list[] тоже не очень подходящее, поскольку это не только список
>> именно fd.
> Принимаю предложения на более удачное название ))
fdhs или fdh_list, но и новое (fd_handler_list) вполне подходит --
больше fd_handler'ов. ))
>> Константа FD_MAX, хоть и не определена в использованных
>> заголовочных файлах, это имя иногда встречается рядом с FD_SET(),
>> FD_CLR() и им подобным макросам в других исходниках как функция
>> FD_MAX(x). На всякий случай...
> Это имя ни с чем не конфликтует.
>
> $ grep -re '\<FD_MAX\>' /usr/include | wc -l
> 0
Да, но используется зарезервированный префикс FD_, что с каким-то
очередным изменением хедеров потенциально может дать конфликт.
>>> +
>>> +enum {
>>> + PIPE_READ,
>>> + PIPE_WRITE,
>>> + PIPE_MAX,
>>> +};
>>> +
>>> +int pipefd[PIPE_MAX];
>> Данным фрагментом ты как бы говоришь: "я точно знаю, во что это
>> превратит компилятор", слишком безапелляционно.
> Стандарт си говорит:
Пока у компилятора не появится глобальная опция, меняющая стартовое
значение для enum, и её не поменяет кто-нибудь в сборочной среде
глобально, беспокоиться и правда не стоит.
> [...]
>>> + fdcount = epoll_wait(epollfd, ev, EV_MAX, 250);
>> Почему 250, а не -1? Напоминает не нужный в этом месте "sleep .25". Если
>> бы демон перезапускался для дефрагментации через какое-то число событий
>> или спустя сколько-то минут, а тут совсем холостой ход получается.
> Это сделано намеренно. Раньше ты спрашивал зачем отправлять wfd родителю
> через пайп. Родитель помечает очередь как F_DIRTY и на следующей итерации
> эта очередь попробует обработаться даже если новых эвентов нет.
Тогда стоит ввести флаг и заменить 250 на что-то вроде:
have_dirty ? 250: -1
>>> +
>>> + if (fdcount < 0) {
>>> + if (errno == EINTR)
>>> + continue;
>>> + fatal("epoll_wait: %m");
>>> + }
>>> +
>>> + for (i = 0; i < fdcount; i++) {
>>> + if (!(ev[i].events & EPOLLIN))
>>> + continue;
>>> + for (int k = 0; k < FD_MAX; k++) {
>>> + if (ev[i].data.fd != fd_list[k].fd)
>>> + continue;
>>> + if (fd_list[k].fd_handler(fd_list[k].fd) != 0)
>> Непонятно, почему из всей структуры Userdata используется только
>> ev->data->fd и потом элементы перебираются в циклах для поиска и
>> сопоставления, хотя в ev->data при epoll_ctl(..., EPOL_CTL_ADD, ...)
>> вполне можно было бы разместить всё самое интересное для ускорения доступа.
> Всё? Это union.
Не заметил сразу, что это union. Раз так, то уже выбранный ev->data->ptr
мне тоже нравится больше.
> Я мог бы положить туда либо fd, либо указатель на
> fd_list[k]. В целом так и стоит сделать. Раньше fd_list не было и остался
> этот атавизм. Но это позволит избавиться от цикла только в этом месте.
>
> [...]
--
WBR, Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
2023-05-20 17:23 ` Leonid Krivoshein
@ 2023-05-20 18:51 ` Alexey Gladkov
2023-05-21 8:53 ` [make-initrd] [PATCH] ueventd: Don't use a epoll timeout when it's not needed Alexey Gladkov
1 sibling, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-20 18:51 UTC (permalink / raw)
To: make-initrd
On Sat, May 20, 2023 at 08:23:26PM +0300, Leonid Krivoshein wrote:
> >> Константа FD_MAX, хоть и не определена в использованных
> >> заголовочных файлах, это имя иногда встречается рядом с FD_SET(),
> >> FD_CLR() и им подобным макросам в других исходниках как функция
> >> FD_MAX(x). На всякий случай...
> > Это имя ни с чем не конфликтует.
> >
> > $ grep -re '\<FD_MAX\>' /usr/include | wc -l
> > 0
>
> Да, но используется зарезервированный префикс FD_, что с каким-то
> очередным изменением хедеров потенциально может дать конфликт.
Это же можно сказать абсолютно про любые префиксы. Например, PIPE_ тоже
зарезервирован поскольку есть PIPE_BUF и может появиться ещё что-то. Если
идти по этому пути, то для всех символов нужно давать
супер_мега_офигительный_префикс_ для уменьшения вероятности конфликта.
Поэтому я предлагаю не пытаться угадывать будущее и не исправлять
несуществующие баги. Если в будущем случится конфликт, то он легко
исправляется переименованием.
> >>> +
> >>> +enum {
> >>> + PIPE_READ,
> >>> + PIPE_WRITE,
> >>> + PIPE_MAX,
> >>> +};
> >>> +
> >>> +int pipefd[PIPE_MAX];
> >> Данным фрагментом ты как бы говоришь: "я точно знаю, во что это
> >> превратит компилятор", слишком безапелляционно.
> > Стандарт си говорит:
>
> Пока у компилятора не появится глобальная опция, меняющая стартовое
> значение для enum, и её не поменяет кто-нибудь в сборочной среде
> глобально, беспокоиться и правда не стоит.
Ты опять предлагаешь исправлять несуществующие ошибки.
> > [...]
> >>> + fdcount = epoll_wait(epollfd, ev, EV_MAX, 250);
> >> Почему 250, а не -1? Напоминает не нужный в этом месте "sleep .25". Если
> >> бы демон перезапускался для дефрагментации через какое-то число событий
> >> или спустя сколько-то минут, а тут совсем холостой ход получается.
> > Это сделано намеренно. Раньше ты спрашивал зачем отправлять wfd родителю
> > через пайп. Родитель помечает очередь как F_DIRTY и на следующей итерации
> > эта очередь попробует обработаться даже если новых эвентов нет.
>
> Тогда стоит ввести флаг и заменить 250 на что-то вроде:
>
> have_dirty ? 250: -1
Хорошая мысль.
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] [PATCH] ueventd: Don't use a epoll timeout when it's not needed
2023-05-20 17:23 ` Leonid Krivoshein
2023-05-20 18:51 ` Alexey Gladkov
@ 2023-05-21 8:53 ` Alexey Gladkov
2023-05-22 4:46 ` Leonid Krivoshein
1 sibling, 1 reply; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-21 8:53 UTC (permalink / raw)
To: make-initrd
The timeout is used to process queues that have unprocessed events
(dirty ones) even when no new events have arrived in the queue. When
there are no dirty queues, then there is no point in extra iterations of
the main loop.
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
datasrc/ueventd/ueventd.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
index 857d3b4f..afe050d0 100644
--- a/datasrc/ueventd/ueventd.c
+++ b/datasrc/ueventd/ueventd.c
@@ -56,7 +56,8 @@ enum {
PIPE_MAX,
};
-int pipefd[PIPE_MAX];
+static int pipefd[PIPE_MAX];
+static int dirty_queues = 0;
#define EV_PAUSE_MASK (IN_CREATE | IN_DELETE | IN_ONLYDIR)
#define EV_ROOT_MASK (IN_CREATE | IN_ONLYDIR)
@@ -121,8 +122,10 @@ int watch_path(int inotifyfd, const char *dir, const char *name, uint32_t mask,
new->next = watch_list;
if (flags & F_QUEUE_DIR) {
- if (!empty_directory(path))
+ if (!empty_directory(path)) {
new->q_flags |= F_DIRTY;
+ dirty_queues++;
+ }
new->q_parentfd = pipefd[PIPE_WRITE];
}
@@ -339,8 +342,10 @@ int process_inotify_events(int inotifyfd)
continue;
}
- if (!(p->q_flags & F_DIRTY))
+ if (!(p->q_flags & F_DIRTY)) {
p->q_flags |= F_DIRTY;
+ dirty_queues++;
+ }
}
}
return 0;
@@ -375,8 +380,10 @@ int process_pipefd_events(int readfd)
for (p = watch_list; p; p = p->next) {
if (!(p->q_flags & F_QUEUE_DIR) || p->q_watchfd != value)
continue;
- if (!(p->q_flags & F_DIRTY))
+ if (!(p->q_flags & F_DIRTY)) {
p->q_flags |= F_DIRTY;
+ dirty_queues++;
+ }
break;
}
}
@@ -521,7 +528,7 @@ int main(int argc, char **argv)
int fdcount;
errno = 0;
- fdcount = epoll_wait(epollfd, ev, EV_MAX, 250);
+ fdcount = epoll_wait(epollfd, ev, EV_MAX, (dirty_queues ? 250 : -1));
if (fdcount < 0) {
if (errno == EINTR)
@@ -542,6 +549,12 @@ int main(int argc, char **argv)
for (e = watch_list; e; e = e->next) {
if (!(e->q_flags & F_QUEUE_DIR) || !(e->q_flags & F_DIRTY) || (e->q_pid != 0) || (e->q_flags & F_PAUSED))
continue;
+
+ if (dirty_queues == 0)
+ rd_fatal("BUG: dirty_queues == 0, but dirty queues are present");
+
+ dirty_queues--;
+
e->q_flags &= ~F_DIRTY;
e->q_pid = spawn_worker(epollfd, e);
}
--
2.33.8
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH] ueventd: Don't use a epoll timeout when it's not needed
2023-05-21 8:53 ` [make-initrd] [PATCH] ueventd: Don't use a epoll timeout when it's not needed Alexey Gladkov
@ 2023-05-22 4:46 ` Leonid Krivoshein
2023-05-22 7:54 ` Alexey Gladkov
` (2 more replies)
0 siblings, 3 replies; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-22 4:46 UTC (permalink / raw)
To: make-initrd
Привет!
Глянул финальную версию в for-master. Было неплохо, а стало намного
лучше. :-)
В ueventd.c(108): утечка path. А в строках 124, 196, 324, 330 и 336 нет
предупреждений? В строке 178 получается странный путь для отслеживания
("%s/queue/pause/."), поскольку параметр name="." -- так задумано или
тут д.б. ".all"?
В memory.c несколько удивляет неудобство интерфейса rd_asprintf_or_die()
-- зачем тащить указатель на переменную в первом параметре и потом её
возвращать? Мне кажется, тут можно обойтись локальной переменной и не
тащить указатель через стек, к тому же 'char *'.
В queue-processor.c вызовы open() в 51, 55 и 59 так и остались не
обёрнуты, на хорошей нагрузке это может приводить к произвольным
фатальным вылетам с Interrupted system call. К слову, абсолютно все
close() в коде теоретически могут быть этому подвержены.
On 5/21/23 11:53, Alexey Gladkov wrote:
> The timeout is used to process queues that have unprocessed events
> (dirty ones) even when no new events have arrived in the queue. When
> there are no dirty queues, then there is no point in extra iterations of
> the main loop.
>
> Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> ---
> datasrc/ueventd/ueventd.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
> index 857d3b4f..afe050d0 100644
> --- a/datasrc/ueventd/ueventd.c
> +++ b/datasrc/ueventd/ueventd.c
> @@ -56,7 +56,8 @@ enum {
> PIPE_MAX,
> };
>
> -int pipefd[PIPE_MAX];
> +static int pipefd[PIPE_MAX];
> +static int dirty_queues = 0;
>
> #define EV_PAUSE_MASK (IN_CREATE | IN_DELETE | IN_ONLYDIR)
> #define EV_ROOT_MASK (IN_CREATE | IN_ONLYDIR)
> @@ -121,8 +122,10 @@ int watch_path(int inotifyfd, const char *dir, const char *name, uint32_t mask,
> new->next = watch_list;
>
> if (flags & F_QUEUE_DIR) {
> - if (!empty_directory(path))
> + if (!empty_directory(path)) {
> new->q_flags |= F_DIRTY;
> + dirty_queues++;
> + }
> new->q_parentfd = pipefd[PIPE_WRITE];
> }
>
> @@ -339,8 +342,10 @@ int process_inotify_events(int inotifyfd)
> continue;
> }
>
> - if (!(p->q_flags & F_DIRTY))
> + if (!(p->q_flags & F_DIRTY)) {
> p->q_flags |= F_DIRTY;
> + dirty_queues++;
> + }
> }
> }
> return 0;
> @@ -375,8 +380,10 @@ int process_pipefd_events(int readfd)
> for (p = watch_list; p; p = p->next) {
> if (!(p->q_flags & F_QUEUE_DIR) || p->q_watchfd != value)
> continue;
> - if (!(p->q_flags & F_DIRTY))
> + if (!(p->q_flags & F_DIRTY)) {
> p->q_flags |= F_DIRTY;
> + dirty_queues++;
> + }
> break;
> }
> }
> @@ -521,7 +528,7 @@ int main(int argc, char **argv)
> int fdcount;
>
> errno = 0;
> - fdcount = epoll_wait(epollfd, ev, EV_MAX, 250);
> + fdcount = epoll_wait(epollfd, ev, EV_MAX, (dirty_queues ? 250 : -1));
>
> if (fdcount < 0) {
> if (errno == EINTR)
> @@ -542,6 +549,12 @@ int main(int argc, char **argv)
> for (e = watch_list; e; e = e->next) {
> if (!(e->q_flags & F_QUEUE_DIR) || !(e->q_flags & F_DIRTY) || (e->q_pid != 0) || (e->q_flags & F_PAUSED))
> continue;
> +
> + if (dirty_queues == 0)
> + rd_fatal("BUG: dirty_queues == 0, but dirty queues are present");
> +
> + dirty_queues--;
> +
> e->q_flags &= ~F_DIRTY;
> e->q_pid = spawn_worker(epollfd, e);
> }
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH] ueventd: Don't use a epoll timeout when it's not needed
2023-05-22 4:46 ` Leonid Krivoshein
@ 2023-05-22 7:54 ` Alexey Gladkov
2023-05-22 9:19 ` Alexey Gladkov
2023-05-22 7:57 ` [make-initrd] [PATCH 1/2] ueventd: Fix memory leak Alexey Gladkov
2023-05-22 7:57 ` [make-initrd] [PATCH 2/2] ueventd: Change interface rd_asprintf_or_die Alexey Gladkov
2 siblings, 1 reply; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-22 7:54 UTC (permalink / raw)
To: make-initrd
On Mon, May 22, 2023 at 07:46:07AM +0300, Leonid Krivoshein wrote:
> Привет!
>
>
> Глянул финальную версию в for-master. Было неплохо, а стало намного
> лучше. :-)
>
> В ueventd.c(108): утечка path.
Да, пропустил утечку. Спасибо!
> А в строках 124, 196, 324, 330 и 336 нет предупреждений?
Нет.
> В строке 178 получается странный путь для отслеживания
> ("%s/queue/pause/."), поскольку параметр name="." -- так задумано или
> тут д.б. ".all"?
Так и задумано. Может это не самое удачное решение для watch_path(). Нужна
была возможность указывать саму директорию filter_dir. Мне не хотелось
дополнительно копировать и подготавливать строку ради единственного вызова
watch_path() на старте. Поэтому в этом и остальных местах в качестве name
просто передаётся ".".
> В memory.c несколько удивляет неудобство интерфейса rd_asprintf_or_die()
> -- зачем тащить указатель на переменную в первом параметре и потом её
> возвращать? Мне кажется, тут можно обойтись локальной переменной и не
> тащить указатель через стек, к тому же 'char *'.
Очень хорошая мысль. Код становится более читабильным.
Правда, наткнулся на предупреждение от компилятора:
datasrc/libinitramfs/memory.c:30:7: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn]
30 | char *rd_asprintf_or_die(const char *fmt, ...)
| ^~~~~~~~~~~~~~~~~~
да, функция делает exit при условии ошибки vasprintf, но при этом функция
явно делает return. Какой тут нафиг noreturn ? Я не нашёл способов
исправить это кроме как выключить -Wmissing-noreturn.
> В queue-processor.c вызовы open() в 51, 55 и 59 так и остались не
> обёрнуты, на хорошей нагрузке это может приводить к произвольным
> фатальным вылетам с Interrupted system call. К слову, абсолютно все
> close() в коде теоретически могут быть этому подвержены.
Я передумал оборачивать open/close. open может получить на медленном
устройстве, что маловероятно для initramfs.
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH] ueventd: Don't use a epoll timeout when it's not needed
2023-05-22 7:54 ` Alexey Gladkov
@ 2023-05-22 9:19 ` Alexey Gladkov
0 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-22 9:19 UTC (permalink / raw)
To: make-initrd
On Mon, May 22, 2023 at 09:54:03AM +0200, Alexey Gladkov wrote:
> On Mon, May 22, 2023 at 07:46:07AM +0300, Leonid Krivoshein wrote:
> > Привет!
> >
> >
> > Глянул финальную версию в for-master. Было неплохо, а стало намного
> > лучше. :-)
> >
> > В ueventd.c(108): утечка path.
>
> Да, пропустил утечку. Спасибо!
>
> > А в строках 124, 196, 324, 330 и 336 нет предупреждений?
>
> Нет.
>
> > В строке 178 получается странный путь для отслеживания
> > ("%s/queue/pause/."), поскольку параметр name="." -- так задумано или
> > тут д.б. ".all"?
>
> Так и задумано. Может это не самое удачное решение для watch_path(). Нужна
> была возможность указывать саму директорию filter_dir. Мне не хотелось
> дополнительно копировать и подготавливать строку ради единственного вызова
> watch_path() на старте. Поэтому в этом и остальных местах в качестве name
> просто передаётся ".".
>
> > В memory.c несколько удивляет неудобство интерфейса rd_asprintf_or_die()
> > -- зачем тащить указатель на переменную в первом параметре и потом её
> > возвращать? Мне кажется, тут можно обойтись локальной переменной и не
> > тащить указатель через стек, к тому же 'char *'.
>
> Очень хорошая мысль. Код становится более читабильным.
>
> Правда, наткнулся на предупреждение от компилятора:
>
> datasrc/libinitramfs/memory.c:30:7: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn]
> 30 | char *rd_asprintf_or_die(const char *fmt, ...)
> | ^~~~~~~~~~~~~~~~~~
>
> да, функция делает exit при условии ошибки vasprintf, но при этом функция
> явно делает return. Какой тут нафиг noreturn ? Я не нашёл способов
> исправить это кроме как выключить -Wmissing-noreturn.
Потому что я написал херню и так компилятор мне на это намекал. Даже
стыдно, что я сразу этого не заметил.
> > В queue-processor.c вызовы open() в 51, 55 и 59 так и остались не
> > обёрнуты, на хорошей нагрузке это может приводить к произвольным
> > фатальным вылетам с Interrupted system call. К слову, абсолютно все
> > close() в коде теоретически могут быть этому подвержены.
>
> Я передумал оборачивать open/close. open может получить на медленном
> устройстве, что маловероятно для initramfs.
>
> --
> Rgrds, legion
>
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] [PATCH 1/2] ueventd: Fix memory leak
2023-05-22 4:46 ` Leonid Krivoshein
2023-05-22 7:54 ` Alexey Gladkov
@ 2023-05-22 7:57 ` Alexey Gladkov
2023-05-22 7:57 ` [make-initrd] [PATCH 2/2] ueventd: Change interface rd_asprintf_or_die Alexey Gladkov
2 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-22 7:57 UTC (permalink / raw)
To: make-initrd
Fixes: 31cbad99 ("Reimplement ueventd")
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
datasrc/ueventd/ueventd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
index afe050d0..c2c23ba3 100644
--- a/datasrc/ueventd/ueventd.c
+++ b/datasrc/ueventd/ueventd.c
@@ -104,8 +104,10 @@ int watch_path(int inotifyfd, const char *dir, const char *name, uint32_t mask,
rd_asprintf_or_die(&path, "%s/%s", dir, name);
int wfd = add_queue_dir(inotifyfd, path, mask);
- if (wfd < 0)
+ if (wfd < 0) {
+ free(path);
return (wfd == -128 ? 0 : wfd);
+ }
if (stat(path, &st) < 0) {
rd_err("stat: %s: %m", path);
--
2.33.8
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] [PATCH 2/2] ueventd: Change interface rd_asprintf_or_die
2023-05-22 4:46 ` Leonid Krivoshein
2023-05-22 7:54 ` Alexey Gladkov
2023-05-22 7:57 ` [make-initrd] [PATCH 1/2] ueventd: Fix memory leak Alexey Gladkov
@ 2023-05-22 7:57 ` Alexey Gladkov
2023-05-22 9:36 ` [make-initrd] [PATCH v2] " Alexey Gladkov
2 siblings, 1 reply; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-22 7:57 UTC (permalink / raw)
To: make-initrd
There is no need to pass a pointer for the result to rd_asprintf_or_die
because the error check is inside the function and the function cannot
fail.
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
Makefile.in | 2 +-
datasrc/libinitramfs/memory.c | 3 ++-
datasrc/libinitramfs/rd/memory.h | 6 +++---
datasrc/ueventd/queue-processor.c | 10 +++-------
datasrc/ueventd/ueventd.c | 11 +++--------
5 files changed, 12 insertions(+), 20 deletions(-)
diff --git a/Makefile.in b/Makefile.in
index 45053fa0..4d133964 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -138,7 +138,7 @@ PATH = $(CURDIR)/$(dest_sbindir):$(CURDIR)/$(dest_bindir):$(shell echo $$PATH)
CFLAGS = @CFLAGS@ \
-Wall -Wextra -W -Wshadow -Wcast-align \
-Wwrite-strings -Wconversion -Waggregate-return -Wstrict-prototypes \
- -Wmissing-prototypes -Wmissing-declarations -Wmissing-noreturn \
+ -Wmissing-prototypes -Wmissing-declarations \
-Wmissing-format-attribute -Wredundant-decls -Wdisabled-optimization \
-Wno-pointer-arith \
-Werror=shadow -Werror=missing-prototypes -Werror=implicit-function-declaration
diff --git a/datasrc/libinitramfs/memory.c b/datasrc/libinitramfs/memory.c
index ea0d292e..f884dd21 100644
--- a/datasrc/libinitramfs/memory.c
+++ b/datasrc/libinitramfs/memory.c
@@ -27,8 +27,9 @@ void *rd_malloc_or_die(size_t size)
return r;
}
-char *rd_asprintf_or_die(char **ptr, const char *fmt, ...)
+char *rd_asprintf_or_die(const char *fmt, ...)
{
+ char **ptr = NULL;
va_list arg;
va_start(arg, fmt);
diff --git a/datasrc/libinitramfs/rd/memory.h b/datasrc/libinitramfs/rd/memory.h
index 415e8394..6694f212 100644
--- a/datasrc/libinitramfs/rd/memory.h
+++ b/datasrc/libinitramfs/rd/memory.h
@@ -5,8 +5,8 @@
#include <stdlib.h>
-void *rd_calloc_or_die(size_t nmemb, size_t size) __attribute__((alloc_size(1, 2)));
-void *rd_malloc_or_die(size_t size) __attribute__((alloc_size(1)));
-char *rd_asprintf_or_die(char **ptr, const char *fmt, ...) __attribute__((nonnull(1, 2),format(printf, 2, 3)));
+void *rd_calloc_or_die(size_t nmemb, size_t size) __attribute__((alloc_size(1, 2),returns_nonnull,warn_unused_result));
+void *rd_malloc_or_die(size_t size) __attribute__((alloc_size(1),returns_nonnull,warn_unused_result));
+char *rd_asprintf_or_die(const char *fmt, ...) __attribute__((nonnull(1),format(printf, 1, 2),returns_nonnull,warn_unused_result));
#endif /* __RD_MEMORY_H__ */
diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
index 908492a7..8b13a1c9 100644
--- a/datasrc/ueventd/queue-processor.c
+++ b/datasrc/ueventd/queue-processor.c
@@ -92,15 +92,11 @@ void process_events(struct watch *queue)
{
rd_info("%s: session=%lu: processing events", queue->q_name, session);
- char *numenv;
-
- rd_asprintf_or_die(&numenv, "SESSION=%lu", session);
+ char *numenv = rd_asprintf_or_die("SESSION=%lu", session);
putenv(numenv);
- char *srcdir, *dstdir;
-
- rd_asprintf_or_die(&srcdir, "%s/%s", filter_dir, queue->q_name);
- rd_asprintf_or_die(&dstdir, "%s/%s", uevent_dir, queue->q_name);
+ char *srcdir = rd_asprintf_or_die("%s/%s", filter_dir, queue->q_name);
+ char *dstdir = rd_asprintf_or_die("%s/%s", uevent_dir, queue->q_name);
move_files(srcdir, dstdir);
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
index c2c23ba3..414c3f51 100644
--- a/datasrc/ueventd/ueventd.c
+++ b/datasrc/ueventd/ueventd.c
@@ -99,9 +99,8 @@ int watch_path(int inotifyfd, const char *dir, const char *name, uint32_t mask,
{
struct stat st;
struct watch *new = NULL;
- char *path;
- rd_asprintf_or_die(&path, "%s/%s", dir, name);
+ char *path = rd_asprintf_or_die("%s/%s", dir, name);
int wfd = add_queue_dir(inotifyfd, path, mask);
if (wfd < 0) {
@@ -173,9 +172,7 @@ int unwatch_path(int inotifyfd, ino_t ino)
void watch_pauses(int inotifyfd)
{
- char *path;
-
- rd_asprintf_or_die(&path, "%s/queue/pause", uevent_confdb);
+ char *path = rd_asprintf_or_die("%s/queue/pause", uevent_confdb);
if (watch_path(inotifyfd, path, ".", EV_PAUSE_MASK, F_PAUSE_DIR) < 0)
exit(EXIT_FAILURE);
@@ -187,9 +184,7 @@ void apply_pause(void)
{
struct watch *p;
DIR *dir;
- char *path;
-
- rd_asprintf_or_die(&path, "%s/queue/pause", uevent_confdb);
+ char *path = rd_asprintf_or_die("%s/queue/pause", uevent_confdb);
dir = xopendir(path);
pause_all = 0;
--
2.33.8
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] [PATCH v2] ueventd: Change interface rd_asprintf_or_die
2023-05-22 7:57 ` [make-initrd] [PATCH 2/2] ueventd: Change interface rd_asprintf_or_die Alexey Gladkov
@ 2023-05-22 9:36 ` Alexey Gladkov
0 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-22 9:36 UTC (permalink / raw)
To: make-initrd
There is no need to pass a pointer for the result to rd_asprintf_or_die
because the error check is inside the function and the function cannot
fail.
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
datasrc/libinitramfs/memory.c | 7 ++++---
datasrc/libinitramfs/rd/memory.h | 7 ++++---
datasrc/ueventd/queue-processor.c | 10 +++-------
datasrc/ueventd/ueventd.c | 11 +++--------
4 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/datasrc/libinitramfs/memory.c b/datasrc/libinitramfs/memory.c
index ea0d292e..cd6a19f7 100644
--- a/datasrc/libinitramfs/memory.c
+++ b/datasrc/libinitramfs/memory.c
@@ -27,14 +27,15 @@ void *rd_malloc_or_die(size_t size)
return r;
}
-char *rd_asprintf_or_die(char **ptr, const char *fmt, ...)
+char *rd_asprintf_or_die(const char *fmt, ...)
{
+ char *ptr;
va_list arg;
va_start(arg, fmt);
- if (vasprintf(ptr, fmt, arg) < 0)
+ if (vasprintf(&ptr, fmt, arg) < 0)
rd_fatal("vasprintf: %m");
va_end(arg);
- return *ptr;
+ return ptr;
}
diff --git a/datasrc/libinitramfs/rd/memory.h b/datasrc/libinitramfs/rd/memory.h
index 415e8394..b8c82909 100644
--- a/datasrc/libinitramfs/rd/memory.h
+++ b/datasrc/libinitramfs/rd/memory.h
@@ -5,8 +5,9 @@
#include <stdlib.h>
-void *rd_calloc_or_die(size_t nmemb, size_t size) __attribute__((alloc_size(1, 2)));
-void *rd_malloc_or_die(size_t size) __attribute__((alloc_size(1)));
-char *rd_asprintf_or_die(char **ptr, const char *fmt, ...) __attribute__((nonnull(1, 2),format(printf, 2, 3)));
+void *rd_calloc_or_die(size_t nmemb, size_t size) __attribute__((alloc_size(1, 2),returns_nonnull,warn_unused_result));
+void *rd_malloc_or_die(size_t size) __attribute__((alloc_size(1),returns_nonnull,warn_unused_result));
+char *rd_asprintf_or_die(const char *fmt, ...) __attribute__((nonnull(1),format(printf, 1, 2),returns_nonnull,
+ warn_unused_result));
#endif /* __RD_MEMORY_H__ */
diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
index 908492a7..8b13a1c9 100644
--- a/datasrc/ueventd/queue-processor.c
+++ b/datasrc/ueventd/queue-processor.c
@@ -92,15 +92,11 @@ void process_events(struct watch *queue)
{
rd_info("%s: session=%lu: processing events", queue->q_name, session);
- char *numenv;
-
- rd_asprintf_or_die(&numenv, "SESSION=%lu", session);
+ char *numenv = rd_asprintf_or_die("SESSION=%lu", session);
putenv(numenv);
- char *srcdir, *dstdir;
-
- rd_asprintf_or_die(&srcdir, "%s/%s", filter_dir, queue->q_name);
- rd_asprintf_or_die(&dstdir, "%s/%s", uevent_dir, queue->q_name);
+ char *srcdir = rd_asprintf_or_die("%s/%s", filter_dir, queue->q_name);
+ char *dstdir = rd_asprintf_or_die("%s/%s", uevent_dir, queue->q_name);
move_files(srcdir, dstdir);
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
index c2c23ba3..414c3f51 100644
--- a/datasrc/ueventd/ueventd.c
+++ b/datasrc/ueventd/ueventd.c
@@ -99,9 +99,8 @@ int watch_path(int inotifyfd, const char *dir, const char *name, uint32_t mask,
{
struct stat st;
struct watch *new = NULL;
- char *path;
- rd_asprintf_or_die(&path, "%s/%s", dir, name);
+ char *path = rd_asprintf_or_die("%s/%s", dir, name);
int wfd = add_queue_dir(inotifyfd, path, mask);
if (wfd < 0) {
@@ -173,9 +172,7 @@ int unwatch_path(int inotifyfd, ino_t ino)
void watch_pauses(int inotifyfd)
{
- char *path;
-
- rd_asprintf_or_die(&path, "%s/queue/pause", uevent_confdb);
+ char *path = rd_asprintf_or_die("%s/queue/pause", uevent_confdb);
if (watch_path(inotifyfd, path, ".", EV_PAUSE_MASK, F_PAUSE_DIR) < 0)
exit(EXIT_FAILURE);
@@ -187,9 +184,7 @@ void apply_pause(void)
{
struct watch *p;
DIR *dir;
- char *path;
-
- rd_asprintf_or_die(&path, "%s/queue/pause", uevent_confdb);
+ char *path = rd_asprintf_or_die("%s/queue/pause", uevent_confdb);
dir = xopendir(path);
pause_all = 0;
--
2.33.8
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] [PATCH 1/3] ueventd: Simplify call of the queue handler
2023-05-20 9:00 ` Leonid Krivoshein
2023-05-20 13:18 ` Alexey Gladkov
@ 2023-05-20 16:37 ` Alexey Gladkov
2023-05-20 16:37 ` [make-initrd] [PATCH 2/3] ueventd: Rename fd_list to fd_handler_list Alexey Gladkov
2023-05-20 16:37 ` [make-initrd] [PATCH 3/3] ueventd: Drop obsolete declarations Alexey Gladkov
3 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-20 16:37 UTC (permalink / raw)
To: make-initrd
There is no need to iterate over fd_list[] on every event. Instead of a
descriptor, we can use a pointer to the structure itself.
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
datasrc/ueventd/ueventd.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
index 67bc3dda..9c20594f 100644
--- a/datasrc/ueventd/ueventd.c
+++ b/datasrc/ueventd/ueventd.c
@@ -401,7 +401,7 @@ int setup_epoll_fd(struct fd_handler list[FD_MAX])
rd_fatal("epoll_create1: %m");
for (int i = 0; i < FD_MAX; i++) {
- ev.data.fd = list[i].fd;
+ ev.data.ptr = &list[i];
if (epoll_ctl(epollfd, EPOLL_CTL_ADD, list[i].fd, &ev) < 0)
rd_fatal("epoll_ctl: %m");
@@ -531,12 +531,11 @@ int main(int argc, char **argv)
for (i = 0; i < fdcount; i++) {
if (!(ev[i].events & EPOLLIN))
continue;
- for (int k = 0; k < FD_MAX; k++) {
- if (ev[i].data.fd != fd_list[k].fd)
- continue;
- if (fd_list[k].fd_handler(fd_list[k].fd) != 0)
- goto done;
- }
+
+ struct fd_handler *fde = ev[i].data.ptr;
+
+ if (fde->fd_handler(fde->fd) != 0)
+ goto done;
}
for (e = watch_list; e; e = e->next) {
--
2.33.8
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] [PATCH 2/3] ueventd: Rename fd_list to fd_handler_list
2023-05-20 9:00 ` Leonid Krivoshein
2023-05-20 13:18 ` Alexey Gladkov
2023-05-20 16:37 ` [make-initrd] [PATCH 1/3] ueventd: Simplify call of the queue handler Alexey Gladkov
@ 2023-05-20 16:37 ` Alexey Gladkov
2023-05-20 16:37 ` [make-initrd] [PATCH 3/3] ueventd: Drop obsolete declarations Alexey Gladkov
3 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-20 16:37 UTC (permalink / raw)
To: make-initrd
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
datasrc/ueventd/ueventd.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
index 9c20594f..2d510d62 100644
--- a/datasrc/ueventd/ueventd.c
+++ b/datasrc/ueventd/ueventd.c
@@ -47,7 +47,7 @@ struct fd_handler {
fdhandler_t fd_handler;
};
-struct fd_handler fd_list[FD_MAX];
+struct fd_handler fd_handler_list[FD_MAX];
enum {
PIPE_READ,
@@ -428,8 +428,8 @@ pid_t spawn_worker(int epollfd, struct watch *queue)
rd_fatal("prctl(PR_SET_PDEATHSIG): %m");
for (int i = 0; i < FD_MAX; i++) {
- if (fd_list[i].fd >= 0)
- close(fd_list[i].fd);
+ if (fd_handler_list[i].fd >= 0)
+ close(fd_handler_list[i].fd);
}
close(epollfd);
@@ -501,18 +501,18 @@ int main(int argc, char **argv)
uevent_confdb = get_param_dir("uevent_confdb");
handler_file = argv[optind++];
- setup_inotify_fd(&fd_list[FD_INOTIFY]);
- setup_signal_fd(&fd_list[FD_SIGNAL]);
- setup_pipe_fd(&fd_list[FD_PIPE]);
+ setup_inotify_fd(&fd_handler_list[FD_INOTIFY]);
+ setup_signal_fd(&fd_handler_list[FD_SIGNAL]);
+ setup_pipe_fd(&fd_handler_list[FD_PIPE]);
- epollfd = setup_epoll_fd(fd_list);
+ epollfd = setup_epoll_fd(fd_handler_list);
- watch_pauses(fd_list[FD_INOTIFY].fd);
+ watch_pauses(fd_handler_list[FD_INOTIFY].fd);
- if (watch_path(fd_list[FD_INOTIFY].fd, filter_dir, ".", EV_ROOT_MASK, F_ROOT_DIR) < 0)
+ if (watch_path(fd_handler_list[FD_INOTIFY].fd, filter_dir, ".", EV_ROOT_MASK, F_ROOT_DIR) < 0)
exit(EXIT_FAILURE);
- watch_queues(fd_list[FD_INOTIFY].fd);
+ watch_queues(fd_handler_list[FD_INOTIFY].fd);
apply_pause();
while (1) {
@@ -550,15 +550,15 @@ done:
n = e->next;
if (e->q_pid)
kill(e->q_pid, SIGKILL);
- inotify_rm_watch(fd_list[FD_INOTIFY].fd, e->q_watchfd);
+ inotify_rm_watch(fd_handler_list[FD_INOTIFY].fd, e->q_watchfd);
free(e);
}
for (i = 0; i < FD_MAX; i++) {
- if (fd_list[i].fd >= 0) {
- if (epoll_ctl(epollfd, EPOLL_CTL_DEL, fd_list[i].fd, NULL) < 0)
+ if (fd_handler_list[i].fd >= 0) {
+ if (epoll_ctl(epollfd, EPOLL_CTL_DEL, fd_handler_list[i].fd, NULL) < 0)
rd_err("epoll_ctl: %m");
- close(fd_list[i].fd);
+ close(fd_handler_list[i].fd);
}
}
close(epollfd);
--
2.33.8
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] [PATCH 3/3] ueventd: Drop obsolete declarations
2023-05-20 9:00 ` Leonid Krivoshein
` (2 preceding siblings ...)
2023-05-20 16:37 ` [make-initrd] [PATCH 2/3] ueventd: Rename fd_list to fd_handler_list Alexey Gladkov
@ 2023-05-20 16:37 ` Alexey Gladkov
3 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-20 16:37 UTC (permalink / raw)
To: make-initrd
After the logging functions were moved to the common library, they were
not removed from the header.
Also includes have been grouped at the top of the header to increase
readability.
Fixes: 78769ff4 ("Move logging functions to the separate library")
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
datasrc/ueventd/path.c | 1 +
datasrc/ueventd/queue-processor.c | 1 +
datasrc/ueventd/ueventd.c | 1 +
datasrc/ueventd/ueventd.h | 32 +++----------------------------
4 files changed, 6 insertions(+), 29 deletions(-)
diff --git a/datasrc/ueventd/path.c b/datasrc/ueventd/path.c
index 835f4fbb..330e8c82 100644
--- a/datasrc/ueventd/path.c
+++ b/datasrc/ueventd/path.c
@@ -5,6 +5,7 @@
#include <fcntl.h>
#include <errno.h>
+#include "rd/logging.h"
#include "ueventd.h"
int is_dot_dir(struct dirent *ent)
diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
index b8383f51..908492a7 100644
--- a/datasrc/ueventd/queue-processor.c
+++ b/datasrc/ueventd/queue-processor.c
@@ -13,6 +13,7 @@
#include <errno.h>
#include "rd/memory.h"
+#include "rd/logging.h"
#include "ueventd.h"
static void event_handler(struct watch *queue, char *path) __attribute__((nonnull(1, 2)));
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
index 2d510d62..857d3b4f 100644
--- a/datasrc/ueventd/ueventd.c
+++ b/datasrc/ueventd/ueventd.c
@@ -20,6 +20,7 @@
#include <errno.h>
#include "rd/memory.h"
+#include "rd/logging.h"
#include "ueventd.h"
#define default_logfile "/var/log/ueventd.log"
diff --git a/datasrc/ueventd/ueventd.h b/datasrc/ueventd/ueventd.h
index 375fa40f..653549f7 100644
--- a/datasrc/ueventd/ueventd.h
+++ b/datasrc/ueventd/ueventd.h
@@ -5,6 +5,8 @@
#include <sys/types.h>
#include <stdint.h>
+#include <unistd.h>
+#include <dirent.h>
#define F_ROOT_DIR (1 << 0)
#define F_QUEUE_DIR (1 << 1)
@@ -31,8 +33,7 @@ extern uint64_t session;
extern void process_events(struct watch *queue) __attribute__((nonnull(1), noreturn));
/* path.c */
-#include <dirent.h>
-
+extern int is_dot_dir(struct dirent *ent) __attribute__((nonnull(1)));
extern DIR *xopendir(const char *path) __attribute__((nonnull(1)));
extern struct dirent *xreaddir(DIR *d, const char *path) __attribute__((nonnull(1, 2)));
extern int empty_directory(const char *path) __attribute__((nonnull(1)));
@@ -43,31 +44,4 @@ extern ssize_t write_loop(int fd, const char *buffer, size_t count) __attribute_
/* process.c */
extern pid_t waitpid_retry(pid_t pid, int *wstatus, int options);
-#include <dirent.h>
-
-extern int is_dot_dir(struct dirent *ent) __attribute__((nonnull(1)));
-
-/* logging.c */
-#include <unistd.h>
-#include <syslog.h>
-#include <stdlib.h>
-#include <stdarg.h>
-
-extern void rd_logging_init(int log_fd, int level, const char *progname);
-extern void rd_logging_close(void);
-extern int rd_logging_level(const char *lvl) __attribute__((nonnull(1)));
-extern void rd_vmessage(const char *fmt, va_list ap) __attribute__((format(printf, 1, 0)));
-extern void rd_log_vmessage(int priority, const char *fmt, va_list ap) __attribute__((format(printf, 2, 0)));
-extern void rd_log_message(int priority, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
-
-#define rd_fatal(format, arg...) \
- do { \
- rd_log_message(LOG_CRIT, "%s:%d: " format, __FILE__, __LINE__, ##arg); \
- _exit(EXIT_FAILURE); \
- } while (0)
-
-#define rd_err(format, arg...) rd_log_message(LOG_ERR, format, ##arg)
-#define rd_info(format, arg...) rd_log_message(LOG_INFO, format, ##arg)
-#define rd_dbg(format, arg...) rd_log_message(LOG_DEBUG, format, ##arg)
-
#endif /* __UEVENTD_H__ */
--
2.33.8
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] [PATCH 2/3] Replace polld by uevent queue
2023-05-04 13:42 [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
@ 2023-05-04 13:42 ` Alexey Gladkov
2023-05-04 13:42 ` [make-initrd] [PATCH 3/3] feature/kickstart: Reset rootdelay timer after kickstart Alexey Gladkov
` (6 subsequent siblings)
8 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-04 13:42 UTC (permalink / raw)
To: make-initrd
Since uevent has become parallel and faster, we can return the polld
functionality back to uevent as a separate queue.
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
data/etc/rc.d/init.d/poll | 25 ++++++-----
data/lib/uevent/each/post/.gitignore | 0
data/lib/uevent/each/post/system-bootable | 4 --
data/lib/uevent/extenders/.gitignore | 0
data/lib/uevent/handlers/poll/100-extenders | 22 +++++++++
data/lib/uevent/handlers/poll/300-boot-method | 29 ++++++++++++
.../poll/400-rootdelay} | 0
data/sbin/polld | 45 -------------------
data/sbin/process-boot-method | 16 -------
.../kickstart/data/etc/rc.d/init.d/kickstart | 10 ++---
10 files changed, 68 insertions(+), 83 deletions(-)
create mode 100644 data/lib/uevent/each/post/.gitignore
delete mode 100755 data/lib/uevent/each/post/system-bootable
create mode 100644 data/lib/uevent/extenders/.gitignore
create mode 100755 data/lib/uevent/handlers/poll/100-extenders
create mode 100755 data/lib/uevent/handlers/poll/300-boot-method
rename data/lib/uevent/{extenders/100-rootdelay => handlers/poll/400-rootdelay} (100%)
delete mode 100755 data/sbin/polld
delete mode 100755 data/sbin/process-boot-method
diff --git a/data/etc/rc.d/init.d/poll b/data/etc/rc.d/init.d/poll
index 8c5e498b..6901f830 100755
--- a/data/etc/rc.d/init.d/poll
+++ b/data/etc/rc.d/init.d/poll
@@ -14,22 +14,23 @@
. /.initrd/initenv
. /etc/init.d/template
+. uevent-sh-functions
-NAME=polld
-PIDFILE="/var/run/$NAME.pid"
-ARGS="--lockfile $LOCKFILE --pidfile $PIDFILE --name $NAME --displayname $NAME"
+start()
+{
+ rmdir -- "$uevent_confdb/queue/pause/poll" 2>/dev/null ||:
-start() {
- start_daemon --background $ARGS "$NAME"
- RETVAL=$?
- return $RETVAL
+ mkdir -p -- /.initrd/uevent/queues/poll/.tmp
+ event="$(make_event poll)"
+ release_event "periodically" "$event"
+
+ return 0
}
-stop() {
- stop_daemon $ARGS "$NAME"
- RETVAL=$?
- [ ! -f "$PIDFILE" ] || rm -f -- "$PIDFILE"
- return $RETVAL
+stop()
+{
+ mkdir -p -- "$uevent_confdb/queue/pause/poll"
+ return 0
}
switch "${1-}"
diff --git a/data/lib/uevent/each/post/.gitignore b/data/lib/uevent/each/post/.gitignore
new file mode 100644
index 00000000..e69de29b
diff --git a/data/lib/uevent/each/post/system-bootable b/data/lib/uevent/each/post/system-bootable
deleted file mode 100755
index 54a39082..00000000
--- a/data/lib/uevent/each/post/system-bootable
+++ /dev/null
@@ -1,4 +0,0 @@
-#!/bin/bash -efu
-
-[ "$1" != udev ] ||
- process-boot-method
diff --git a/data/lib/uevent/extenders/.gitignore b/data/lib/uevent/extenders/.gitignore
new file mode 100644
index 00000000..e69de29b
diff --git a/data/lib/uevent/handlers/poll/100-extenders b/data/lib/uevent/handlers/poll/100-extenders
new file mode 100755
index 00000000..2968c666
--- /dev/null
+++ b/data/lib/uevent/handlers/poll/100-extenders
@@ -0,0 +1,22 @@
+#!/bin/bash -eu
+
+. /.initrd/initenv
+
+[ "${RDLOG-}" != 'console' ] &&
+ logfile=/var/log/polld.log ||
+ logfile=/dev/console
+
+exec >"$logfile" 2>&1
+
+. shell-error
+. uevent-sh-functions
+
+message_time=1
+
+export queuedir="$1"
+export QUEUE="${queuedir##*/}"
+
+for bin in "$extendir"/*; do
+ [ ! -x "$bin" ] || "$bin" ||
+ message "$QUEUE: session=$SESSION: extender failed: $bin"
+done
diff --git a/data/lib/uevent/handlers/poll/300-boot-method b/data/lib/uevent/handlers/poll/300-boot-method
new file mode 100755
index 00000000..2057024f
--- /dev/null
+++ b/data/lib/uevent/handlers/poll/300-boot-method
@@ -0,0 +1,29 @@
+#!/bin/bash -eu
+
+. /.initrd/initenv
+
+[ "${RDLOG-}" != 'console' ] &&
+ logfile=/var/log/polld.log ||
+ logfile=/dev/console
+
+exec >"$logfile" 2>&1
+
+method=
+{ read -r method < /etc/initrd/method; } >/dev/null 2>&1 ||:
+
+cd "/lib/initrd/boot/method/$method"
+
+if [ -x ./check ] && ./check; then
+ . shell-error
+ message_time=1
+
+ queuedir="$1"
+ QUEUE="${queuedir##*/}"
+
+ message "$QUEUE: session=$SESSION: $method ready. acting!"
+
+ ./action
+fi
+
+# slow down the busy loop of this queue
+sleep 0.2
diff --git a/data/lib/uevent/extenders/100-rootdelay b/data/lib/uevent/handlers/poll/400-rootdelay
similarity index 100%
rename from data/lib/uevent/extenders/100-rootdelay
rename to data/lib/uevent/handlers/poll/400-rootdelay
diff --git a/data/sbin/polld b/data/sbin/polld
deleted file mode 100755
index 1f6b05b3..00000000
--- a/data/sbin/polld
+++ /dev/null
@@ -1,45 +0,0 @@
-#!/bin/bash -eu
-
-. shell-error
-. shell-signal
-
-. /.initrd/initenv
-. uevent-sh-functions
-
-message_time=1
-pidfile="/var/run/$PROG.pid"
-logfile="/var/log/$PROG.log"
-
-exit_handler()
-{
- local rc="$?"
- trap - EXIT
- rm -f -- "$pidfile"
- exit $rc
-}
-
-[ "${RDLOG-}" != 'console' ] ||
- logfile=/dev/console
-
-[ ! -f "$pidfile" ] ||
- fatal "already running"
-
-set_cleanup_handler exit_handler
-echo "$$" >"$pidfile"
-
-exec >"$logfile" 2>&1
-message "starting server ..."
-
-while [ -f "$pidfile" ]; do
- for fn in "$extendir"/*; do
- [ -x "$fn" ] || continue
- name="${fn##*/}"
- name="${name#[0-9][0-9][0-9]-}"
-
- "$fn" ||
- message "extender failed: $fn"
- done
-
- process-boot-method ||:
- sleep 1
-done
diff --git a/data/sbin/process-boot-method b/data/sbin/process-boot-method
deleted file mode 100755
index e9d3ecf2..00000000
--- a/data/sbin/process-boot-method
+++ /dev/null
@@ -1,16 +0,0 @@
-#!/bin/bash -efu
-
-. shell-error
-message_time=1
-
-method=
-{ read -r method < /etc/initrd/method; } >/dev/null 2>&1 ||:
-
-d="/lib/initrd/boot/method/$method"
-c="$d/check"
-
-[ -x "$c" ] && "$c" ||
- exit 0
-
-message "$method ready. acting!"
-"$d/action"
diff --git a/features/kickstart/data/etc/rc.d/init.d/kickstart b/features/kickstart/data/etc/rc.d/init.d/kickstart
index 8d6e09f0..e36b9470 100755
--- a/features/kickstart/data/etc/rc.d/init.d/kickstart
+++ b/features/kickstart/data/etc/rc.d/init.d/kickstart
@@ -22,15 +22,13 @@
KSFILE="/etc/ks.conf.d/$KSFILE"
if [ -s "$KSFILE" ]; then
- cp -f /etc/initrd/method /etc/initrd/method.prev
- echo none > /etc/initrd/method
+ . uevent-sh-functions
- kickstart -v "$KSFILE"
+ mkdir -p -- "$uevent_confdb/queue/pause/poll"
- . uevent-sh-functions
+ kickstart -v "$KSFILE"
+ rmdir -- "$uevent_confdb/queue/pause/poll"
rmdir -- "$uevent_confdb/queue/pause/udev"
rmdir -- "$uevent_confdb/queue/pause/md-raid-member"
-
- cp -f /etc/initrd/method.prev /etc/initrd/method
fi
--
2.33.7
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] [PATCH 3/3] feature/kickstart: Reset rootdelay timer after kickstart
2023-05-04 13:42 [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
2023-05-04 13:42 ` [make-initrd] [PATCH 1/3] " Alexey Gladkov
2023-05-04 13:42 ` [make-initrd] [PATCH 2/3] Replace polld by uevent queue Alexey Gladkov
@ 2023-05-04 13:42 ` Alexey Gladkov
2023-05-06 19:45 ` [make-initrd] ueventd: Add a prefix to the logging functions to avoid name collisions Alexey Gladkov
` (5 subsequent siblings)
8 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-04 13:42 UTC (permalink / raw)
To: make-initrd
If the rootdelay timer has started counting down, then after poll is
unpaused, a timeout may occur. This is because the rootdelay remembers
the time and if the kickstart took a long time it will timeout the next
time the rootdelay is checked.
To avoid this we reset the timer before we unpause poll.
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
data/bin/initrd-sh-functions | 8 ++++++++
data/lib/uevent/handlers/poll/400-rootdelay | 13 ++++++-------
features/kickstart/data/etc/rc.d/init.d/kickstart | 3 +++
3 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/data/bin/initrd-sh-functions b/data/bin/initrd-sh-functions
index 1f2c1a8d..f33885ba 100644
--- a/data/bin/initrd-sh-functions
+++ b/data/bin/initrd-sh-functions
@@ -106,6 +106,7 @@ readline()
}
_rootdelay_pause=/.initrd/rootdelay/pause
+_rootdelay_timestamp=/.initrd/rootdelay/deadline
rootdelay_pause()
{
@@ -122,6 +123,13 @@ rootdelay_paused()
[ -d "$_rootdelay_pause" ]
}
+rootdelay_reset_timer()
+{
+ local now
+ now="$(date +'%s')"
+ echo $(( $now + $ROOTDELAY )) > "$_rootdelay_timestamp"
+}
+
initrd_features=/.initrd/features
has_feature()
diff --git a/data/lib/uevent/handlers/poll/400-rootdelay b/data/lib/uevent/handlers/poll/400-rootdelay
index e7089956..cac86622 100755
--- a/data/lib/uevent/handlers/poll/400-rootdelay
+++ b/data/lib/uevent/handlers/poll/400-rootdelay
@@ -23,31 +23,30 @@ fi
first_iter=
now="$(date +'%s')"
-timestamp="/.initrd/rootdelay/deadline"
consmsg="/.initrd/rootdelay/message"
-if [ ! -f "$timestamp" ]; then
+if [ ! -f "$_rootdelay_timestamp" ]; then
first_iter=1
deadline=$(( $now + $ROOTDELAY ))
- mkdir -p -- "${timestamp%/*}"
- echo $deadline > "$timestamp"
+ mkdir -p -- "${_rootdelay_timestamp%/*}"
+ echo $deadline > "$_rootdelay_timestamp"
else
- read -r deadline < "$timestamp"
+ read -r deadline < "$_rootdelay_timestamp"
fi
deadline="${deadline:-$now}"
delay=$(( $deadline - $now ))
if rootdelay_paused; then
- echo $(( $now + $delay )) > "$timestamp"
+ echo $(( $now + $delay )) > "$_rootdelay_timestamp"
exit 0
fi
if [ "$delay" -le 0 ]; then
if ! resume_checked; then
set_resume_checked
- echo $(( $now + $ROOTDELAY )) > "$timestamp"
+ echo $(( $now + $ROOTDELAY )) > "$_rootdelay_timestamp"
exit 0
fi
diff --git a/features/kickstart/data/etc/rc.d/init.d/kickstart b/features/kickstart/data/etc/rc.d/init.d/kickstart
index e36b9470..ef01bcd6 100755
--- a/features/kickstart/data/etc/rc.d/init.d/kickstart
+++ b/features/kickstart/data/etc/rc.d/init.d/kickstart
@@ -23,11 +23,14 @@
if [ -s "$KSFILE" ]; then
. uevent-sh-functions
+ . initrd-sh-functions
mkdir -p -- "$uevent_confdb/queue/pause/poll"
kickstart -v "$KSFILE"
+ rootdelay_reset_timer
+
rmdir -- "$uevent_confdb/queue/pause/poll"
rmdir -- "$uevent_confdb/queue/pause/udev"
rmdir -- "$uevent_confdb/queue/pause/md-raid-member"
--
2.33.7
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] ueventd: Add a prefix to the logging functions to avoid name collisions
2023-05-04 13:42 [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
` (2 preceding siblings ...)
2023-05-04 13:42 ` [make-initrd] [PATCH 3/3] feature/kickstart: Reset rootdelay timer after kickstart Alexey Gladkov
@ 2023-05-06 19:45 ` Alexey Gladkov
2023-05-06 19:45 ` [make-initrd] ueventd: Allow to configure the log destination Alexey Gladkov
` (4 subsequent siblings)
8 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-06 19:45 UTC (permalink / raw)
To: make-initrd
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
datasrc/ueventd/logging.c | 10 +++---
datasrc/ueventd/memory.c | 6 ++--
datasrc/ueventd/path.c | 4 +--
datasrc/ueventd/queue-processor.c | 28 +++++++--------
datasrc/ueventd/ueventd.c | 60 +++++++++++++++----------------
datasrc/ueventd/ueventd.h | 20 +++++------
6 files changed, 64 insertions(+), 64 deletions(-)
diff --git a/datasrc/ueventd/logging.c b/datasrc/ueventd/logging.c
index 671f6814..3a04a5a9 100644
--- a/datasrc/ueventd/logging.c
+++ b/datasrc/ueventd/logging.c
@@ -12,7 +12,7 @@
int log_priority = LOG_INFO;
-int logging_level(const char *name)
+int rd_logging_level(const char *name)
{
if (!strcasecmp(name, "debug")) return LOG_DEBUG;
if (!strcasecmp(name, "info")) return LOG_INFO;
@@ -21,7 +21,7 @@ int logging_level(const char *name)
return log_priority;
}
-void logging_init(int loglevel)
+void rd_logging_init(int loglevel)
{
if (!getenv("UEVENTD_USE_STDERR")) {
char *rdlog = getenv("RDLOG");
@@ -32,7 +32,7 @@ void logging_init(int loglevel)
FILE *cons = fopen(logfile, "w+");
if (!cons)
- fatal("open(%s): %m", logfile);
+ rd_fatal("open(%s): %m", logfile);
fclose(stderr);
stderr = cons;
@@ -41,11 +41,11 @@ void logging_init(int loglevel)
log_priority = loglevel;
}
-void logging_close(void)
+void rd_logging_close(void)
{
}
-void message(int priority, const char *fmt, ...)
+void rd_message(int priority, const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
diff --git a/datasrc/ueventd/memory.c b/datasrc/ueventd/memory.c
index cb911d58..674928c0 100644
--- a/datasrc/ueventd/memory.c
+++ b/datasrc/ueventd/memory.c
@@ -14,8 +14,8 @@ void *xcalloc(size_t nmemb, size_t size)
{
void *r = calloc(nmemb, size);
if (!r)
- fatal("calloc: allocating %lu*%lu bytes: %m",
- (unsigned long) nmemb, (unsigned long) size);
+ rd_fatal("calloc: allocating %lu*%lu bytes: %m",
+ (unsigned long) nmemb, (unsigned long) size);
return r;
}
@@ -25,7 +25,7 @@ char *xasprintf(char **ptr, const char *fmt, ...)
va_start(arg, fmt);
if (vasprintf(ptr, fmt, arg) < 0)
- fatal("vasprintf: %m");
+ rd_fatal("vasprintf: %m");
va_end(arg);
return *ptr;
diff --git a/datasrc/ueventd/path.c b/datasrc/ueventd/path.c
index 7880ad5c..835f4fbb 100644
--- a/datasrc/ueventd/path.c
+++ b/datasrc/ueventd/path.c
@@ -18,7 +18,7 @@ DIR *xopendir(const char *path)
{
DIR *dir = opendir(path);
if (!dir)
- fatal("opendir: %s: %m", path);
+ rd_fatal("opendir: %s: %m", path);
return dir;
}
@@ -32,7 +32,7 @@ struct dirent *xreaddir(DIR *d, const char *path)
if (!ent) {
if (!errno)
return NULL;
- fatal("readdir: %s: %m", path);
+ rd_fatal("readdir: %s: %m", path);
}
return ent;
}
diff --git a/datasrc/ueventd/queue-processor.c b/datasrc/ueventd/queue-processor.c
index ab5e03e4..74e6dfc0 100644
--- a/datasrc/ueventd/queue-processor.c
+++ b/datasrc/ueventd/queue-processor.c
@@ -24,20 +24,20 @@ void event_handler(struct watch *queue, char *path)
pid_t pid = vfork();
if (pid < 0) {
- err("fork: %m");
+ rd_err("fork: %m");
} else if (!pid) {
execve(argv[0], argv, environ);
- fatal("exec: %s: %m", argv[0]);
+ rd_fatal("exec: %s: %m", argv[0]);
} else {
int status = 0;
if (waitpid_retry(pid, &status, 0) != pid || !WIFEXITED(status))
- info("%s: session=%lu: %s failed",
- queue->q_name, session, handler_file);
+ rd_info("%s: session=%lu: %s failed",
+ queue->q_name, session, handler_file);
else
- info("%s: session=%lu: %s finished with return code %d",
- queue->q_name, session, handler_file, WEXITSTATUS(status));
+ rd_info("%s: session=%lu: %s finished with return code %d",
+ queue->q_name, session, handler_file, WEXITSTATUS(status));
}
}
@@ -47,27 +47,27 @@ void move_files(char *srcdir, char *dstdir)
int srcfd, dstfd;
if ((srcfd = open(srcdir, O_RDONLY | O_CLOEXEC)) < 0)
- fatal("open: %s: %m", srcdir);
+ rd_fatal("open: %s: %m", srcdir);
errno = 0;
if ((dstfd = open(dstdir, O_RDONLY | O_CLOEXEC)) < 0) {
if (errno == ENOENT) {
if (mkdir(dstdir, 0755) < 0)
- fatal("mkdir: %s: %m", dstdir);
+ rd_fatal("mkdir: %s: %m", dstdir);
dstfd = open(dstdir, O_RDONLY | O_CLOEXEC);
}
if (dstfd < 0)
- fatal("open: %s: %m", dstdir);
+ rd_fatal("open: %s: %m", dstdir);
}
DIR *d = fdopendir(srcfd);
if (!d)
- fatal("fdopendir: %m");
+ rd_fatal("fdopendir: %m");
while ((ent = xreaddir(d, srcdir)) != NULL) {
if (ent->d_name[0] != '.' && ent->d_type == DT_REG &&
renameat(srcfd, ent->d_name, dstfd, ent->d_name) < 0)
- fatal("rename `%s/%s' -> `%s/%s': %m", srcdir, ent->d_name, dstdir, ent->d_name);
+ rd_fatal("rename `%s/%s' -> `%s/%s': %m", srcdir, ent->d_name, dstdir, ent->d_name);
}
closedir(d);
@@ -81,14 +81,14 @@ void signal_unhandled_events(struct watch *queue)
len = write_loop(queue->q_parentfd,
(char *) &queue->q_watchfd, sizeof(queue->q_watchfd));
if (len < 0)
- err("write(pipe): %m");
+ rd_err("write(pipe): %m");
- info("%s: session=%lu: retry with the events remaining in the queue", queue->q_name, session);
+ rd_info("%s: session=%lu: retry with the events remaining in the queue", queue->q_name, session);
}
void process_events(struct watch *queue)
{
- info("%s: session=%lu: processing events", queue->q_name, session);
+ rd_info("%s: session=%lu: processing events", queue->q_name, session);
char *numenv = NULL;
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
index f5923367..f4b89cb9 100644
--- a/datasrc/ueventd/ueventd.c
+++ b/datasrc/ueventd/ueventd.c
@@ -85,7 +85,7 @@ int add_queue_dir(int inotifyfd, const char *path, uint32_t mask)
if (errno == EEXIST) {
return -128;
}
- err("inotify failed to watch: %s: %m", path);
+ rd_err("inotify failed to watch: %s: %m", path);
}
return ret;
}
@@ -103,7 +103,7 @@ int watch_path(int inotifyfd, const char *dir, const char *name, uint32_t mask,
return (wfd == -128 ? 0 : wfd);
if (stat(path, &st) < 0) {
- err("stat: %s: %m", path);
+ rd_err("stat: %s: %m", path);
goto fail;
}
@@ -127,7 +127,7 @@ int watch_path(int inotifyfd, const char *dir, const char *name, uint32_t mask,
watch_list = new;
- info("watch path: %s", path);
+ rd_info("watch path: %s", path);
free(path);
return 0;
fail:
@@ -216,7 +216,7 @@ void watch_queues(int inotifyfd)
{
DIR *dir = opendir(filter_dir);
if (!dir)
- fatal("opendir: %s: %m", filter_dir);
+ rd_fatal("opendir: %s: %m", filter_dir);
struct dirent *ent;
@@ -225,7 +225,7 @@ void watch_queues(int inotifyfd)
continue;
if (watch_path(inotifyfd, filter_dir, ent->d_name, EV_QUEUE_MASK, F_QUEUE_DIR) < 0)
- fatal("unable to start watching: %s/%s", filter_dir, ent->d_name);
+ rd_fatal("unable to start watching: %s/%s", filter_dir, ent->d_name);
}
closedir(dir);
}
@@ -238,7 +238,7 @@ int process_signal_events(int signfd)
size = read_retry(signfd, &fdsi, sizeof(fdsi));
if (size != sizeof(fdsi)) {
- err("unable to read signal info");
+ rd_err("unable to read signal info");
return 0;
}
@@ -248,7 +248,7 @@ int process_signal_events(int signfd)
if (pid <= 0) {
if (pid < 0 && errno != ECHILD) {
- err("waitpid: %m");
+ rd_err("waitpid: %m");
return -1;
}
break;
@@ -262,7 +262,7 @@ int process_signal_events(int signfd)
return 0;
}
- info("got signal %d, exit", fdsi.ssi_signo);
+ rd_info("got signal %d, exit", fdsi.ssi_signo);
return -1;
}
@@ -279,10 +279,10 @@ void setup_signal_fd(struct fd_handler *el)
sigaddset(&mask, SIGCHLD);
if ((el->fd = signalfd(-1, &mask, SFD_NONBLOCK | SFD_CLOEXEC)) < 0)
- fatal("signalfd: %m");
+ rd_fatal("signalfd: %m");
if (sigprocmask(SIG_BLOCK, &mask, NULL) < 0)
- fatal("sigprocmask: %m");
+ rd_fatal("sigprocmask: %m");
el->fd_handler = process_signal_events;
}
@@ -297,7 +297,7 @@ int process_inotify_events(int inotifyfd)
if (len < 0) {
if (errno == EAGAIN)
break;
- fatal("read: %m");
+ rd_fatal("read: %m");
} else if (!len) {
break;
}
@@ -315,7 +315,7 @@ int process_inotify_events(int inotifyfd)
continue;
if (event->mask & IN_DELETE_SELF) {
- info("unwatch path: %s", p->q_name);
+ rd_info("unwatch path: %s", p->q_name);
unwatch_path(inotifyfd, p->q_ino);
continue;
}
@@ -328,9 +328,9 @@ int process_inotify_events(int inotifyfd)
if (p->q_flags & F_PAUSE_DIR) {
if (event->mask & IN_CREATE)
- info("%s: queue paused", event->name);
+ rd_info("%s: queue paused", event->name);
else if (event->mask & IN_DELETE)
- info("%s: queue unpaused", event->name);
+ rd_info("%s: queue unpaused", event->name);
apply_pause();
continue;
}
@@ -345,7 +345,7 @@ int process_inotify_events(int inotifyfd)
void setup_inotify_fd(struct fd_handler *el)
{
if ((el->fd = inotify_init1(IN_NONBLOCK | IN_CLOEXEC)) < 0)
- fatal("inotify_init1: %m");
+ rd_fatal("inotify_init1: %m");
el->fd_handler = process_inotify_events;
}
@@ -363,7 +363,7 @@ int process_pipefd_events(int readfd)
if (len < 0) {
if (errno == EAGAIN)
break;
- fatal("read(pipe): %m");
+ rd_fatal("read(pipe): %m");
} else if (!len) {
break;
}
@@ -383,7 +383,7 @@ int process_pipefd_events(int readfd)
void setup_pipe_fd(struct fd_handler *el)
{
if (pipe2(pipefd, O_NONBLOCK | O_CLOEXEC) < 0)
- fatal("pipe2: %m");
+ rd_fatal("pipe2: %m");
el->fd = pipefd[PIPE_READ];
el->fd_handler = process_pipefd_events;
@@ -395,13 +395,13 @@ int setup_epoll_fd(struct fd_handler list[FD_MAX])
struct epoll_event ev = { .events = EPOLLIN };
if ((epollfd = epoll_create1(EPOLL_CLOEXEC)) < 0)
- fatal("epoll_create1: %m");
+ rd_fatal("epoll_create1: %m");
for (int i = 0; i < FD_MAX; i++) {
ev.data.fd = list[i].fd;
if (epoll_ctl(epollfd, EPOLL_CTL_ADD, list[i].fd, &ev) < 0)
- fatal("epoll_ctl: %m");
+ rd_fatal("epoll_ctl: %m");
}
return epollfd;
@@ -415,14 +415,14 @@ pid_t spawn_worker(int epollfd, struct watch *queue)
pid = fork();
if (pid < 0) {
- err("fork: %m");
+ rd_err("fork: %m");
return 0;
} else if (pid > 0) {
return pid;
}
if (prctl(PR_SET_PDEATHSIG, SIGKILL) < 0)
- fatal("prctl(PR_SET_PDEATHSIG): %m");
+ rd_fatal("prctl(PR_SET_PDEATHSIG): %m");
for (int i = 0; i < FD_MAX; i++) {
if (fd_list[i].fd >= 0)
@@ -434,7 +434,7 @@ pid_t spawn_worker(int epollfd, struct watch *queue)
sigemptyset(&mask);
if (sigprocmask(SIG_SETMASK, &mask, NULL) < 0)
- fatal("sigprocmask: %m");
+ rd_fatal("sigprocmask: %m");
process_events(queue);
@@ -445,7 +445,7 @@ char *get_param_dir(const char *name)
{
char *value = getenv(name);
if (!value)
- fatal("please set `%s' env variable", name);
+ rd_fatal("please set `%s' env variable", name);
return value;
}
@@ -463,7 +463,7 @@ int main(int argc, char **argv)
exit(0);
break;
case 'l':
- loglevel = logging_level(optarg);
+ loglevel = rd_logging_level(optarg);
break;
default:
exit(1);
@@ -472,11 +472,11 @@ int main(int argc, char **argv)
}
if (optind == argc)
- fatal("specify handler program");
+ rd_fatal("specify handler program");
- logging_init(loglevel);
+ rd_logging_init(loglevel);
- info("starting server ...");
+ rd_info("starting server ...");
filter_dir = get_param_dir("filterdir");
uevent_dir = get_param_dir("ueventdir");
@@ -507,7 +507,7 @@ int main(int argc, char **argv)
if (fdcount < 0) {
if (errno == EINTR)
continue;
- fatal("epoll_wait: %m");
+ rd_fatal("epoll_wait: %m");
}
for (i = 0; i < fdcount; i++) {
@@ -540,12 +540,12 @@ done:
for (i = 0; i < FD_MAX; i++) {
if (fd_list[i].fd >= 0) {
if (epoll_ctl(epollfd, EPOLL_CTL_DEL, fd_list[i].fd, NULL) < 0)
- err("epoll_ctl: %m");
+ rd_err("epoll_ctl: %m");
close(fd_list[i].fd);
}
}
close(epollfd);
- logging_close();
+ rd_logging_close();
return EXIT_SUCCESS;
}
diff --git a/datasrc/ueventd/ueventd.h b/datasrc/ueventd/ueventd.h
index a5e50379..e7ee1a8b 100644
--- a/datasrc/ueventd/ueventd.h
+++ b/datasrc/ueventd/ueventd.h
@@ -56,21 +56,21 @@ extern int is_dot_dir(struct dirent *ent) __attribute__((nonnull(1)));
#include <syslog.h>
#include <stdlib.h>
-extern void logging_init(int level);
-extern void logging_close(void);
-extern int logging_level(const char *lvl) __attribute__((nonnull(1)));
-extern void message(int priority, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
+extern void rd_logging_init(int level);
+extern void rd_logging_close(void);
+extern int rd_logging_level(const char *lvl) __attribute__((nonnull(1)));
+extern void rd_message(int priority, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
-#define __message(level, format, arg...) message(level, format, ##arg)
+#define __rd_message(level, format, arg...) rd_message(level, format, ##arg)
-#define fatal(format, arg...) \
+#define rd_fatal(format, arg...) \
do { \
- __message(LOG_CRIT, "%s:%d: " format, __FILE__, __LINE__, ##arg); \
+ rd_message(LOG_CRIT, "%s:%d: " format, __FILE__, __LINE__, ##arg); \
_exit(EXIT_FAILURE); \
} while (0)
-#define err(format, arg...) __message(LOG_ERR, format, ##arg)
-#define info(format, arg...) __message(LOG_INFO, format, ##arg)
-#define dbg(format, arg...) __message(LOG_DEBUG, format, ##arg)
+#define rd_err(format, arg...) __rd_message(LOG_ERR, format, ##arg)
+#define rd_info(format, arg...) __rd_message(LOG_INFO, format, ##arg)
+#define rd_dbg(format, arg...) __rd_message(LOG_DEBUG, format, ##arg)
#endif /* __UEVENTD_H__ */
--
2.33.7
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] ueventd: Allow to configure the log destination
2023-05-04 13:42 [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
` (3 preceding siblings ...)
2023-05-06 19:45 ` [make-initrd] ueventd: Add a prefix to the logging functions to avoid name collisions Alexey Gladkov
@ 2023-05-06 19:45 ` Alexey Gladkov
2023-05-06 19:45 ` [make-initrd] ueventd: Pass the program name when initializing the logger Alexey Gladkov
` (3 subsequent siblings)
8 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-06 19:45 UTC (permalink / raw)
To: make-initrd
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
datasrc/ueventd/logging.c | 27 ++++++---------------------
datasrc/ueventd/ueventd.c | 19 ++++++++++++++++++-
datasrc/ueventd/ueventd.h | 2 +-
3 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/datasrc/ueventd/logging.c b/datasrc/ueventd/logging.c
index 3a04a5a9..525ff7fd 100644
--- a/datasrc/ueventd/logging.c
+++ b/datasrc/ueventd/logging.c
@@ -8,9 +8,8 @@
#include "ueventd.h"
-#define default_logfile "/var/log/ueventd.log"
-
int log_priority = LOG_INFO;
+int log_fd = STDERR_FILENO;
int rd_logging_level(const char *name)
{
@@ -21,24 +20,10 @@ int rd_logging_level(const char *name)
return log_priority;
}
-void rd_logging_init(int loglevel)
+void rd_logging_init(int fd, int loglevel)
{
- if (!getenv("UEVENTD_USE_STDERR")) {
- char *rdlog = getenv("RDLOG");
- const char *logfile = default_logfile;
-
- if (rdlog && !strcasecmp(rdlog, "console"))
- logfile = "/dev/console";
-
- FILE *cons = fopen(logfile, "w+");
- if (!cons)
- rd_fatal("open(%s): %m", logfile);
-
- fclose(stderr);
- stderr = cons;
- }
-
log_priority = loglevel;
+ log_fd = fd;
}
void rd_logging_close(void)
@@ -52,12 +37,12 @@ void rd_message(int priority, const char *fmt, ...)
if (priority <= log_priority) {
time_t ts = time(NULL);
struct tm *t = localtime(&ts);
- fprintf(stderr, "[%04d-%02d-%02d %02d:%02d:%02d] %s: ",
+ dprintf(log_fd, "[%04d-%02d-%02d %02d:%02d:%02d] %s: ",
t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
t->tm_hour, t->tm_min, t->tm_sec,
program_invocation_short_name);
- vfprintf(stderr, fmt, ap);
- fprintf(stderr, "\n");
+ vdprintf(log_fd, fmt, ap);
+ dprintf(log_fd, "\n");
}
va_end(ap);
}
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
index f4b89cb9..d015d3da 100644
--- a/datasrc/ueventd/ueventd.c
+++ b/datasrc/ueventd/ueventd.c
@@ -21,6 +21,8 @@
#include "ueventd.h"
+#define default_logfile "/var/log/ueventd.log"
+
char *uevent_confdb;
char *filter_dir;
char *uevent_dir;
@@ -474,7 +476,22 @@ int main(int argc, char **argv)
if (optind == argc)
rd_fatal("specify handler program");
- rd_logging_init(loglevel);
+ if (!getenv("UEVENTD_USE_STDERR")) {
+ char *rdlog = getenv("RDLOG");
+ const char *logfile = default_logfile;
+
+ if (rdlog && !strcasecmp(rdlog, "console"))
+ logfile = "/dev/console";
+
+ FILE *cons = fopen(logfile, "w+");
+ if (!cons)
+ rd_fatal("open(%s): %m", logfile);
+
+ fclose(stderr);
+ stderr = cons;
+ }
+
+ rd_logging_init(fileno(stderr), loglevel);
rd_info("starting server ...");
diff --git a/datasrc/ueventd/ueventd.h b/datasrc/ueventd/ueventd.h
index e7ee1a8b..a783d48d 100644
--- a/datasrc/ueventd/ueventd.h
+++ b/datasrc/ueventd/ueventd.h
@@ -56,7 +56,7 @@ extern int is_dot_dir(struct dirent *ent) __attribute__((nonnull(1)));
#include <syslog.h>
#include <stdlib.h>
-extern void rd_logging_init(int level);
+extern void rd_logging_init(int log_fd, int level);
extern void rd_logging_close(void);
extern int rd_logging_level(const char *lvl) __attribute__((nonnull(1)));
extern void rd_message(int priority, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
--
2.33.7
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] ueventd: Pass the program name when initializing the logger
2023-05-04 13:42 [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
` (4 preceding siblings ...)
2023-05-06 19:45 ` [make-initrd] ueventd: Allow to configure the log destination Alexey Gladkov
@ 2023-05-06 19:45 ` Alexey Gladkov
2023-05-06 19:45 ` [make-initrd] ueventd: Rewrite logging function to make it more atomic Alexey Gladkov
` (2 subsequent siblings)
8 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-06 19:45 UTC (permalink / raw)
To: make-initrd
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
datasrc/ueventd/logging.c | 11 +++++++----
datasrc/ueventd/ueventd.c | 2 +-
datasrc/ueventd/ueventd.h | 2 +-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/datasrc/ueventd/logging.c b/datasrc/ueventd/logging.c
index 525ff7fd..ac316aab 100644
--- a/datasrc/ueventd/logging.c
+++ b/datasrc/ueventd/logging.c
@@ -10,6 +10,7 @@
int log_priority = LOG_INFO;
int log_fd = STDERR_FILENO;
+const char *log_progname = NULL;
int rd_logging_level(const char *name)
{
@@ -20,10 +21,11 @@ int rd_logging_level(const char *name)
return log_priority;
}
-void rd_logging_init(int fd, int loglevel)
+void rd_logging_init(int fd, int loglevel, const char *progname)
{
log_priority = loglevel;
log_fd = fd;
+ log_progname = progname;
}
void rd_logging_close(void)
@@ -37,10 +39,11 @@ void rd_message(int priority, const char *fmt, ...)
if (priority <= log_priority) {
time_t ts = time(NULL);
struct tm *t = localtime(&ts);
- dprintf(log_fd, "[%04d-%02d-%02d %02d:%02d:%02d] %s: ",
+ dprintf(log_fd, "[%04d-%02d-%02d %02d:%02d:%02d] ",
t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
- t->tm_hour, t->tm_min, t->tm_sec,
- program_invocation_short_name);
+ t->tm_hour, t->tm_min, t->tm_sec);
+ if (log_progname)
+ dprintf(log_fd, "%s: ", log_progname);
vdprintf(log_fd, fmt, ap);
dprintf(log_fd, "\n");
}
diff --git a/datasrc/ueventd/ueventd.c b/datasrc/ueventd/ueventd.c
index d015d3da..0ce5be00 100644
--- a/datasrc/ueventd/ueventd.c
+++ b/datasrc/ueventd/ueventd.c
@@ -491,7 +491,7 @@ int main(int argc, char **argv)
stderr = cons;
}
- rd_logging_init(fileno(stderr), loglevel);
+ rd_logging_init(fileno(stderr), loglevel, program_invocation_short_name);
rd_info("starting server ...");
diff --git a/datasrc/ueventd/ueventd.h b/datasrc/ueventd/ueventd.h
index a783d48d..ddba98bf 100644
--- a/datasrc/ueventd/ueventd.h
+++ b/datasrc/ueventd/ueventd.h
@@ -56,7 +56,7 @@ extern int is_dot_dir(struct dirent *ent) __attribute__((nonnull(1)));
#include <syslog.h>
#include <stdlib.h>
-extern void rd_logging_init(int log_fd, int level);
+extern void rd_logging_init(int log_fd, int level, const char *progname);
extern void rd_logging_close(void);
extern int rd_logging_level(const char *lvl) __attribute__((nonnull(1)));
extern void rd_message(int priority, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
--
2.33.7
^ permalink raw reply [flat|nested] 45+ messages in thread
* [make-initrd] ueventd: Rewrite logging function to make it more atomic
2023-05-04 13:42 [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
` (5 preceding siblings ...)
2023-05-06 19:45 ` [make-initrd] ueventd: Pass the program name when initializing the logger Alexey Gladkov
@ 2023-05-06 19:45 ` Alexey Gladkov
2023-05-07 12:48 ` [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
2023-05-13 11:50 ` Alexey Gladkov
8 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-06 19:45 UTC (permalink / raw)
To: make-initrd
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
datasrc/ueventd/logging.c | 107 +++++++++++++++++++++++++++++++++-----
datasrc/ueventd/ueventd.h | 17 +++---
2 files changed, 104 insertions(+), 20 deletions(-)
diff --git a/datasrc/ueventd/logging.c b/datasrc/ueventd/logging.c
index ac316aab..14539752 100644
--- a/datasrc/ueventd/logging.c
+++ b/datasrc/ueventd/logging.c
@@ -1,8 +1,9 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
+#include <sys/uio.h>
#include <stdio.h>
#include <stdarg.h>
-#include <strings.h>
+#include <string.h>
#include <errno.h>
#include <time.h>
@@ -11,6 +12,7 @@
int log_priority = LOG_INFO;
int log_fd = STDERR_FILENO;
const char *log_progname = NULL;
+size_t log_progname_len = 0;
int rd_logging_level(const char *name)
{
@@ -25,27 +27,108 @@ void rd_logging_init(int fd, int loglevel, const char *progname)
{
log_priority = loglevel;
log_fd = fd;
- log_progname = progname;
+ if (progname) {
+ log_progname = progname;
+ log_progname_len = strlen(progname);
+ }
}
void rd_logging_close(void)
{
}
-void rd_message(int priority, const char *fmt, ...)
+enum {
+ LOG_FMT_DATETIME,
+ LOG_FMT_DELIM_1,
+ LOG_FMT_PROGNAME,
+ LOG_FMT_DELIM_2,
+ LOG_FMT_MESSAGE,
+ LOG_FMT_EOL,
+ LOG_FMT_MAX,
+};
+
+void rd_vmessage(const char *fmt, va_list ap)
+{
+ char stack_date[38]; // "[1970-01-01 00:00:00]"
+ char stack_msg[80];
+ struct iovec iov[LOG_FMT_MAX];
+
+ memset(iov, 0, sizeof(iov));
+
+ int used;
+ struct tm t;
+ time_t now = time(NULL);
+
+ if (localtime_r(&now, &t) == &t) {
+ used = snprintf(stack_date, sizeof(stack_date),
+ "[%04d-%02d-%02d %02d:%02d:%02d]",
+ t.tm_year + 1900, t.tm_mon + 1, t.tm_mday,
+ t.tm_hour, t.tm_min, t.tm_sec);
+
+ iov[LOG_FMT_DATETIME].iov_base = stack_date;
+ iov[LOG_FMT_DATETIME].iov_len = (size_t) used;
+
+ iov[LOG_FMT_DELIM_1].iov_base = (char *) " ";
+ iov[LOG_FMT_DELIM_1].iov_len = 1;
+ }
+
+ if (log_progname) {
+ iov[LOG_FMT_PROGNAME].iov_base = (char *) log_progname;
+ iov[LOG_FMT_PROGNAME].iov_len = log_progname_len;
+
+ iov[LOG_FMT_DELIM_2].iov_base = (char *) ": ";
+ iov[LOG_FMT_DELIM_2].iov_len = 2;
+ }
+
+ iov[LOG_FMT_EOL].iov_base = (char *) "\n";
+ iov[LOG_FMT_EOL].iov_len = 1;
+
+ va_list ap2;
+ size_t size;
+ char *buf = NULL;
+
+ va_copy(ap2, ap);
+ used = vsnprintf(NULL, 0, fmt, ap2);
+ va_end(ap2);
+
+ if (used >= (int) sizeof(stack_msg)) {
+ size = (size_t) used + 1;
+ buf = malloc(size);
+ }
+
+ if (!buf) {
+ size = sizeof(stack_msg);
+ buf = stack_msg;
+ }
+
+ va_copy(ap2, ap);
+ used = vsnprintf(buf, size, fmt, ap2);
+ va_end(ap2);
+
+ iov[LOG_FMT_MESSAGE].iov_base = buf;
+ iov[LOG_FMT_MESSAGE].iov_len = (size_t) used;
+
+ fsync(log_fd);
+ TEMP_FAILURE_RETRY(writev(log_fd, iov, LOG_FMT_MAX));
+
+ if (buf != stack_msg)
+ free(buf);
+
+ return;
+}
+
+void rd_log_vmessage(int priority, const char *fmt, va_list ap)
+{
+ if (priority <= log_priority)
+ rd_vmessage(fmt, ap);
+}
+
+void rd_log_message(int priority, const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
if (priority <= log_priority) {
- time_t ts = time(NULL);
- struct tm *t = localtime(&ts);
- dprintf(log_fd, "[%04d-%02d-%02d %02d:%02d:%02d] ",
- t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
- t->tm_hour, t->tm_min, t->tm_sec);
- if (log_progname)
- dprintf(log_fd, "%s: ", log_progname);
- vdprintf(log_fd, fmt, ap);
- dprintf(log_fd, "\n");
+ rd_vmessage(fmt, ap);
}
va_end(ap);
}
diff --git a/datasrc/ueventd/ueventd.h b/datasrc/ueventd/ueventd.h
index ddba98bf..732d0c61 100644
--- a/datasrc/ueventd/ueventd.h
+++ b/datasrc/ueventd/ueventd.h
@@ -55,22 +55,23 @@ extern int is_dot_dir(struct dirent *ent) __attribute__((nonnull(1)));
#include <unistd.h>
#include <syslog.h>
#include <stdlib.h>
+#include <stdarg.h>
extern void rd_logging_init(int log_fd, int level, const char *progname);
extern void rd_logging_close(void);
-extern int rd_logging_level(const char *lvl) __attribute__((nonnull(1)));
-extern void rd_message(int priority, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
-
-#define __rd_message(level, format, arg...) rd_message(level, format, ##arg)
+extern int rd_logging_level(const char *lvl) __attribute__((nonnull(1)));
+extern void rd_vmessage(const char *fmt, va_list ap) __attribute__((format(printf, 1, 0)));
+extern void rd_log_vmessage(int priority, const char *fmt, va_list ap) __attribute__((format(printf, 2, 0)));
+extern void rd_log_message(int priority, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
#define rd_fatal(format, arg...) \
do { \
- rd_message(LOG_CRIT, "%s:%d: " format, __FILE__, __LINE__, ##arg); \
+ rd_log_message(LOG_CRIT, "%s:%d: " format, __FILE__, __LINE__, ##arg); \
_exit(EXIT_FAILURE); \
} while (0)
-#define rd_err(format, arg...) __rd_message(LOG_ERR, format, ##arg)
-#define rd_info(format, arg...) __rd_message(LOG_INFO, format, ##arg)
-#define rd_dbg(format, arg...) __rd_message(LOG_DEBUG, format, ##arg)
+#define rd_err(format, arg...) rd_log_message(LOG_ERR, format, ##arg)
+#define rd_info(format, arg...) rd_log_message(LOG_INFO, format, ##arg)
+#define rd_dbg(format, arg...) rd_log_message(LOG_DEBUG, format, ##arg)
#endif /* __UEVENTD_H__ */
--
2.33.7
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 0/3] Reimplement ueventd
2023-05-04 13:42 [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
` (6 preceding siblings ...)
2023-05-06 19:45 ` [make-initrd] ueventd: Rewrite logging function to make it more atomic Alexey Gladkov
@ 2023-05-07 12:48 ` Alexey Gladkov
2023-05-13 11:50 ` Alexey Gladkov
8 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-07 12:48 UTC (permalink / raw)
To: make-initrd
On Thu, May 04, 2023 at 03:42:49PM +0200, Alexey Gladkov wrote:
> # Изменения.
>
> Ueventd был переписан с нуля на си. Это открыло возможность для оптимизаций,
> увеличило читабильность кода, дало возможность уйти от передачи эвентов через
> файловую систему.
>
> Как следствие сейчас пропала необходимость в отдельном процессе polld. Его
> фунционал может обратно вернуться в ueventd в качестве отдельной очереди. Плюс
> проверка rootdelay теперь находится в этой отдельной очереди.
>
> Возможно стоит пойти дальше и уйти от периодической проверки создав опять же
> поток эвентов и "базу данных". Но это справедливо и для текущей архитектуры
> эвентов.
На самом деле реализация сделана "в лоб". Более эффективно было бы
создавать по процессу на очередь, который следит только за одной
директорией-очередью и в этом случае форков будет меньше. Но вместе с тем
код станет более громоздким, потому что эвент лупов нужно делать
несколько.
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 0/3] Reimplement ueventd
2023-05-04 13:42 [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
` (7 preceding siblings ...)
2023-05-07 12:48 ` [make-initrd] [PATCH 0/3] Reimplement ueventd Alexey Gladkov
@ 2023-05-13 11:50 ` Alexey Gladkov
2023-05-14 20:15 ` Leonid Krivoshein
8 siblings, 1 reply; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-13 11:50 UTC (permalink / raw)
To: make-initrd
On Thu, May 04, 2023 at 03:42:49PM +0200, Alexey Gladkov wrote:
> # Предыстория.
>
> Когда-то давно в initramfs был один цикл, в котором обрабатывались все эвенты из
> udev и в конце каждой итерации производилась проверка возможности загрузиться.
> Это давало предсказуемый порядок, но создавало проблему с производительностью.
> Плюс ко всему этому требовался эвент, чтобы дать толчёк для проверки возможности
> дальнейшей загрузки.
>
> Чтобы решить эти проблемы единый цикл был разделён на ueventd (обработчик
> эвентов) и polld (периодическое выполнение скриптов). Это позволило развязать
> эвенты и условия дальнейшей загрузки.
>
> С появлением необходимости обработки эвентов от сетевых устройств, конфигурации
> терминалов и некоторых других, в ueventd была реализована поддержка нескольких
> очередей. Эвенты каждой очереди обрабатываются последовательно, но каждая
> очередь обрабатывается параллельно.
>
> В то время распараллеливание было сделано через использование утилиты inotify из
> busybox. Это работало, но поскольку inotify это утилита общего назначения, то
> оптимизировать или кастомизировать работу демона было очень затруднительно.
>
> # Изменения.
>
> Ueventd был переписан с нуля на си. Это открыло возможность для оптимизаций,
> увеличило читабильность кода, дало возможность уйти от передачи эвентов через
> файловую систему.
>
> Как следствие сейчас пропала необходимость в отдельном процессе polld. Его
> фунционал может обратно вернуться в ueventd в качестве отдельной очереди. Плюс
> проверка rootdelay теперь находится в этой отдельной очереди.
>
> Возможно стоит пойти дальше и уйти от периодической проверки создав опять же
> поток эвентов и "базу данных". Но это справедливо и для текущей архитектуры
> эвентов.
Смерджено с follow-up исправлениями. Также смерджил патчсет создающий
общую библиотеку для утилит на си внутри initramfs.
> ---
>
> Alexey Gladkov (3):
> Reimplement ueventd
> Replace polld by uevent queue
> feature/kickstart: Reset rootdelay timer after kickstart
>
> data/bin/initrd-sh-functions | 8 +
> data/bin/uevent-sh-functions | 3 +
> data/etc/rc.d/init.d/poll | 25 +-
> data/etc/rc.d/init.d/uevent | 9 +-
> data/lib/uevent/each/post/.gitignore | 0
> data/lib/uevent/each/post/system-bootable | 4 -
> data/lib/uevent/extenders/.gitignore | 0
> data/lib/uevent/handlers/poll/100-extenders | 22 +
> data/lib/uevent/handlers/poll/300-boot-method | 29 +
> .../poll/400-rootdelay} | 13 +-
> data/sbin/polld | 45 --
> data/sbin/process-boot-method | 16 -
> data/sbin/ueventd | 180 ------
> data/sbin/ueventd-queue | 65 +++
> datasrc/ueventd/Makefile.mk | 13 +
> datasrc/ueventd/logging.c | 63 ++
> datasrc/ueventd/memory.c | 32 +
> datasrc/ueventd/path.c | 80 +++
> datasrc/ueventd/process.c | 17 +
> datasrc/ueventd/queue-processor.c | 116 ++++
> datasrc/ueventd/ueventd.c | 551 ++++++++++++++++++
> datasrc/ueventd/ueventd.h | 76 +++
> .../kickstart/data/etc/rc.d/init.d/kickstart | 11 +-
> 23 files changed, 1108 insertions(+), 270 deletions(-)
> create mode 100644 data/lib/uevent/each/post/.gitignore
> delete mode 100755 data/lib/uevent/each/post/system-bootable
> create mode 100644 data/lib/uevent/extenders/.gitignore
> create mode 100755 data/lib/uevent/handlers/poll/100-extenders
> create mode 100755 data/lib/uevent/handlers/poll/300-boot-method
> rename data/lib/uevent/{extenders/100-rootdelay => handlers/poll/400-rootdelay} (80%)
> delete mode 100755 data/sbin/polld
> delete mode 100755 data/sbin/process-boot-method
> delete mode 100755 data/sbin/ueventd
> create mode 100755 data/sbin/ueventd-queue
> create mode 100644 datasrc/ueventd/Makefile.mk
> create mode 100644 datasrc/ueventd/logging.c
> create mode 100644 datasrc/ueventd/memory.c
> create mode 100644 datasrc/ueventd/path.c
> create mode 100644 datasrc/ueventd/process.c
> create mode 100644 datasrc/ueventd/queue-processor.c
> create mode 100644 datasrc/ueventd/ueventd.c
> create mode 100644 datasrc/ueventd/ueventd.h
>
> --
> 2.33.7
>
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 0/3] Reimplement ueventd
2023-05-13 11:50 ` Alexey Gladkov
@ 2023-05-14 20:15 ` Leonid Krivoshein
2023-05-14 20:49 ` Alexey Gladkov
0 siblings, 1 reply; 45+ messages in thread
From: Leonid Krivoshein @ 2023-05-14 20:15 UTC (permalink / raw)
To: make-initrd
On 5/13/23 14:50, Alexey Gladkov wrote:
> Смерджено с follow-up исправлениями. Также смерджил патчсет создающий
> общую библиотеку для утилит на си внутри initramfs.
Означает ли это, что предыдущие коммиты из рассылки уже нет смысла
досматривать? В принципе, на гитхабе всё равно смотреть удобней и в
остальном я пока только успел всё прочитать и немного разобраться в
удалённом коде на shell.
--
WBR,
Leonid Krivoshein.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [make-initrd] [PATCH 0/3] Reimplement ueventd
2023-05-14 20:15 ` Leonid Krivoshein
@ 2023-05-14 20:49 ` Alexey Gladkov
0 siblings, 0 replies; 45+ messages in thread
From: Alexey Gladkov @ 2023-05-14 20:49 UTC (permalink / raw)
To: make-initrd
On Sun, May 14, 2023 at 11:15:36PM +0300, Leonid Krivoshein wrote:
>
> On 5/13/23 14:50, Alexey Gladkov wrote:
> > Смерджено с follow-up исправлениями. Также смерджил патчсет создающий
> > общую библиотеку для утилит на си внутри initramfs.
>
> Означает ли это, что предыдущие коммиты из рассылки уже нет смысла
> досматривать? В принципе, на гитхабе всё равно смотреть удобней и в
> остальном я пока только успел всё прочитать и немного разобраться в
> удалённом коде на shell.
Смотреть всё равно есть смысл. Изменения пока даже не в master, а в
for-master и я их активно тестирую.
Просто я ждал с моей точки зрения достаточно долго и у меня уже накопились
зависимые изменения. Поддерживать и делать rebase разным бранчам уже стало
сложно.
--
Rgrds, legion
^ permalink raw reply [flat|nested] 45+ messages in thread