kgiusti commented on a change in pull request #1366:
URL: https://github.com/apache/qpid-dispatch/pull/1366#discussion_r707394576
##########
File path: src/log.c
##########
@@ -365,41 +390,21 @@ static void qd_log_source_defaults(qd_log_source_t
*log_source) {
memset ( log_source->severity_histogram, 0, sizeof(uint64_t) *
(N_LEVEL_INDICES) );
}
-/// Caller must hold the log_source_lock
-static qd_log_source_t *qd_log_source_lh(const char *module)
-{
- qd_log_source_t *log_source = lookup_log_source_lh(module);
- if (!log_source)
- {
- log_source = NEW(qd_log_source_t);
- ZERO(log_source);
- log_source->module = (char*) malloc(strlen(module) + 1);
- strcpy(log_source->module, module);
- qd_log_source_defaults(log_source);
- DEQ_INSERT_TAIL(source_list, log_source);
- qd_entity_cache_add(QD_LOG_STATS_TYPE, log_source);
- }
- return log_source;
-}
-
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);
- sys_mutex_unlock(log_source_lock);
+ qd_log_source_t* src = lookup_log_source(module);
return src;
}
qd_log_source_t *qd_log_source_reset(const char *module)
Review comment:
Issue: qd_log_source_defaults changes the log_source's data without
holding the source's lock. qd_log_source_reset() is called by the mgmt thread
so it is modifying the log source without holding the lock.
##########
File path: src/log.c
##########
@@ -353,7 +378,7 @@ static void write_log(qd_log_source_t *log_source,
qd_log_entry_t *entry)
if (syslog_level != -1)
syslog(syslog_level, "%s", log_str);
}
- sys_mutex_unlock(log_sink_list_lock);
+ sys_mutex_unlock(log_source->lock);
}
/// Reset the log source to the default state
Review comment:
log_source->sink = 0 will drop a reference to the sink without
decref'ing the sink's reference count
##########
File path: src/log.c
##########
@@ -59,12 +88,11 @@ struct qd_log_entry_t {
struct timeval time;
char text[TEXT_MAX];
};
-
ALLOC_DECLARE(qd_log_entry_t);
ALLOC_DEFINE(qd_log_entry_t);
-
DEQ_DECLARE(qd_log_entry_t, qd_log_list_t);
static qd_log_list_t entries = {0};
+sys_mutex_t * entries_lock = 0;
Review comment:
code convention: please remove space between * and entries_lock
##########
File path: src/log.c
##########
@@ -506,23 +504,49 @@ PyObject *qd_log_recent_py(long limit) {
return NULL;
}
+static void _add_log_source (const char * module_name) {
+ qd_log_source_t *log_source;
+ log_source = NEW(qd_log_source_t);
+ ZERO(log_source);
+ log_source->module = (char*) malloc(strlen(module_name) + 1);
Review comment:
pro-tip: replace lines 511+512 with a single call to qd_strdup (see
ctools.h).
##########
File path: src/log.c
##########
@@ -365,41 +390,21 @@ static void qd_log_source_defaults(qd_log_source_t
*log_source) {
memset ( log_source->severity_histogram, 0, sizeof(uint64_t) *
(N_LEVEL_INDICES) );
}
-/// Caller must hold the log_source_lock
-static qd_log_source_t *qd_log_source_lh(const char *module)
-{
- qd_log_source_t *log_source = lookup_log_source_lh(module);
- if (!log_source)
- {
- log_source = NEW(qd_log_source_t);
- ZERO(log_source);
- log_source->module = (char*) malloc(strlen(module) + 1);
- strcpy(log_source->module, module);
- qd_log_source_defaults(log_source);
- DEQ_INSERT_TAIL(source_list, log_source);
- qd_entity_cache_add(QD_LOG_STATS_TYPE, log_source);
- }
- return log_source;
-}
-
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);
- sys_mutex_unlock(log_source_lock);
+ qd_log_source_t* src = lookup_log_source(module);
return src;
}
qd_log_source_t *qd_log_source_reset(const char *module)
{
- sys_mutex_lock(log_source_lock);
- qd_log_source_t* src = qd_log_source_lh(module);
+ qd_log_source_t* src = qd_log_source(module);
qd_log_source_defaults(src);
- sys_mutex_unlock(log_source_lock);
return src;
}
-static void qd_log_source_free_lh(qd_log_source_t* src) {
+// This is called only during finalize, which does not hold locks.
+static void qd_log_source_free(qd_log_source_t* src) {
Review comment:
Need to free src->lock as well
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
