os/kernel/crashlog_writer: Add crash log flash save #6899

Kimhyunjong121 · 2025-07-24T10:16:25Z

Developed a new feature to store logs during assert situations. It only operates when a crash occurs in the user space.

The following modifications are required:

Avoid using double vsnprintf.
Check available space in the file system before wrting.
Check available heap memory before compression.
Store compressed data in the heap instead of the data section.
Add a unique identifier or delimiter to the file name.
Implement a retry count mechanism.
Ensure logs from functions like __alert() are also captured.

ewoodev · 2025-07-28T01:31:33Z

os/kernel/log_dump/log_dump.c

+static unsigned char compressed_buf[ASSERT_LOG_BUF_SIZE]; 
+static unsigned int compressed_buf_size;
+static char assert_log[ASSERT_LOG_BUF_SIZE];
+static int assert_situation=0;
+static int assert_log_index=0;
+static int log_len=0;
+
+/* Structures used to decompress and print compressed log data*/
+static char tm[ASSERT_LOG_BUF_SIZE];


It appears that this feature is not utilizing the same buffer as the existing log_dump.
In that case, please proceed with adding a completely new file.

ewoodev · 2025-07-28T04:49:17Z

os/include/tinyara/log_dump/log_dump_internal.h

+ *   This is used to store logs that are output via lldbg() during a user crash situation into a buffer.
+ *
+ ****************************************************************************/
+void assert_log_to_buffer(FAR const char *fmt, va_list ap);


which code is calling this?

ewoodev · 2025-07-28T04:51:38Z

os/arch/arm/src/armv7-a/arm_assert.c

+#ifdef CONFIG_BINMGR_RECOVERY
+	if (IS_FAULT_IN_USER_SPACE(asserted_location)) {
+		/* start assert log saving */
+		set_assert_situation();


our crash code is started on fault isr like arm_dataabort.c
the fault isr logs also matter.

And do we need to make start log saving point?

ewoodev · 2025-07-28T04:54:50Z

os/kernel/binary_manager/binary_manager_recovery.c

+	set_notassert_situation();
+	assert_log_to_file();
+	assert_log_compress();
+	compressed_assert_log_to_file();
+	compressed_assert_log_read();
+	decompress_and_print();


the functions are calling here only,
we don't need to call theae seperatly.

How about call onec api here?

log_dump_crash_save_to_file()

ewoodev · 2025-07-28T05:08:04Z

os/kernel/log_dump/log_dump.c

+void assert_log_compress(){
+
+	lldbg("\n\n================compress start==============\n\n");
+	compressed_buf_size=ASSERT_LOG_BUF_SIZE;
+	compress_block(compressed_buf,&compressed_buf_size,assert_log,ASSERT_LOG_BUF_SIZE);
+	lldbg("compressed buf size: %d\n",compressed_buf_size);
+	lldbg("\n====================compress end =====================\n");
+
+}


The code is exculed during crash recovery.

So, we need check time for compress.
If It is long, not compress is better.

for it, How about make configuration compress or not.

I conducted an experiment where, after printing the entire log, I measured the time it takes to save the log to a file using clock(). The measurement was repeated multiple times.

When the log size was 5370 bytes:
-With compression-
run run1 run2 run3 run4 run5 mean
time(s) 0.09 0.129 0.042 0.091 0.134 0.0972
compressed size: 1638 bytes

-Without compression-
run run1 run2 run3 run4 run5 mean
time(s) 0.037 0.033 0.082 0.357 0.034 0.095

When the log size was 27971 bytes (It was randomly generated using rand()):
-With compression-
run run1 run2 run3 run4 run5 mean
time(s) 0.046 0.374 0.046 0.136 0.093 0.139
comp size(bytes) 3596 4483 3523 3593 3591

-Without compression-
run run1 run2 run3 run4 run5 mean
time(s) 0.883 0.355 0.58 0.269 0.079 0.256

When the log size is small, the time taken is similar whether compression is used or not. However, as the log size increases, using compression becomes faster overall. It seems that the time spent on writing dominates more than the time spent on compression. Therefore, it may not be necessary to create a separate configuration for this.

ewoodev · 2025-07-28T05:08:54Z

os/kernel/binary_manager/binary_manager_recovery.c

+	compressed_assert_log_read();
+	decompress_and_print();
 	/* Create loader to reload binary */
 	ret = binary_manager_execute_loader(LOADCMD_RELOAD, bin_idx);


we need to make option to system reboot here.

ewoodev

Please check coding rule

Taejun-Kwon · 2025-07-28T05:59:54Z

os/kernel/log_dump/log_dump.c

+		return -1;
+	}
+	lldbg("file open success\n");
+	int log_size=write(fd,assert_log,strlen(assert_log));


below is better code.

int len = strlen(assert_log); int ret = write(fd, assert_log, len); close(fd); if (ret != len) { lldbg("file write failed ret : %d len : %d errno : %d\n", ret, len, errno); return -1; }

Taejun-Kwon · 2025-07-28T06:00:27Z

os/kernel/log_dump/log_dump.c

+	int ret=write(fd,compressed_buf,compressed_buf_size);
+	if(ret<0){
+		lldbg("file write failed\n");
+		return -1;


file should be closed here.

seokhun-eom24 · 2025-07-29T00:35:24Z

os/kernel/log_dump/log_dump.c

+}
+int compressed_assert_log_to_file(){
+	lldbg("\n\n================compress file write start==============\n\n");
+	int fd=open("/mnt/ssert_logsave_zip",O_WRONLY|O_CREAT|O_TRUNC);


Use macro for /mnt/ssert_logsave_zip would be better.

seokhun-eom24 · 2025-07-29T00:36:54Z

os/kernel/log_dump/log_dump.c

+}
+int set_assert_situation(){
+	lldbg("\n\n\n===============================FROM THIS, lldbg LOG WILL BE SAVED!!================================\n\n\n\n");
+	assert_situation=1;


You can use assert_situation = true;

seokhun-eom24 · 2025-07-29T00:37:48Z

os/include/syslog.h

 */
 int lowvsyslog(int priority, FAR const char *format, va_list ap);

+


Unnecessary file changes

Developed a new feature to store logs during assert situations. It only operates when a crash occurs in the user space. The following modifications are required: 1. Avoid using double vsnprintf. -> resolved 2. Check available space in the file system before wrting. 3. Check available heap memory before compression. 4. Store compressed data in the heap instead of the data section. -> resolved 5. Add a unique identifier or delimiter to the file name. -> resolved 6. Implement a retry count mechanism. -> resolved 7. Ensure logs from functions like __alert() are also captured. 8. Allows selecting wheter to reboot or reload after saving the log. -> resolved

On top of the existing functionality: 1. Added the ability to record the time taken to save the log into a file. 2. Improved code readability. 3. Minimized the use of global variables. 4. Added functionality to read logs saved in a file.

seokhun-eom24 · 2025-07-30T10:26:48Z

os/include/tinyara/crashlog_writer/crashlog_writer.h

@@ -0,0 +1,57 @@
+/****************************************************************************
+ *
+ * Copyright 2022 Samsung Electronics All Rights Reserved.


2022 -> 2025.
Apply it to other new files as well.

seokhun-eom24 · 2025-07-30T10:32:55Z

os/kernel/crashlog_writer/Kconfig

+    default y
+    ---help---
+        Controls whether compression is applied.
+        if the value is 0 compression is not performed.


It should be n instead of 0 since it's a boolean value.

seokhun-eom24 · 2025-07-30T10:46:43Z

os/kernel/crashlog_writer/crashlog_writer.c

+		if (ret < 0) {
+			lldbg("read failed\n");
+			free(compressed_buf);
+			return -1;


You can use ERROR instead of -1. OK for 0.

// defined in <sys/types.h> #define ERROR -1 #define OK 0

jeongarmy · 2025-07-31T00:33:12Z

os/kernel/crashlog_writer/crashlog_writer.c

+static int g_start_to_save = 0;
+static int g_crash_log_index = 0;
+static char g_filename[MAX_FILENAME_LEN];
+#ifndef REBOOT_AFTER_LOG_SAVE


CONFIG_ is missing

jeongarmy · 2025-07-31T00:35:00Z

os/kernel/crashlog_writer/crashlog_writer.c

+static int is_valid_filename(const char *name, int *out_number) 
+{
+    char expected_prefix[16];
+    snprintf(expected_prefix, sizeof(expected_prefix), "crashlog_");


Please define and use prefix value
#define XXX_PREFIX "crashlog_"

jeongarmy · 2025-07-31T00:39:58Z

os/kernel/binary_manager/binary_manager_recovery.c

 		goto reboot_board;
 	}
+#endif
+#ifdef CONFIG_CRASH_LOG_SAVE


Please use defined values, not hard-coded values for better readability.

#define XXXX_ENABLE 1 #define XXXX_DISABLE 0

jeongarmy · 2025-07-31T01:09:40Z

os/kernel/crashlog_writer/crashlog_writer.c

+*   Saves the crash log to a file. 
+*   If the flag is 1, the log is saved in compressed form; if it is 0, it is saved uncompressed.
+*************************************************************************************/
+char *save_crash_log(int flag)


If flag is used for compression only, we'd better to change it to bool compression.
If there will be more options for logging, we can support this with flag setting like below.
For examples,

#define FLAGS_COMPRESS (1 << 0)
#define FLAGS_XXXXX (1 << 1)

caller
flag |= FLAGS_COMPRESS;
save_crash_log(flag);

save_crash_log(int flag)
{

if (sem->flags & FLAGS_COMPRESS) {
ret = crash_log_compress();
} else {
ret = crash_log_to_file();
}
}

jeongarmy · 2025-07-31T01:10:48Z

os/kernel/crashlog_writer/crashlog_writer.c

+		char buf[100];
+#ifdef CONFIG_COMPRESSION_FLAG
+		snprintf(buf, sizeof(buf), "C: %d -> %d ", log_size, ret);
+		write(fd, buf, strlen(buf));


Please add result checking and error handling.

ewoodev · 2025-07-30T05:58:31Z

os/arch/arm/src/armv7-a/arm_assert.c

+#ifdef CONFIG_CRASH_LOG_SAVE
+#ifdef CONFIG_BINMGR_RECOVERY


Let's check CONFIG_CRASH_LOG_SAVE only here.

you already made dependency BINMGR_RECOVERY

ewoodev · 2025-07-31T03:26:23Z

os/kernel/crashlog_writer/crashlog_writer.c

+		decompress_block(buf, &tmp_buf_size, compressed_buf, &compressed_buf_size);
+		free(compressed_buf);
+		return tmp_buf_size;
+	}


we need to remove log file after read

ewoodev · 2025-07-31T03:51:04Z

os/kernel/binary_manager/binary_manager_recovery.c

+#ifdef CONFIG_COMPRESSION_FLAG
+	save_crash_log(1);
+#else
+	save_crash_log(0);
+#endif


If we select compress or not using CONFIG_COMPRESSION_FLAG, we don't need to have dynamic check and branch compress or not.
but, if it is for checking memory not enough state, we need to have branch dynamically.

and binary manager don't need to know about commpress or not, Let's move these into save_crash_log

void save_crash_log() { #ifdef CONFIG_CRASH_LOG_COMPRESS /* check memory largest chunk */ if (memory enough) { /* compress log */ /* saving compressed log into fs */ } #ifdef CONFIG_CRASH_LOG_SUPPORT_MEMORY_FULL else { /* just saving log into fs */ } #endif #else /* just saving log into fs */ #endif }

ewoodev · 2025-07-31T04:00:39Z

lib/libc/stdio/lib_lowoutstream.c

 	DEBUGASSERT(this);
+#ifdef __KERNEL__
+#ifdef CONFIG_CRASH_LOG_SAVE
+	crash_log_to_buffer(ch);


This feature covers the storage of all logs output in lldbg as well as crash logs

This feature supports log saving to fs with compress.

How about module naming like ll_log_dump?

ewoodev · 2025-07-31T05:49:57Z

os/arch/arm/src/armv7-a/arm_assert.c

+#ifdef CONFIG_BINMGR_RECOVERY
+	if (IS_FAULT_IN_USER_SPACE(asserted_location)) {
+		/* start assert log saving */
+		set_store_to_buffer_flag(1);


the fault handler start point might not be here. like arm_dataabort.c
the fault function also have log, the info matters

so in my opinion, we need to set default save on, and we need to stop and start between specific section menually

1. Refactored function and config names for consistency. 2. Updated log file naming convention. 3. Added functionality to read the most recent log file.

ewoodev · 2025-07-31T05:53:49Z

lib/libc/stdio/lib_lowoutstream.c

+#ifdef __KERNEL__
+#ifdef CONFIG_CRASH_LOG_SAVE


How about below

#if defined(__KERNEL__) && defined(CONFIG_CRASH_LOG_SAVE) void crash_log_to_buffer(ch); #else #define crash_log_to_buffer(ch) (void)

This makes it possible to specify in the api header that it can only be used in kernel.

The contents of os/kernel/crash_log have been moved to os/kernel/log_dump

ewoodev · 2025-08-07T07:15:15Z

os/kernel/log_dump/lowlog_dump_save_ch.c

+void lowlog_dump_init_buf()
+{
+	for (int i = 0; i < CONFIG_LOWLOG_DUMP_BUF_SIZE; i++) {
+		g_lowlog[i] = '\0';
+	}
+	g_lowlog_index = 0;
+}


do we need this? the bss is already 0.

if clear is needed, let's do in save to flash

ewoodev · 2025-08-07T07:17:12Z

os/kernel/log_dump/lowlog_dump_save.c

+#endif
+#endif
+	return OK;
+}


Please add end of line

ewoodev · 2025-08-07T07:22:20Z

os/kernel/log_dump/lowlog_dump_save.c

+*************************************************************************************/
+static int lowlog_dump_save_uncomp(char *filenm)
+{
+	lldbg("\n\n\n===================================crash log file saving start==========================================\n\n\n");


the log is info, Let's replace to ldpllvdbg

ewoodev · 2025-08-07T07:22:44Z

os/kernel/log_dump/lowlog_dump_save.c

+	char new_filename[MAX_FILENAME_LEN];
+	int ret = lowlog_dump_get_next_filename(new_filename);
+	if (ret < 0) {
+		lldbg("get_next_filename() failed\n");


Let's replace ldplldbg

ewoodev · 2025-08-07T07:25:09Z

os/kernel/log_dump/lowlog_dump_save.c

+static int lowlog_dump_get_next_filename(char *new_filename) 
+{
+    DIR *dir = opendir(DIR_PATH);
+    if (!dir) {


the coding rule indentation Tab Size:4

…nfig - Removed lowlog_dump_set_flag.c - Added lowlog_dump_pause.c and lowlog_dump_resume.c - Redesigned logging behavior: lldbg logs between pause/resume are no longer saved - Removed retry count config - Replaced with CONFIG_LOWLOG_DUMP_MAX_FILES

All lldbg() calls have been updated to ldpllvdbg() Added TODO comments where further improvements or context handling may be needed.

ewoodev · 2025-08-07T09:02:48Z

os/arch/arm/src/armv7-a/arm_dataabort.c

 #ifdef CONFIG_PAGING
 uint32_t *arm_dataabort(uint32_t *regs, uint32_t dfar, uint32_t dfsr)
-{
+{	


please revert unnecessary change

ewoodev · 2025-08-07T09:02:53Z

os/arch/arm/src/armv7-a/arm_assert.c

 	} else {
 		asserted_location = kernel_assert_location;
 	}
-


please revert unnecessary change

ewoodev · 2025-08-07T09:04:23Z

os/arch/arm/src/armv7-a/arm_dataabort.c

-{
+{	
+#ifdef CONFIG_LOWLOG_DUMP
+	lowlog_dump_set_store_flag(1);


it looks missed remove, pelease check

ewoodev · 2025-08-07T09:05:57Z

os/kernel/log_dump/lowlog_dump_pause.c

+*************************************************************************************/
+void lowlog_dump_pause(void)
+{
+	ldpllvdbg("\n\n\n==========================================================TO THIS!!==========================================================\n\n\n\n");


what's mean TO THIS???

ewoodev · 2025-08-07T09:06:51Z

os/arch/arm/src/armv7-a/arm_dataabort.c

-{
+{	
+#ifdef CONFIG_LOWLOG_DUMP
+	lowlog_dump_set_store_flag(1);


do we need this here?

ewoodev · 2025-08-07T09:06:54Z

os/kernel/log_dump/lowlog_dump.h

+/****************************************************************************
+ * Function Prototypes
+ ****************************************************************************/
+int lowlog_dump_get_retry_count(void);


it looks not be removed.

ewoodev · 2025-08-07T09:06:55Z

os/kernel/log_dump/lowlog_dump.h

+/****************************************************************************
+ * Function Prototypes
+ ****************************************************************************/
+int lowlog_dump_get_retry_count(void);


it looks not removed.

ewoodev · 2025-08-07T09:17:04Z

os/arch/arm/src/armv7-a/arm_assert.c

 	if (IS_FAULT_IN_USER_SPACE(asserted_location)) {
 		/* Recover user fault through binary manager */
 		binary_manager_recover_userfault();
+		state = NORMAL_STATE; 


Please revert unnecessary change

Remove unnecessary code

ewoodev · 2025-08-25T23:09:11Z

os/kernel/log_dump/lowlog_dump.h

+#include <stdint.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+#include <debug.h>


Please remove unnecessary including header

ewoodev · 2025-08-25T23:20:16Z

os/kernel/binary_manager/binary_manager_recovery.c

+#ifdef CONFIG_LOWLOG_DUMP
+	lowlog_dump_save();
+#ifdef CONFIG_LOWLOG_DUMP_REBOOT
+	binary_manager_reset_board(REBOOT_SYSTEM_BINARY_RECOVERYFAIL);


Reboot Reason should not be overwritten here.
in the time, we already writed reason reason about cause of crash.

/* Do not overwrite reboot reason here */ boardctl(BOARDIOC_RESET, EXIT_SUCCESS);

ewoodev · 2025-08-25T23:25:43Z

os/kernel/log_dump/lowlog_dump_ch.c

+	if (lowlog_dump_get_store_flag() && g_lowlog_index < CONFIG_LOWLOG_DUMP_BUF_SIZE) {
+		g_lowlog[g_lowlog_index++] = ch;
+	}


In the previous review, we decided to turn on the log save from the start of booting.
this is because we need to call the start function to all asset occurrence codes in order to turn on log storage in case of asset occurrence.

However, for this to work properly, the g_lowlog buffer must be implemented as a ring buffer.
This is already full due to boot log, so it will not be able to save the assert log.

Suggested change

if (lowlog_dump_get_store_flag() && g_lowlog_index < CONFIG_LOWLOG_DUMP_BUF_SIZE) {

g_lowlog[g_lowlog_index++] = ch;

}

if (lowlog_dump_get_store_flag()) {

g_lowlog[g_lowlog_tail] = ch;

g_lowlog_tail = (g_lowlog_tail + 1) & BUFFER_MASK;

if (g_lowlog_tail == g_lowlog_head) {

g_lowlog_head = (g_lowlog_head + 1) & BUFFER_MASK;

}

}

ewoodev · 2025-08-25T23:32:36Z

os/kernel/log_dump/lowlog_dump_pause.c

+void lowlog_dump_init()
+{
+    lowlog_dump_init_buf();
+    lowlog_dump_init_flag();
+}


the function is not called anywhere
If it is unnecessary, please remove.

ewoodev · 2025-08-25T23:32:52Z

os/kernel/log_dump/lowlog_dump_read.c

+	ldpllvdbg("most_recent_file_name is %s\n",most_recent_file_name);
+	lowlog_dump_read(most_recent_file_name, buf, buf_size);
+    return OK;
+}


please add last line

ewoodev · 2025-08-26T00:08:46Z

os/kernel/log_dump/lowlog_dump_save.c

+    int pre_filename_comp_len = strlen(PRE_FILENAME_COMP);
+    int pre_filename_uncomp_len = strlen(PRE_FILENAME_UNCOMP);
+    int name_len = strlen(name);
+    if (name_len > pre_filename_comp_len && !strncmp(name, PRE_FILENAME_COMP, pre_filename_comp_len)) {
+        for (int i = pre_filename_comp_len; i < name_len; i++) {
+            if (!isdigit(name[i])) {
+                return ERROR;
+            }
+        }
+        *out_number = atoi(name + pre_filename_comp_len);
+        return OK;
+    }
+    else if (name_len > pre_filename_uncomp_len && !strncmp(name, PRE_FILENAME_UNCOMP, pre_filename_uncomp_len)) {
+        for (int i = pre_filename_uncomp_len; i < name_len; i++) {
+            if (!isdigit(name[i])) {
+                return ERROR;
+            }
+        }
+        *out_number = atoi(name + pre_filename_uncomp_len);
+        return OK;
+    }


#ifdef CONFIG_LOWLOG_DUMP_COMPRESS ret = lowlog_dump_compress_and_save(filenm); #else ret = lowlog_dump_save_uncomp(filenm); #endif

If compression is determined by CONFIG_LOWLOG_DUMP_COMPRESS, we do not need to check both of these here.

Suggested change

int pre_filename_comp_len = strlen(PRE_FILENAME_COMP);

int pre_filename_uncomp_len = strlen(PRE_FILENAME_UNCOMP);

int name_len = strlen(name);

if (name_len > pre_filename_comp_len && !strncmp(name, PRE_FILENAME_COMP, pre_filename_comp_len)) {

for (int i = pre_filename_comp_len; i < name_len; i++) {

if (!isdigit(name[i])) {

return ERROR;

}

}

*out_number = atoi(name + pre_filename_comp_len);

return OK;

}

else if (name_len > pre_filename_uncomp_len && !strncmp(name, PRE_FILENAME_UNCOMP, pre_filename_uncomp_len)) {

for (int i = pre_filename_uncomp_len; i < name_len; i++) {

if (!isdigit(name[i])) {

return ERROR;

}

}

*out_number = atoi(name + pre_filename_uncomp_len);

return OK;

}

#ifdef CONFIG_LOWLOG_DUMP_COMPRESS

#define PRE_FILENAME "lowlog_dump_comp"

#else

#define PRE_FILENAME "lowlog_dump"

#endif

int pre_filename_len = strlen(PRE_FILENAME);

int name_len = strlen(name);

if (name_len > pre_filename_len && !strncmp(name, PRE_FILENAME, pre_filename_len)) {

for (int i = pre_filename_len; i < name_len; i++) {

if (!isdigit(name[i])) {

return ERROR;

}

}

*out_number = atoi(name + pre_filename_len);

return OK;

}

ewoodev · 2025-08-26T00:09:27Z

os/kernel/log_dump/lowlog_dump_save.c

+#ifdef CONFIG_LOWLOG_DUMP_COMPRESS
+	snprintf(tmp, MAX_FILENAME_LEN - path_len - 1, "%s%d", PRE_FILENAME_COMP, max_number + 1);
+#else
+	snprintf(tmp, MAX_FILENAME_LEN - path_len - 1, "%s%d", PRE_FILENAME_UNCOMP, max_number + 1);
+#endif


please check my above comment

ewoodev · 2025-08-26T00:33:20Z

os/kernel/log_dump/lowlog_dump_save.c

+static int lowlog_dump_save_uncomp(char *filenm)
+{
+	ldpllvdbg("\n\n\n===================================crash log file saving start==========================================\n\n\n");
+	char new_filename[MAX_FILENAME_LEN];
+	int ret = lowlog_dump_get_next_filename(new_filename);
+	if (ret < 0) {
+		ldpllvdbg("get_next_filename() failed\n");
+		return ERROR;
+	}
+	int fd = open(new_filename, O_WRONLY | O_CREAT | O_TRUNC);
+	if (fd < 0) {
+		ldpllvdbg("file open failed\n");
+		return ERROR;
+	}
+	int log_size = write(fd, lowlog_dump_get_buf(), lowlog_dump_get_size());
+	close(fd);
+	if (log_size < 0) {
+		ldpllvdbg("file write failed\n");
+		return ERROR;
+	}
+	snprintf(filenm, MAX_FILENAME_LEN, "%s", new_filename);
+	ldpllvdbg("file write success, count:%d\n", log_size);
+	ldpllvdbg("\n\n\n===========================================crash log file saving end=====================================================\n\n\n");
+	return log_size;
+}
+
+/*************************************************************************************
+* Name: lowlog_dump_save_comp
+*
+* Description: 
+*   Saves the compressed log from the buffer to a file.
+*************************************************************************************/
+static int lowlog_dump_save_comp(char *filenm, unsigned char *compressed_buf, unsigned int compressed_buf_size)
+{
+	ldpllvdbg("\n\n================================================================compress file write start===========================================================\n\n");
+	char new_filename[MAX_FILENAME_LEN];
+	if (compressed_buf == NULL) {
+		return ERROR;
+	}
+	int ret = lowlog_dump_get_next_filename(new_filename);
+	ldpllvdbg("filename is %s\n", new_filename);
+	if (ret < 0) {
+		ldpllvdbg("get_next_filename() failed\n");
+		return ERROR;
+	}
+	int fd = open(new_filename, O_WRONLY | O_CREAT | O_TRUNC);
+	if (fd < 0) {
+		ldpllvdbg("file open failed\n");
+		return ERROR;
+	}
+	ret = write(fd, compressed_buf, compressed_buf_size);
+	if (ret < 0) {
+		ldpllvdbg("file write failed\n");
+		close(fd);
+		return ERROR;
+	}
+	close(fd);
+	snprintf(filenm, MAX_FILENAME_LEN, "%s", new_filename);
+	ldpllvdbg("write count: %d\n", ret);
+	ldpllvdbg("\n\n==========================================================compress file write end===========================================================\n\n");
+	return ret;
+}


the two function are almost same

we can reduce duplicated code like below

static int lowlog_dump_save_to_file(char *log_buf, size_t size) { /* file save codes */ } #ifdef CONFIG_LOWLOG_DUMP_COMPRESS static int lowlog_dump_save_to_file_with_comp(char *log_buf, size_t size) { /* compress codes */ return lowlog_dump_save_to_file(compressed_buf, compressed_buf_size); } #endif #ifdef CONFIG_LOWLOG_DUMP_COMPRESS ret = lowlog_dump_save_to_file_with_comp(lowlog_dump_get_buf(), lowlog_dump_get_size()); #else ret = lowlog_dump_save_to_file(lowlog_dump_get_buf(), lowlog_dump_get_size()); #endif

ewoodev · 2025-08-26T00:34:28Z

os/kernel/log_dump/lowlog_dump_save.c

+static int lowlog_dump_compress_and_save(char *filenm)
+{
+	ldpllvdbg("\n\n=====================================================compress start=====================================================\n\n");
+    int malloc_size = lowlog_dump_get_size() / 3;


how did you decide compress buffer malloc size?

ewoodev · 2025-08-26T00:37:06Z

os/kernel/log_dump/lowlog_dump_save.c

+#ifdef CONFIG_LOWLOG_DUMP_RECORD_TIME
+	clock_t end = clock();
+	double elapsed_time = (double)(end - start) / CLOCKS_PER_SEC;
+	lldbg("\n\nElapsed time: %.6f seconds\n\n", elapsed_time);
+	char buf[MAX_FILENAME_LEN];
+	snprintf(buf, sizeof(buf), "%s/result", DIR_PATH);
+
+	int fd = open(buf, O_CREAT | O_WRONLY | O_APPEND);
+	if (fd < 0) {
+		return ERROR;
+	}
+//TODO : The number of snprintf and write calls should be reduced.
+//TODO : add result checking and error handling about snprintf and write.
+//TODO : result size should be limited.
+#ifdef CONFIG_LOWLOG_DUMP_COMPRESS
+	snprintf(buf, sizeof(buf), "C: %d -> %d ", log_size, ret);
+	write(fd, buf, strlen(buf));
+#else
+	snprintf(buf, sizeof(buf), "U: %d ", ret);
+	write(fd, buf, strlen(buf));
+#endif
+	snprintf(buf, sizeof(buf), "in %.6fs", elapsed_time);
+	write(fd, buf, strlen(buf));
+	snprintf(buf, sizeof(buf), " (%s)\n", filenm);
+	write(fd, buf, strlen(buf));
+	close(fd);
+#endif


This appears to be a debugging code for calculating log storage time.
If yes, please remove this code.

samsung-singh

Added crash log flash save in os kernel crashlog writer module, this change is good to merge.

ewoodev reviewed Jul 28, 2025

View reviewed changes

Taejun-Kwon reviewed Jul 28, 2025

View reviewed changes

seokhun-eom24 reviewed Jul 29, 2025

View reviewed changes

Kimhyunjong121 force-pushed the 7_24 branch 3 times, most recently from dba5e00 to 8cfcc29 Compare July 30, 2025 04:43

Kimhyunjong121 changed the title ~~os/kernel/log_dump: Add crash log flash save~~ os/kernel/crashlog_writer: Add crash log flash save Jul 30, 2025

Kimhyunjong121 added 2 commits July 30, 2025 17:21

Kimhyunjong121 force-pushed the 7_24 branch from 8cfcc29 to ebd15e6 Compare July 30, 2025 08:23

seokhun-eom24 reviewed Jul 30, 2025

View reviewed changes

jeongarmy reviewed Jul 31, 2025

View reviewed changes

ewoodev reviewed Jul 31, 2025

View reviewed changes

os/kernel/crashlog_writer: Modify crash log flash save

ce9c49f

1. Refactored function and config names for consistency. 2. Updated log file naming convention. 3. Added functionality to read the most recent log file.

ewoodev approved these changes Aug 1, 2025

View reviewed changes

os/kernel/log_dump: Add crash log flash save

5abfde1

The contents of os/kernel/crash_log have been moved to os/kernel/log_dump

Kimhyunjong121 force-pushed the 7_24 branch from 0c36b1e to 5abfde1 Compare August 5, 2025 06:16

ewoodev reviewed Aug 7, 2025

View reviewed changes

Kimhyunjong121 added 2 commits August 7, 2025 16:54

Replace all lldbg() calls with ldpllvdbg()

e424bf6

All lldbg() calls have been updated to ldpllvdbg() Added TODO comments where further improvements or context handling may be needed.

ewoodev reviewed Aug 7, 2025

View reviewed changes

Refactor: remove unnecessary committed

c618e14

Remove unnecessary code

ewoodev reviewed Aug 26, 2025

View reviewed changes

Kimhyunjong121 force-pushed the 7_24 branch from 5fff51f to c618e14 Compare September 10, 2025 08:45

samsung-singh approved these changes Oct 24, 2025

View reviewed changes

		*/
		int lowvsyslog(int priority, FAR const char *format, va_list ap);

-	if (lowlog_dump_get_store_flag() && g_lowlog_index < CONFIG_LOWLOG_DUMP_BUF_SIZE) {
-		g_lowlog[g_lowlog_index++] = ch;
-	}
+  if (lowlog_dump_get_store_flag()) {
+      g_lowlog[g_lowlog_tail] = ch;
+      g_lowlog_tail = (g_lowlog_tail + 1) & BUFFER_MASK;
+      if (g_lowlog_tail == g_lowlog_head) {
+          g_lowlog_head = (g_lowlog_head + 1) & BUFFER_MASK;
+      }
+  }

os/kernel/crashlog_writer: Add crash log flash save #6899

Are you sure you want to change the base?

os/kernel/crashlog_writer: Add crash log flash save #6899

Uh oh!

Conversation

Kimhyunjong121 commented Jul 24, 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!

ewoodev 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seokhun-eom24 Jul 30, 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!

Choose a reason for hiding this comment

Uh oh!

jeongarmy Jul 31, 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!

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!

seokhun-eom24 Jul 30, 2025 •

edited

Loading

jeongarmy Jul 31, 2025 •

edited

Loading

ewoodev Aug 25, 2025 •

edited

Loading