Conversation
|
| exit(1); | ||
| } | ||
|
|
||
| for (;;) |
There was a problem hiding this comment.
無限ループはwhileのほうが個人的には自然だと思いました。
好みかもしれません。
There was a problem hiding this comment.
たしかにそうですね、直近 Golang に触れていたこともあって引っ張られてました。while にしようと思います。
| server_addr.sin_addr.s_addr = INADDR_ANY; | ||
| server_addr.sin_port = htons(8080); | ||
| result = bind(listen_sock, (struct sockaddr *)&server_addr, sizeof(server_addr)); | ||
| if (result == -1) |
There was a problem hiding this comment.
resultの使いまわしだと、なんのチェックか上の行を見直す手間が少しあると思いました。
変数名をそれぞれ変えるか、
if文の中にbind(listen_sock, (struct sockaddr *)&server_addr, sizeof(server_addr))を直接入れてもいいのかなと思いました。
There was a problem hiding this comment.
ここはちょっと個人的にも迷ったところでした。以下3パターンで迷って、他の2つが冗長だったりコードを読みにくいかなと思って、result を繰り返し使うのを採用しました。（直前の行で代入されるので許容範囲かなと）
- 各 System call の結果を格納する
resultを繰り返し使用 - エラーチェックにのみ使用する
bind_resultのような変数を定義 - 変数名に入れず、そのまま評価
ですが、ここでコメントもらってわかりにくいのかもと思ったので、修正を検討しようと思います。（エラーハンドリングまわり全般をリファクタリングしたいと思っているので、現状はこのままでリファクタリング時にまとめて修正する想定です）
There was a problem hiding this comment.
if文の中にbind(listen_sock, (struct sockaddr *)&server_addr, sizeof(server_addr))を直接入れてもいいのかなと思いました。
再度検討して、エラーハンドリング全般を上記対応のように修正することにしました。コメントありがとうございました！（514a8de）
hayashi-ay
left a comment
hayashi-ay
left a comment
There was a problem hiding this comment.
コメントしました。次のスコープで実装予定のものとかぶっているかもしれないですが
| if (result == -1) | ||
| { | ||
| perror("server: bind()"); | ||
| close(listen_sock); |
There was a problem hiding this comment.
あっても良いですが、プロセス終了時にOS側で解放してくれると思うので、なくても良いですね
There was a problem hiding this comment.
たしかになくても解放されそうですが、個人的にリソース管理は明示的にすることの利点が大きいと考えているので、このままにしておこうと思います。
| { | ||
| unsigned int client_len; | ||
| struct sockaddr_in client_addr; | ||
| int conn_sock = accept(listen_sock, (struct sockaddr *)&client_addr, &client_len); |
There was a problem hiding this comment.
次のスコープかもしれないですが、selectなどで多重化して複数リクエスト受け取れるようにしたいですね。
| } | ||
|
|
||
| char buf[256]; | ||
| result = recv(conn_sock, buf, sizeof(buf) - 1, 0); |
There was a problem hiding this comment.
255文字以上受け取れるようにしたいですね。
あとmanも目を通しておくと良いと思います。
https://man7.org/linux/man-pages/man2/recv.2.html
There was a problem hiding this comment.
255文字以上受け取れるようにしたいですね。
イーサネット上の TCP では1460バイトが最大セグメントサイズなので、それは超えない範囲で1024バイトくらいがよいですかね？（なんとなく 2 のべき乗で設定しておくようなイメージがあったので）
There was a problem hiding this comment.
コメントが良くなかったかもしれないです。バッファサイズ以上の長さの文字列を受け取れるようにしたいです。
あとMSSはTCPレイヤーの話でアプリケーション側からは関係ないと思います。
There was a problem hiding this comment.
バッファサイズ以上の長さの文字列を受け取れるようにしたいです。
あ、なるほどです。f934b8c で修正してみました。recv() が 0 を返すまで buffer の範囲で繰り返しデータを受け取って送り返すループを追加したんですが、イメージ合っていますか？（server 側の buffer を小さくして動作確認してみた感じだと意図通り動いてそうではありました）
あとMSSはTCPレイヤーの話でアプリケーション側からは関係ないと思います。
たしかにそうですね、訂正ありがとうございます！
| exit(1); | ||
| } | ||
|
|
||
| char buf[256]; |
There was a problem hiding this comment.
たぶんL52でsizeof(buf) - 1まで読み取るようにしていて、NULL終端を担保しているつもりだと思うんですけど、たぶん0で値が初期化されている保証はなくて、環境によってはゴミ値が入っている可能性があると思います。（参考文献などなくてすいませんが）
There was a problem hiding this comment.
なるほどです、たしかに担保できていなさそうです。明示的に buf を初期化することで NULL 終端をしておこうと思います。
エコーサーバーというと、一般にこっちを指すみたいですね。勘違いしてたので修正します、コメントありがとうございました！ |
|
レビューありがとうございました！一旦マージして追加実装をしようと思いますが、もし他にもコメントあればマージ後でもぜひお願いします！ |
3 participants
Description
クライアント側で標準入力に入力した文字列を送信し、サーバー側で受け取った文字列をそのままクライアント側に送り返し、クライアント側で表示するものを実装
クライアント側で標準入力に入力した文字列を送信し、サーバー側でそのまま表示するものを実装エラーハンドリングとしては、エラー発生時にすでに作成済みの socket は close してから exit するようにした
Usage
以下コマンドでサーバー、クライアントをそれぞれ実行可能
$ pwd /path/to/c-echoNote
ひとまず最小限の実装までなので、大まかな方向性が正しそうであればこの PR をマージして以下の対応に取り組む予定