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/03/13 15:39:17 UTC
[GitHub] [incubator-nuttx] fjpanag opened a new pull request #3050: Added support for multiple syslog channels.
fjpanag opened a new pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050
## Summary
Adds support for multiple syslog channels, logging simultaneously.
## Impact
None on existing configurations. It defaults to 1 channel, as it was previously.
## Testing
Tested on a custom STM32F4 board, logging simultaneously on serial and in an SD file.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r598109299
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
@xiaoxiang781216 I settled in for the implementation that you propose.
Please review, so I can proceed with documentation.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593888974
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
I don't think this will work as intended.
`syslog_channel` is used in many places, like `syslog_file_channel` or `syslog_console_channel`.
I would expect a call to `syslog_file_channel` for example to add another channel, instead of overwriting the old one.
Alternatively, `syslog_channel` may be completely removed, and substituted with `syslog_channel_add` and `syslog_channel_remove`, so the user has the ability to set up whatever log channels combination they want?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593928197
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
Maybe, we can change syslog_channel behaviour base on the value of CONFIG_SYSLOG_MAX_CHANNELS:
1. Use the replacement semantics when CONFIG_SYSLOG_MAX_CHANNELS == 1
2. Use the addition semantics when CONFIG_SYSLOG_MAX_CHANNELS > 1
At least, we should allow user switch to syslog_file_channel when CONFIG_SYSLOG_MAX_CHANNELS equals 1
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599643976
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
There are two stream(emerge v.s. syslog):
```
if (priority == LOG_EMERG)
{
/* Use the SYSLOG emergency stream */
emergstream(&stream.public);
}
else
{
/* Use the normal SYSLOG stream */
syslogstream_create(&stream);
}
```
The first(emerge) call syslog_force directly, the second(syslog) save the result into iob buffer and flush through syslog_write at the end. So, only LOG_EMERG(rare used) has the problem you describe.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593923796
##########
File path: drivers/syslog/syslog_putc.c
##########
@@ -55,48 +55,57 @@
int syslog_putc(int ch)
{
- DEBUGASSERT(g_syslog_channel != NULL);
+ int i;
- /* Is this an attempt to do SYSLOG output from an interrupt handler? */
-
- if (up_interrupt_context() || sched_idletask())
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
{
-#if defined(CONFIG_SYSLOG_INTBUFFER)
- if (up_interrupt_context())
- {
- /* Buffer the character in the interrupt buffer.
- * The interrupt buffer will be flushed before the next normal,
- * non-interrupt SYSLOG output.
- */
+ if (g_syslog_channel[i] == NULL)
+ break;
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599643976
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
There are two stream(emerge v.s. syslog):
```
if (priority == LOG_EMERG)
{
/* Use the SYSLOG emergency stream */
emergstream(&stream.public);
}
else
{
/* Use the normal SYSLOG stream */
syslogstream_create(&stream);
}
```
The first(emerge) call syslog_force directly, the second(syslog) save the result into iob buffer and flush through syslog_write at the end.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798562079
@acassis Do you mean a new board config file? As a 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593923851
##########
File path: drivers/syslog/syslog_write.c
##########
@@ -56,37 +56,44 @@
static ssize_t syslog_default_write(FAR const char *buffer, size_t buflen)
{
- size_t nwritten;
+ int i;
+ size_t nwritten = 0;
- if (up_interrupt_context() || sched_idletask())
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
{
- for (nwritten = 0; nwritten < buflen; nwritten++)
+ if (g_syslog_channel[i] == NULL)
+ break;
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599597115
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
> > All methods need be protected by spin lock or critical section.
>
> I just cannot understand how syslog is protected now.
> Following the code from `syslog()` to `nx_vsyslog()`, I don't see any protection (e.g. semaphore etc).
>
The protection doesn't need before since g_syslog_channel is never changed at the runtime.
> Shouldn't syslog be thread-safe? In the sense that multiple tasks may log simultaneously?
Yes, syslog is not only thead-safe but also interrupt safe(true for uart, ram and lwl, false for file and dev). The protection is done inside the driver level. syslog/nx_vsyslog use stack variable only except g_syslog_channel, which need to protect now with your new code.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599773118
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
> Interestingly enough, the issue persists (I would expect the output to be OK now, too).
> I haven't managed to follow the code, and how this is caused.
>
> In any case though, I would expect the system logger to be thread-safe under all circumstances...
it depends on how you define thread-safe. The default behaviour doesn't cause the hang or lose the data.
> I would insist that a semaphore has to be added to all the public functions of the syslog.
No, you can't use semaphore here, syslog can be called from interrupt context.
> I am not sure if this is now the issue, but the buffered syslog output is surely to have problem too.
>
> If the output string is longer than `CONFIG_IOB_BUFSIZE`, then the buffer will be outputed prematurely, causing the exact same race condition as without buffer.
>
> The fact that the buffer may be flushed before the whole syslog message has been written to it, defeats completely its puprose...
Yes, if your output is longer than CONFIG_IOB_BUFSIZE, the buffer has to be flushed in the middle.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798674850
> @acassis Do you mean a new board config file? As a PR?
Yep!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-805975114
@v01d I don't know why this is merged, but I think this part of the work is finished and works correctly.
Since there are a lot to be done, I would personally prefer to have them separated, to better manage them instead of a huge mega-PR.
But I don't actually care. You may revert it if you feel doing so.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593931594
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
I can think of another solution. (Which I also don't like very much...)
Maybe the first call to `syslog_channel` will substitute the default channel, and every subsequent call will append the new channel to the list.
On the other hand, any reason of not just moving to `_add` & `_remove` functions?
I will most probably add the `_remove` variant in this PR, because it is needed (I just don't have the time today).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-806331971
> Why was this merged? The discussion seemed to be ongoing.
The implementation is done if we ignore the add/remove channel dynamically at the runtime. What need to improve include:
1. Document the new API
2. Support the runtime modification
Both could be done by separated PR. That's why I merge 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-805976788
Sure, let's keep it then.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r598210517
##########
File path: drivers/syslog/syslog_flush.c
##########
@@ -64,21 +64,27 @@
int syslog_flush(void)
{
- DEBUGASSERT(g_syslog_channel != NULL);
+ int i;
+
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ break;
#ifdef CONFIG_SYSLOG_INTBUFFER
- /* Flush any characters that may have been added to the interrupt
- * buffer.
- */
+ /* Flush any characters that may have been added to the interrupt
+ * buffer.
+ */
- syslog_flush_intbuffer(g_syslog_channel, true);
+ syslog_flush_intbuffer(g_syslog_channel[i], true);
Review comment:
We just have one interrupt buffer, so the loop need move into syslog_flush_intbuffer otherwise, all channels except the first one lose the log happen in the interrupt handler.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593930898
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
I think this will be very confusing for the users... I don't like dual functionality of a single function.
No matter what is the final interface, the operation of each function should be clear and constant.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798929593
> > BTW, I'm not following how this interacts with Kconfig. Right now there are some options that indicate where to send syslog via a choice and other options. If this enables multiple outputs, shouldn't that be changed to a multiple choice? Kconfig options for syslog are also a bit messy, so any improvement in that sense would also be welcome.
>
> I mentioned Kconfig in the broader sense, compared to providing just a another board config.
> Generally, for me, Kconfig help entries are the first stop in looking for information for a feature.
I understand, I was simply mentioning that I didn't quite get how the change introduced in this PR works in conjunction with existing options as I see those are not affected by this PR, as it is not clear from the PR description.
>
> Let's finalize the interface of the new functionality first, and then I will update the documentation. And possibly improve syslog Kconfig in general...
I personally think that the best time to update docs to account for changes in a PR is as part of the PR itself. Changing the docs is just changing a text file. Most of the time the subsequent doc update PR never happens (not saying this is your case, just an observation of past PRs). If you want to make the change to the docs in another PR, go ahead. I just think it would be a good practice to adopt.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-806331971
> Why was this merged? The discussion seemed to be ongoing.
The implementation is done if we ignore the add/remove channel dynamically at the runtime. What need to improve include:
1. Document the new API
2. Support the runtime modification
Both could be done by separated 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599610352
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
But, till `nx_vsyslog()` there is no protection, which mean that this function may be called concurrently by multiple threads.
Since `nx_vsyslog()` makes several (unprotected) calls to `lib_sprintf()`, I would expect that the output can be garbled.
Something like:
```
[1616508998.000][1616508998.001] message2 message1
```
instead of:
```
[1616508998.000] message1
[1616508998.001] message2
```
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599576900
##########
File path: drivers/syslog/syslog_write.c
##########
@@ -56,37 +56,44 @@
static ssize_t syslog_default_write(FAR const char *buffer, size_t buflen)
{
- size_t nwritten;
+ int i;
+ size_t nwritten = 0;
- if (up_interrupt_context() || sched_idletask())
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
{
- for (nwritten = 0; nwritten < buflen; nwritten++)
+ if (g_syslog_channel[i] == NULL)
+ break;
+
+ if (up_interrupt_context() || sched_idletask())
{
-#ifdef CONFIG_SYSLOG_INTBUFFER
- if (up_interrupt_context())
+ for (nwritten = 0; nwritten < buflen; nwritten++)
{
- syslog_add_intbuffer(*buffer++);
- }
- else
+#ifdef CONFIG_SYSLOG_INTBUFFER
+ if (up_interrupt_context())
+ {
+ syslog_add_intbuffer(*buffer++);
Review comment:
fixed
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599710027
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
Yes, I haven't enabled it.
This option is by default disabled and unavailable.
It depends on `CONFIG_SYSLOG_WRITE`, which is not avaiable in the default NSH configuration.
I had it selected by enabling `CONFIG_SYSLOG_FILE` (which is unrelated and unneeded in this example).
Interestingly enough, the issue persists (I would expect the output to be OK now, too).
I haven't managed to follow the code, and how this is caused.
In any case though, I would expect the system logger to be thread-safe under all circumstances...
I would insist that a semaphore has to be added to all the public functions of the syslog.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593936943
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -92,7 +92,11 @@ static const struct syslog_channel_s g_default_channel =
/* This is the current syslog channel in use */
-FAR const struct syslog_channel_s *g_syslog_channel = &g_default_channel;
+FAR const struct syslog_channel_s
+ *g_syslog_channel[CONFIG_SYSLOG_MAX_CHANNELS] =
+ {
+ &g_default_channel
+ };
Review comment:
Like this:
```
FAR const struct syslog_channel_s *
g_syslog_channel[CONFIG_SYSLOG_MAX_CHANNELS] =
{
&g_default_channel
};
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r594028016
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
> I can think of another solution. (Which I also don't like very much...)
>
> Maybe the first call to `syslog_channel` will substitute the default channel, and every subsequent call will append the new channel to the list.
>
> On the other hand, any reason of not just moving to `_add` & `_remove` functions?
`_add` functions can't resolve the problem I mentioned before. We need the replacement semantics when CONFIG_SYSLOG_MAX_CHANNELS == 1 regardless how you name the function.
> I will most probably add the `_remove` variant in this PR, because it is needed (I just don't have the time today).
Yes, I think so.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r596971261
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599630746
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
No this is not real output.
But still I cannot understand this.
`lib_sprintf()` is finally calling the `putc(c,stream)` macro, which may end up to a plain call to `syslog_putc()`.
I don't see any buffering take place, that will concatenate all `lib_sprintf()` calls to a single string to be outputed atomically.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r594583999
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
OK, lets recap this:
- `syslog_channel` will be retained and it will substitute the default channel with the provided one.
- `syslog_channel_add` will be introduced and it will append a new channel to the list.
- `syslog_channel_remove` will be introduced and it will remove a channel from the list.
Some questions:
- Which is the "default" channel? May I assume that it will always be the first in the list?
- Functions like `syslog_file_channel` will use the substitute or the add function? I would go for the addition.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599692248
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
Ok, you forget to enable CONFIG_SYSLOG_BUFFER, please try 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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593923901
##########
File path: drivers/syslog/syslog_write.c
##########
@@ -116,13 +123,21 @@ static ssize_t syslog_default_write(FAR const char *buffer, size_t buflen)
ssize_t syslog_write(FAR const char *buffer, size_t buflen)
{
#ifdef CONFIG_SYSLOG_INTBUFFER
+ int i;
+
if (!up_interrupt_context() && !sched_idletask())
{
/* Flush any characters that may have been added to the interrupt
* buffer.
*/
- syslog_flush_intbuffer(g_syslog_channel, false);
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ break;
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599597115
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
> > All methods need be protected by spin lock or critical section.
>
> I just cannot understand how syslog is protected now.
> Following the code from `syslog()` to `nx_vsyslog()`, I don't see any protection (e.g. semaphore etc).
>
The protection doesn't need before since g_syslog_channel is never changed at the runtime.
> Shouldn't syslog be thread-safe? In the sense that multiple tasks may log simultaneously?
Yes, syslog is not only thead-safe but also interrupt safe(true for uart, ram and lwl, false for file and dev). The protection is done inside the driver level. syslog/nx_vsyslog use stack variables only except g_syslog_channel, which need to protect now with your new code.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599683602
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
I am sorry but I cannot see what you describe...
Here is a stack trace while the logger is outputing characters:
I don't see in any of these functions any kind of buffering.
---
In fact is was very easy to replicate what I imagined above.
I did:
```c
task_create("test1", 128, 2048, test_th, 0);
task_create("test2", 129, 2048, test_th, 0);
```
and
```c
int test_th(int argc, char ** argv)
{
while(1)
{
usleep(rand() % 10000);
syslog(LOG_INFO, "A test message.\n");
}
}
```
And I got this awesome output:
*Note!* You may not see this happen usually, because the default NuttX scheduler frequency is very coarse.
And *usually* the output has finished, before the next thread takes over. I had to set the scheduler frequency to 1kHz for this example (or you may use a log channel that needs more than 10ms to output everything completely).
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798927500
> Wouldn't it better to properly document the features in Kconfig help entries, or in the "proper" NuttX documentation?
> Maybe I should document it in `nuttx/Documentation/components/drivers/special/syslog.rst`?
Yes, please. This should actually be the norm when adding features/changing behavior.
BTW, I'm not following how this interacts with Kconfig. Right now there are some options that indicate where to send syslog via a choice and other options. If this enables multiple outputs, shouldn't that be changed to a *multiple* choice? Kconfig options for syslog are also a bit messy, so any improvement in that sense would also be welcome.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798547189
Very nice @fjpanag ! If you can, please supply a board example and how to use 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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798928139
> BTW, I'm not following how this interacts with Kconfig. Right now there are some options that indicate where to send syslog via a choice and other options. If this enables multiple outputs, shouldn't that be changed to a multiple choice? Kconfig options for syslog are also a bit messy, so any improvement in that sense would also be welcome.
I mentioned Kconfig in the broader sense, compared to providing just a another board config.
Generally, for me, Kconfig help entries are the first stop in looking for information for a feature.
Let's finalize the interface of the new functionality first, and then I will update the documentation. And possibly improve syslog Kconfig in general...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798896024
> > @acassis Do you mean a new board config file? As a PR?
>
> Yep! Also include in the README.txt of the board an instruction to use the example
I would like to provide an example, but please not in another board config.
Every time that I need to use a new (for me) feature, I have to check the source code of a hideous amount of arch'es, boards and configs, find what is useful and/or applicable for my port etc etc... I just feel this is getting bad.
Wouldn't it better to properly document the features in Kconfig help entries, or in the "proper" NuttX documentation?
Maybe I should document it in `nuttx/Documentation/components/drivers/special/syslog.rst`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593914487
##########
File path: drivers/syslog/syslog_putc.c
##########
@@ -55,48 +55,57 @@
int syslog_putc(int ch)
{
- DEBUGASSERT(g_syslog_channel != NULL);
+ int i;
- /* Is this an attempt to do SYSLOG output from an interrupt handler? */
-
- if (up_interrupt_context() || sched_idletask())
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
{
-#if defined(CONFIG_SYSLOG_INTBUFFER)
- if (up_interrupt_context())
- {
- /* Buffer the character in the interrupt buffer.
- * The interrupt buffer will be flushed before the next normal,
- * non-interrupt SYSLOG output.
- */
+ if (g_syslog_channel[i] == NULL)
+ break;
Review comment:
ditto
##########
File path: drivers/syslog/syslog_force.c
##########
@@ -55,18 +55,27 @@
int syslog_force(int ch)
{
- DEBUGASSERT(g_syslog_channel != NULL &&
- g_syslog_channel->sc_force != NULL);
+ int i;
+
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ break;
Review comment:
need add {} around break
##########
File path: drivers/syslog/syslog_write.c
##########
@@ -56,37 +56,44 @@
static ssize_t syslog_default_write(FAR const char *buffer, size_t buflen)
{
- size_t nwritten;
+ int i;
+ size_t nwritten = 0;
- if (up_interrupt_context() || sched_idletask())
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
{
- for (nwritten = 0; nwritten < buflen; nwritten++)
+ if (g_syslog_channel[i] == NULL)
+ break;
Review comment:
ditto
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -92,7 +92,11 @@ static const struct syslog_channel_s g_default_channel =
/* This is the current syslog channel in use */
-FAR const struct syslog_channel_s *g_syslog_channel = &g_default_channel;
+FAR const struct syslog_channel_s
+ *g_syslog_channel[CONFIG_SYSLOG_MAX_CHANNELS] =
+ {
+ &g_default_channel
+ };
Review comment:
remove the extra space at the begin
##########
File path: drivers/syslog/syslog_write.c
##########
@@ -116,13 +123,21 @@ static ssize_t syslog_default_write(FAR const char *buffer, size_t buflen)
ssize_t syslog_write(FAR const char *buffer, size_t buflen)
{
#ifdef CONFIG_SYSLOG_INTBUFFER
+ int i;
+
if (!up_interrupt_context() && !sched_idletask())
{
/* Flush any characters that may have been added to the interrupt
* buffer.
*/
- syslog_flush_intbuffer(g_syslog_channel, false);
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ break;
Review comment:
ditto
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-803500087
> There is something that I do not really understand, regarding removing syslog channels.
>
> In case of `syslog_channel_remove`, or even previously when `syslog_channel` was overwritting the existing channel.
> How do the resources of the old channel were deallocated?
>
Yes, your concern is right. syslog_channel is designed to switch the default channel to the new one during the initialization, not for switch in the runtime dynamically. So the deallocation isn't a big issue.
> For example, when `syslog_file_channel` is used, and then this channel is removed, how the underlying file will be closed?
>
> _Maybe there is more to be done in this PR regarding this aspect?_
Yes, we need to extedn syslog_channel_s(e.g. unlink or unregister) to notify the channel.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593917975
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
And what is the "default" channel that `syslog_channel` will substitute?
Functions like `syslog_file_channel` shall call `syslog_channel` or `syslog_channel_add`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis edited a comment on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
acassis edited a comment on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798674850
> @acassis Do you mean a new board config file? As a PR?
Yep! Also include in the README.txt of the board an instruction to use the example
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798904626
> > > @acassis Do you mean a new board config file? As a PR?
> >
> >
> > Yep! Also include in the README.txt of the board an instruction to use the example
>
> I would like to provide an example, but please not in another board config.
>
> Every time that I need to use a new (for me) feature, I have to check the source code of a hideous amount of arch'es, boards and configs, find what is useful and/or applicable for my port etc etc... I just feel this is getting bad.
>
> Wouldn't it better to properly document the features in Kconfig help entries, or in the "proper" NuttX documentation?
> Maybe I should document it in `nuttx/Documentation/components/drivers/special/syslog.rst`?
I agree, documentation it is better than proliferating a new board config. In the other hand, have a board config is very useful for testing, people don't need to go through all the steps and miss something.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593784949
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
Should we keep the old semantics(replace the default one)? and add a new function(e.g. syslog_channel_add or syslog_channel_register).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r596965422
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -92,7 +92,11 @@ static const struct syslog_channel_s g_default_channel =
/* This is the current syslog channel in use */
-FAR const struct syslog_channel_s *g_syslog_channel = &g_default_channel;
+FAR const struct syslog_channel_s
+ *g_syslog_channel[CONFIG_SYSLOG_MAX_CHANNELS] =
+ {
+ &g_default_channel
+ };
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r596966763
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
I added the `_remove` function.
Please give me some time to think about the optimal implementation of `syslog_channel`, regarding appending / overwritting etc...
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593915420
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
The problem by changing `syslog_channel` from replacement to addition will break the usage in the default config(CONFIG_SYSLOG_MAX_CHANNELS == 1), because syslog_channel always fail to find the empty slot. We should at leaset keep the replacement semantics for CONFIG_SYSLOG_MAX_CHANNELS == 1.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599575830
##########
File path: drivers/syslog/syslog_flush.c
##########
@@ -64,21 +64,27 @@
int syslog_flush(void)
{
- DEBUGASSERT(g_syslog_channel != NULL);
+ int i;
+
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ break;
#ifdef CONFIG_SYSLOG_INTBUFFER
- /* Flush any characters that may have been added to the interrupt
- * buffer.
- */
+ /* Flush any characters that may have been added to the interrupt
+ * buffer.
+ */
- syslog_flush_intbuffer(g_syslog_channel, true);
+ syslog_flush_intbuffer(g_syslog_channel[i], true);
Review comment:
Correct, I didn't notice this. Fixed.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-803500087
> There is something that I do not really understand, regarding removing syslog channels.
>
> In case of `syslog_channel_remove`, or even previously when `syslog_channel` was overwritting the existing channel.
> How do the resources of the old channel were deallocated?
>
Yes, your concern is right. syslog_channel is designed to switch the default channel to the new one during the initialization, not for switch in the runtime dynamically. So the deallocation isn't a big issue.
> For example, when `syslog_file_channel` is used, and then this channel is removed, how the underlying file will be closed?
>
> _Maybe there is more to be done in this PR regarding this aspect?_
Yes, we need to extend syslog_channel_s(e.g. unlink or unregister) to notify the channel.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-805769575
Why was this merged? The discussion seemed to be ongoing.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593896248
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
I am thinking that a function like `syslog_channel_remove` will be needed.
For example, in cases like logging in a file in an external medium (e.g. SD Card), the syslog channel must be removed when the medium is also unmounted.
In the case of adding and removing channels dynamically, what will be the default channel?
It will not be sure that it will be the first in the list...
I am leaning more towards dropping `syslog_channel` and adding `syslog_channel_add` & `syslog_channel_remove`, if you agree.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599616190
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
> But, till `nx_vsyslog()` there is no protection, which mean that this function may be called concurrently by multiple threads.
>
Yes, of course.
> Since `nx_vsyslog()` makes several (unprotected) calls to `lib_sprintf()`, I would expect that the output can be garbled.
>
lib_sprintf output to a local buffer, different thread has the different buffer, so no problem here.
> Something like:
>
> ```
> [1616508998.000][1616508998.001] message2 message1
> ```
>
> instead of:
>
> ```
> [1616508998.000] message1
> [1616508998.001] message2
> ```
Is it the real output? or just your imagination.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-805841079
Can we revert it until we are sure things work as expected? Also, we discussed adding Documentation once implementation was settled, which was now left out. @fjpanag I'll leave it to you to decide.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798932356
> > I personally think that the best time to update docs to account for changes in a PR is as part of the PR itself. Changing the docs is just changing a text file. Most of the time the subsequent doc update PR never happens (not saying this is your case, just an observation of past PRs). If you want to make the change to the docs in another PR, go ahead. I just think it would be a good practice to adopt.
>
> No, you misunderstood me. I will do it in the same PR. Just at a later time. After deciding on @xiaoxiang781216 's comments above.
Ah, sorry, cool then.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-805748517
@xiaoxiang781216 I see this is merged already.
I guess the rest of the work to be done should be in separate PRs?
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599576543
##########
File path: drivers/syslog/syslog_putc.c
##########
@@ -55,48 +55,57 @@
int syslog_putc(int ch)
{
- DEBUGASSERT(g_syslog_channel != NULL);
+ int i;
- /* Is this an attempt to do SYSLOG output from an interrupt handler? */
-
- if (up_interrupt_context() || sched_idletask())
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
{
-#if defined(CONFIG_SYSLOG_INTBUFFER)
- if (up_interrupt_context())
- {
- /* Buffer the character in the interrupt buffer.
- * The interrupt buffer will be flushed before the next normal,
- * non-interrupt SYSLOG output.
- */
+ if (g_syslog_channel[i] == NULL)
+ break;
+
+ /* Is this an attempt to do SYSLOG output from an interrupt handler? */
- return syslog_add_intbuffer(ch);
+ if (up_interrupt_context() || sched_idletask())
+ {
+#if defined(CONFIG_SYSLOG_INTBUFFER)
+ if (up_interrupt_context())
+ {
+ /* Buffer the character in the interrupt buffer.
+ * The interrupt buffer will be flushed before the next
+ * normal,non-interrupt SYSLOG output.
+ */
+
+ return syslog_add_intbuffer(ch);
Review comment:
I don't understand the problem, the loop exited there.
But nevertheless, is it fixed now?
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-798931581
> I personally think that the best time to update docs to account for changes in a PR is as part of the PR itself. Changing the docs is just changing a text file. Most of the time the subsequent doc update PR never happens (not saying this is your case, just an observation of past PRs). If you want to make the change to the docs in another PR, go ahead. I just think it would be a good practice to adopt.
No, you misunderstood me. I will do it in the same PR. Just at a later time. After deciding on @xiaoxiang781216 's comments above.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r594601995
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
> OK, lets recap this:
>
> * `syslog_channel` will be retained and it will substitute the default channel with the provided one.
> * `syslog_channel_add` will be introduced and it will append a new channel to the list.
> * `syslog_channel_remove` will be introduced and it will remove a channel from the list.
>
> Some questions:
>
> * Which is the "default" channel? May I assume that it will always be the first in the list?
Most likely the first, or if `syslog_channel` remove all channel before add self then we don't need to know which one is the default.
> * Functions like `syslog_file_channel` will use the substitute or the add function? I would go for the addition.
But, it will fail to add in case of CONFIG_SYSLOG_MAX_CHANNELS == 1. That's why I suggest that `syslog_channel` change the behaviour based on CONFIG_SYSLOG_MAX_CHANNELS, or syslog_file_channel call different API(syslog_channel/syslog_channel_add) to register self.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599587529
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
> All methods need be protected by spin lock or critical section.
I just cannot understand how syslog is protected now.
Following the code from `syslog()` to `nx_vsyslog()`, I don't see any protection (e.g. semaphore etc).
Shouldn't syslog be thread-safe? In the sense that multiple tasks may log simultaneously?
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599683602
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
I am sorry but I cannot see what you describe...
Here is a stack trace while the logger is outputing characters:
I don't see in any of these functions any kind of buffering.
---
In fact is was very easy to replicate what I imagined above.
I did:
```c
task_create("test1", 128, 2048, test_th, 0);
task_create("test2", 129, 2048, test_th, 0);
```
and
```c
int test_th(int argc, char ** argv)
{
while(1)
{
usleep(rand() % 10000);
syslog(LOG_INFO, "A test message.\n");
}
}
```
And I got this awesome output:
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r598210613
##########
File path: drivers/syslog/syslog_putc.c
##########
@@ -55,48 +55,57 @@
int syslog_putc(int ch)
{
- DEBUGASSERT(g_syslog_channel != NULL);
+ int i;
- /* Is this an attempt to do SYSLOG output from an interrupt handler? */
-
- if (up_interrupt_context() || sched_idletask())
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
{
-#if defined(CONFIG_SYSLOG_INTBUFFER)
- if (up_interrupt_context())
- {
- /* Buffer the character in the interrupt buffer.
- * The interrupt buffer will be flushed before the next normal,
- * non-interrupt SYSLOG output.
- */
+ if (g_syslog_channel[i] == NULL)
+ break;
+
+ /* Is this an attempt to do SYSLOG output from an interrupt handler? */
- return syslog_add_intbuffer(ch);
+ if (up_interrupt_context() || sched_idletask())
+ {
+#if defined(CONFIG_SYSLOG_INTBUFFER)
+ if (up_interrupt_context())
+ {
+ /* Buffer the character in the interrupt buffer.
+ * The interrupt buffer will be flushed before the next
+ * normal,non-interrupt SYSLOG output.
+ */
+
+ return syslog_add_intbuffer(ch);
Review comment:
The same ch may put into the interrupt buffer duplicately.
##########
File path: drivers/syslog/syslog_flush.c
##########
@@ -64,21 +64,27 @@
int syslog_flush(void)
{
- DEBUGASSERT(g_syslog_channel != NULL);
+ int i;
+
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ break;
#ifdef CONFIG_SYSLOG_INTBUFFER
- /* Flush any characters that may have been added to the interrupt
- * buffer.
- */
+ /* Flush any characters that may have been added to the interrupt
+ * buffer.
+ */
- syslog_flush_intbuffer(g_syslog_channel, true);
+ syslog_flush_intbuffer(g_syslog_channel[i], true);
Review comment:
We just have one interrupt buffer, so the loop need move into syslog_flush_intbuffer otherwise, all channel except the first one lose the log happen in the interrupt handler.
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
There are more work to support the add/remove channel dynamically:
1. All methods need be protected by spin lock or critical section.
2. Need notify syslog driver it has been removed
So, it's may better to split the work into two part:
1. Support register multiple channels only in the initialization phase
2. Support register/unregister the channel in the runtime dynamically
##########
File path: drivers/syslog/syslog_putc.c
##########
@@ -55,48 +55,57 @@
int syslog_putc(int ch)
{
- DEBUGASSERT(g_syslog_channel != NULL);
+ int i;
- /* Is this an attempt to do SYSLOG output from an interrupt handler? */
-
- if (up_interrupt_context() || sched_idletask())
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
{
-#if defined(CONFIG_SYSLOG_INTBUFFER)
- if (up_interrupt_context())
- {
- /* Buffer the character in the interrupt buffer.
- * The interrupt buffer will be flushed before the next normal,
- * non-interrupt SYSLOG output.
- */
+ if (g_syslog_channel[i] == NULL)
+ break;
+
+ /* Is this an attempt to do SYSLOG output from an interrupt handler? */
- return syslog_add_intbuffer(ch);
+ if (up_interrupt_context() || sched_idletask())
+ {
+#if defined(CONFIG_SYSLOG_INTBUFFER)
+ if (up_interrupt_context())
+ {
+ /* Buffer the character in the interrupt buffer.
+ * The interrupt buffer will be flushed before the next
+ * normal,non-interrupt SYSLOG output.
+ */
+
+ return syslog_add_intbuffer(ch);
+ }
+ else
+#endif
+ {
+ /* Force the character to the SYSLOG device immediately
+ * (if possible).
+ * This means that the interrupt data may not be in
+ * synchronization with output data that may have been
+ * buffered by sc_putc().
+ */
+
+ DEBUGASSERT(g_syslog_channel[i]->sc_force != NULL);
+
+ g_syslog_channel[i]->sc_force(ch);
+ }
}
else
-#endif
{
- /* Force the character to the SYSLOG device immediately
- * (if possible).
- * This means that the interrupt data may not be in synchronization
- * with output data that may have been buffered by sc_putc().
- */
-
- DEBUGASSERT(g_syslog_channel->sc_force != NULL);
-
- return g_syslog_channel->sc_force(ch);
- }
- }
- else
- {
- DEBUGASSERT(g_syslog_channel->sc_putc != NULL);
+ DEBUGASSERT(g_syslog_channel[i]->sc_putc != NULL);
#ifdef CONFIG_SYSLOG_INTBUFFER
- /* Flush any characters that may have been added to the interrupt
- * buffer.
- */
+ /* Flush any characters that may have been added to the interrupt
+ * buffer.
+ */
- syslog_flush_intbuffer(g_syslog_channel, false);
+ syslog_flush_intbuffer(g_syslog_channel[i], false);
Review comment:
ditto
##########
File path: drivers/syslog/syslog_write.c
##########
@@ -56,37 +56,44 @@
static ssize_t syslog_default_write(FAR const char *buffer, size_t buflen)
{
- size_t nwritten;
+ int i;
+ size_t nwritten = 0;
- if (up_interrupt_context() || sched_idletask())
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
{
- for (nwritten = 0; nwritten < buflen; nwritten++)
+ if (g_syslog_channel[i] == NULL)
+ break;
+
+ if (up_interrupt_context() || sched_idletask())
{
-#ifdef CONFIG_SYSLOG_INTBUFFER
- if (up_interrupt_context())
+ for (nwritten = 0; nwritten < buflen; nwritten++)
{
- syslog_add_intbuffer(*buffer++);
- }
- else
+#ifdef CONFIG_SYSLOG_INTBUFFER
+ if (up_interrupt_context())
+ {
+ syslog_add_intbuffer(*buffer++);
Review comment:
ditto
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599576677
##########
File path: drivers/syslog/syslog_putc.c
##########
@@ -55,48 +55,57 @@
int syslog_putc(int ch)
{
- DEBUGASSERT(g_syslog_channel != NULL);
+ int i;
- /* Is this an attempt to do SYSLOG output from an interrupt handler? */
-
- if (up_interrupt_context() || sched_idletask())
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
{
-#if defined(CONFIG_SYSLOG_INTBUFFER)
- if (up_interrupt_context())
- {
- /* Buffer the character in the interrupt buffer.
- * The interrupt buffer will be flushed before the next normal,
- * non-interrupt SYSLOG output.
- */
+ if (g_syslog_channel[i] == NULL)
+ break;
+
+ /* Is this an attempt to do SYSLOG output from an interrupt handler? */
- return syslog_add_intbuffer(ch);
+ if (up_interrupt_context() || sched_idletask())
+ {
+#if defined(CONFIG_SYSLOG_INTBUFFER)
+ if (up_interrupt_context())
+ {
+ /* Buffer the character in the interrupt buffer.
+ * The interrupt buffer will be flushed before the next
+ * normal,non-interrupt SYSLOG output.
+ */
+
+ return syslog_add_intbuffer(ch);
+ }
+ else
+#endif
+ {
+ /* Force the character to the SYSLOG device immediately
+ * (if possible).
+ * This means that the interrupt data may not be in
+ * synchronization with output data that may have been
+ * buffered by sc_putc().
+ */
+
+ DEBUGASSERT(g_syslog_channel[i]->sc_force != NULL);
+
+ g_syslog_channel[i]->sc_force(ch);
+ }
}
else
-#endif
{
- /* Force the character to the SYSLOG device immediately
- * (if possible).
- * This means that the interrupt data may not be in synchronization
- * with output data that may have been buffered by sc_putc().
- */
-
- DEBUGASSERT(g_syslog_channel->sc_force != NULL);
-
- return g_syslog_channel->sc_force(ch);
- }
- }
- else
- {
- DEBUGASSERT(g_syslog_channel->sc_putc != NULL);
+ DEBUGASSERT(g_syslog_channel[i]->sc_putc != NULL);
#ifdef CONFIG_SYSLOG_INTBUFFER
- /* Flush any characters that may have been added to the interrupt
- * buffer.
- */
+ /* Flush any characters that may have been added to the interrupt
+ * buffer.
+ */
- syslog_flush_intbuffer(g_syslog_channel, false);
+ syslog_flush_intbuffer(g_syslog_channel[i], false);
Review comment:
fixed
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593923766
##########
File path: drivers/syslog/syslog_force.c
##########
@@ -55,18 +55,27 @@
int syslog_force(int ch)
{
- DEBUGASSERT(g_syslog_channel != NULL &&
- g_syslog_channel->sc_force != NULL);
+ int i;
+
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ break;
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r599715309
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -135,14 +139,82 @@ static int syslog_default_putc(int ch)
int syslog_channel(FAR const struct syslog_channel_s *channel)
{
+#if (CONFIG_SYSLOG_MAX_CHANNELS != 1)
+ int i;
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
+#if (CONFIG_SYSLOG_MAX_CHANNELS == 1)
+ g_syslog_channel[0] = channel;
return OK;
+#else
+ for (i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
+ {
+ if (g_syslog_channel[i] == NULL)
+ {
+ g_syslog_channel[i] = channel;
+ return OK;
+ }
+ else if (g_syslog_channel[i] == channel)
+ {
+ return OK;
+ }
+ }
+#endif
+ }
+
+ return -EINVAL;
+}
+
+/****************************************************************************
+ * Name: syslog_channel_remove
+ *
+ * Description:
+ * Removes an already configured SYSLOG channel from the list of used
+ * channels.
+ *
+ * Input Parameters:
+ * channel - Provides the interface to the channel to be removed.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success. A negated errno value is returned
+ * on any failure.
+ *
+ ****************************************************************************/
+
+int syslog_channel_remove(FAR const struct syslog_channel_s *channel)
Review comment:
I am not sure if this is now the issue, but the buffered syslog output is surely to have problem too.
If the output string is longer than `CONFIG_IOB_BUFSIZE`, then the buffer will be outputed prematurely, causing the exact same race condition as without buffer.
The fact that the buffer may be flushed before the whole syslog message has been written to it, defeats completely its puprose...
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#issuecomment-803317118
There is something that I do not really understand, regarding removing syslog channels.
In case of `syslog_channel_remove`, or even previously when `syslog_channel` was overwritting the existing channel.
How do the resources of the old channel were deallocated?
For example, when `syslog_file_channel` is used, and then this channel is removed, how the underlying file will be closed?
*Maybe there is more to be done in this PR regarding this aspect?*
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593915420
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -141,8 +145,14 @@ int syslog_channel(FAR const struct syslog_channel_s *channel)
{
DEBUGASSERT(channel->sc_putc != NULL && channel->sc_force != NULL);
- g_syslog_channel = channel;
- return OK;
+ for (int i = 0; i < CONFIG_SYSLOG_MAX_CHANNELS; i++)
Review comment:
The problem by changing `syslog_channel` from replacement to addition will break the usage in the default config(CONFIG_SYSLOG_MAX_CHANNELS == 1), because syslog_channel always fail to find the empty slot. We should at leaset keep the replacement semantics in case of CONFIG_SYSLOG_MAX_CHANNELS == 1.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3050: Added support for multiple syslog channels.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3050:
URL: https://github.com/apache/incubator-nuttx/pull/3050#discussion_r593923184
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -92,7 +92,11 @@ static const struct syslog_channel_s g_default_channel =
/* This is the current syslog channel in use */
-FAR const struct syslog_channel_s *g_syslog_channel = &g_default_channel;
+FAR const struct syslog_channel_s
+ *g_syslog_channel[CONFIG_SYSLOG_MAX_CHANNELS] =
+ {
+ &g_default_channel
+ };
Review comment:
Can you please explain which space do you refer to?
----------------------------------------------------------------
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:
users@infra.apache.org