From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <legion@altlinux.ru>
Date: Mon, 15 May 2023 12:43:40 +0200
From: Alexey Gladkov <legion@altlinux.ru>
To: make-initrd@lists.altlinux.org
Message-ID: <ZGIM3OKjStcnGgzm@example.org>
References: <cover.1683200226.git.gladkov.alexey@gmail.com>
 <6b444ed922286eb3df8f5322b1bddf9c55753eb8.1683200226.git.gladkov.alexey@gmail.com>
 <ecf9f7a7-5f84-adaa-c374-1d02c8a7ff76@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ecf9f7a7-5f84-adaa-c374-1d02c8a7ff76@gmail.com>
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: <make-initrd.lists.altlinux.org>
List-Unsubscribe: <https://lists.altlinux.org/mailman/options/make-initrd>,
 <mailto:make-initrd-request@lists.altlinux.org?subject=unsubscribe>
List-Archive: <http://lists.altlinux.org/pipermail/make-initrd>
List-Post: <mailto:make-initrd@lists.altlinux.org>
List-Help: <mailto:make-initrd-request@lists.altlinux.org?subject=help>
List-Subscribe: <https://lists.altlinux.org/mailman/listinfo/make-initrd>,
 <mailto:make-initrd-request@lists.altlinux.org?subject=subscribe>
X-List-Received-Date: Mon, 15 May 2023 10:43:42 -0000
Archived-At: <http://lore.altlinux.org/make-initrd/ZGIM3OKjStcnGgzm@example.org/>
List-Archive: <http://lore.altlinux.org/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