FEATURE: Add cmdlog optional filtering #935

6unYoung · 2025-11-19T08:49:32Z

🔗 Related Issue

https://github.com/jam2in/arcus-works/issues/626

⌨️ What I did

cmdlog filter 명령을 처리하는 process_cmdlog_filter() 함수를 추가했습니다.
필터링 검사를 수행하는 is_cmdlog_filter_match() 함수를 추가했습니다.
cmdlog_write()가 기존과 동일한 동작을 하게끔 command에 대한 로직을 수정했습니다.
cmdlog_add(), cmdlog_remove(), cmdlog_list() 함수를 추가했습니다.
cmd_log_global의 멤버 변수로 필터 리스트 struct cmd_log_filter filters[CMDLOG_FILTER_MAXNUM]을 추가했습니다.

명세

필터 추가 명령어
cmdlog filter add (command <cmd> | key <key> | command <cmd> key <key>)로 필터를 등록한다. command, key 옵션 중 하나는 생략할 수 있다. 생략된 옵션은 필터링하지 않는다. 문자 유효성 검사는 수행하지만, 명령어와 키의 존재 여부를 검사하진 않는다. 필터 수에는 제한이 존재한다.

예시

$ cmdlog start
$ cmdlog filter add command add key 01
    0. Filter: command = add, key = 01
$ add 01 0 0 2  // 로깅
$ add 01:02 0 5 1  // 로깅x(키 불일치)

$ cmdlog filter add command bop_insert
    1. Filter: command = bop_insert, key =
$ bop insert  // 필터가 일치해 명령의 결과와 상관없이 로깅
    CLIENT_ERROR bad command line format

// 옵션 모두 생략 시 에러
$ cmdlog filter add
    CLIENT_ERROR bad command line format

// 파라미터가 유효한지 검사
$ cmdlog filter add key \t\n
    CLIENT_ERROR bad command line format
$ cmdlog filter add command add key
    CLIENT_ERROR bad command line format
$ cmdlog filter add key a:b:d!#-^
    2. Filter: command = , key = a:b:d!#-^

// 단 파라미터의 존재 여부를 검사하진 않음
$ cmdlog filter add command dumm1
    2. Filter: command = dumm1, key =

// 필터 최대 개수에 제한 존재
$ cmdlog filter add key a
    3. Filter: command = , key = a
...
$ cmdlog filter add key b
    CLIENT_ERROR filter list is full

필터 삭제 명령어
cmdlog filter remove (index)로 필터를 삭제한다. 인덱스를 입력하면 찾아 삭제하고, 생략 시 모든 필터를 삭제한다.

예시

$ cmdlog filter remove 99
    CLIENT_ERROR  invalid parameters
$ cmdlog filter remove 2
    1 filters removed
$ cmdlog filter remove
    9 filters removed
$ cmdlog filter remove
    0 filters removed
$ cmdlog filter remove 1
    CLIENT_ERROR invalid parameters

필터 출력 명령어
cmdlog filter list로 필터 목록을 출력한다. 현재 필터 수와 최대 필터 수를 같이 출력한다.

예시

$ cmdlog filter list
    (2/10)
    1. command=     key=01
    2. command=add  key=02:02

6unYoung · 2025-11-20T08:34:48Z

의견이 필요한 부분이 몇 개 있어 확인 부탁드립니다.

필터 추가 시 키를 무시하거나 필요로 하지 않는 명령(mget, flush, help, ..)에 대해 임의로 키를 같이 넣은 경우, 명령어가 실제로 입력될 때 키를 필요로 하지 않는 명령에 대해 키를 nul로 처리하므로 해당 필터는 절대 매칭되지 않습니다. 이런 경우에 대해 키를 입력하지 못하도록 방어 코드가 필요할까요?
필터를 동적으로 변경하는 기능은 없어도 된다고 하셨는데, 동적 변경을 막으려고 하면 코드가 다소 지저분해질 것 같습니다.
기존 코드에서는 process_cmdlog_command()에서 cmdlog_start()등을 호출할 때 already_check 변수를 인자로 넘겨 cmdlog.c에 존재하는 cmdlog_in_use의 true/false 여부에 따라 에러 처리를 하고 있습니다. 그런데 filter 기능은 별도의 subcommand를 요구하므로 process_cmdlog_filter()를 만들어 cmdlog_filter_add()등을 호출하게 했습니다. 따라서 기존 코드와 같은 레벨에서 에러 핸들링도 안되고, 리턴 코드로 넘기기에도 좋진 않아 보입니다.
그래서 일단 동적 변경을 허용하게 했는데, 기능적으로 문제가 될까요?

ing-eoking · 2025-11-20T09:23:03Z

개인 의견입니다.

필터 추가 시 키를 무시하거나 필요로 하지 않는 명령(mget, flush, help, ..)에 대해 임의로 키를 같이 넣은 경우, 명령어가 실제로 입력될 때 키를 필요로 하지 않는 명령에 대해 키를 nul로 처리하므로 해당 필터는 절대 매칭되지 않습니다. 이런 경우에 대해 키를 입력하지 못하도록 방어 코드가 필요할까요?

이 경우는 사용자가 잘못 넣은 경우에 해당하므로 별다른 처리를 할 필요가 없을 것 같습니다.
필터 조건에 명령어를 없는 명령어를 넣는 경우와 유사하지 않을까 생각됩니다. (ex. cmd=abcdef)

필터를 동적으로 변경하는 기능은 없어도 된다고 하셨는데, 동적 변경을 막으려고 하면 코드가 다소 지저분해질 것 같습니다.
기존 코드에서는 process_cmdlog_command()에서 cmdlog_start()등을 호출할 때 already_check 변수를 인자로 넘겨 cmdlog.c에 존재하는 cmdlog_in_use의 true/false 여부에 따라 에러 처리를 하고 있습니다. 그런데 filter 기능은 별도의 subcommand를 요구하므로 process_cmdlog_filter()를 만들어 cmdlog_filter_add()등을 호출하게 했습니다. 따라서 기존 코드와 같은 레벨에서 에러 핸들링도 안되고, 리턴 코드로 넘기기에도 좋진 않아 보입니다. 그래서 일단 동적 변경을 허용하게 했는데, 기능적으로 문제가 될까요?

동적으로 변경되어도 상관없을 것 같습니다.

namsic · 2025-12-04T03:01:09Z

cmdlog.c

+    } else if ((command[0] == 'l' ||
+               command[0] == 's' ||
+               command[0] == 'm' ||
+               command[0] == 'b') &&
+               strcmp(command + 1, "op") == 0) {
+        return 2;
+    }
+
+    return 1;


이 함수 호출되는 시점이 ntokens 검증하기 전인데,
tokens[key_idx]가 항상 존재하는 것이 보장되나요?

방어 코드가 빠져 있었습니다. key_idx 별 ntokens 검사 로직을 추가했습니다.

현재 -1 -> 2 -> 나머지 1 순서로 비교하고 있습니다.

key가 있는 명령을 먼저 확인하여 1 또는 2 반환하고,
그 외(알 수 없는 경우 포함) 명령에 대해 -1 반환하는 것이 가장 마지막이어야 자연스러운 형태이지 않나 생각합니다.

또는, command가 아닌 tokens 인자로 받으면 key 문자열을 바로 반환할 수 있어 보입니다.
ntokens 비교 로직 등도 자연스럽게 함수 안쪽으로 이동될 것입니다.

namsic · 2025-12-04T03:03:03Z

cmdlog.c

+    if (key_idx > 0) {
+        snprintf(key, CMDLOG_FILTER_KEY_MAXLEN + 1, "%s", tokens[key_idx].value);
+    }


key에 문자열 복사하지 않고 tokens[key_idx].value 값 바로 사용했을 때 문제가 되는 부분이 있나요?

바로 사용해도 문제는 없습니다. key 지역변수를 사용한 이유는 key를 요구하지 않는 command의 경우 key_idx가 -1이 되므로 인덱스 접근을 배제해야 하는데, 비교 중 -1을 체크하기보단 사전에 확인하고 필터 순회 중에는 비교만 수행하는 게 가독성이 더 좋아보여 선택한 것입니다.

가독성을 위해 지역변수 두는 것에는 크게 이견 없지만,
배열에 문자열을 다시 쓰지 않고 단순히 tokens 또는 empty string을 가리키는 포인터로 관리해도
가독성 측면에서 현재 PR 구현과 크게 차이가 없을 것입니다.

namsic · 2025-12-04T03:10:18Z

memcached.c

+            int len = command_len + key_len + 26;
+            char *buf = (char *)malloc(len);
+            if (buf) {
+                snprintf(buf, len, "\tFilter: command = %s, key = %s", command, key);
+                out_string(c, buf);
+                free(buf);


아래와 같이 응답 마지막 부분이 잘려 보입니다.

$ telnet devbox.gcp.namsic.dev 11211 Trying 35.216.32.213... Connected to devbox.gcp.namsic.dev. Escape character is '^]'. cmdlog filter add command add key 01 Filter: command = add, key = cmdlog filter add command replace Filter: command = replace, key

출력 버퍼 크기를 수정했습니다.

namsic · 2025-12-04T11:14:08Z

cmdlog.c

+    } else if ((command[0] == 'l' ||
+               command[0] == 's' ||
+               command[0] == 'm' ||
+               command[0] == 'b') &&
+               strcmp(command + 1, "op") == 0) {
+        return 2;
+    }
+
+    return 1;


현재 -1 -> 2 -> 나머지 1 순서로 비교하고 있습니다.

key가 있는 명령을 먼저 확인하여 1 또는 2 반환하고,
그 외(알 수 없는 경우 포함) 명령에 대해 -1 반환하는 것이 가장 마지막이어야 자연스러운 형태이지 않나 생각합니다.

또는, command가 아닌 tokens 인자로 받으면 key 문자열을 바로 반환할 수 있어 보입니다.
ntokens 비교 로직 등도 자연스럽게 함수 안쪽으로 이동될 것입니다.

namsic · 2025-12-04T11:20:08Z

cmdlog.c

+    if (key_idx > 0) {
+        snprintf(key, CMDLOG_FILTER_KEY_MAXLEN + 1, "%s", tokens[key_idx].value);
+    }


가독성을 위해 지역변수 두는 것에는 크게 이견 없지만,
배열에 문자열을 다시 쓰지 않고 단순히 tokens 또는 empty string을 가리키는 포인터로 관리해도
가독성 측면에서 현재 PR 구현과 크게 차이가 없을 것입니다.

namsic · 2025-12-04T11:31:26Z

cmdlog.c

+static int get_key_idx_by_command(const char *command)
+{
+    assert(command);
+    if (strstr(command, "mget") ||


strcmp()가 아닌 strstr() 사용하는 이유는 무엇인가요?

*mget* 형태의 명령이 모두 매칭될 것인데,
다른 명령들이 정확한 일치 여부를 확인하는 것에 비하여 모호한 로직으로 보입니다.

mget, bop_mget, bop_smget 명령들은 모두 key 검사를 수행하지 않기로 했기 때문에 나열하지 않고 strstr()로 문자열 매칭을 사용했습니다.

namsic · 2025-12-04T11:50:28Z

cmdlog.c

+    if (cmdlog.nfilters >= CMDLOG_FILTER_MAXNUM) {
+        return 0;
+    }


lock 잡은 상태로 확인해야 하지 않는지?

is_cmdlog_filter_match(), cmdlog_filter_add(), cmdlog_filter_list() lock 관련 코드 수정했습니다.

cmdlog.c

6unYoung · 2025-12-08T07:48:47Z

@jhpark816
리뷰 부탁드립니다.

jhpark816 · 2025-12-08T07:58:37Z

@6unYoung
cmdlog filter add [command <cmd>] [key <key>] 형식은
인자가 없는 cmdlog filter add 명령도 가능하다는 것을 나타냅니다.
따라서, 의미에 맞게 수정해 주세요.

6unYoung · 2025-12-08T08:18:55Z

cmdlog filter add [command <cmd>] [key <key>] 형식은 인자가 없는 cmdlog filter add 명령도 가능하다는 것을 나타냅니다. 따라서, 의미에 맞게 수정해 주세요.

의미에 맞는 EBNF 형식으로 수정했습니다.

jhpark816 · 2025-12-08T08:46:29Z

cmdlog.c

+            inputstr[nwritten++] = ' ';
+            ptr++;
+        }
+    }


@6unYoung
여기 코드 수정은 버그를 제거하기 위한 것인가요?
기존 코드를 조금 수정해서도 해결할 수 있을 것 같은 데요. 어떤가요?

해당 코드는 cmdlog_write() 호출 전에 tokenize_command()를 먼저 호출하게 수정했기 때문에 추가되었습니다. command가 tokenized된 상태이므로 널 문자를 건너뛰면서 커맨드 라인 전체를 입력받게 했습니다.

jhpark816 · 2025-12-08T08:51:56Z

memcached.c

        return;
    }

+    ntokens = tokenize_command(command, cmdlen, tokens, MAX_TOKENS);


@6unYoung
tokenize_command() 수행하고 나면, command 문자열이 훼손됩니다.
따라서, command 문자열을 그대로 로깅하는 것이 어려울 것 같은 데요.
확인해 보았나요?

jhpark816

일부 리뷰

jhpark816 · 2025-12-08T10:33:51Z

memcached.c

+        }
+    } else {
+        out_string(c, "ERROR unknown command");
+    }


DOCS도 반영해 주시죠.
정확한 인터페이스를 확인하고자 합니다.

jhpark816 · 2025-12-08T10:36:31Z

memcached.c

+            if (!safe_strtol(tokens[3].value, &idx)) {
+                idx = -1;
+            }
+        }


다시 살펴보니, 아래 인터페이스가 직관적이고 명확한 것 같습니다.

remove <index> remove all

jhpark816 · 2025-12-08T10:37:37Z

memcached.c

+            if (buf) {
+                snprintf(buf, buf_size, "\tFilter: command = %s, key = %s", command, key);
+                out_string(c, buf);
+                free(buf);


OK\r\n 응답하면 될 것 같습니다. 어떤가요?

jhpark816 · 2025-12-08T10:38:48Z

memcached.c

+            command = tokens[4].value;
+            command_len = tokens[4].length;
+            key = tokens[6].value;
+            key_len = tokens[6].length;


key 다음에 command 명시하는 것은 허용하지 않네요.

위에서 제시한 인터페이스에는 해당 형식을 허용하지 않습니다.
-cmd 같이 순서 상관 없는 옵션 형식이 아니고, 위치가 정해지고 선택 가능한 인자 방식으로 표기했기에 순서 바꿈은 허용하지 않았습니다.

jhpark816 · 2025-12-08T10:50:09Z

memcached.c

+                out_string(c, "SERVER_ERROR out of memory");
+            }
+        } else if (ret == 0) {
+            out_string(c, "CLIENT_ERROR filter list is full");


CLIENT_ERROR 보다는 SERVER_ERROR가 나은 것 같습니다.

jhpark816 · 2025-12-08T10:50:33Z

memcached.c

+            int buf_size = 32;
+            char buf[buf_size];
+            snprintf(buf, buf_size, "\t%d filters removed", ret);
+            out_string(c, buf);


여기도 아래와 같이 구분되어야 할 것 같습니다.

OK

SERVER_ERROR filter list is empty

jhpark816 · 2025-12-08T11:02:36Z

cmdlog.c

+        strcmp(command + 1, "op") == 0) {
+        if ((cmdlen == 8 && memcmp(command, "bop_mget", 8) == 0) ||
+            (cmdlen == 9 && memcmp(command, "bop_smget", 9) == 0)) {
+            return -1;


-1 리턴하는 이유는 무엇인가요?

mget류 명령의 경우에 key를 별도의 라인으로 입력받기에 키를 필터링하기에 번거롭고, 기존 cmdlog 동작에서도 키 라인을 로깅하지 않고 있습니다.
따라서 mget류 명령은 key를 사용하지 않으므로 -1을 반환하고 있습니다.

jhpark816 · 2025-12-08T11:04:26Z

cmdlog.c

+               (cmdlen == 6 && memcmp(command, "delete", 6) == 0) ||
+               (cmdlen == 7 && memcmp(command, "getattr", 7) == 0) ||
+               (cmdlen == 7 && memcmp(command, "setattr", 7) == 0) ||
+               (cmdlen == 5 && memcmp(command, "touch", 5) == 0)) {


기존에는 모든 명령을 로깅할 수 있었지만,
현재는 config, zkensemble 등의 명령을 로깅하지 않네요.

로깅 가능합니다.

jhpark816 · 2025-12-08T11:06:03Z

cmdlog.c

+    pthread_mutex_lock(&cmdlog.lock);
+    for (int i = 0; i < cmdlog.nfilters; ++i) {
+        if (cmdlog.filters[i].command[0] == '\0' || strcmp(command, cmdlog.filters[i].command) == 0) {
+            if (cmdlog.filters[i].key[0] == '\0' || strcmp(key, cmdlog.filters[i].key) == 0) {


exact match만 허용되나요?

네 맞습니다

jhpark816 · 2025-12-08T11:06:24Z

cmdlog.c

+
+    pthread_mutex_lock(&cmdlog.lock);
+    for (int i = 0; i < cmdlog.nfilters; ++i) {
+        if (cmdlog.filters[i].command[0] == '\0' || strcmp(command, cmdlog.filters[i].command) == 0) {


lop 명령 전체를 로깅하는 것은 안 되나요?

collection type에 대해 부분 매칭을 허용하면 다른 명령어에 대해서도 동일한 동작을 기대할 것 같은데, 해당 부분을 고려해야 할 것 같습니다.

jhpark816 · 2025-12-09T03:21:28Z

memcached.c

-    if (ntokens < 3 || ntokens > 4) {
-        out_string(c, "\t* Usage: cmdlog [start [path] | stop | stats]\n");
+    if (ntokens < 3) {
+        out_string(c, "\t* Usage: cmdlog [start [path] | stop | stats | filter [add|remove|list]]\n");


아래 notation으로 변경하시죠.

cmdlog { start [path] | stop | stats | filter { add | remove | list } }

6unYoung force-pushed the feature_cmdlog branch 3 times, most recently from 5ceae4f to d16a097 Compare November 19, 2025 09:17

ing-eoking self-requested a review November 20, 2025 09:23

namsic reviewed Dec 4, 2025

View reviewed changes

6unYoung force-pushed the feature_cmdlog branch 2 times, most recently from 82661ae to bb79672 Compare December 4, 2025 04:28

namsic reviewed Dec 4, 2025

View reviewed changes

6unYoung force-pushed the feature_cmdlog branch 2 times, most recently from 1fcd0b9 to 5924b63 Compare December 5, 2025 02:29

6unYoung requested a review from namsic December 5, 2025 03:00

namsic approved these changes Dec 5, 2025

View reviewed changes

6unYoung marked this pull request as draft December 8, 2025 06:22

6unYoung force-pushed the feature_cmdlog branch 2 times, most recently from d384f48 to b0db1d8 Compare December 8, 2025 07:11

6unYoung marked this pull request as ready for review December 8, 2025 07:12

jhpark816 reviewed Dec 8, 2025

View reviewed changes

6unYoung force-pushed the feature_cmdlog branch 2 times, most recently from d8ae576 to 7fa8720 Compare December 8, 2025 08:50

jhpark816 reviewed Dec 8, 2025

View reviewed changes

6unYoung force-pushed the feature_cmdlog branch 2 times, most recently from 0e94dd1 to 9ae5839 Compare December 9, 2025 01:06

FEATURE: Add cmdlog optional filtering

f4f96c4

6unYoung force-pushed the feature_cmdlog branch from 9ae5839 to f4f96c4 Compare December 9, 2025 01:26

jhpark816 reviewed Dec 9, 2025

View reviewed changes

FEATURE: Add cmdlog optional filtering #935

Are you sure you want to change the base?

FEATURE: Add cmdlog optional filtering #935

Conversation

6unYoung commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Related Issue

⌨️ What I did

명세

Uh oh!

6unYoung commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ing-eoking commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

6unYoung commented Dec 8, 2025

Uh oh!

jhpark816 commented Dec 8, 2025

Uh oh!

6unYoung commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

6unYoung Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

6unYoung commented Nov 19, 2025 •

edited

Loading

6unYoung commented Nov 20, 2025 •

edited

Loading

6unYoung commented Dec 8, 2025 •

edited

Loading

6unYoung Dec 8, 2025 •

edited

Loading

6unYoung Dec 9, 2025 •

edited

Loading