acassis commented on a change in pull request #3842:
URL: https://github.com/apache/incubator-nuttx/pull/3842#discussion_r645026965
##########
File path: drivers/syslog/syslog_filechannel.c
##########
@@ -50,6 +54,62 @@
FAR static struct syslog_channel_s *g_syslog_file_channel;
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+#ifdef CONFIG_SYSLOG_FILE_ROTATE
+static void log_rotate(FAR const char *log_file)
+{
+ int fd;
+ off_t size;
+ struct stat f_stat;
+ char * backup_file;
Review comment:
char *backup_file;
##########
File path: drivers/syslog/syslog_filechannel.c
##########
@@ -50,6 +54,62 @@
FAR static struct syslog_channel_s *g_syslog_file_channel;
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+#ifdef CONFIG_SYSLOG_FILE_ROTATE
+static void log_rotate(FAR const char *log_file)
+{
+ int fd;
+ off_t size;
+ struct stat f_stat;
+ char * backup_file;
+
+ /* Get the size of the current log file. */
+
+ fd = open(log_file, O_RDONLY);
+ if (fd < 0)
+ {
+ return;
+ }
+
+ fstat(fd, &f_stat);
+ size = f_stat.st_size;
+ close(fd);
+
+ /* If it does not exceed the limit we are OK. */
+
+ if (size < CONFIG_SYSLOG_FILE_SIZE_LIMIT)
+ {
+ return;
+ }
+
+ /* Construct the backup file name. */
+
+ backup_file = malloc(strlen(log_file) + 3);
+ if (backup_file == NULL)
+ {
+ return;
+ }
+
+ sprintf(backup_file, "%s.0", log_file);
+
+ /* Delete any old backup files. */
+
+ if (access(backup_file, F_OK) == 0)
+ {
+ remove(backup_file);
+ }
+
+ /* Rotate the log. */
+
+ rename(log_file, backup_file);
+
+ free(backup_file);
Review comment:
It is not rotating the log, but only creating a backup of the last log
file.
I proper rotating log should define how many log files the system will
support and it should always move all the previous log files to log.NNN + 1. It
could happen until the max log files supported is reached. When it happen the
oldest log file is deleted and replaced with oldest - 1.
If you don't want to implement it, I suggest rename the function from rotate
log to backup log, to avoid confusion :-)
##########
File path: drivers/syslog/syslog_filechannel.c
##########
@@ -50,6 +54,62 @@
FAR static struct syslog_channel_s *g_syslog_file_channel;
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+#ifdef CONFIG_SYSLOG_FILE_ROTATE
+static void log_rotate(FAR const char *log_file)
+{
+ int fd;
+ off_t size;
+ struct stat f_stat;
+ char * backup_file;
+
+ /* Get the size of the current log file. */
+
+ fd = open(log_file, O_RDONLY);
+ if (fd < 0)
+ {
+ return;
+ }
+
+ fstat(fd, &f_stat);
+ size = f_stat.st_size;
+ close(fd);
+
+ /* If it does not exceed the limit we are OK. */
+
+ if (size < CONFIG_SYSLOG_FILE_SIZE_LIMIT)
+ {
+ return;
+ }
+
+ /* Construct the backup file name. */
+
+ backup_file = malloc(strlen(log_file) + 3);
+ if (backup_file == NULL)
+ {
+ return;
+ }
+
+ sprintf(backup_file, "%s.0", log_file);
+
+ /* Delete any old backup files. */
+
+ if (access(backup_file, F_OK) == 0)
+ {
+ remove(backup_file);
+ }
+
+ /* Rotate the log. */
+
+ rename(log_file, backup_file);
+
+ free(backup_file);
Review comment:
It is not rotating the log, but only creating a backup of the last log
file.
A proper rotating log should define how many log files the system will
support and it should always move all the previous log files to log.NNN + 1. It
could happen until the max log files supported is reached. When it happen the
oldest log file is deleted and replaced with oldest - 1.
If you don't want to implement it, I suggest rename the function from rotate
log to backup log, to avoid confusion :-)
##########
File path: drivers/syslog/syslog_filechannel.c
##########
@@ -50,6 +54,62 @@
FAR static struct syslog_channel_s *g_syslog_file_channel;
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+#ifdef CONFIG_SYSLOG_FILE_ROTATE
+static void log_rotate(FAR const char *log_file)
+{
+ int fd;
+ off_t size;
+ struct stat f_stat;
+ char * backup_file;
+
+ /* Get the size of the current log file. */
+
+ fd = open(log_file, O_RDONLY);
+ if (fd < 0)
+ {
+ return;
+ }
+
+ fstat(fd, &f_stat);
+ size = f_stat.st_size;
+ close(fd);
+
+ /* If it does not exceed the limit we are OK. */
+
+ if (size < CONFIG_SYSLOG_FILE_SIZE_LIMIT)
+ {
+ return;
+ }
+
+ /* Construct the backup file name. */
+
+ backup_file = malloc(strlen(log_file) + 3);
+ if (backup_file == NULL)
+ {
+ return;
+ }
+
+ sprintf(backup_file, "%s.0", log_file);
+
+ /* Delete any old backup files. */
+
+ if (access(backup_file, F_OK) == 0)
+ {
+ remove(backup_file);
+ }
+
+ /* Rotate the log. */
+
+ rename(log_file, backup_file);
+
+ free(backup_file);
Review comment:
It is not rotating the log, but only creating a backup of the last log
file.
A proper rotating log should define how many log files the system will
support and it should always move all the previous log files to log.NNN + 1. It
could happen until the max log files supported is reached. When it happen the
oldest log file is deleted and replaced with oldest - 1.
If you don't want to implement it, I suggest renaming the function and the
Kconfig from rotate log to backup log, to avoid confusion :-)
##########
File path: drivers/syslog/syslog_filechannel.c
##########
@@ -50,6 +54,62 @@
FAR static struct syslog_channel_s *g_syslog_file_channel;
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+#ifdef CONFIG_SYSLOG_FILE_ROTATE
+static void log_rotate(FAR const char *log_file)
+{
+ int fd;
+ off_t size;
+ struct stat f_stat;
+ char * backup_file;
+
+ /* Get the size of the current log file. */
+
+ fd = open(log_file, O_RDONLY);
+ if (fd < 0)
+ {
+ return;
+ }
+
+ fstat(fd, &f_stat);
+ size = f_stat.st_size;
+ close(fd);
+
+ /* If it does not exceed the limit we are OK. */
+
+ if (size < CONFIG_SYSLOG_FILE_SIZE_LIMIT)
+ {
+ return;
+ }
+
+ /* Construct the backup file name. */
+
+ backup_file = malloc(strlen(log_file) + 3);
+ if (backup_file == NULL)
+ {
+ return;
+ }
+
+ sprintf(backup_file, "%s.0", log_file);
+
+ /* Delete any old backup files. */
+
+ if (access(backup_file, F_OK) == 0)
+ {
+ remove(backup_file);
+ }
+
+ /* Rotate the log. */
+
+ rename(log_file, backup_file);
+
+ free(backup_file);
Review comment:
> It is still a rotation, it just happens that there is only one
rotation available. :)
>
> Renaming this as "backup" would imply that this is a backup functionality.
Which is not.
> A backup would also keep the original file in place, while here it is
deleted.
>
> This can be very easily be extended to keep multiple rotations, instead of
only one.
> I have done this in the past, and I can easily add it to this
implementation.
>
> But I intentionally left it out. Do we really need so many log files?
> If you think so, I will add it tomorrow.
I think it should be useful for some system. Imagine a system were we keep
it in the field and after some amount to days (i.e. 30 days) you can go there
and get the complete log history.
I can merge this one and wait for your improvement over it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
