[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1146: DISPATCH-2055: Eliminated log_lock and used the log_source_lock instead

[GitBox <gi...@apache.org>]

ChugR commented on a change in pull request #1146:
URL: https://github.com/apache/qpid-dispatch/pull/1146#discussion_r622470098



##########
File path: src/log.c
##########
@@ -222,27 +222,36 @@ static level_t levels[] = {
     LEVEL("critical", QD_LOG_CRITICAL, LOG_CRIT)
 };
 
+static const level_t invalid_level = {"invalid", -2, -2, 0};
+
 static char level_names[TEXT_MAX] = {0}; /* Set up in qd_log_initialize */
 
 /// Return NULL and set qd_error if not a valid bit.
-static const level_t* level_for_bit(int bit) {
+static const level_t* level_for_bit(int bit, bool log_error) {
     level_index_t i = 0;
     while (i < N_LEVELS && levels[i].bit != bit) ++i;
     if (i == N_LEVELS) {
-        qd_error(QD_ERROR_CONFIG, "'%d' is not a valid log level bit.", bit);
-        return NULL;
+        if (log_error)
+            qd_error(QD_ERROR_CONFIG, "'%d' is not a valid log level bit.", bit);
+        return &invalid_level;
     }
     return &levels[i];
 }
 
+static bool is_log_level_invalid(const level_t *level)
+{
+    return ! strcmp(level->name, "invalid");

Review comment:
       A better (faster) comparison might be 
       level->bit != -2 
   Then inline it.

##########
File path: src/log.c
##########
@@ -425,7 +434,6 @@ void qd_vlog_impl(qd_log_source_t *source, qd_log_level_t level, bool check_leve
     }
 
     // Bounded buffer of log entries, keep most recent.
-    sys_mutex_lock(log_lock);
     qd_log_entry_t *entry = new_qd_log_entry_t();
     DEQ_ITEM_INIT(entry);
     entry->module = source->module;

Review comment:
       source->module is a char*. Copying the pointer to entry leaves it vulnerable to deletion as described in the comment at line 446. This string should be copied into entry the same way that entry->file is strdup'd on line 441. Then the entry is not depending on source still existing the whole time entry is in the entries list (line 542).




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



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