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

[Alexey Gladkov <legion@altlinux.ru>]

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