Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement) socket #18

Merged
merged 9 commits into from
Sep 5, 2023
Merged

implement) socket #18

merged 9 commits into from
Sep 5, 2023

Conversation

ak0327
Copy link
Owner

@ak0327 ak0327 commented Aug 21, 2023

実装内容

  • branch sockets_satushiの引き継ぎ実装
    • socket, bind, listen
    • confの反映(とりあえずIP, PORTを直で与えることに)
  • test cb47d80
    • ベースは update) fix main, add test #20 参照
    • constructor
    • getter
    • client接続(正常, listen()上限超過)
    • macOS, Linuxでチェック(一応windows環境も構築できるらしいが...)
  • main()への結合など -> implement) server #21 Server classが呼び出した方が良さそうなので、こっちで対応

残課題など

  • エラーハンドリング
    • Socket class内でエラーが起きた場合、strerror(errno)を出力、status = ERRORとしているため、呼び出し側でエラー処理が必要になる(例外を投げると呼び出し側の認知負荷(catchの要否など)が増えそうなので)
    • そもそもこの方式で良かったのか...?
    • status不要で、fdだけで判定(fd == -1でERROR、fd != -1でOK)した方が良いのか?そもそもstatusのint 0 or -1をenum result OK or ERRORみたいにしたほうがいいのか?
  • confの受取
    • とりあえず、最低限必要なcahr *server_ip, char *server_portを受け取る形としているが、confを受け取るよう変更が必要になりそう
  • test
    • socketをmaxfdまで作成するテストはうまくいかず。何個作ってもfd=3, デストラクタが走っている?
    • テストケースが弱いかも
    • macOS, Linuxで挙動が異なる点があった(一旦テストケースはコメントアウト)
      • PORT 0 : macはerror, LinuxはOK
      • PORT 65536 : macはerror, LinuxはOK(unsigned shortで0になっていそう)
      • IP: 255.255.255.255 : macはerror, LinuxはOK
    • UNIT_TEST_SH は消してもいいかも
  • confのバリデーション
    • socket作成で使うserverのIP, PORTのバリデーションはどこで?socket作成に失敗したらエラーで良いか?
    • 同様に、confのバリデーションは各項目を使うタイミングでないとチェックできない気がするが、それで良いのか?

@ak0327 ak0327 linked an issue Aug 21, 2023 that may be closed by this pull request
@ak0327 ak0327 self-assigned this Aug 21, 2023
@ak0327 ak0327 force-pushed the 17-implement-socket branch 3 times, most recently from 28af063 to 4032ccb Compare August 22, 2023 00:54
@ak0327 ak0327 force-pushed the 17-implement-socket branch 6 times, most recently from d639a2a to 60564b1 Compare August 24, 2023 14:26
@ak0327 ak0327 force-pushed the 17-implement-socket branch from 325dcc4 to 034e571 Compare August 25, 2023 01:33
@ak0327 ak0327 marked this pull request as ready for review August 25, 2023 01:42
@ak0327 ak0327 requested review from naharagu and molhot August 25, 2023 01:42
@ak0327 ak0327 mentioned this pull request Aug 24, 2023
@ak0327 ak0327 marked this pull request as draft September 4, 2023 00:38
@ak0327
Copy link
Owner Author

ak0327 commented Sep 4, 2023

エラーハンドリングの一貫性持たせるために、一旦review取り下げます

@molhot molhot marked this pull request as ready for review September 5, 2023 14:19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このファイルはどういう背景で存在しているのかお聞きしたいです!

Copy link
Contributor

@molhot molhot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

全体的に内容に齟齬がない、綺麗なコードだったと思います!!
CMakeLists.txt
に関してどういう風に使われているか(聞いていたら申し訳ないのですが)お聞きしたいのみです!
概ね僕も同じように作ったので安心でした!(僕のやつがベースなんでしたっけ。。。

@molhot molhot merged commit 44289ab into main Sep 5, 2023
@ak0327 ak0327 mentioned this pull request Sep 6, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement) socket
2 participants