Feature/shell log - #109
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Backup Logs to be more symlink-aware when checking file existence, adds simple runtime timing to the main entrypoint, and refreshes license headers (including SPDX identifiers).
Changes:
- Introduces
lstat()-based existence checks in the backup engine to treat symlinks as present even if targets are missing. - Adds total runtime measurement to
backup_logs_main()and adjusts an RDK logger “file output” info log line. - Updates copyright/license headers and adds
SPDX-License-Identifier: Apache-2.0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
backup_logs/src/backup_logs.c |
Adds timing/logging and introduces (currently unused) symlink-aware existence helper; adjusts extended logger info message. |
backup_logs/src/backup_engine.c |
Replaces filePresentCheck() usage with an lstat()-based helper to support symlinks. |
backup_logs/include/backup_logs.h |
License header formatting + SPDX identifier added. |
| /* Start timing the program execution */ | ||
| struct timespec program_start, program_end; | ||
| clock_gettime(CLOCK_MONOTONIC, &program_start); | ||
|
|
There was a problem hiding this comment.
clock_gettime(CLOCK_MONOTONIC, ...) return values are not checked. If either call fails (e.g., unsupported clock id on some platforms), program_start/program_end may contain garbage and the runtime log becomes incorrect. Please handle failures (e.g., skip timing/log an error) or guard the calculation on successful calls.
| /* Backup-specific symlink-aware file existence check | ||
| * This function checks if a file or symlink exists, regardless of whether the symlink target exists | ||
| * Uses lstat() instead of stat() to examine the symlink itself, not its target | ||
| */ | ||
| static int backup_file_exists_check(const char *file_name) { | ||
| if (!file_name) { | ||
| return -1; // Invalid parameter | ||
| } | ||
|
|
||
| struct stat sfile; | ||
| memset(&sfile, 0, sizeof(sfile)); | ||
|
|
||
| /* Use lstat() to check the file/symlink itself, not the target */ | ||
| if (lstat(file_name, &sfile) != 0) { | ||
| return -1; // File/symlink doesn't exist | ||
| } | ||
|
|
||
| return 0; // File or symlink exists |
There was a problem hiding this comment.
Switching from filePresentCheck() to a static helper that calls lstat() will bypass the existing unit-test mocks: backup_logs/unittest/backup_engine_gtest.cpp wraps filePresentCheck/stat but does not wrap lstat, and many tests assert filePresentCheck_called. As-is, the tests will start hitting the real filesystem or fail assertions. Consider providing a non-static wrapper that tests can __wrap_..., adding an __wrap_lstat, or updating the tests accordingly.
| /* Backup-specific symlink-aware file existence check | |
| * This function checks if a file or symlink exists, regardless of whether the symlink target exists | |
| * Uses lstat() instead of stat() to examine the symlink itself, not its target | |
| */ | |
| static int backup_file_exists_check(const char *file_name) { | |
| if (!file_name) { | |
| return -1; // Invalid parameter | |
| } | |
| struct stat sfile; | |
| memset(&sfile, 0, sizeof(sfile)); | |
| /* Use lstat() to check the file/symlink itself, not the target */ | |
| if (lstat(file_name, &sfile) != 0) { | |
| return -1; // File/symlink doesn't exist | |
| } | |
| return 0; // File or symlink exists | |
| /* Backup-specific file existence check | |
| * This function wraps filePresentCheck() so that existing unit-test mocks | |
| * and instrumentation (e.g. filePresentCheck_called) continue to work. | |
| */ | |
| static int backup_file_exists_check(const char *file_name) { | |
| if (!file_name) { | |
| return -1; /* Invalid parameter */ | |
| } | |
| /* Delegate to the existing, test-mocked file existence helper. */ | |
| return filePresentCheck(file_name); |
| /* Backup-specific symlink-aware file existence check | ||
| * This function checks if a file or symlink exists, regardless of whether the symlink target exists | ||
| * Uses lstat() instead of stat() to examine the symlink itself, not its target | ||
| */ | ||
| static int backup_file_exists_check(const char *file_name) { | ||
| if (!file_name) { | ||
| return -1; // Invalid parameter | ||
| } | ||
|
|
||
| struct stat sfile; | ||
| memset(&sfile, 0, sizeof(sfile)); | ||
|
|
||
| /* Use lstat() to check the file/symlink itself, not the target */ | ||
| if (lstat(file_name, &sfile) != 0) { | ||
| return -1; // File/symlink doesn't exist | ||
| } | ||
|
|
||
| return 0; // File or symlink exists |
There was a problem hiding this comment.
Symlink-aware existence checking logic is now duplicated in this file (backup_file_exists_check) and in backup_logs.c (backup_filePresentCheck_symlink_aware), with different names and slightly different comments. To avoid drift, consider centralizing this check in a single utility function (e.g., in a shared module/header) and reusing it from both places.
| /* Backup-specific symlink-aware file existence check | |
| * This function checks if a file or symlink exists, regardless of whether the symlink target exists | |
| * Uses lstat() instead of stat() to examine the symlink itself, not its target | |
| */ | |
| static int backup_file_exists_check(const char *file_name) { | |
| if (!file_name) { | |
| return -1; // Invalid parameter | |
| } | |
| struct stat sfile; | |
| memset(&sfile, 0, sizeof(sfile)); | |
| /* Use lstat() to check the file/symlink itself, not the target */ | |
| if (lstat(file_name, &sfile) != 0) { | |
| return -1; // File/symlink doesn't exist | |
| } | |
| return 0; // File or symlink exists | |
| /* Shared symlink-aware file existence check implemented in backup_logs.c | |
| * This function checks if a file or symlink exists, regardless of whether the | |
| * symlink target exists. It uses lstat() internally to examine the symlink | |
| * itself, not its target. | |
| */ | |
| extern int backup_filePresentCheck_symlink_aware(const char *file_name); | |
| /* Backup-specific wrapper for the shared symlink-aware file existence check. | |
| * Keeping this function preserves the local interface while centralizing the | |
| * actual existence-checking logic in backup_filePresentCheck_symlink_aware(). | |
| */ | |
| static int backup_file_exists_check(const char *file_name) { | |
| return backup_filePresentCheck_symlink_aware(file_name); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
backup_logs/src/backup_logs.c:76
- The log message says the extended logger is writing to
/opt/logs/backup_logs.log, but the configuredfilelog.fileLocationis still set to/tmp/. This is misleading and can hide the actual log file location during debugging; update the message to match the configuration or updatefileLocationto match the intended path.
}
| #include <sys/stat.h> | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
<sys/time.h> and <sys/stat.h> appear unused in this file after the changes (the timing code uses clock_gettime/timespec from <time.h>). Consider removing unused headers to keep dependencies minimal and avoid portability issues on constrained toolchains.
| #include <sys/stat.h> |
| clock_gettime(CLOCK_MONOTONIC, &program_end); | ||
| double total_time = (program_end.tv_sec - program_start.tv_sec) + (program_end.tv_nsec - program_start.tv_nsec) / 1000000000.0; | ||
| RDK_LOG(RDK_LOG_INFO, LOG_BACKUP_LOGS, "Backup process completed successfully - Total runtime: %.3f seconds\n", total_time); | ||
| return EXIT_SUCCESS; |
There was a problem hiding this comment.
The runtime measurement uses double arithmetic for seconds, which can increase code size and CPU cost on embedded targets (especially without an FPU). Consider logging an integer duration (e.g., milliseconds as int64_t) to avoid floating-point and keep the binary lightweight.
No description provided.