|
>>4379 68user ありがとうございます。 おかげさまでつい先ほど本番サーバへ移行が完了しました。 本番サーバではリモートコマンドはできないポリシーになっていたようなのでftp でtarファイルを転送し、rootで展開しました。 |
|
>>4386 DNS勉強中 > ここは私なりに解釈して記述しています。 わたしの意図どおりの実装になっています。 > またstruct DNS_Query *q;の必要性が分からなかったので、これは記述していません。 struct DNS_Question { short id short flag; short QDCOUNT; short ANCOUNT; short NSCOUNT; short ARCOUNT; struct DNS_Query *query; }; と書くと、DNS_Question というものは 「id・flag・{QD,AN,NS,AR}COUNT・DNS_Query から成り立つ」 という関係がソース上に明示されます。プログラムは「データ構造＋アルゴリズム」 ですので、正確なデータ構造を最初に明示しておけば、ソースを理解しやすくなる ということです。 また、こうしておけば、 main(){ make_question(&DNS_question); } make_question(DNS_Question *question){ question->id = ...; question->flag = ...; make_query(question->query); } make_query(DNS_Query *query){ query を作成 } というふうに、変数の受け渡しがすっきりします。 ただ、フラットな構成の方がわかりやすい場合もありますので、ケースバイケースかと 思います。 > もっともパディングが心配という事が良く理解できていないので、 わたしの環境では int main (){ struct { char a[1]; int b; } hoge; printf("%d\n", sizeof(hoge)); } の結果は 8 になります。構造体のメンバ間にどれだけのパディングが入るかは 環境依存なので、パディングが入らないことを前提としたコーディングをするのは 怖いなぁ、ということです。 > 後このソースの問題点ですが、いま自分が気づいているところでは2点あります。 > 変数 search と 変数 DNS_question.flag の保守性です。 なるほど、学校の課題にも関わらず細かな部分の完成度を高めたいというのは 素晴らしいですね。余計なお世話かもしれませんが細かいところまで見てみました。 > #include <netdb.h> 必要なヘッダファイルを include していません。その結果、引数チェックが 行われず、戻り値は int として扱われているはず。 このことに気づかないということは、警告オプションを有効にしていないと推察 されます。gcc であれば -Wall オプションを付けると幸せな人生を送れます。 > char *make_domain(char *domain_name); プロトタイプ宣言で変数名は不要で、 char *make_domain(char *); でよいです。別に引数名を記述しても動作には影響ないですが、引数名を変更 したときに直し忘れてズレが出るので、わたしは書きません。/usr/include/*.h にも書いてないです。 > fprintf(stderr, "Error: Socket作成失敗\n"); なぜ失敗したのかがわからないため、システムコールのエラー時には errno を 出力しましょう。わたしは fprintf(stderr, "Error: Socket作成失敗。errno[%s]\n", strerror(errno)); とするのが好みですが、perror(3) などを使う人も多いです。 bind 失敗時も同様。エラーメッセージが Bind error であるのと Bind error [Address already in use] と表示されるのでは、対処時間に大きな差が出ると思います。 ちなみに gethostbyname(3) のエラー処理が抜けています。gethostbyname は ソケットまわりのシステムコール呼び出し時のエラー以外にも、データ内容の エラーが発生する可能性があるので herror(3) や hstrerror(3) を使います。 > return -1; main で戻せる値は 0〜255 です。あと、exit と return が混在しているのが 気になります。 > questions = two_bytes(int_buf[search], int_buf[search+1]); > search += 2; two_bytes・four_bytes のインタフェースがいまいちです。データ取得と インクリメントを別々に行うと、インクリメント忘れ・インクリメントミスが 発生する危険性があります。 たとえば questions = two_bytes(&int_buf, &search); とインタフェースを変え、 int two_bytes(int *int_buf, int *search){ int sum; sum = int_buf[*search] * 16 * 16 + int_buf[*(search+1)]; *search += 2; return sum; } とすればデータ解析とインクリメントを同時に行えます。ここまでくれば、 struct DNS_Response { char data[10000]; int int_data[10000]; int pos; }; という構造体を作った方がよいかもしれません。 上記と関連しますが、 additional = two_bytes(&int_buf[search], &search); search += 1; これは一見すると謎なコードです。2バイト取得しているのに 1バイトしか進めて いないので、わたしはしばらく悩みました。結局は get_domain() の先頭で 1バイト 進めているから問題ないのですが、直感に反するコーディングだと思います。 > int conv_class(char *class){ > switch (class) { > case 1: printf("1 (INTERNET)\n"); break; > case 2: printf("2 (CSNET)\n"); break; > case 3: printf("3 (CNAME)\n"); break; > case 4: printf("4 (HESIOD)\n"); break; > case 5: printf("5 (ANY)\n"); break; > } わたしなら、 typedef struct { char name[100]; int code; } codelist_t; としておいて、 int conv_class(char *class){ codelist_t class_list[] = { {"INTERNET", 1}, {"CSNET", 2}, {"CHAOS", 3}, {"HESIOD", 4}, {"ANY", 5}, }; for ( i=0 ; i<sizeof(class_list)/sizeof(class_list[0]) ; i++ ){ if ( strcmp(class, class_list[i].str) == 0 ){ return class_list[i].code; } } と直します。class_list は void class_print(int class) で共用してもよいでしょう (resolver-1.pl と同じやり方)。 > memset(&client, '\0', sizeof(client)); > host[j] = 0; /* 最後に0を代入する */ memset は 0 で埋めて、host[j] は '\0' で終端すべきです。文字終端の '\0' と 値としての 0 を区別しておくと、後から読んだ人に意図が伝わりますので。 あとは命名について。関数名は動詞、変数名は名詞という基本ルールがあるよう ですが、徹底されていないように思います。 いまいちだと思う関数名: int domain_count(char *); → get_domain_length などが適切では。 int end_domain(int, int *); → is_XXX とか check_XXX がよいのでは。 int resource_data(int *, int, int, char *); → 名詞になっている。しかし他の hogehoge_print より一段レベルが高いので、 混同しないような命名をしたいところ。 void conv_time(int); → 「変換」といいつつ実際に行っているのは「出力」。 いまいちだと思う変数名: int search; → 動詞に見える。pos あたりでいいのでは。 int questions; int answer; int authority; int additional; → s が付いていたり付いていなかったり。あと、この変数の本質は「個数」で あるはず。であれば question_count とか question_num などにしたい。 int flag = 0; → flag や tmp という変数名が許されるのは変数スコープが 5行以内の場合 (わたしの中では)。それ以上ならもう少し長い名前を。 > int resource_data(int *int_buf, int search, int max, char *print); int max → record_num あたりで。 char *print → record_name あたりで。 > int domain_count(char *argv) argv は渡す側の変数名であって、この引数の本質は「ホスト名」であるため、 int domain_count(char *hostname) int domain_count(char *host) などがよいと思います。 と、わたしならこう書くという部分はあるものの、それでも可読性が高い と感じました。たとえば codelist_t は保守性は高いのですが、ぱっと見 読みやすいのは case 文の羅列です。勉強用のソースであれば、下手に 構造化しすぎることなく、あえてベタ書きしたほうがいいのではないかと 考え始めています。 あと、hoge_print 関数も、わたしはこういう書き方をせずに char *a_record_print(int_buf, search); と文字列を返すほうですが、メモリ確保を行うのは呼び出し側か、関数の 中で static に持つか、など悩ましいです。すっぱり割り切って関数内で printf するのも、保守性はともかく可読性は悪くない、と思いました。 あと、 - two_bytes・four_bytes という関数は便利で読みやすい。 - int_buf にコピーするのは、型変換のバグを避ける手法としては 優れているのかも (ちょっと逃げっぽいけど)。 - perl 版の可読性を落としているのはやはり pack。num2bin などの 関数を作り、直接 pack を呼ぶのは避けた方がよいのかも。 - やはり C は構造体がすっきり書けるのがよいところ。perl の場合、 ハッシュだとタイプミス耐性がないし、Class::Struct だとインクリメント が面倒だし、package を作るのは大げさ。 などといろいろと思うところはありました。自分が書いたものを他人が リライトすると、いろいろと気づくことがあっておもしろいですね。 いろいろ偉そうに書きましたが、指摘したことは当ページでは全然実践できて いません (エラーチェックは甘いし、herror(3) は使っていないし、resolver-1.pl ではデータ取得とインクリメントを別々に行っているし)。その辺はご容赦ください。 |
|
>>4388 68userさん いろいろと細かい点までアドバイスをしていただきましてありがとうございます。 大変勉強になります。 全てに指摘点に対しご回答差し上げたいのですが、 まだ指摘された部分の修正作業を行っておりませんので、 一通り読ませていただき、気づいた点について記述させていただきます。 まず return と exit の混在ですが、 これは conv_type 関数や conv_class 関数の部分だけを 指した指摘でしょうか。 それとも exit を一つも使わない方が良いという事でしょうか。 return と exit の混在のデメリットが分からなかったので。 後、変数 search ですが、仰るとおりインターフェースがいまいちです。 何故なら現在位置を返す関数と返さない関数が混在しているからです。 ここの処理がうまく思いつかなかったので search += 2; で逃げていました(^^;。 そして変数 search の大きな問題が仰るとおり > additional = two_bytes(&int_buf[search], &search); > search += 1; です。 ここは作った当人で無いと分からないですよね(^^;。 その理由は、開発途中で方針を変更してしまったためです。 そのため現在位置は ・解析し終わった部分の、次の部分の先頭に現在位置を合わせている所 ・解析し終わった部分の末尾に現在位置を合わせている所 の2パターンが発生してしまいました。 その代償が search += 2; と search += 1; の混在です。 ここは人に解析してもらう際にはまずいので修正します。 あと host[j] = 0; ですが、ここでは www.livedoor.com というドメイン名を 3www8livedoor3com0(数字の部分は文字ではなく、文字コード)に変換しているのですが、host[j] = 0; は ドメインの最後に文字コードの 0 を連結する と言う意味で記述しました。 何となく思ったのですが、 3www8livedoor3com0 の最後の 0 は終端という意味かな。 そう考えると仰られている意味が分かりました。 その部分に関しては、記述ミスと言うより、私の理解が間違っていたのかなと思います。 それと、int_buf は完全に逃げです(^^;。 ここの部分のうまい処理が思いつきませんでした。 他の指摘された部分につきましても、順次修正して良いものを作りたいと思います。 お忙しい中、いろいろご指摘いただきありがとうございます。 |
|
>>4389 DNS勉強中 > まず return と exit の混在ですが、 main から抜ける際に return と exit が混在していることを指したつもりでした。 下請け関数から exit するのは、狙っているのなら別にいいんじゃないかと思います (課題のプログラムですし)。変な値がやってきたら終了してしまうようでは、本格的な 運用で使うことはできませんが、 - 本格的な運用では使えないことを認識している。 - どういう要求が出てきたときに問題が起こるかなんとなく想像できる。 たとえば web サーバのログから IP アドレスを拾って、複数の名前解決を 高速に行うプログラムを作りたいという要求があったとすると、この プログラムではひとつでも変なレスポンスがくると exit してしまうので 実運用には耐えられない。 - 本格的な運用で使用する場合は、どこをどういうふうに書き換えればよいか だいたい把握している。 なら問題ないと思います。 > return と exit の混在のデメリットが分からなかったので。 わたしの中では以下のようなルールにしています。 1. 基本的に main まで戻る。エラー発生時の情報出力を行うことを考えると、 各関数の役割分担が明確になるから。 2. 例外として、引数解析エラー表示の usage 関数内では exit してよい。 理由は、処理が始まっておらず後始末が不要であることと、usage は 数箇所〜数十箇所から呼ばれる可能性があり、呼び出し元での exit 忘れが怖いこと。 if ( argv[i][0] != '-' ){ usage(); exit(1); /* これを書き忘れると処理が続行してしまう */ } 1 の詳細は以下のとおり。 今回のように 1件だけのデータを処理する場合は、変な値がきたときに ほげフラグが 1〜5 の範囲外です などと出力すれば問題ないが、 一度に複数のデータを処理するバッチの場合は ほげフラグが 1〜5 の範囲外です。受注番号[%d] レコード番号[%d] 顧客番号 [%d] などとより多くの情報を出力しないと、どのデータがおかしいのか特定できない。 とはいえ、下請け関数である 文字列→コード変換関数に、 conv_type(query_type, order_no, rec_no, customer_no); などとエラー発生時に備えて各種情報を渡すのは変。エラー出力用のグローバル変数を 作って err_order_no = order_no; err_rec_no = rec_no; err_customer = customer_no; conv_type(query_type); とする、なんてのも馬鹿馬鹿しい (後者はたまに見かけてイラッときますが)。 となると、 int main(){ ret = file_main(); if ( ret == -1 ){ logging("エラー発生。処理中断・ロールバックし、全データを元に戻します"); /* 後始末を一箇所にまとめる */ } } int file_main(){ fp = fopen(filename[i], "rw"); while (1){ fgets(fp, buf, sizeof(buf)); ret = hoge_main(buf); if ( ret == -1 ){ logging("hoge_main でエラー発生。ファイル名[%d] 行番号[%d]", filename[i], line); return -1; } } } int hoge_main(char *buf){ type = conv_type(query_type); if ( type == -1 ){ logging("query_type に変換に失敗。受注番号[%d]", rec_no): return -1; } } int conv_type(char *type){ if ( 異常値 ){ logging("type が異常値 [%s]", type); return -1; } } として main まで戻るのが現実的。conv_type はファイル名だの受注番号だの 責任範囲外のことを知らなくてもよいが、最終的にはエラーリカバリに必要な 情報が揃う。 Java などのいまどきな言語であれば例外を使うべきだろうと思います。 > 3www8livedoor3com0(数字の部分は文字ではなく、文字コード)に変換して > いるのですが、host[j] = 0; は ドメインの最後に文字コードの 0 を連結 > すると言う意味で記述しました。 戻り値を strcpy(DNS_Send.QUERIES, make_domain(argv[2])); と文字列として扱うのであれば、末尾は文字終端 '\0' であるべきで、 host[j] = 0; より host[j] = '\0'; にすべきという意味で書きましたが、たしかに文字列終端でもあり、DNS 的な 0 終端でもあるわけで微妙なところですね。 バイナリ列とみなして長さを明示的に管理し、strcpy のかわりに memcpy を 使うか、あるいは動作的には問題ないのでコメントで補足しておくか、あるいは 修正は不要なのか…。 > それと、int_buf は完全に逃げです(^^;。 いいんじゃないでしょうか。「バグを出さない」ということを第一目的とするなら アリだと思います。わたしなどいまだに char と unsigned char あたりの暗黙の 型変換ではまったりしていますので、キャストがちりばめられているソースだったら 解読に苦労していたかもしれません。 ちなみに単なる「char」が signed であるか unsigned であるかは環境依存です。 とはいえ、わたしは一般的な UNIX しか知らないので、char が unsinged char である環境は見たことはないですが。 |
|
>>4389 DNS勉強中 あと、さらに細かいことをひとつ。インデントは正確につけましょう。フラグ表示 の部分をわかりやすくするために、ひとつ余計にインデントしていますが、 { フラグ表示 } と、ブロックで囲めばすみます。 インデントはプログラマがつけるものではなくエディタが勝手につけるものであり (emacs の M-x indent-region などで)、もしエディタが変なふうにインデントして しまったら、それは括弧の過不足や文字列の閉じ忘れが原因、などと気づくことが できます (python などは除く)。 # というわたしのポリシーはなかなか理解してもらえない。なぜ手でインデントを # 付ける人が多いのだろう (って、素の vi やメモ帳を使ったりするからだが)。 # めんどくさくないのかなぁ。 |
|
>>4388 68user >>4390 68user >>4391 68userさん 今日は上記3レスでアドバイスいただいた内容についてある程度修正したり、 理解に努めておりました。 ・ struct DNS_Query *query; の必要性について理解いたしました。 確かに仰られるとおり、ソースが理解しやすくなると思いました。 そのため、その部分は修正しました。 (ただ make_question, make_query との関係上、一部修正しております)。 また、make_question と make_query を作成することにより、 main 部の記述を減らし、ソースを読みやすくしました。 ただ、記述していただいたとおりに作れなかったので、メイン部に make_question(&DNS_question); count = make_query(&DNS_query, argc, argv[2], argv[3], argv[4]); と2つにわけて記述しております。 ・ パディングについても理解いたしました。 確かに私の以前のソースはパディングが入らない前提で組んでありました。 やっと意図されたことが分かりました(^^;。 ・ /usr/include/bfd.h をみて、 プロトタイプ宣言について仰られていることを確認・理解し、修正しました。 ・ perror を用いエラー処理を追加しました。 ・ 抜けていた gethostbyname のエラー処理を追加しました。 ただし、 herror や hstrerror を Windowsで使う方法が分からなかったので、 とりあえず perror を使っております。 ・ return と exit の混在を解消しました。 それにともない、 main に戻らずに終了していた所を、 main に戻って終了するように変更しました。 またそれらしいエラー情報を出力するように変更しました。 ・ two_bytes の記述変更について理解いたしました。 アドレス渡しで現在位置を変更して、 戻り値で2バイトの値を取得する方法は大変参考になりました。 確かに、教えていただいた記述の方が何かと便利ですので、 ほぼ同じソースを移植させていただきました。 それに伴い、謎の search += 1; を修正しました。 また four_bytes も同様にソースを変更しました。 この作業を実施したことにより、 search += 2; や search += 4; を大幅に減らすことが出来ました。 ・ conv_type, conv_class, class_print, type_print を codelist_t を使用する形に変更しました。 ただし、 void class_print(int class) を用いた共用の記述の仕方が 分からなかったので、 codelist_t {class, type}_list[] = { ... };をグローバル化して共用しています。 また、 qr_print, opcode_print, aa_print, tc_print, rd_print, ra_print, rcode_print も同様に codelist_t { ... }_list[]化しました(こちらはローカル)。 ・ 0 と \0 の混在を修正しました。 ・ 変数名や関数名の命名を修正しました。 ・ フラグ部分のインデントを変更しました。 ブロックで囲むと言う発想が出てこず、 かといって、インデントなりコメントで上下を囲むなどしないと、 のちのち分からないだろうなと思い、 インデントしてしまったのですが、 ブロックで囲むと言う方法が教えていただき、大変参考になりました。 とりあえず今日修正できたところは以上です。 上記以外の部分についてはまだ手をつけておりません。 他に、今まで教えていただいた事を他の部分にも流用できないか、 ソースを一通り読み直してみて調べてみようと思います。 また search を触ったことのより、ある程度検証をする必要があるのかなと思います。 月曜日は予定が入っているので、まとめるのは難しいかも知れませんが、 火曜日くらいにある程度まとめて一度アップしてみようと思います。 いろいろ参考になるアドバイスをいただきまして、ありがとうございます。 |