You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/06/02 02:14:53 UTC
[GitHub] [logging-log4cxx] rm5248 opened a new pull request, #111: Logcxx 556
rm5248 opened a new pull request, #111:
URL: https://github.com/apache/logging-log4cxx/pull/111
Fix a few issues with the syslog appender:
- Add default layout if none was provided(segfault when using default constructor)
- Check for minimum message size
- Added test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a diff in pull request #111: Logcxx 556
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on code in PR #111:
URL: https://github.com/apache/logging-log4cxx/pull/111#discussion_r887619267
##########
src/main/cpp/syslogappender.cpp:
##########
@@ -326,11 +327,17 @@ void SyslogAppender::append(const spi::LoggingEventPtr& event, Pool& p)
while ( start != msg.end() )
{
- LogString::iterator end = start + maxMessageLength - 12;
+ LogString::iterator end;// = start + maxMessageLength - 12;
+
+// if ( end > msg.end() )
+// {
+// end = msg.end();
+// }
- if ( end > msg.end() )
- {
+ if( msg.size() > (maxMessageLength - MINIMUM_MESSAGE_SIZE) ) {
Review Comment:
`msg.size() > (maxMessageLength - MINIMUM_MESSAGE_SIZE)` seems to not change within the loop, so put it above to declaration of `end`?
##########
src/main/cpp/syslogappender.cpp:
##########
@@ -326,11 +327,17 @@ void SyslogAppender::append(const spi::LoggingEventPtr& event, Pool& p)
while ( start != msg.end() )
{
- LogString::iterator end = start + maxMessageLength - 12;
+ LogString::iterator end;// = start + maxMessageLength - 12;
+
+// if ( end > msg.end() )
+// {
+// end = msg.end();
+// }
- if ( end > msg.end() )
- {
+ if( msg.size() > (maxMessageLength - MINIMUM_MESSAGE_SIZE) ) {
Review Comment:
`maxMessageLength - MINIMUM_MESSAGE_SIZE` looks like some sort of `msgWindowLen[gth]`? Doesn't seem to change, so might be maintained on instance level, calculated with `setMaxMessageLength`. Makes code easier to understand in my opinion, like with removing `12` in favour of some name.
##########
src/main/cpp/syslogappender.cpp:
##########
@@ -326,11 +327,12 @@ void SyslogAppender::append(const spi::LoggingEventPtr& event, Pool& p)
while ( start != msg.end() )
{
- LogString::iterator end = start + maxMessageLength - 12;
+ LogString::iterator end;
- if ( end > msg.end() )
- {
+ if( msg.size() > (maxMessageLength - MINIMUM_MESSAGE_SIZE) ) {
Review Comment:
Line 349 contains the following line which might be changed to using `MINIMUM_MESSAGE_SIZE` instead:
char buf[12];
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 merged pull request #111: Logcxx 556
Posted by GitBox <gi...@apache.org>.
rm5248 merged PR #111:
URL: https://github.com/apache/logging-log4cxx/pull/111
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a diff in pull request #111: Logcxx 556
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on code in PR #111:
URL: https://github.com/apache/logging-log4cxx/pull/111#discussion_r888676793
##########
src/main/cpp/syslogappender.cpp:
##########
@@ -322,33 +323,37 @@ void SyslogAppender::append(const spi::LoggingEventPtr& event, Pool& p)
if ( msg.size() > maxMessageLength )
{
+ int numberOfFullChunks = msg.size() / maxMessageLength;
LogString::iterator start = msg.begin();
+ LogString::iterator end = msg.begin() + maxMessageLength;
Review Comment:
> // Note: MSVC/Windows does not like it when you seek past the end of the string
Doesn't that comment apply for this line as well? Shouldn't there be a check for number of chunks aka the length of the string being greater than `maxMessageLength`?
LogString::iterator end = numberOfFullChunks > 0|1 ? (msg.begin() + maxMessageLength) : msg.end();
Not sure about the case when message length and `maxMessageLength` are equal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a diff in pull request #111: Logcxx 556
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on code in PR #111:
URL: https://github.com/apache/logging-log4cxx/pull/111#discussion_r889528235
##########
src/main/cpp/syslogappender.cpp:
##########
@@ -322,33 +323,37 @@ void SyslogAppender::append(const spi::LoggingEventPtr& event, Pool& p)
if ( msg.size() > maxMessageLength )
{
+ int numberOfFullChunks = msg.size() / maxMessageLength;
LogString::iterator start = msg.begin();
+ LogString::iterator end = msg.begin() + maxMessageLength;
Review Comment:
I didn't recognize the `if` before. :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #111: Logcxx 556
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #111:
URL: https://github.com/apache/logging-log4cxx/pull/111#discussion_r888519002
##########
src/main/cpp/syslogappender.cpp:
##########
@@ -326,11 +327,17 @@ void SyslogAppender::append(const spi::LoggingEventPtr& event, Pool& p)
while ( start != msg.end() )
{
- LogString::iterator end = start + maxMessageLength - 12;
+ LogString::iterator end;// = start + maxMessageLength - 12;
+
+// if ( end > msg.end() )
+// {
+// end = msg.end();
+// }
- if ( end > msg.end() )
- {
+ if( msg.size() > (maxMessageLength - MINIMUM_MESSAGE_SIZE) ) {
Review Comment:
Actually, that brings up a different point: what I have doesn't work in the first place - what should be happening is that you should be splitting it up into N messages. The issue indicates that windows does not like to have an iterator past the end of the string(which this code does). So it really just needs to calculate the N chunks at the beginning and do the correct number of iterations to split up the string properly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #111: Logcxx 556
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #111:
URL: https://github.com/apache/logging-log4cxx/pull/111#discussion_r889525325
##########
src/main/cpp/syslogappender.cpp:
##########
@@ -322,33 +323,37 @@ void SyslogAppender::append(const spi::LoggingEventPtr& event, Pool& p)
if ( msg.size() > maxMessageLength )
{
+ int numberOfFullChunks = msg.size() / maxMessageLength;
LogString::iterator start = msg.begin();
+ LogString::iterator end = msg.begin() + maxMessageLength;
Review Comment:
Unless I have something wrong with my math, that line of code should only be relevant if the message is larger than the maximum message size(in which case it needs to split it up into several chunks).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org