Issue249 select error log level #255

ayase-mstk · 2024-07-02T05:26:09Z

変更点

error_logで出力レベルを変更できるようにしました。
log fdの管理はAccessLog classとErrorLog classでそれぞれ行うようにしました。
writeErrorlogを使用したところで、エラーレベルを引数に追加しました。
test/conf/directive_err_test.shに少しテストを追加しました。
make fcleanでデフォルトエラーログファイルを削除するようにしました。

見ていただきたい点

ErrorLog.hppの設計
utils/syscall_wrapper.cppのエラーログレベルがこれで大丈夫か

YungTatyu

早速の対応ありがとうございます。

YungTatyu · 2024-07-02T07:18:15Z

Makefile

@@ -55,7 +57,7 @@ clean:

 fclean: clean
 	$(MAKE) fclean -C $(TEST_CGI_DIR)
-	$(RM) $(NAME)
+	$(RM) $(NAME) $(LOG_FILE)


logやerror fileは削除されないほうがいいんじゃないでしょうか？

知らぬ間にログがとても増えてしまっても良くないかなと思いこのようにしました。
消すのはあくまでもデフォルトのログなので、残したければ他の名前のログに書き込むようにすれば大丈夫かなとも思いました。

あくまで、makefileはbuildの自動化ツールなので、ログなどは特に触らないで統一してもいいかもしれません。
それかtestを実行する際に、テストスクリプトでlogを削除することもいいかもしれません
一つの案なので、参考程度に

一理ありますね。ただ実はログディレクトリはmakefileで作っているので、すでにログに多少触ってはいます。
テストスクリプトでのみ作成されているようなlogはすべて削除するようにしていますが、デフォルトで作られるログはどうするか悩ましいですね。あとで相談させてください。

YungTatyu · 2024-07-02T07:20:31Z

src/cgi/CgiExecutor.cpp

@@ -36,7 +36,7 @@ void cgi::CgiExecutor::executeCgiScript(const HttpRequest& request, const HttpRe
  prepareCgiExecution(request, response, full_path, cgi_sock, cli_sock);
  execve(this->script_path_.c_str(), const_cast<char* const*>(this->argv_.data()),
         const_cast<char* const*>(this->meta_vars_.data()));
-  WebServer::writeErrorlog(error::strSysCallError("execve") + "\n");
+  WebServer::writeErrorlog(error::strSysCallError("execve"), config::WARN);


syscallのエラーは、emerg統一でいいのではないでしょうか
あえて、warnにしましたか？

そうですね、あえて全てEMERGしなかったというのはおっしゃる通りです。
しかしエラーの基準が少し曖昧であることは確かなので、chatgptに聞いてみたところ以下のように出ました。
これによると、一般的にWARN以上が異常動作かもしれません。またemergはサーバーが使用不能になるほどのエラーかなと思っています。なので例えばaccessなどは使いところによってはEMERGだと大袈裟すぎると思っています。
またcgi周りのエラーもサーバーの仕様が続行不可能とまでは言えないかなとも思いました。

それともすべてシステムコールは統一でEMERGのほうがわかりやすいでしょうか？

DEBUG（デバッグ）概要: 開発中の詳細な情報。通常は問題のトラブルシューティングに役立つ情報。使用例: 変数の値、関数の呼び出し、ループの中のステップなど。 INFO（情報）概要: 正常動作の過程を記録するためのメッセージ。使用例: サービスの開始や停止、設定の読み込みなどのイベント。 NOTICE（通知）概要: 通常の動作中に注意を要する事象。使用例: 設定ファイルの非推奨設定の使用、予期しないが致命的ではない事象。 WARNING（警告）概要: 予期しない動作が発生したが、システムの運用には問題ない事象。使用例: メモリの使用量が高いが、まだ正常に動作している場合など。 ERROR（エラー）概要: システムが正常に動作できない事象が発生したが、アプリケーションは続行可能。使用例: ファイルの読み取り失敗、リソースの不足など。 CRITICAL（重大）概要: 重大なエラーであり、システムの一部が正常に機能しない。使用例: データベースの接続失敗、重要なコンポーネントのクラッシュなど。 ALERT（警報）概要: 速やかに対処が必要な状態。使用例: アプリケーションのクラッシュ、セキュリティの侵害が発生した場合など。 EMERGENCY（緊急）概要: システムが使用不可能な状態。使用例: システム全体の停止、重大なハードウェア障害など。

WARNよりはERRORのほうがいいかと思いました。
warnは注意というイメージが強いので

ではシステムコールは最低でもERRORにしようと思います。

YungTatyu · 2024-07-02T07:29:36Z

src/config/LogFd.cpp

+  if (syscall_wrapper::Access(path, F_OK, false) == 0 && syscall_wrapper::Access(path, W_OK, true) == -1) {
+    std::cerr << error::strSysCallError("access", path) << std::endl;


エラー出力するなら、ログに書き込まなくてもいいかと思いました。
この処理はサーバーが立ち上がるまえだと思うので、なおさらエラー出力だけでいいかもです。

一応nginxはbindなどが失敗していたら標準エラー出力とログの両方に書いていたので合わせました。
無くした方がいいでしょうか？

今のままでいいと思います

YungTatyu · 2024-07-02T07:31:01Z

src/config/LogFd.cpp

+  return syscall_wrapper::Open(log_path, kLogFileFlags, kLogFileMode);
+}
+
+bool config::checkFileAccess(const std::string& path) {


haveWritePermission()
みたいな命名のほうがわかりやすいかと思いました。

おっしゃる通りですね。修正します。

YungTatyu · 2024-07-02T07:33:27Z

src/config/Parser.cpp

+    case DEBUG:
+      mask |= DEBUG;  // fall through
+    case INFO:
+      mask |= INFO;  // fall through
+    case NOTICE:
+      mask |= NOTICE;  // fall through
+    case WARN:
+      mask |= WARN;  // fall through
+    case ERROR:
+      mask |= ERROR;  // fall through
+    case CRIT:
+      mask |= CRIT;  // fall through
+    case ALERT:
+      mask |= ALERT;  // fall through
+    case EMERG:
+      mask |= EMERG;  // fall through
+    default:
+      break;


breakがなくても、compileエラーにならないんですね

defaultがなくてもcompile通りそうだと思いましたがどうでしょうか

// fall throghというコメントがあれば、breakしないでそれ以下のcaseに進めるようです。つまり、CRITであれば、CRITとALERTとEMERGのcaseの中の処理がされます。
defaultはなくてもいけそうです。ありがとうございます。

YungTatyu · 2024-07-02T07:38:27Z

src/config/conf.cpp

+std::string config::LogLevelToStr(LOG_LEVEL level) {
+  switch (level) {
+    case DEBUG:
+      return "debug";
+    case INFO:
+      return "info";
+    case NOTICE:
+      return "notice";
+    case WARN:
+      return "warn";
+    case ERROR:
+      return "error";
+    case CRIT:
+      return "crit";
+    case ALERT:
+      return "alert";
+    case EMERG:
+      return "emerg";
+    default:
+      return "unknown";
+  }
+}


error log classのメンバ関数にしたらどうでしょうか

enum LOG_LEVELがconfig namespaceにあるのでこのようにしたのですがErrorLog classにあるほうがいいでしょうか？

ErrorLog classの方が整理されていると思います。
少なくとも、ErrorLog.hppに書いてあげたらいいと思います。

かしこまりました。

YungTatyu · 2024-07-02T07:44:03Z

src/server/NetworkIOHandler.cpp

@@ -64,7 +66,8 @@ int NetworkIOHandler::setupSocket(const std::string& address, unsigned int port)
    if (re == -1)
      throw std::runtime_error(error::strSysCallError("bind", "to " + address + ":" + utils::toStr(port)));

-    syscall_wrapper::Listen(listen_fd, SOMAXCONN);
+    re = syscall_wrapper::Listen(listen_fd, SOMAXCONN);


例外をスローするときは、mainでキャッチして、改行も出力しています。
なので、改行をつける必要はありません

エラーレベルも出力されないようになっていると思います。

ありがとうございます。改行は多分つけてないと思います。
throwするところは以下のように変更しようと思います。

throw std::runtime_error("webserv: [emerg] " + error::strSysCallError("bind", "to " + address + ":" + utils::toStr(port)));

socket()のラッパー関数が改行を含んじゃっています。

listen()も同様です

ありがとうございます。やっと気づきました。（笑）

YungTatyu · 2024-07-02T07:49:58Z

test/conf/conf_files/directive_error/errorLog_upper_case_level.conf

+
+http {
+  server {
+    error_log logs/error.log DEBUG;


このファイルで、ほかのすべてのエラーレベルも一緒にテストしてあげたらいいと思います

たぶんconfのエラーは一つのファイルで一つまでしかエラーを検出できないのでエラーレベルの数だけファイルを作ることになるかと思います。

YungTatyu · 2024-07-02T07:54:05Z

src/server/ConfigHandler.cpp

+  // access_logがコンテキストで指定されていれば出力する
+  if (location && utils::hasDirective(*location, kAccessLog)) {
+    for (size_t i = 0; i < location->access_log_list_.size(); i++) {
+      if (utils::writeChunks(location->access_log_list_[i].getFd(), msg) == -1)


これ全体的にmsgから改行削除しているので、ログに改行がない状態になりませんか？

改行を削除したのはwriteErrorLogだけで、こちらはwriteAccessLogなので大丈夫かと思います。

YungTatyu · 2024-07-03T10:00:45Z

src/config/LogFd.cpp

-      std::cerr << "webserv: [emerg] realpath() \""
-                << "."
-                << "\" failed (" << errno << ": " << strerror(errno) << ")" << std::endl;
+      std::cerr << error::strSysCallError("realpath") << std::endl;


これもエラーレベルの出力が消えてしまっているとおもいます。

ありがとうございます。

YungTatyu · 2024-07-03T10:02:39Z

src/config/LogFd.cpp

+bool config::haveWritePermission(const std::string& path) {
+  // ファイルはあるが、write権限がない時ときはerror
+  if (syscall_wrapper::Access(path, F_OK, false) == 0 && syscall_wrapper::Access(path, W_OK, true) == -1) {
+    std::cerr << error::strSysCallError("access", path) << std::endl;


これも、エラーレベルの出力が消えてしまっているとおもいます。
error namespaceにエラー出力の関数を作ってしまうのがいいかもしれません。

printErrorという関数をerror namespaceに追加しました。

YungTatyu · 2024-07-03T10:05:24Z

src/config/conf.cpp

+    default:
+      return "unknown";


defautlは削除してもいいと思います。
あとこの関数は、ErrorLog.hppにかいてほしいです

修正しました。

YungTatyu · 2024-07-03T10:08:36Z

test/conf/init_log_test.cpp

@@ -96,6 +96,15 @@ class InitLogTest : public ::testing::Test {
    delete config_;
  }

+  template <typename T>
+  int getFdNum(T log_list) {


以下のような命名のほうがいいと思います。
getterは計算量が1

int countFds(T log_list) {

ありがとうございます。

YungTatyu · 2024-07-05T04:12:13Z

src/config/conf.cpp

+const int config::ErrorLog::kDefaultLevel_ =
+    config::WARN | config::ERROR | config::CRIT | config::ALERT | config::EMERG;


これ多分、errorlog.hpp で定義できちゃうと思います。
kTypeと同じように。

できました！ありがとうございます。
非整数型だけがヘッダー外で定義しないといけないものみたいですね！

YungTatyu · 2024-07-05T04:15:35Z

src/server/NetworkIOHandler.cpp

@@ -62,9 +64,11 @@ int NetworkIOHandler::setupSocket(const std::string& address, unsigned int port)
    // 失敗したとき？
    re = syscall_wrapper::Bind(listen_fd, (struct sockaddr*)&servaddr, sizeof(servaddr));
    if (re == -1)
-      throw std::runtime_error(error::strSysCallError("bind", "to " + address + ":" + utils::toStr(port)));
+      throw std::runtime_error("webserv: [emerg] " +
+                               error::strSysCallError("bind", "to " + address + ":" + utils::toStr(port)));


throwするときに、
webserv [error level]がついてるやつとついていないものが混在しています。
どちらが正解なんでしょうか

webserv [error level]
をちゃんとつけてあげるのが正解ぽいです。

気づきませんでした。ありがとうございます。

ayase-mstk added 24 commits July 2, 2024 01:42

⚡ change design of log class

47b6134

⚡ change method in LogFd header

06eb165

⚡ add parseLogLevel

c095484

⚡ change writeLog and related methods

a3b28cc

⚡ modify http response method related to the time

fdaf0b0

⚡ modify error::strSysCallErorr

e94266e

🐛 modify copy assignment operator & type

7565057

🐛 move definition of LogLevelToStr

83aa85c

⚡ corresponding to new argument of writeErrorLog

4e19942

⚡ modify writeErrorLog test

9b623bc

⚡ add error_log err test

f0aa9f5

♻️ refactoring init_log_test

5b1289c

♻️ rename test dir

34d347e

♻️ auto format

bc67da3

♻️ refactoring LogFd class

1c915e2

🐛 modify LogFd

24bfd22

♻️ refactoring writeErrorLog

48fba28

🐛 modify writeErrorLog test

d646281

⚡ make fclean delete default log files

f075c7c

♻️ add log when worker_connections are over

8295605

♻️ refactoring

7d7a492

♻️ refactoring writeErrorLog

6a58031

🐛 modify to write default access_log

f358dfd

Merge main into issue249-select_error_log_level

66b43ab

YungTatyu reviewed Jul 2, 2024

View reviewed changes

ayase-mstk added 5 commits July 2, 2024 17:18

♻️ modify throw msg

40f1276

♻️ delete waste switch case

fee6137

♻️ rename method in LogFd class

284f0b5

♻️ refactoring

94f721f

♻️ move LogLevelToStr

17ecc7a

ayase-mstk added 3 commits July 2, 2024 22:11

⚡ change syscall log level

e64d276

♻️ delete waste new lines

bb37291

♻️ resolve conflict

811955b

YungTatyu reviewed Jul 3, 2024

View reviewed changes

ayase-mstk added 3 commits July 3, 2024 22:20

♻️ rename test method

ebb0c92

⚡ a method defined in ErrorLog header

c0f334f

⚡ create printError

128beec

YungTatyu reviewed Jul 5, 2024

View reviewed changes

ayase-mstk added 2 commits July 5, 2024 13:54

♻️ initialize const variable in ErrorLog header

15b32d0

♻️ add error level header to exception msg

4fc099d

YungTatyu approved these changes Jul 5, 2024

View reviewed changes

ayase-mstk merged commit bae1c26 into main Jul 5, 2024
5 checks passed

		if (syscall_wrapper::Access(path, F_OK, false) == 0 && syscall_wrapper::Access(path, W_OK, true) == -1) {
		std::cerr << error::strSysCallError("access", path) << std::endl;

		const int config::ErrorLog::kDefaultLevel_ =
		config::WARN \| config::ERROR \| config::CRIT \| config::ALERT \| config::EMERG;

Issue249 select error log level #255

Issue249 select error log level #255

Conversation

ayase-mstk commented Jul 2, 2024 • edited Loading

変更点

見ていただきたい点

YungTatyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayase-mstk Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayase-mstk commented Jul 2, 2024 •

edited

Loading

ayase-mstk Jul 2, 2024 •

edited

Loading