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:
us...@infra.apache.org
