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 2021/07/01 14:12:28 UTC
[GitHub] [incubator-nuttx] Donny9 opened a new pull request #4028: syslog: fix log confusion when multi task writing together
Donny9 opened a new pull request #4028:
URL: https://github.com/apache/incubator-nuttx/pull/4028
## Summary
Let's to do "syslog_write_t sc_write" function to fix log confusion when multi task writing together for syslog and ramlog.
## Impact
normal output log for multi task environment.
## Testing
daily test.
--
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] fjpanag commented on pull request #4028: syslog: fix log confusion when multi task writing together
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #4028:
URL: https://github.com/apache/incubator-nuttx/pull/4028#issuecomment-872953736
Hi @Donny9
I had a look at the changes, but I don't think that this will fix the issue.
The problem is not the thread safety of the drivers, rather the multiple calls to `lib_sprintf()`.
See #3599.
Adding a `sc_write()` to all channels seems a good way forward.
If `syslog_write_t` is modified to also accept a `force` flag, we can eliminate the `syslog_putc_t` too.
But I think a semaphore at this place will not solve the actual issue. Depending on the solution to the initial problem, it may not be needed at all.
Do you have any plans for further development on this?
(I have also spent quite some time thinking about this and experimenting, but I haven't reached a "good" final implementation...)
--
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 #4028: syslog: fix log confusion when multi task writing together
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4028:
URL: https://github.com/apache/incubator-nuttx/pull/4028
--
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 pull request #4028: syslog: fix log confusion when multi task writing together
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4028:
URL: https://github.com/apache/incubator-nuttx/pull/4028#issuecomment-873003321
> @xiaoxiang781216
>
> > This patch only address the mess issue in case 1. The lock ensure that the caller of syslog_write don't interlace with each other.
>
> Don't forget that also buffering needs fixing. In case of overflow it will split the message in multiple `syslog_write` calls, which again leaves room for this bug to appear....
Yes, it can't fix too, we need reserve at least one line buffer.
--
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] fjpanag commented on pull request #4028: syslog: fix log confusion when multi task writing together
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #4028:
URL: https://github.com/apache/incubator-nuttx/pull/4028#issuecomment-875532427
@xiaoxiang781216 Sorry for the late response, I just had the time to check on the code.
Yes, I believe it is better.
At least some cases are now protected.
--
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] fjpanag commented on a change in pull request #4028: syslog: fix log confusion when multi task writing together
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #4028:
URL: https://github.com/apache/incubator-nuttx/pull/4028#discussion_r665291248
##########
File path: drivers/syslog/ramlog.c
##########
@@ -283,12 +286,89 @@ static int ramlog_addchar(FAR struct ramlog_dev_s *priv, char ch)
return OK;
}
+/****************************************************************************
+ * Name: ramlog_addbuf
+ ****************************************************************************/
+
+static ssize_t ramlog_addbuf(FAR struct ramlog_dev_s *priv,
+ FAR const char *buffer, size_t len)
+{
+ int readers_waken;
+ ssize_t nwritten;
+ char ch;
+ int ret;
+
+ ret = nxsem_wait(&priv->rl_exclsem);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ for (nwritten = 0; (size_t)nwritten < len; nwritten++)
+ {
+ /* Get the next character to output */
+
+ ch = buffer[nwritten];
+
+ /* Then output the character */
+
+ ret = ramlog_addchar(priv, ch);
+ if (ret < 0)
+ {
+ /* The buffer is full and nothing was saved. The remaining
+ * data to be written is dropped on the floor.
+ */
+
+ break;
+ }
+ }
+
+ /* Was anything written? */
+
+ if (nwritten > 0)
+ {
+ readers_waken = 0;
+
+#ifndef CONFIG_RAMLOG_NONBLOCKING
+ /* Are there threads waiting for read data? */
+
+ readers_waken = ramlog_readnotify(priv);
+#endif
+
+ /* If there are multiple readers, some of them might block despite
+ * POLLIN because first reader might read all data. Favor readers
+ * and notify poll waiters only if no reader was awaken, even if the
+ * latter may starve.
+ *
+ * This also implies we do not have to make these two notify
+ * operations a critical section.
+ */
+
+ if (readers_waken == 0)
+ {
+ /* Notify all poll/select waiters that they can read from the
+ * FIFO.
+ */
+
+ ramlog_pollnotify(priv, POLLIN);
+ }
+ }
+
+ /* We always have to return the number of bytes requested and NOT the
+ * number of bytes that were actually written. Otherwise, callers
+ * probably retry, causing same error condition again.
+ */
+
+ nxsem_post(&priv->rl_exclsem);
+ return len;
+}
+
/****************************************************************************
* Name: ramlog_read
Review comment:
Just a minor nit. The comment was not updated to reflect the new function's name.
--
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 edited a comment on pull request #4028: syslog: fix log confusion when multi task writing together
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4028:
URL: https://github.com/apache/incubator-nuttx/pull/4028#issuecomment-872989530
> Hi @Donny9
>
> I had a look at the changes, but I don't think that this will fix the issue.
> The problem is not the thread safety of the drivers, rather the multiple calls to `lib_sprintf()`.
> See #3599.
lib_sprintf format result to lib_syslogstream_s, two cases here:
1. CONFIG_SYSLOG_BUFFER equals y, syslogstream_flush output the log through syslog_write
2. CONFIG_SYSLOG_BUFFER equals n, syslogstream_addchar output the log through syslog_putc
This patch only address the mess issue in case 1. The lock ensure that the caller of syslog_write don't interlace with each other.
>
> Adding a `sc_write()` to all channels seems a good way forward.
> If `syslog_write_t` is modified to also accept a `force` flag, we can eliminate the `syslog_putc_t` too.
>
Yes, but sc_putc/sc_force_putc is more simpler to implement than sc_write.
> But I think a semaphore at this place will not solve the actual issue. Depending on the solution to the initial problem, it may not be needed at all.
>
> Do you have any plans for further development on this?
> (I have also spent quite some time thinking about this and experimenting, but I haven't reached a "good" final implementation...)
Case 1 should be addressed by:
1. syslog driver self(e.g. ramlog, devlog), if it provide the multithread safe sc_write
2. syslog_default_write with this patch
But, it's impossible to fix case 2.
--
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 pull request #4028: syslog: fix log confusion when multi task writing together
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4028:
URL: https://github.com/apache/incubator-nuttx/pull/4028#issuecomment-873032420
@fjpanag do you think this patch fix some case of mess after discussion?
--
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 pull request #4028: syslog: fix log confusion when multi task writing together
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4028:
URL: https://github.com/apache/incubator-nuttx/pull/4028#issuecomment-872989530
> Hi @Donny9
>
> I had a look at the changes, but I don't think that this will fix the issue.
> The problem is not the thread safety of the drivers, rather the multiple calls to `lib_sprintf()`.
> See #3599.
lib_sprintf format result to lib_syslogstream_s, two cases here:
1. CONFIG_SYSLOG_BUFFER equals y, syslogstream_flush output the log through syslog_write
2. CONFIG_SYSLOG_BUFFER equals n, syslogstream_addchar output the log through syslog_putc
This patch only address the mess issue in case 1. The lock ensure that the caller of syslog_write don't interlace with each other.
>
> Adding a `sc_write()` to all channels seems a good way forward.
> If `syslog_write_t` is modified to also accept a `force` flag, we can eliminate the `syslog_putc_t` too.
>
Yes, but sc_putc/sc_force_putc is more simpler to implement than sc_write.
> But I think a semaphore at this place will not solve the actual issue. Depending on the solution to the initial problem, it may not be needed at all.
>
> Do you have any plans for further development on this?
> (I have also spent quite some time thinking about this and experimenting, but I haven't reached a "good" final implementation...)
Case 1 should be addressed by:
1. syslog driver self(e.g. ramlog, devlog), if it provide the multithread safe sc_write
2. syslog_default_write with this path
But, it's impossible to fix case 2.
--
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] fjpanag commented on pull request #4028: syslog: fix log confusion when multi task writing together
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #4028:
URL: https://github.com/apache/incubator-nuttx/pull/4028#issuecomment-872999660
@xiaoxiang781216
> This patch only address the mess issue in case 1. The lock ensure that the caller of syslog_write don't interlace with each other.
Don't forget that also buffering needs fixing. In case of overflow it will split the message in multiple `syslog_write` calls, which again leaves room for this bug to appear....
--
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