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/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) 
можно оставить как есть, назвав почти одним именем, но только, если 
очень хочется кого-то запутать. :-) Имя fd_list[] тоже не очень 
подходящее, поскольку это не только список именно fd. Константа FD_MAX, 
хоть и не определена в использованных заголовочных файлах, это имя 
иногда встречается рядом с FD_SET(), FD_CLR() и им подобным макросам в 
других исходниках как функция FD_MAX(x). На всякий случай...


> +
> +enum {
> +	PIPE_READ,
> +	PIPE_WRITE,
> +	PIPE_MAX,
> +};
> +
> +int pipefd[PIPE_MAX];

Данным фрагментом ты как бы говоришь: "я точно знаю, во что это 
превратит компилятор", слишком безапелляционно. Тут define's или хотя бы 
явное указание значений вполне уместно, с учётом дальнейшего 
использования в строках 122 и 388. Кроме того, pipefd не объявлен как 
static, можно не засорять глобальное пространство имён.


> [...]
> +
> +int main(int argc, char **argv)
> +{
> [...]
> +	while (1) {

Не любишь ты использовать for (;;)....


> +		struct epoll_event ev[EV_MAX];
> +		int fdcount;
> +
> +		errno = 0;
> +		fdcount = epoll_wait(epollfd, ev, EV_MAX, 250);

Почему 250, а не -1? Напоминает не нужный в этом месте "sleep .25". Если 
бы демон перезапускался для дефрагментации через какое-то число событий 
или спустя сколько-то минут, а тут совсем холостой ход получается.


> +
> +		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, ...) 
вполне можно было бы разместить всё самое интересное для ускорения доступа.


> +					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.