[make-initrd] [PATCH 1/3] Reimplement ueventd
Alexey Gladkov
legion at altlinux.ru
Mon May 15 13:43:40 MSK 2023
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 at lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd
--
Rgrds, legion
More information about the Make-initrd
mailing list