From 0fc068cc9c5ab6832478561804b667f477a31cba Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Fri, 12 May 2017 06:58:33 -0600 Subject: [PATCH 1/2] syslog: Avoid flushing syslog_stream buffer, if possible, until lib_vsprintf() completely parses the format. This assures that the flush will flush the entire output, even data that may potentially follow the linefeed. And, in that case, it cannot be interleaved with other devug output. Suggested by Jussi Kivilinna. --- TODO | 2 +- drivers/syslog/syslog_stream.c | 100 ++++++++++++++++----------------- drivers/syslog/vsyslog.c | 6 +- 3 files changed, 52 insertions(+), 56 deletions(-) diff --git a/TODO b/TODO index c0b3443a83..25e5127b8d 100644 --- a/TODO +++ b/TODO @@ -490,7 +490,7 @@ o pthreads (sched/pthreads) Status: Not really open. This is just the way it is. Priority: Nothing additional is planned. - Title: PTHREAD FILES IN WRONG LOCATTION + Title: PTHREAD FILES IN WRONG LOCATION Description: There are many pthread interface functions in files located in sched/pthread. These should be moved from that location to libc/pthread. In the flat build, this really does not matter, diff --git a/drivers/syslog/syslog_stream.c b/drivers/syslog/syslog_stream.c index ac7e144ceb..b463a3e5ef 100644 --- a/drivers/syslog/syslog_stream.c +++ b/drivers/syslog/syslog_stream.c @@ -54,6 +54,46 @@ * Private Functions ****************************************************************************/ +/**************************************************************************** + * Name: syslogstream_flush + ****************************************************************************/ + +#ifdef CONFIG_SYSLOG_BUFFER +static int syslogstream_flush(FAR struct lib_syslogstream_s *stream) +{ + FAR struct iob_s *iob; + int ret = OK; + + DEBUGASSERT(stream != NULL); + iob = stream->iob; + + /* Do we have an IO buffer? Is there anything buffered? */ + + if (iob != NULL && iob->io_len > 0) + { + /* Yes write the buffered data */ + + do + { + ssize_t nbytes = syslog_write((FAR const char *)iob->io_data, + (size_t)iob->io_len); + if (nbytes < 0) + { + ret = -get_errno(); + } + else + { + iob->io_len = 0; + ret = OK; + } + } + while (ret == -EINTR); + } + + return ret; +} +#endif + /**************************************************************************** * Name: syslogstream_putc ****************************************************************************/ @@ -88,7 +128,7 @@ static void syslogstream_putc(FAR struct lib_outstream_s *this, int ch) { /* Yes.. then flush the buffer */ - (void)this->flush(this); + syslogstream_flush(stream); } } else @@ -123,48 +163,6 @@ static void syslogstream_putc(FAR struct lib_outstream_s *this, int ch) } } -/**************************************************************************** - * Name: syslogstream_flush - ****************************************************************************/ - -#ifdef CONFIG_SYSLOG_BUFFER -static int syslogstream_flush(FAR struct lib_outstream_s *this) -{ - FAR struct lib_syslogstream_s *stream; - FAR struct iob_s *iob; - int ret = OK; - - DEBUGASSERT(this != NULL); - stream = (FAR struct lib_syslogstream_s *)this; - iob = stream->iob; - - /* Do we have an IO buffer? Is there anything buffered? */ - - if (iob != NULL && iob->io_len > 0) - { - /* Yes write the buffered data */ - - do - { - ssize_t nbytes = syslog_write((FAR const char *)iob->io_data, - (size_t)iob->io_len); - if (nbytes < 0) - { - ret = -get_errno(); - } - else - { - iob->io_len = 0; - ret = OK; - } - } - while (ret == -EINTR); - } - - return ret; -} -#endif - /**************************************************************************** * Public Functions ****************************************************************************/ @@ -187,17 +185,19 @@ static int syslogstream_flush(FAR struct lib_outstream_s *this) void syslogstream_create(FAR struct lib_syslogstream_s *stream) { - DEBUGASSERT(stream != NULL); - #ifdef CONFIG_SYSLOG_BUFFER FAR struct iob_s *iob; +#endif + + DEBUGASSERT(stream != NULL); /* Initialize the common fields */ stream->public.put = syslogstream_putc; - stream->public.flush = syslogstream_flush; + stream->public.flush = lib_noflush; stream->public.nput = 0; +#ifdef CONFIG_SYSLOG_BUFFER /* Allocate an IOB */ iob = iob_alloc(true); @@ -211,10 +211,6 @@ void syslogstream_create(FAR struct lib_syslogstream_s *stream) iob->io_offset = 0; iob->io_pktlen = 0; } -#else - stream->public.put = syslogstream_putc; - stream->public.flush = lib_noflush; - stream->public.nput = 0; #endif } @@ -242,6 +238,10 @@ void syslogstream_destroy(FAR struct lib_syslogstream_s *stream) if (stream->iob != NULL) { + /* Flush the output buffered in the IOB */ + + syslogstream_flush(stream); + /* Free the IOB */ iob_free(stream->iob); diff --git a/drivers/syslog/vsyslog.c b/drivers/syslog/vsyslog.c index 0b993b7186..76ba239968 100644 --- a/drivers/syslog/vsyslog.c +++ b/drivers/syslog/vsyslog.c @@ -131,12 +131,8 @@ int _vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap) ret = lib_vsprintf(&stream.public, fmt, *ap); - /* Flush the output */ - - stream.public.flush(&stream.public); - #ifdef CONFIG_SYSLOG_BUFFER - /* Destroy the syslog stream buffer */ + /* Flush and destroy the syslog stream buffer */ if (priority != LOG_EMERG) { From 1c9859520fc3b6d6bf5ad425a546a93d94695ebb Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Fri, 12 May 2017 07:31:50 -0600 Subject: [PATCH 2/2] syslog: There is yet another place where the output can get split. That is in syslog_dev_write(): It will break up the stream to insert a CR before the LF. This can that can be avoid be generating the CR-LF sequence in the buffer and then detecting and ignoring valid CR-LF sequences, rather than expecting syslog_dev_write() to insert the CR in this case. I don't like the idea that syslog_dev_write() still scans the entire output buffer to expand CR-LF sequence. This seems really wasteful, especially in this case where we can be sure that the is no CR or LF without a matching LF or CR. Bu, I think, the existing behavior in syslog_dev_write() must be retained because it is needed in other contexts. --- drivers/syslog/syslog_device.c | 15 +++++++++- drivers/syslog/syslog_stream.c | 55 ++++++++++++++++++++++++---------- 2 files changed, 53 insertions(+), 17 deletions(-) diff --git a/drivers/syslog/syslog_device.c b/drivers/syslog/syslog_device.c index 0d49ea4e8a..4cc65734ad 100644 --- a/drivers/syslog/syslog_device.c +++ b/drivers/syslog/syslog_device.c @@ -518,9 +518,22 @@ ssize_t syslog_dev_write(FAR const char *buffer, size_t buflen) remaining > 0; endptr++, remaining--) { + bool crlf = false; + + /* Check for a CR-LF sequence */ + + if (remaining > 1 && + ((endptr[0] == '\r' && endptr[1] == '\n') || + (endptr[0] == '\n' && endptr[1] == '\r'))) + { + endptr++; + remaining--; + crlf = true; + } + /* Special case carriage return and line feed */ - if (*endptr == '\r' || *endptr == '\n') + if (!crlf && (*endptr == '\r' || *endptr == '\n')) { /* Write everything up to this point, ignore the special * character. diff --git a/drivers/syslog/syslog_stream.c b/drivers/syslog/syslog_stream.c index b463a3e5ef..2119b9e917 100644 --- a/drivers/syslog/syslog_stream.c +++ b/drivers/syslog/syslog_stream.c @@ -94,6 +94,33 @@ static int syslogstream_flush(FAR struct lib_syslogstream_s *stream) } #endif +/**************************************************************************** + * Name: syslogstream_addchar + ****************************************************************************/ + +static void syslogstream_addchar(FAR struct lib_syslogstream_s *stream, int ch) +{ + FAR struct iob_s *iob = stream->iob; + + /* Add the incoming character to the buffer */ + + iob->io_data[iob->io_len] = ch; + iob->io_len++; + + /* Increment the total number of bytes buffered. */ + + stream->public.nput++; + + /* Is the buffer full? */ + + if (iob->io_len >= CONFIG_IOB_BUFSIZE) + { + /* Yes.. then flush the buffer */ + + syslogstream_flush(stream); + } +} + /**************************************************************************** * Name: syslogstream_putc ****************************************************************************/ @@ -105,31 +132,27 @@ static void syslogstream_putc(FAR struct lib_outstream_s *this, int ch) if (ch != '\r') { #ifdef CONFIG_SYSLOG_BUFFER - FAR struct lib_syslogstream_s *stream; - FAR struct iob_s *iob; + FAR struct lib_syslogstream_s *stream = + (FAR struct lib_syslogstream_s *)this; - DEBUGASSERT(this != NULL); - stream = (FAR struct lib_syslogstream_s *)this; - iob = stream->iob; + DEBUGASSERT(stream != NULL); /* Do we have an IO buffer? */ - if (iob != NULL) + if (stream->iob != NULL) { - /* Yes.. Add the incoming character to the buffer */ + /* Is this a linefeed? */ - iob->io_data[iob->io_len] = ch; - iob->io_len++; - this->nput++; - - /* Is the buffer full? */ - - if (iob->io_len >= CONFIG_IOB_BUFSIZE) + if (ch == '\n') { - /* Yes.. then flush the buffer */ + /* Yes... pre-pend carriage return */ - syslogstream_flush(stream); + syslogstream_addchar(stream, '\r'); } + + /* Add the incoming character to the buffer */ + + syslogstream_addchar(stream, ch); } else #endif