[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

2021-06-01 Thread GitBox 


ganeshmurthy commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643221281



##
File path: src/log.c
##
@@ -518,6 +544,7 @@ void qd_log_initialize(void)
 default_log_source->includeTimestamp = true;
 default_log_source->includeSource = 0;
 default_log_source->sink = log_sink_lh(SINK_STDERR);
+default_log_source->add_log_stats = false;

Review comment:
   I wonder if the call to ZERO should be moved inside the 
qd_log_source_defaults() function. If we do that, the qd_log_source_defaults() 
can be called from the place where ZERO is currently being called from. And 
then, there would be no need to set the add_log_stats to false.




-- 
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



-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

2021-06-01 Thread GitBox 


ganeshmurthy commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643210620



##
File path: src/log.c
##
@@ -518,6 +544,7 @@ void qd_log_initialize(void)
 default_log_source->includeTimestamp = true;
 default_log_source->includeSource = 0;
 default_log_source->sink = log_sink_lh(SINK_STDERR);
+default_log_source->add_log_stats = false;

Review comment:
   If you look at 
https://github.com/apache/qpid-dispatch/blob/main/src/log.c#L368, the 
ZERO(log_source); is called. This sets all bits to 0 on the log_source which 
then automatically sets the add_log_stats to false, so no need to set the 
add_log_stats explicitly to false in qd_log_source_defaults() which is called 
right after the call to ZERO(log_source); 
   But yes, if ZERO(log_source);  was not called in the future, then 
qd_log_source_defaults() would take care of defaulting the add_log_stats flag. 




-- 
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



-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

2021-06-01 Thread GitBox 


ganeshmurthy commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643192133



##
File path: src/log.c
##
@@ -648,8 +685,12 @@ qd_error_t qd_log_entity(qd_entity_t *entity)
 src->includeSource = include_source;
 }
 
+bool add_log_stats = src->add_log_stats;
+src->add_log_stats = false;
 sys_mutex_unlock(log_source_lock);
-
+if(add_log_stats) {
+qd_entity_cache_add(QD_LOG_STATS_TYPE, src);

Review comment:
   I am unsure why we need to add the QD_LOG_STATS_TYPE here ?
   I commented this line and the logStats was still created because 
qd_log_source() is creating 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:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

2021-06-01 Thread GitBox 


ganeshmurthy commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643129435



##
File path: src/log.c
##
@@ -518,6 +544,7 @@ void qd_log_initialize(void)
 default_log_source->includeTimestamp = true;
 default_log_source->includeSource = 0;
 default_log_source->sink = log_sink_lh(SINK_STDERR);
+default_log_source->add_log_stats = false;

Review comment:
   This line might not be necessary because the call to qd_log_source() 
already sets the add_log_stats flag to false.




-- 
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



-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

2021-06-01 Thread GitBox 


ganeshmurthy commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643105917



##
File path: src/log.c
##
@@ -379,7 +398,14 @@ qd_log_source_t *qd_log_source(const char *module)
 {
 sys_mutex_lock(log_source_lock);
 qd_log_source_t* src = qd_log_source_lh(module);
+// Note to Mick: Mick! Please don't access log_source fields
+// outside of the log_source lock. That's why there's a lock, you know.
+bool add_me = src->add_me;

Review comment:
   Can we please call this flag add_log_stats or something instead of 
add_me ?




-- 
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



-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org