Use := as separator and remove multiple logger settings in one argument

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com> Co-authored-by: Tomoya.Fujita <Tomoya.Fujita@sony.com> Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
ros2 · Jun 30, 2020 · 03047a9 · 03047a9
1 parent 57f93b5
commit 03047a9
Show file tree

Hide file tree

Showing 7 changed files with 128 additions and 167 deletions.
diff --git a/rcl/include/rcl/log_level.h b/rcl/include/rcl/log_level.h
@@ -25,22 +25,25 @@ extern "C"
 {
 #endif
 
-/// A logger item to specify a name and log_level
+/// A logger item to specify a name and a log level
 typedef struct rcl_logger_setting_t
 {
   /// name for the logger.
   char * name;
   /// level for the logger.
-  int level;
+  rcl_log_severity_t level;
 } rcl_logger_setting_t;
 
-/// Hold log_level.
+/// Hold default logger level and other logger setting.
 typedef struct rcl_log_level_t
 {
-  /// Default log level (represented by `RCUTILS_LOG_SEVERITY` enum) or -1 if not specified.
-  int default_log_level;
-  struct rcl_logger_setting_t * logger_settings;   ///<  Array of logger
-  size_t num_loggers;       ///< Number of loggers
+  /// Default logger level
+  rcl_log_severity_t default_logger_level;
+  /// Array of logger setting
+  struct rcl_logger_setting_t * logger_settings;
+  /// Number of logger settings
+  size_t num_logger_settings;
+  /// Allocator used to allocate objects in this struct
   rcl_allocator_t allocator;
 } rcl_log_level_t;
 
@@ -51,7 +54,7 @@ typedef struct rcl_log_level_t
 RCL_PUBLIC
 RCL_WARN_UNUSED
 rcl_log_level_t *
-rcl_log_level_init(const rcutils_allocator_t allocator);
+rcl_log_level_init(const rcl_allocator_t allocator);
 
 /// \brief Copy log level structure, allocate memory by using allocator of rcl_log_level_t
 /// \param[in] log_level points to the log level struct to be copied
@@ -62,7 +65,7 @@ rcl_log_level_t * rcl_log_level_copy(const rcl_log_level_t * log_level);
 
 
 /// \brief Free log level structure
-/// \param[in] log_level points to the populated log level structure
+/// \param[in] log_level points to the log level structure to be freed
 RCL_PUBLIC
 void
 rcl_log_level_fini(rcl_log_level_t * log_level);

diff --git a/rcl/include/rcl/types.h b/rcl/include/rcl/types.h
@@ -114,4 +114,7 @@ typedef rmw_ret_t rcl_ret_t;
 /// typedef for rmw_serialized_message_t;
 typedef rmw_serialized_message_t rcl_serialized_message_t;
 
+/// typedef for RCUTILS_LOG_SEVERITY;
+typedef enum RCUTILS_LOG_SEVERITY rcl_log_severity_t;
+
 #endif  // RCL__TYPES_H_
diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c
@@ -42,8 +42,7 @@ extern "C"
 
 #define RCL_ENABLE_FLAG_PREFIX "--enable-"
 #define RCL_DISABLE_FLAG_PREFIX "--disable-"
-#define RCL_LOG_LEVEL_FLAG_COMMA ","
-#define RCL_LOG_LEVEL_FLAG_SEPARATOR "="
+#define RCL_LOG_LEVEL_FLAG_SEPARATOR ":="
 
 /// Parse an argument that may or may not be a remap rule.
 /**
@@ -151,15 +150,15 @@ rcl_arguments_get_log_level(
 {
   RCL_CHECK_ARGUMENT_FOR_NULL(arguments, RCL_RET_INVALID_ARGUMENT);
   RCL_CHECK_ARGUMENT_FOR_NULL(arguments->impl, RCL_RET_INVALID_ARGUMENT);
-  RCL_CHECK_ARGUMENT_FOR_NULL(arguments->impl->log_level, RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(arguments->impl->log_level_settings, RCL_RET_INVALID_ARGUMENT);
   RCL_CHECK_ARGUMENT_FOR_NULL(log_level, RCL_RET_INVALID_ARGUMENT);
 
   if (NULL != *log_level) {
     RCL_SET_ERROR_MSG("Output log level pointer is not null.");
     return RCL_RET_INVALID_ARGUMENT;
   }
 
-  *log_level = rcl_log_level_copy(arguments->impl->log_level);
+  *log_level = rcl_log_level_copy(arguments->impl->log_level_settings);
   if (NULL == *log_level) {
     return RCL_RET_BAD_ALLOC;
   }
@@ -323,8 +322,8 @@ rcl_parse_arguments(
     ret = RCL_RET_BAD_ALLOC;
     goto fail;
   }
-  args_impl->log_level = rcl_log_level_init(allocator);
-  if (NULL == args_impl->log_level) {
+  args_impl->log_level_settings = rcl_log_level_init(allocator);
+  if (NULL == args_impl->log_level_settings) {
     ret = RCL_RET_BAD_ALLOC;
     goto fail;
   }
@@ -435,7 +434,9 @@ rcl_parse_arguments(
       // Attempt to parse argument as log level configuration
       if (strcmp(RCL_LOG_LEVEL_FLAG, argv[i]) == 0) {
         if (i + 1 < argc) {
-          if (RCL_RET_OK == _rcl_parse_log_level(argv[i + 1], allocator, args_impl->log_level)) {
+          if (RCL_RET_OK ==
+            _rcl_parse_log_level(argv[i + 1], allocator, args_impl->log_level_settings))
+          {
             RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Got log level: %s\n", argv[i + 1]);
             ++i;  // Skip flag here, for loop will skip value.
             continue;
@@ -668,9 +669,11 @@ rcl_parse_arguments(
   }
 
   // Drop logger log level if none was found
-  if (0U == args_impl->log_level->num_loggers && args_impl->log_level->logger_settings) {
-    allocator.deallocate(args_impl->log_level->logger_settings, allocator.state);
-    args_impl->log_level->logger_settings = NULL;
+  if (0U == args_impl->log_level_settings->num_logger_settings &&
+    args_impl->log_level_settings->logger_settings)
+  {
+    allocator.deallocate(args_impl->log_level_settings->logger_settings, allocator.state);
+    args_impl->log_level_settings->logger_settings = NULL;
   }
 
   return RCL_RET_OK;
@@ -954,9 +957,9 @@ rcl_arguments_fini(
       args->impl->num_remap_rules = 0;
     }
 
-    if (args->impl->log_level) {
-      rcl_log_level_fini(args->impl->log_level);
-      args->impl->log_level = NULL;
+    if (args->impl->log_level_settings) {
+      rcl_log_level_fini(args->impl->log_level_settings);
+      args->impl->log_level_settings = NULL;
     }
 
     args->impl->allocator.deallocate(args->impl->unparsed_args, args->impl->allocator.state);
@@ -1640,88 +1643,66 @@ _rcl_parse_log_level(
 {
   RCL_CHECK_ARGUMENT_FOR_NULL(arg, RCL_RET_INVALID_ARGUMENT);
   RCL_CHECK_ARGUMENT_FOR_NULL(log_level, RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(log_level->logger_settings, RCL_RET_INVALID_ARGUMENT);
 
-  size_t comma_len = strlen(RCL_LOG_LEVEL_FLAG_COMMA);
   size_t separator_len = strlen(RCL_LOG_LEVEL_FLAG_SEPARATOR);
   rcutils_ret_t ret = RCUTILS_RET_OK;
-  const char * p = arg;
-
-  do {
-    char * comma = strstr(p, RCL_LOG_LEVEL_FLAG_COMMA);
-    char * item;
-    if (comma == NULL) {
-      item = rcutils_strdup(p, allocator);
-    } else {
-      item = rcutils_strndup(p, comma - p, allocator);
-    }
-
-    if (item == NULL) {
-      RCL_SET_ERROR_MSG("failed to allocate memory for logger item");
+  char * name = NULL;
+  char * level = NULL;
+  int logger_level = 0;
+  char * separator = strstr(arg, RCL_LOG_LEVEL_FLAG_SEPARATOR);
+
+  if (separator) {
+    struct rcl_logger_setting_t * logger_log_level =
+      &log_level->logger_settings[log_level->num_logger_settings];
+    name = rcutils_strndup(arg, separator - arg, allocator);
+    if (name == NULL) {
+      RCL_SET_ERROR_MSG("failed to allocate memory for logger name");
       return RCL_RET_BAD_ALLOC;
     }
-    if (strlen(item) == 0) {
-      RCL_SET_ERROR_MSG("Argument has a empty logger item");
-      allocator.deallocate(item, allocator.state);
+    if (strlen(name) == 0) {
+      RCL_SET_ERROR_MSG("Argument has an invalid logger item that name is empty");
+      allocator.deallocate(name, allocator.state);
       return RCL_RET_ERROR;
     }
 
-    char * separator = strstr(item, RCL_LOG_LEVEL_FLAG_SEPARATOR);
-    if (separator) {
-      struct rcl_logger_setting_t * logger_log_level =
-        &log_level->logger_settings[log_level->num_loggers];
-      logger_log_level->name = rcutils_strndup(item, separator - item, allocator);
-      if (logger_log_level->name == NULL) {
-        RCL_SET_ERROR_MSG("failed to allocate memory for logger name");
-        allocator.deallocate(item, allocator.state);
-        return RCL_RET_BAD_ALLOC;
-      }
-      if (strlen(logger_log_level->name) == 0) {
-        RCL_SET_ERROR_MSG("Argument has an invalid logger item that name is empty");
-        allocator.deallocate(logger_log_level->name, allocator.state);
-        allocator.deallocate(item, allocator.state);
-        return RCL_RET_ERROR;
-      }
-      char * value = rcutils_strdup(separator + separator_len, allocator);
-      if (value == NULL) {
-        RCL_SET_ERROR_MSG("failed to allocate memory for logger level");
-        allocator.deallocate(logger_log_level->name, allocator.state);
-        allocator.deallocate(item, allocator.state);
-        return RCL_RET_BAD_ALLOC;
-      }
-      if (strlen(value) == 0) {
-        RCL_SET_ERROR_MSG("Argument has an invalid logger item that level is empty");
-        allocator.deallocate(value, allocator.state);
-        allocator.deallocate(logger_log_level->name, allocator.state);
-        allocator.deallocate(item, allocator.state);
-        return RCL_RET_ERROR;
-      }
-      ret = rcutils_logging_severity_level_from_string(
-        value,
-        allocator,
-        &logger_log_level->level);
-      allocator.deallocate(value, allocator.state);
-      if (RCUTILS_RET_OK == ret) {
-        log_level->num_loggers += 1;
-      } else {
-        allocator.deallocate(logger_log_level->name, allocator.state);
-      }
-    } else {
-      ret = rcutils_logging_severity_level_from_string(
-        item, allocator, &log_level->default_log_level);
+    level = rcutils_strdup(separator + separator_len, allocator);
+    if (level == NULL) {
+      RCL_SET_ERROR_MSG("failed to allocate memory for logger level");
+      allocator.deallocate(name, allocator.state);
+      return RCL_RET_BAD_ALLOC;
     }
-
-    allocator.deallocate(item, allocator.state);
-    if (RCUTILS_RET_OK != ret) {
-      RCL_SET_ERROR_MSG("Argument does not use a valid severity level");
+    if (strlen(level) == 0) {
+      RCL_SET_ERROR_MSG("Argument has an invalid logger item that level is empty");
+      allocator.deallocate(name, allocator.state);
+      allocator.deallocate(level, allocator.state);
       return RCL_RET_ERROR;
     }
 
-    if (comma == NULL) {
-      p = comma;
+    ret = rcutils_logging_severity_level_from_string(
+      level,
+      allocator,
+      &logger_level);
+    allocator.deallocate(level, allocator.state);
+    if (RCUTILS_RET_OK == ret) {
+      logger_log_level->name = name;
+      logger_log_level->level = (rcl_log_severity_t)logger_level;
+      log_level->num_logger_settings += 1;
     } else {
-      p = comma + comma_len;
+      allocator.deallocate(name, allocator.state);
     }
-  } while (p != NULL);
+  } else {
+    ret = rcutils_logging_severity_level_from_string(
+      arg, allocator, &logger_level);
+    if (RCUTILS_RET_OK == ret) {
+      log_level->default_logger_level = (rcl_log_severity_t)logger_level;
+    }
+  }
+
+  if (RCUTILS_RET_OK != ret) {
+    RCL_SET_ERROR_MSG("Argument does not use a valid severity level");
+    return RCL_RET_ERROR;
+  }
 
   return RCL_RET_OK;
 }
@@ -1950,7 +1931,7 @@ _rcl_allocate_initialized_arguments_impl(rcl_arguments_t * args, rcl_allocator_t
   rcl_arguments_impl_t * args_impl = args->impl;
   args_impl->num_remap_rules = 0;
   args_impl->remap_rules = NULL;
-  args_impl->log_level = NULL;
+  args_impl->log_level_settings = NULL;
   args_impl->external_log_config_file = NULL;
   args_impl->unparsed_args = NULL;
   args_impl->num_unparsed_args = 0;

diff --git a/rcl/src/rcl/arguments_impl.h b/rcl/src/rcl/arguments_impl.h
@@ -52,7 +52,7 @@ typedef struct rcl_arguments_impl_t
   int num_remap_rules;
 
   /// Log level parsed from arguments.
-  rcl_log_level_t * log_level;
+  rcl_log_level_t * log_level_settings;
   /// A file used to configure the external logging library
   char * external_log_config_file;
   /// A boolean value indicating if the standard out handler should be used for log output

diff --git a/rcl/src/rcl/log_level.c b/rcl/src/rcl/log_level.c
@@ -26,7 +26,7 @@ extern "C"
 #define MAX_NUM_LOGGER_SETTING_ENTRIES 256U
 
 rcl_log_level_t *
-rcl_log_level_init(const rcutils_allocator_t allocator)
+rcl_log_level_init(const rcl_allocator_t allocator)
 {
   RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "invalid allocator", return NULL);
   rcl_log_level_t * log_level = allocator.allocate(sizeof(rcl_log_level_t), allocator.state);
@@ -43,8 +43,8 @@ rcl_log_level_init(const rcutils_allocator_t allocator)
     return NULL;
   }
 
-  log_level->default_log_level = -1;
-  log_level->num_loggers = 0;
+  log_level->default_logger_level = RCUTILS_LOG_SEVERITY_UNSET;
+  log_level->num_logger_settings = 0;
   log_level->allocator = allocator;
   return log_level;
 }
@@ -62,8 +62,8 @@ rcl_log_level_t * rcl_log_level_copy(
     return NULL;
   }
 
-  out_log_level->default_log_level = log_level->default_log_level;
-  for (size_t i = 0; i < log_level->num_loggers; ++i) {
+  out_log_level->default_logger_level = log_level->default_logger_level;
+  for (size_t i = 0; i < log_level->num_logger_settings; ++i) {
     out_log_level->logger_settings[i].name =
       rcutils_strdup(log_level->logger_settings[i].name, allocator);
     if (NULL == out_log_level->logger_settings[i].name) {
@@ -72,7 +72,7 @@ rcl_log_level_t * rcl_log_level_copy(
     }
     out_log_level->logger_settings[i].level =
       log_level->logger_settings[i].level;
-    out_log_level->num_loggers++;
+    out_log_level->num_logger_settings++;
   }
 
   return out_log_level;
@@ -88,10 +88,10 @@ rcl_log_level_fini(
 {
   rcutils_allocator_t allocator = log_level->allocator;
   if (log_level->logger_settings) {
-    for (size_t i = 0; i < log_level->num_loggers; ++i) {
+    for (size_t i = 0; i < log_level->num_logger_settings; ++i) {
       allocator.deallocate(log_level->logger_settings[i].name, allocator.state);
     }
-    log_level->num_loggers = 0;
+    log_level->num_logger_settings = 0;
 
     allocator.deallocate(log_level->logger_settings, allocator.state);
     log_level->logger_settings = NULL;

diff --git a/rcl/src/rcl/logging.c b/rcl/src/rcl/logging.c
@@ -64,7 +64,7 @@ rcl_logging_configure_with_output_handler(
   RCUTILS_LOGGING_AUTOINIT
     g_logging_allocator = *allocator;
   int default_level = -1;
-  rcl_log_level_t * log_level = global_args->impl->log_level;
+  rcl_log_level_t * log_level_settings = global_args->impl->log_level_settings;
   const char * config_file = global_args->impl->external_log_config_file;
   g_rcl_logging_stdout_enabled = !global_args->impl->log_stdout_disabled;
   g_rcl_logging_rosout_enabled = !global_args->impl->log_rosout_disabled;
@@ -73,18 +73,16 @@ rcl_logging_configure_with_output_handler(
   rcl_ret_t status = RCL_RET_OK;
   g_rcl_logging_num_out_handlers = 0;
 
-  if (log_level) {
-    default_level = log_level->default_log_level;
-    if (default_level >= 0) {
-      rcutils_logging_set_default_logger_level(default_level);
-    }
+  if (log_level_settings) {
+    default_level = (int)log_level_settings->default_logger_level;
+    rcutils_logging_set_default_logger_level(default_level);
 
-    for (size_t i = 0; i < log_level->num_loggers; ++i) {
-      status = rcutils_logging_set_logger_level(
-        log_level->logger_settings[i].name,
-        log_level->logger_settings[i].level);
-      if (RCL_RET_OK != status) {
-        return status;
+    for (size_t i = 0; i < log_level_settings->num_logger_settings; ++i) {
+      rcutils_ret_t status = rcutils_logging_set_logger_level(
+        log_level_settings->logger_settings[i].name,
+        (int)log_level_settings->logger_settings[i].level);
+      if (RCUTILS_RET_OK != status) {
+        return RCL_RET_ERROR;
       }
     }
   }