|
>>4453 DNS勉強中 > http://kansai2channeler.hp.infoseek.co.jp/cgi-bin/joyful/img/1374.zip そろそろケチをつけるところもあまりなくて、重箱の隅的ではありますがいくつか。 > char *tc_search(int tc){ > static char tc_result[20]; > ... > return tc_result; > } こういうふうにしてしまうと char *p = tc_search(tc_1); char *q = tc_search(tc_2); という使い方ができなくなります (p が指す内容が上書きされてしまうため)。 スレッドセーフでなくなるのと、利用者のレベルが低ければ余計なバグを生む 危険性があります。 UNIX のライブラリ関数でも、最初は localtime・ctime・strtok・gethostbyname などの 非スレッドセーフ版しかありませんでしたが、localtime_r・ctime_r・strtok_r・ gethostbyname_r などのスレッドセーフ版を用意する羽目になってしまいました。 承知の上でそうしておられるならば問題ないと思います。 > /* フラグ一覧を出力 */ > qr = (DNS_response.int_data[DNS_response.pos] & 0x80) >> 7; > printf(" QR: (Query/Response): %s\n", qr_search(qr)); > > opcode = ((DNS_response.int_data[DNS_response.pos] & 0x40) >> 6) * 8; > opcode += ((DNS_response.int_data[DNS_response.pos] & 0x20) >> 5) * 4; > opcode += ((DNS_response.int_data[DNS_response.pos] & 0x10) >> 4) * 2; > opcode += (DNS_response.int_data[DNS_response.pos] & 0x08) >> 3; > printf(" OPCODE: %s\n", opcode_search(opcode)); わかりやすい変数名を使うこと自体は非常によいのですが、同じ値を繰り返し 使用する場合は、 int flag = DNS_response.int_data[DNS_response.pos]; qr = (flag & 0x80) >> 7; printf(" QR: (Query/Response): %s\n", qr_search(qr)); opcode = ((flag & 0x40) >> 6) * 8; opcode += ((flag & 0x20) >> 5) * 4; opcode += ((flag & 0x10) >> 4) * 2; opcode += (flag & 0x08) >> 3; printf(" OPCODE: %s\n", opcode_search(opcode)); などと一時変数に代入することをお勧めします。狙いは可読性向上と、変更時の 修正ミス防止です。わたしの場合は、 for ( int i=0 ; i<length ; i++ ){ struct *hoge_p = hoge_list.buf[x]; printf("%s\n", hoge_p->foo); printf("%s\n", hoge_p->bar); printf("%s\n", hoge_p->baz); } などと、ループ中でもよく一時変数に代入します。これは、ループ先頭の struct *hoge_p = hoge_list.buf[x]; で、 このループ中で参照するのは hoge_list.buf[x] だけ。hoge_list.buf[x+1] を 参照したりはしないよ。 というメッセージを送っているつもりです (効果があるかどうかは知りませんが)。 > DNS_query->data[0] = DNS_packet->id >> 8; > DNS_query->data[1] = DNS_packet->id & 0xff; > ... > DNS_query->data[11] = DNS_packet->ARCOUNT & 0xff; > for (i=12, j=0; i<12+domain_length; i++, j++) > DNS_query->data[i] = DNS_question_record->Name[j]; この場合は、変更時の作業量削減とマジックナンバ排除のため一時変数を使うべきです。 char *data_p = DNS_query->data; *(data_p++) = DNS_packet->id >> 8; *(data_p++) = DNS_packet->id & 0xff; ... *(data_p++) = DNS_packet->ARCOUNT & 0xff; for ( i=0; i<domain_length; i++) *(data_p++) = DNS_question_record->Name[i]; > FLAG flag_list[] = {{ /* 外側の中括弧は構造体の、内側の中括弧は配列の初期値を示す */ > 0, > ... > }}; > > return ((flag_list->qr * 128 + flag_list->opcode * 8 + .... )); FLAG flag = { 0, ... }; return (flag.qr*128 + flag.opcode*8 ... ); でよいと思います。あと、ビットシフトをしたい場合は乗算ではなく、>> や << で 表現した方が素直かなと思いました。 > int make_question_record(struct DNS_Question_Record *DNS_question_record, > int argc, char *hostname, char *type, char *class){ > /* 照会タイプが何も入力されていなければデフォルトの A を代入する */ > if (argc >= 4) > strcpy(query_type, type); > else > strcpy(query_type, "A"); ここまで argc を引っ張らず、もっと上位の関数で query_type を設定すべきと 思います (preprocessing あたりで)。 argc を引っ張りすぎると、もし引数のインタフェースを変更した場合、ソース 全体を調べて argc 関連の部分を修正することになってしまいます。 > /* DNS_response_before_conv->recv_bufはchar型なので、 > その中に格納されている数字は+127(0000007e)までしか表現できない */ しょーもないことですが、複数行のコメントは /* DNS_response_before_conv->recv_bufはchar型なので、 * その中に格納されている数字は+127(0000007e)までしか表現できない */ などと書けば、grep したときにその行がコメントであることが一目でわかります。 > goto comp2; 再起であれば、これまでの状態を脳内スタックに積んで、新たに呼ばれた関数の 引数と戻り値だけを考えればいいですが、こういう goto は覚えておかなくては ならない状態が多すぎるので、こういう goto はわたしは使いません (というか 頭が混乱してしまうので使えません)。 わたしは、エラー発生時の脱出以外で goto を使わない派です。 1ヶ月以上引っ張ってしまったわりに、たいしたレビューができませんで 申し訳ないです。 |