Feedback - Githubissues

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/Makefile#L27 Tento glob matchne i hlavičky pro daemon_c. Ale dívám se že se ta proměnná v Makefile stejně.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_daemon.c#L32 Bylo by dobré alespoň něco zalogovat pokud pid < 0.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_daemon.c#L64 Použil bych místo 1024 konstantu nebo sizeof, ať se délka toho pole neobjevuje na třech různých místech.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_daemon.c#L66 Pokud by měl démon asynchroně zvládat i zprávy rozložené do více TCP paketů tak by tady bylo potřeba zatím načtenou část zprávy uložit do bufferu a vrátit se do hlavní smyčky s epoll_wait. break způsobí že se částečná zpráva zpracuje jako by byla úplná.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_daemon.c#L93 Tady je nutné ošetřit případ -1 < n < response_len a případ EAGAIN/EWOULDBLOCK. Pokud by server měl být úplně asynchronní tak by musel ukládat neodeslanou část odpovědi do nějakého bufferu asociovaného s tímto FD.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_io.h#L9 Tady bych preferoval buď dalším argumentem předávat maximální délku pole prvního argumentu, nebo alespoň přidat komentář s předpoklady pro volání této funkce.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_io.c#L61 V této aplikaci to není takový problém, ale obecně není blokující spánek v asynchronním serveru žádoucí. Optimální by to bylo řešit to pomocí timeoutu epoll_wait, což bohužel znamená docela dost kódu navíc. Nebo vyčítat /proc/stat periodicky a v print_cpu_stats použít poslední hodnotu.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_io.c#L80 Tady bych taky ocenil dokumentaci o požadavcích na pole map.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_server.c#L113 Místo 64 by měl být počet prvků pole events.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_server.c#L89 Opět to v této aplikaci není problém, ale obecně by se každý otevřený filedescriptor měl i někde zavřít. A neměl by leaknout ani v případech kdy funkce server_work vrací chybu.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_server.c#L101 Je flag EPOLLET potřeba?

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_server.c#L128 Bylo by dobré chyby alespoň logovat. A není potřeba ošetřit případ kdy druhá strana zavřela TCP spojení, a zavřít FD na naší straně, a případně na něj předtím zavolat epoll_ctl(EPOLL_CTL_DEL)?

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_server.c#L139 Přepíše předchozí hodnotu. Ideální by bylo mít seznam všech FD klientů + příslušející data (e.g. výše zmíněné buffery).

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_daemon.c#L4 Oceňuju nepoužití threadů:)

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/Makefile#L27

Tento glob matchne i hlavičky pro daemon_c. Ale dívám se že se ta proměnná v Makefile stejně.

Proměnnou jsem oddělal

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_daemon.c#L32

Bylo by dobré alespoň něco zalogovat pokud pid < 0.

Zalogováno.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_daemon.c#L64

Použil bych místo 1024 konstantu nebo sizeof, ať se délka toho pole neobjevuje na třech různých místech.

Konstantu jsem přidal do hlavičky c_server.h...tam kde to nehází warningy ale používám sizeof() https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_daemon.c#L66

Pokud by měl démon asynchroně zvládat i zprávy rozložené do více TCP paketů tak by tady bylo potřeba zatím načtenou část zprávy uložit do bufferu a vrátit se do hlavní smyčky s epoll_wait. break způsobí že se částečná zpráva zpracuje jako by byla úplná.

Tady jsem to opravdu neudělal příliš obecně. Abych však v tomto případě detekoval konec zprávy, použil jsem NL symbol na konci každé zprávy - takže se můžu vrátit do do epoll_wait, avšak ve zprávě by se tento symbol neměl vyskytovat jinde, než na konci, jinak je chování nedefinované. Klient z tohoto projektu ho tam za uživatelský vstup vždy přilepí. https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_daemon.c#L93

Tady je nutné ošetřit případ -1 < n < response_len a případ EAGAIN/EWOULDBLOCK. Pokud by server měl být úplně asynchronní tak by musel ukládat neodeslanou část odpovědi do nějakého bufferu asociovaného s tímto FD.

Nyní ukládám, ale zatím tam nemám žadný mechanismus na odeslání zbytku zprávy (TODO) https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_io.h#L9

Tady bych preferoval buď dalším argumentem předávat maximální délku pole prvního argumentu, nebo alespoň přidat komentář s předpoklady pro volání této funkce.

Všude, kde se to vyskytovalo by to mělo být fixlé. https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_io.c#L61

V této aplikaci to není takový problém, ale obecně není blokující spánek v asynchronním serveru žádoucí. Optimální by to bylo řešit to pomocí timeoutu epoll_wait, což bohužel znamená docela dost kódu navíc. Nebo vyčítat /proc/stat periodicky a v print_cpu_stats použít poslední hodnotu.

Mělo by to fungovat - přidal jsem timer descriptor a one shot eventu, pokud si to žádá callback z hlavní epoll_wait smyčky. Prakticky tedy když přijde "cpu" command, tak se udělá snapshot /proc/stats a vytvoří se timerfd, který se zařadí do epoll a po obdržení této eventy se pak odpálí callback, který dokončí výpočet na základě původního snapshotu z /proc/stats https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_io.c#L80

Tady bych taky ocenil dokumentaci o požadavcích na pole map.

Psal jsem komentář do hlavičky, ale přidal jsem je i do definice, aby to bylo vidět přímo. https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_server.c#L113

Místo 64 by měl být počet prvků pole events.

Spraveno

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_server.c#L89

Opět to v této aplikaci není problém, ale obecně by se každý otevřený filedescriptor měl i někde zavřít. A neměl by leaknout ani v případech kdy funkce server_work vrací chybu.

Přidal jsem sigterm signál a obsluhu zavírání všech deskriptorů a čištění bufferů. Mělo by se to zavírat nsad všude, ale ještě to zkontroluju v dalším kroku (kdyby budu mimo jiné ještě upravovat toho c++ démona) https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_server.c#L101

Je flag EPOLLET potřeba?

Ne, tady ne - zafixováno https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_server.c#L128

Bylo by dobré chyby alespoň logovat. A není potřeba ošetřit případ kdy druhá strana zavřela TCP spojení, a zavřít FD na naší straně, a případně na něj předtím zavolat epoll_ctl(EPOLL_CTL_DEL)?

Vyřešil jsem funkcí zavírání deskriptorů výše. Stejně tak ty stavy. Nicméně ještě to projdu jednou. https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_server.c#L139

Přepíše předchozí hodnotu. Ideální by bylo mít seznam všech FD klientů + příslušející data (e.g. výše zmíněné buffery).

Zapracováno - posílám eventama novou strukturu, která nese jak deskriptor, tak buffery.

https://github.com/nitrram/tcpserv/blob/b114987b863161e61baa85a77fc399529debedc8/daemon/c_daemon.c#L4

Oceňuju nepoužití threadů:)

Odstraněny nepotřebné includy (dokonce jsem si našel i linkované pthready v makefilu :))

nitrram / tcpserv

Feedback #1