You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/02/03 20:11:55 UTC
[GitHub] [incubator-nuttx] pkarashchenko opened a new pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
pkarashchenko opened a new pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408
## Summary
The end of line (CR-LF sequence) is not detected correctly and file data are never synchronized when `syslog_dev_write` is called. This leads to a situation when log file on the SD card (FAT32 and maybe other FS) works not stable. With this change `syslog_file_channel()` now operates stable.
## Impact
`syslog_file_channel()` users.
## Testing
Tested with SAME70 based board and logging to SD card.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r799292494
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -491,18 +511,29 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
{
nwritten = file_write(&syslog_dev->sl_file,
g_syscrlf, 2);
+
+ /* Synchronize the file when each CR-LF is encountered
+ * (i.e., implements line buffering always).
+ */
+
+#ifndef CONFIG_DISABLE_MOUNTPOINT
Review comment:
can we remove #ifdef and let syslog_dev_flush do the check?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r798999813
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -453,7 +453,7 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
{
/* Check for pre-formatted CR-LF sequence */
- if (remaining > 1 &&
+ if (remaining > 2 &&
Review comment:
Ok. Let me check the case for "\r\n" separately
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r799301989
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -491,18 +511,29 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
{
nwritten = file_write(&syslog_dev->sl_file,
g_syscrlf, 2);
+
+ /* Synchronize the file when each CR-LF is encountered
+ * (i.e., implements line buffering always).
+ */
+
+#ifndef CONFIG_DISABLE_MOUNTPOINT
Review comment:
Done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r799059881
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -453,7 +453,7 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
{
/* Check for pre-formatted CR-LF sequence */
- if (remaining > 1 &&
+ if (remaining > 2 &&
Review comment:
I reviewed the logic and made a better solution
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r799296598
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -491,18 +511,29 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
{
nwritten = file_write(&syslog_dev->sl_file,
g_syscrlf, 2);
+
+ /* Synchronize the file when each CR-LF is encountered
+ * (i.e., implements line buffering always).
+ */
+
+#ifndef CONFIG_DISABLE_MOUNTPOINT
Review comment:
Sure
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r798991604
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -453,7 +453,7 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
{
/* Check for pre-formatted CR-LF sequence */
- if (remaining > 1 &&
+ if (remaining > 2 &&
Review comment:
Did you test without this modification? I think it basically means "if remaining equal 2 or higher", with this modification now it is "if remaing equal 3 or higher". So, imagine the case where remaining is equal 2 and it is exactly (rare but can happen) : "\r\n", this test will fail.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r799300983
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -451,38 +451,58 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
if (*endptr == '\r' || *endptr == '\n')
{
+ /* Write everything up to the position of the special
+ * character.
+ *
+ * - buffer points to next byte to output.
+ * - endptr points to the special character.
+ */
+
+ writelen = (size_t)((uintptr_t)endptr - (uintptr_t)buffer);
+ if (writelen > 0)
+ {
+ nwritten = file_write(&syslog_dev->sl_file,
+ buffer, writelen);
+ if (nwritten < 0)
+ {
+ ret = (int)nwritten;
+ goto errout_with_sem;
+ }
+ }
+
/* Check for pre-formatted CR-LF sequence */
if (remaining > 1 &&
((endptr[0] == '\r' && endptr[1] == '\n') ||
(endptr[0] == '\n' && endptr[1] == '\r')))
{
- /* Just skip over pre-formatted CR-LF or LF-CR sequence */
+ nwritten = file_write(&syslog_dev->sl_file,
+ g_syscrlf, 2);
- endptr++;
- remaining--;
- }
- else
- {
- /* Write everything up to the position of the special
- * character.
- *
- * - buffer points to next byte to output.
- * - endptr points to the special character.
+ /* Synchronize the file when each CR-LF is encountered
+ * (i.e., implements line buffering always).
*/
- writelen = (size_t)((uintptr_t)endptr - (uintptr_t)buffer);
- if (writelen > 0)
+#ifndef CONFIG_DISABLE_MOUNTPOINT
+ if (nwritten > 0)
{
- nwritten = file_write(&syslog_dev->sl_file,
- buffer, writelen);
- if (nwritten < 0)
- {
- ret = (int)nwritten;
- goto errout_with_sem;
- }
+ syslog_dev_flush(channel);
Review comment:
I do not see how this can be done. The `nwritten = file_write(&syslog_dev->sl_file, g_syscrlf, 2);` should be called either in case of `if (remaining > 1 && ((endptr[0] == '\r' && endptr[1] == '\n') || (endptr[0] == '\n' && endptr[1] == '\r')))` or in case of `if (*endptr == '\n')`, but not for `if (*endptr == '\r')`. Also `endptr++;` and `remaining--;` should be done only if we are having pre-formatted CR-LF sequence.
If you can come with a suggestion, then I will analyze and integrate it into this PR.
I will also think about this
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r799292189
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -451,38 +451,58 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
if (*endptr == '\r' || *endptr == '\n')
{
+ /* Write everything up to the position of the special
+ * character.
+ *
+ * - buffer points to next byte to output.
+ * - endptr points to the special character.
+ */
+
+ writelen = (size_t)((uintptr_t)endptr - (uintptr_t)buffer);
+ if (writelen > 0)
+ {
+ nwritten = file_write(&syslog_dev->sl_file,
+ buffer, writelen);
+ if (nwritten < 0)
+ {
+ ret = (int)nwritten;
+ goto errout_with_sem;
+ }
+ }
+
/* Check for pre-formatted CR-LF sequence */
if (remaining > 1 &&
((endptr[0] == '\r' && endptr[1] == '\n') ||
(endptr[0] == '\n' && endptr[1] == '\r')))
{
- /* Just skip over pre-formatted CR-LF or LF-CR sequence */
+ nwritten = file_write(&syslog_dev->sl_file,
+ g_syscrlf, 2);
- endptr++;
- remaining--;
- }
- else
- {
- /* Write everything up to the position of the special
- * character.
- *
- * - buffer points to next byte to output.
- * - endptr points to the special character.
+ /* Synchronize the file when each CR-LF is encountered
+ * (i.e., implements line buffering always).
*/
- writelen = (size_t)((uintptr_t)endptr - (uintptr_t)buffer);
- if (writelen > 0)
+#ifndef CONFIG_DISABLE_MOUNTPOINT
+ if (nwritten > 0)
{
- nwritten = file_write(&syslog_dev->sl_file,
- buffer, writelen);
- if (nwritten < 0)
- {
- ret = (int)nwritten;
- goto errout_with_sem;
- }
+ syslog_dev_flush(channel);
Review comment:
we can merge the most code in if and else block:
```
nwritten = file_write(&syslog_dev->sl_file, g_syscrlf, 2);
if (nwritten < 0)
{
ret = (int)nwritten;
goto errout_with_sem;
}
syslog_dev_flush(channel);
if (remaining > 1 &&
((endptr[0] == '\r' && endptr[1] == '\n') ||
(endptr[0] == '\n' && endptr[1] == '\r')))
{
endptr++;
remaining--;
}
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r799300983
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -451,38 +451,58 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
if (*endptr == '\r' || *endptr == '\n')
{
+ /* Write everything up to the position of the special
+ * character.
+ *
+ * - buffer points to next byte to output.
+ * - endptr points to the special character.
+ */
+
+ writelen = (size_t)((uintptr_t)endptr - (uintptr_t)buffer);
+ if (writelen > 0)
+ {
+ nwritten = file_write(&syslog_dev->sl_file,
+ buffer, writelen);
+ if (nwritten < 0)
+ {
+ ret = (int)nwritten;
+ goto errout_with_sem;
+ }
+ }
+
/* Check for pre-formatted CR-LF sequence */
if (remaining > 1 &&
((endptr[0] == '\r' && endptr[1] == '\n') ||
(endptr[0] == '\n' && endptr[1] == '\r')))
{
- /* Just skip over pre-formatted CR-LF or LF-CR sequence */
+ nwritten = file_write(&syslog_dev->sl_file,
+ g_syscrlf, 2);
- endptr++;
- remaining--;
- }
- else
- {
- /* Write everything up to the position of the special
- * character.
- *
- * - buffer points to next byte to output.
- * - endptr points to the special character.
+ /* Synchronize the file when each CR-LF is encountered
+ * (i.e., implements line buffering always).
*/
- writelen = (size_t)((uintptr_t)endptr - (uintptr_t)buffer);
- if (writelen > 0)
+#ifndef CONFIG_DISABLE_MOUNTPOINT
+ if (nwritten > 0)
{
- nwritten = file_write(&syslog_dev->sl_file,
- buffer, writelen);
- if (nwritten < 0)
- {
- ret = (int)nwritten;
- goto errout_with_sem;
- }
+ syslog_dev_flush(channel);
Review comment:
I do not see how this can be done. The `nwritten = file_write(&syslog_dev->sl_file, g_syscrlf, 2);` should be called either in case of `if (remaining > 1 && ((endptr[0] == '\r' && endptr[1] == '\n') || (endptr[0] == '\n' && endptr[1] == '\r')))` or in case of `if (*endptr == '\n')`, but not for `if (*endptr == '\r')`. Also `endptr++;` and `remaining--;` should be done only if we are having pre-formatted CR-LF sequence.
If you can come with a suggestion, then I will analyze and integrate it into this PR
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r799636896
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -451,38 +451,58 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
if (*endptr == '\r' || *endptr == '\n')
{
+ /* Write everything up to the position of the special
+ * character.
+ *
+ * - buffer points to next byte to output.
+ * - endptr points to the special character.
+ */
+
+ writelen = (size_t)((uintptr_t)endptr - (uintptr_t)buffer);
+ if (writelen > 0)
+ {
+ nwritten = file_write(&syslog_dev->sl_file,
+ buffer, writelen);
+ if (nwritten < 0)
+ {
+ ret = (int)nwritten;
+ goto errout_with_sem;
+ }
+ }
+
/* Check for pre-formatted CR-LF sequence */
if (remaining > 1 &&
((endptr[0] == '\r' && endptr[1] == '\n') ||
(endptr[0] == '\n' && endptr[1] == '\r')))
{
- /* Just skip over pre-formatted CR-LF or LF-CR sequence */
+ nwritten = file_write(&syslog_dev->sl_file,
+ g_syscrlf, 2);
- endptr++;
- remaining--;
- }
- else
- {
- /* Write everything up to the position of the special
- * character.
- *
- * - buffer points to next byte to output.
- * - endptr points to the special character.
+ /* Synchronize the file when each CR-LF is encountered
+ * (i.e., implements line buffering always).
*/
- writelen = (size_t)((uintptr_t)endptr - (uintptr_t)buffer);
- if (writelen > 0)
+#ifndef CONFIG_DISABLE_MOUNTPOINT
+ if (nwritten > 0)
{
- nwritten = file_write(&syslog_dev->sl_file,
- buffer, writelen);
- if (nwritten < 0)
- {
- ret = (int)nwritten;
- goto errout_with_sem;
- }
+ syslog_dev_flush(channel);
Review comment:
Yes, it's much better than the previous version
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5408: drivers/syslog/syslog_device: fix flushing data when end of line is detected
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5408:
URL: https://github.com/apache/incubator-nuttx/pull/5408#discussion_r799418467
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -451,38 +451,58 @@ static ssize_t syslog_dev_write(FAR struct syslog_channel_s *channel,
if (*endptr == '\r' || *endptr == '\n')
{
+ /* Write everything up to the position of the special
+ * character.
+ *
+ * - buffer points to next byte to output.
+ * - endptr points to the special character.
+ */
+
+ writelen = (size_t)((uintptr_t)endptr - (uintptr_t)buffer);
+ if (writelen > 0)
+ {
+ nwritten = file_write(&syslog_dev->sl_file,
+ buffer, writelen);
+ if (nwritten < 0)
+ {
+ ret = (int)nwritten;
+ goto errout_with_sem;
+ }
+ }
+
/* Check for pre-formatted CR-LF sequence */
if (remaining > 1 &&
((endptr[0] == '\r' && endptr[1] == '\n') ||
(endptr[0] == '\n' && endptr[1] == '\r')))
{
- /* Just skip over pre-formatted CR-LF or LF-CR sequence */
+ nwritten = file_write(&syslog_dev->sl_file,
+ g_syscrlf, 2);
- endptr++;
- remaining--;
- }
- else
- {
- /* Write everything up to the position of the special
- * character.
- *
- * - buffer points to next byte to output.
- * - endptr points to the special character.
+ /* Synchronize the file when each CR-LF is encountered
+ * (i.e., implements line buffering always).
*/
- writelen = (size_t)((uintptr_t)endptr - (uintptr_t)buffer);
- if (writelen > 0)
+#ifndef CONFIG_DISABLE_MOUNTPOINT
+ if (nwritten > 0)
{
- nwritten = file_write(&syslog_dev->sl_file,
- buffer, writelen);
- if (nwritten < 0)
- {
- ret = (int)nwritten;
- goto errout_with_sem;
- }
+ syslog_dev_flush(channel);
Review comment:
I can try to do:
```
/* Check for pre-formatted CR-LF sequence */
if (remaining > 1 &&
((endptr[0] == '\r' && endptr[1] == '\n') ||
(endptr[0] == '\n' && endptr[1] == '\r')))
{
writelen = sizeof(g_syscrlf);
/* Skip over pre-formatted CR-LF or LF-CR sequence */
endptr++;
remaining--;
}
else
{
/* Ignore the carriage return, but for the linefeed, output
* both a carriage return and a linefeed.
*/
if (*endptr == '\n')
{
writelen = sizeof(g_syscrlf);
}
else
{
writelen = 0;
}
}
if (writelen > 0)
{
nwritten = file_write(&syslog_dev->sl_file,
g_syscrlf, writelen);
/* Synchronize the file when each CR-LF is encountered
* (i.e., implements line buffering always).
*/
if (nwritten > 0)
{
syslog_dev_flush(channel);
}
if (nwritten < 0)
{
ret = (int)nwritten;
goto errout_with_sem;
}
}
```
@xiaoxiang781216 what do you think?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org