TATEISHI Katsuyuki
tatei****@oss*****
2010年 6月 29日 (火) 15:30:02 JST
田沼さん、 立石です。お疲れさまです。 <tanum****@users*****>-san wrote: > This is an automated email from the git hooks/post-receive script. It was > generated because a ref change was pushed to "ultramonkey-l7-v2" repository > containing the "ultramonkey-l7" project. > > The branch, tproxy has been created > at 11d85152c6ebf8a1f6bd45111b5a0280b32bbf0a (commit) tproxyブランチの内容をパッチから分かる範囲でレビューしました。 以下コメントです。 1. 新たに stdio.h をインクルードしているヘッダがいくつかあり ますが、コンパイル時にsnprintfまわりでエラーがでることに対 する修正ですよね？ tproxy 関係でないのでブランチ（すくなく ともコミット）をわけたほうがいいと思います。(gccの新版対応 でしょうか) # 万が一 tporxy 関連の変更を取り消したくなったときとかを考 # えると分けておいた方がいいです。 あと私の環境(Fedora12)だと include/l7vs.h は stdio.h のイ ンクルードを消してもコンパイル時のエラー等は出ません。また make distcheck の際にエラーにならないよう以下のパッチが必 要でした。 ============================================================ diff --git a/snmpagent/l7snmpagent.cpp b/snmpagent/l7snmpagent.cpp index 5303170..dd672e6 100644 --- a/snmpagent/l7snmpagent.cpp +++ b/snmpagent/l7snmpagent.cpp @@ -1,3 +1,4 @@ +#include <stdio.h> #include <signal.h> #include "logger_wrapper.h" ============================================================ 2. configure.ac の書き方はいいと思います。 3. include/l7vs_conn.h で IP_TRANSPARENT を定義してますが、 linux/in.h 全体を読み込みたくないからですよね？ /usr/include 配下のファイルもほとんどこのファイルを読み込 んでない上、普通インクルードしそうなファイルから暗黙にイン クルードされることもなさそうなので、この処理でいいと思いま す。 4. include/l7vsadm.h のコメントはtypoでしょうか。code -> mode? ============================================================ @@ -128,6 +130,7 @@ struct l7vsadm_option_data { int category_all_flag; //!< All Log-Category flag int replication_start_flag; //!< Start Replication flag enum PARAMETER_COMPONENT_TAG reload_param; //!< Parameter Reload Componet + enum l7vs_dest_forward_type forward; //!< Forward code }; //! L7vsadm command code list. ============================================================ 5. コメントでは Forward mode(「モード」), enum のタグ名では l7vs_dest_forward_type(「タイプ」)となっているので mode か type のどちらかに統一したほうがメンテナンス性は上がると思 います。また各構造体における新しいメンバ変数 forward は fwdmode あるいは fwdtype とかの名前の方がわかりやすい気が します。 6. src/conn.c の下記コードですが・・・ ============================================================ diff --git a/src/conn.c b/src/conn.c index fa184f2..835f5d4 100644 --- a/src/conn.c +++ b/src/conn.c @@ -3139,6 +3139,20 @@ l7vs_conn_connect_rs(struct l7vs_conn *conn, struct l7vs_dest *dest) /*------ DEBUG LOG END ------*/ } +#ifdef IP_TRANSPARENT + if (dest->forward == L7VS_DEST_FORWARD_TPROXY) { + int value = 1; + ret = setsockopt(s, SOL_IP, IP_TRANSPARENT, &value, sizeof(int)); + if (ret != 0) { + LOGGER_PUT_LOG_WARN(LOG_CAT_L7VSD_SYSTEM_SOCKET,/*XXX*/999, "setsockopt(IP_TRANSPARENT) not supported on this platform."); + } else { + ret = bind(s, (struct sockaddr*) &conn->caddr, sizeof(struct sockaddr)); + if (ret != 0) { + LOGGER_PUT_LOG_WARN(LOG_CAT_L7VSD_SYSTEM_SOCKET,/*XXX*/999, "Cannot bind client address."); + } + } + } +#endif /*-------- DEBUG LOG --------*/ if (LOG_LV_DEBUG == logger_get_log_level(LOG_CAT_L7VSD_NETWORK)) { char addr_str[DEBUG_STR_LEN] = {0}; ============================================================ * setsockopt のリターンコードが 0 でない場合に IP_TRANSPARENT が実装されていないものと決め打ちにするのは 良くないと思います。エラーには EBADF や EFAULT とかもある ので。 * IP_TRANSPARENT が実装されていない処理系ではそもそも include/l7vs_conn.hで IP_TRANSPARENT が定義されないので、 #ifdef 内部のコードを通らないと思います。 * ここに到達する以前に l7vsadm で指定できないようガードされ てると思いますが、実際に setsockopt する箇所でもチェック するのは賛成です。 以上を踏まえて、 * (dest->forward == L7VS_DEST_FORWARD_TPROXY) が真でかつ、 IP_TRANSPARENT が定義されていない場合に LOGGER_PUT_LOG_WARN(LOG_CAT_L7VSD_SYSTEM_SOCKET,/*XXX*/999, "setsockopt(IP_TRANSPARENT) not supported on this platform."); を呼ぶようにして、IP_TRANSPARENT が定義されている場合にの み setsockopt を呼び、setsockopt() がエラーでリターンした 場合は errno を見て本当のエラーをログ出力するほうが良いと 思います(これはbind()がエラーになった場合も同様です)。 7. src/l7vsadm_main.c の -m オプションって必要ですかね？(-t だけじゃダメ？) 必要な場合は・・・ * src/l7vsadm_main.c でリアルサーバオプションに -m も -t 指 定しなかった場合のデフォルト値の設定処理を入れませんか？ ファイル内 static なl7vsadm_option_dataが変数なので0、つ まりL7VS_DEST_FORWARD_MASQに初期化されるとは思います が・・・ * 同 l7vsadm_main.c で -m と -t を同時に指定したときはエラー で弾いた方がいいと思います(または後から指定した方が有効で あると文書に明記するか)。 9. マニュアル(man/l7vsadm.8, man/l7directord.8)も修正が必要だ と思います。特に、tproxy は l7vsd だけでは完結できず、 iptablesのルール投入も必要だと思いますので、そのことにつ いても触れておく必要があると思います。 最悪「Linux の Documentation/networking/tproxy.txt 見ろ」 で。 10. sorry サーバも tproxy を選択できるとうれしいです。(今は masq 決め打ちですよね？) 11. fallback サーバは l7vsd からすると単なるリアルサーバなの で、今回 l7vsd/l7vsadm側に特別な修正は不要だと理解しまし た。(fallbackサーバというものを知ってるのは l7directord だけですよね) 以上です。 -- TATEISHI Katsuyuki <tatei****@oss*****>
UltraMonkey-L7
Fork