Re: [make-initrd] [PATCH 1/3] Reimplement ueventd

[Leonid Krivoshein <klark.devel@gmail.com>]

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.