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

[Leonid Krivoshein <klark.devel@gmail.com>]

On 5/20/23 16:18, Alexey Gladkov wrote:
> On Sat, May 20, 2023 at 12:00:14PM +0300, Leonid Krivoshein wrote:
>> [...]
>>> [...]
>>> +
>>> +typedef int (*fdhandler_t)(int);
>>> +
>>> +struct fd_handler {
>>> +	int fd;
>>> +	fdhandler_t fd_handler;
>>> +};
>>> +
>>> +struct fd_handler fd_list[FD_MAX];
>> [...]
>> Имя fd_list[] тоже не очень подходящее, поскольку это не только список
>> именно fd.
> Принимаю предложения на более удачное название ))

fdhs или fdh_list, но и новое (fd_handler_list) вполне подходит -- 
больше fd_handler'ов. ))


>> Константа FD_MAX, хоть и не определена в использованных
>> заголовочных файлах, это имя иногда встречается рядом с FD_SET(),
>> FD_CLR() и им подобным макросам в других исходниках как функция
>> FD_MAX(x). На всякий случай...
> Это имя ни с чем не конфликтует.
>
> $ grep -re '\<FD_MAX\>' /usr/include | wc -l
> 0

Да, но используется зарезервированный префикс FD_, что с каким-то 
очередным изменением хедеров потенциально может дать конфликт.


>>> +
>>> +enum {
>>> +	PIPE_READ,
>>> +	PIPE_WRITE,
>>> +	PIPE_MAX,
>>> +};
>>> +
>>> +int pipefd[PIPE_MAX];
>> Данным фрагментом ты как бы говоришь: "я точно знаю, во что это
>> превратит компилятор", слишком безапелляционно.
> Стандарт си говорит:

Пока у компилятора не появится глобальная опция, меняющая стартовое 
значение для enum, и её не поменяет кто-нибудь в сборочной среде 
глобально, беспокоиться и правда не стоит.


> [...]
>>> +		fdcount = epoll_wait(epollfd, ev, EV_MAX, 250);
>> Почему 250, а не -1? Напоминает не нужный в этом месте "sleep .25". Если
>> бы демон перезапускался для дефрагментации через какое-то число событий
>> или спустя сколько-то минут, а тут совсем холостой ход получается.
> Это сделано намеренно. Раньше ты спрашивал зачем отправлять wfd родителю
> через пайп. Родитель помечает очередь как F_DIRTY и на следующей итерации
> эта очередь попробует обработаться даже если новых эвентов нет.

Тогда стоит ввести флаг и заменить 250 на что-то вроде:

have_dirty ? 250: -1


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

Не заметил сразу, что это union. Раз так, то уже выбранный ev->data->ptr 
мне тоже нравится больше.


> Я мог бы положить туда либо fd, либо указатель на
> fd_list[k]. В целом так и стоит сделать. Раньше fd_list не было и остался
> этот атавизм. Но это позволит избавиться от цикла только в этом месте.
>
> [...]


-- 
WBR, Leonid Krivoshein.