From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Sat, 20 May 2023 15:18:35 +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: Sat, 20 May 2023 13:18:38 -0000 Archived-At: List-Archive: 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 '\' /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