Skip to content

fix: configuration parsing error mmap_log_buffer_size #2363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix: configuration parsing error mmap_log_buffer_size #2363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion fw/access_log.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ static TfwMmapBufferHolder *mmap_buffer;
static DEFINE_PER_CPU_ALIGNED(char[ACCESS_LOG_BUF_SIZE], access_log_buf);
static DEFINE_PER_CPU_ALIGNED(u64, mmap_log_dropped);
static long mmap_log_buffer_size;
static char *tfw_logger_config_path = NULL;

/** Build string consists of chunks that belong to the header value.
* If header value is empty, then it returns "-" to be nginx-like.
Expand Down Expand Up @@ -576,7 +577,21 @@ cfg_access_log_set(TfwCfgSpec *cs, TfwCfgEntry *ce)
bool off = false;

TFW_CFG_CHECK_VAL_N(>, 0, cs, ce);
TFW_CFG_CHECK_NO_ATTRS(cs, ce);

// Check for logger_config attribute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use C++ style comments in C, https://github.com/tempesta-tech/tempesta/blob/master/CodingStyle . In general https://tempesta-tech.com/knowledge-base/Development-guidelines/ is worth reading

const char *logger_config = tfw_cfg_get_attr(ce, "logger_config", NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete this code, tfw_logger_config_path is not used now, there is no sense to keep it. If it become necessary in future it's not a big deal to add it.

if (logger_config) {
// Save the logger config path for later use
if (tfw_logger_config_path)
kfree(tfw_logger_config_path);
tfw_logger_config_path = kstrdup(logger_config, GFP_KERNEL);
if (!tfw_logger_config_path)
return -ENOMEM;

// Set access_log type to mmap since we're using external config
access_log_type |= ACCESS_LOG_MMAP;
return 0;
}

TFW_CFG_ENTRY_FOR_EACH_VAL(ce, i, val) {
if (strcasecmp(val, "off") == 0) {
Expand All @@ -596,6 +611,19 @@ cfg_access_log_set(TfwCfgSpec *cs, TfwCfgEntry *ce)
return -EINVAL;
}

// Parse other attributes if using mmap
if (access_log_type & ACCESS_LOG_MMAP) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this part. For tfw_logger we do validation only in tempesta.sh file, but now we must do it in tfw_logger.

mmap_host = tfw_cfg_get_attr(ce, "mmap_host", NULL);
mmap_log = tfw_cfg_get_attr(ce, "mmap_log", NULL);
mmap_user = tfw_cfg_get_attr(ce, "mmap_user", NULL);
mmap_password = tfw_cfg_get_attr(ce, "mmap_password", NULL);

if (!mmap_host || !mmap_log) {
T_ERR_NL("if mmap is enabled in access log, there have to be mmap_host and mmap_log options\n");
return -EINVAL;
}
}

return 0;
}

Expand All @@ -620,6 +648,11 @@ tfw_access_log_start(void)
static void
tfw_access_log_stop(void)
{
if (tfw_logger_config_path) {
kfree(tfw_logger_config_path);
tfw_logger_config_path = NULL;
}

tfw_mmap_buffer_free(mmap_buffer);
mmap_buffer = NULL;
}
Expand Down
71 changes: 49 additions & 22 deletions scripts/tempesta.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,19 +126,30 @@ templater()
fi
done < "$tfw_cfg_path"

opts=$(get_opts "$cfg_content" "access_log")
while read -r line; do
if [ $(opt_exists "$line" "mmap"; echo $?) -ne 0 ]; then
tfw_logger_should_start=1
mmap_log=$(get_opt_value "$line" "mmap_log")
mmap_host=$(get_opt_value "$line" "mmap_host")
mmap_user=$(get_opt_value "$line" "mmap_user")
mmap_password=$(get_opt_value "$line" "mmap_password")

[[ -n "$mmap_log" && -n "$mmap_host" ]] ||
error "if mmaps enabled in access log, there have to be mmap_host and mmap_log options"
fi
done <<< "$opts"
tfw_logger_config_path=$(echo "$cfg_content" | grep -oP 'access_log\s+.*logger_config=\K[^;]*' | sed 's/"//g' | sed "s/'//g")
if [ -n "$tfw_logger_config_path" ]; then
tfw_logger_should_start=1
else
opts=$(get_opts "$cfg_content" "access_log")
while read -r line; do
if [ $(opt_exists "$line" "mmap"; echo $?) -ne 0 ]; then
tfw_logger_should_start=1
mmap_log=$(get_opt_value "$line" "mmap_log")
mmap_host=$(get_opt_value "$line" "mmap_host")
mmap_user=$(get_opt_value "$line" "mmap_user")
mmap_password=$(get_opt_value "$line" "mmap_password")

[[ -n "$mmap_log" && -n "$mmap_host" ]] ||
error "if mmaps enabled in access log, there have to be mmap_host and mmap_log options"
fi
done <<< "$opts"
fi

# Check for buffer size
buffer_size=$(echo "$cfg_content" | grep -oP 'mmap_log_buffer_size\s+\K[^;]*')
if [ -n "$buffer_size" ]; then
mmap_log_buffer_size=$buffer_size
fi

cfg_content=$(remove_opts_by_mask "$cfg_content" "mmap_")

Expand Down Expand Up @@ -311,14 +322,29 @@ start_tfw_logger()
return
fi

if [ -z "$mmap_host" ] || [ -z "$mmap_log" ]; then
error "You need to specify 'mmap_host' and 'mmap_log' "`
`"if access_log mmap was specified"
return
fi
# Check if we have a JSON config path specified
if [ -n "$tfw_logger_config_path" ]; then
echo "...starting tfw_logger with JSON config: $tfw_logger_config_path"
"$utils_path/tfw_logger" --config="$tfw_logger_config_path" || \
error "cannot start tfw_logger daemon"
else
# Traditional approach with command line parameters
if [ -z "$mmap_host" ] || [ -z "$mmap_log" ]; then
error "You need to specify 'mmap_host' and 'mmap_log' if access_log mmap was specified"
return
fi

echo "...starting tfw_logger with command line arguments"
cmd_args="--clickhouse-host=$mmap_host --log-path=$mmap_log"

"$utils_path/tfw_logger" -H "$mmap_host" -l "$mmap_log" -u "$mmap_user" -p "$mmap_password" ||
# Add optional parameters
[ -n "$mmap_user" ] && cmd_args="$cmd_args --clickhouse-user=$mmap_user"
[ -n "$mmap_password" ] && cmd_args="$cmd_args --clickhouse-password=$mmap_password"
[ -n "$mmap_log_buffer_size" ] && cmd_args="$cmd_args --buffer-size=$mmap_log_buffer_size"

"$utils_path/tfw_logger" $cmd_args || \
error "cannot start tfw_logger daemon"
fi

start_time=$(date +%s)
while [[ ! -f "$tfw_logger_pid_path" ]]; do
Expand All @@ -329,18 +355,19 @@ start_tfw_logger()
sysctl -e -w net.tempesta.state=stop
unload_modules
tfw_irqbalance_revert
error "tfw_logger failed to start, see $mmap_log for details"
error "tfw_logger failed to start, see logs for details"
fi

sleep 0.1
done

}

stop_tfw_logger()
{
if [ -e $tfw_logger_pid_path ]; then
"$utils_path/tfw_logger" -s
echo "...stopping tfw_logger"
"$utils_path/tfw_logger" --stop || \
echo "Warning: Failed to stop tfw_logger daemon gracefully"
fi
}

Expand Down
4 changes: 2 additions & 2 deletions utils/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ CLICKHOUSE_BUILD_DIR := $(CLICKHOUSE_DIR)/build
CLICKHOUSE_REPO_URL := https://github.com/ClickHouse/clickhouse-cpp.git

CXX := g++
LDFLAGS := -lfmt -lboost_program_options
LDFLAGS := -lfmt -lboost_program_options -lboost_filesystem
LDFLAGS += -lclickhouse-cpp-lib -L $(CLICKHOUSE_BUILD_DIR)/clickhouse
LDFLAGS += -lcityhash -L $(CLICKHOUSE_BUILD_DIR)/contrib/cityhash/cityhash
LDFLAGS += -lzstdstatic -L $(CLICKHOUSE_BUILD_DIR)/contrib/zstd/zstd
Expand All @@ -33,7 +33,7 @@ CXXFLAGS := -O3 -Wall -Wextra -std=c++23 -DPAGE_SIZE=$(PAGE_SIZE)
CXXFLAGS += $(shell pkgconf -cflags spdlog)
CXXFLAGS += -I $(CLICKHOUSE_DIR)
CXXFLAGS += -I $(CLICKHOUSE_DIR)/contrib/absl
SRCS := tfw_logger.cc mmap_buffer.cc clickhouse.cc
SRCS := tfw_logger.cc mmap_buffer.cc clickhouse.cc tfw_logger_config.cc
OBJS := $(SRCS:.cc=.o)
TARGET := tfw_logger

Expand Down
Loading