From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 15 May 2023 12:43:40 +0200 From: Alexey Gladkov To: make-initrd@lists.altlinux.org Message-ID: References: <6b444ed922286eb3df8f5322b1bddf9c55753eb8.1683200226.git.gladkov.alexey@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [make-initrd] [PATCH 1/3] Reimplement ueventd X-BeenThere: make-initrd@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: make-initrd@lists.altlinux.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 May 2023 10:43:42 -0000 Archived-At: List-Archive: 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 > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#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); > > Компилятор не ругается здесь только потому, что подключен > через "ueventd.h", а ругаться у него целых два повода (execve и > environ). Если ты предполагаешь, что тут должна быть ругань про отсутствие определения execve и environ, то ты ошибаешься. Ругани тут нет так как определения приезжают через ueventd.h. Или ты про какие поводы для ругани ? > Это я к тому, что в твоём заголовочном файле он подключен для > logging.c, а не для queue-processor.c. Согласно man 7 environ можно не > объявлять эту переменную в коде, если подключается с > _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 "...". > Нужно подключить заголовочный файл либо использовать > какое-то приведение типов. Пренебречь этим, как сейчас, можно только > полагаясь на конкретные платформы. Я знаю про 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