From: Igor Peshansky <igorpeshansky@github.com>
Date: Sun, 11 Sep 2016 23:39:58 +0000 (-0400)
Subject: Address review comments:
X-Git-Tag: collectd-5.7.0~104^2
X-Git-Url: https://git.verplant.org/?a=commitdiff_plain;h=d2a80178a8de377e4e270e2e84fdcb23eb057437;p=collectd.git

Address review comments:
- Code simplifications,
- Leak fixes,
- ERROR -> WARNING.
---

diff --git a/src/target_replace.c b/src/target_replace.c
index 975acbce..dba3a8cf 100644
--- a/src/target_replace.c
+++ b/src/target_replace.c
@@ -38,7 +38,7 @@ struct tr_action_s
 {
   regex_t re;
   char *replacement;
-  int may_be_empty;
+  _Bool may_be_empty;
 
   tr_action_t *next;
 };
@@ -113,7 +113,7 @@ static void tr_meta_data_action_destroy (tr_meta_data_action_t *act) /* {{{ */
 } /* }}} void tr_meta_data_action_destroy */
 
 static int tr_config_add_action (tr_action_t **dest, /* {{{ */
-    const oconfig_item_t *ci, int may_be_empty)
+    const oconfig_item_t *ci, _Bool may_be_empty)
 {
   tr_action_t *act;
   int status;
@@ -158,8 +158,7 @@ static int tr_config_add_action (tr_action_t **dest, /* {{{ */
   if (act->replacement == NULL)
   {
     ERROR ("tr_config_add_action: tr_strdup failed.");
-    regfree (&act->re);
-    sfree (act);
+    tr_action_destroy (act);
     return (-ENOMEM);
   }
 
@@ -181,7 +180,7 @@ static int tr_config_add_action (tr_action_t **dest, /* {{{ */
 } /* }}} int tr_config_add_action */
 
 static int tr_config_add_meta_action (tr_meta_data_action_t **dest, /* {{{ */
-    const oconfig_item_t *ci, int should_delete)
+    const oconfig_item_t *ci, _Bool should_delete)
 {
   tr_meta_data_action_t *act;
   int status;
@@ -230,14 +229,6 @@ static int tr_config_add_meta_action (tr_meta_data_action_t **dest, /* {{{ */
   act->key = NULL;
   act->replacement = NULL;
 
-  act->key = tr_strdup (ci->values[0].value.string);
-  if (act->key == NULL)
-  {
-    ERROR ("tr_config_add_meta_action: tr_strdup failed.");
-    sfree (act);
-    return (-ENOMEM);
-  }
-
   status = regcomp (&act->re, ci->values[1].value.string, REG_EXTENDED);
   if (status != 0)
   {
@@ -253,14 +244,20 @@ static int tr_config_add_meta_action (tr_meta_data_action_t **dest, /* {{{ */
     return (-EINVAL);
   }
 
+  act->key = tr_strdup (ci->values[0].value.string);
+  if (act->key == NULL)
+  {
+    ERROR ("tr_config_add_meta_action: tr_strdup failed.");
+    tr_meta_data_action_destroy (act);
+    return (-ENOMEM);
+  }
+
   if (!should_delete) {
     act->replacement = tr_strdup (ci->values[2].value.string);
     if (act->replacement == NULL)
     {
       ERROR ("tr_config_add_meta_action: tr_strdup failed.");
-      sfree (act->key);
-      regfree (&act->re);
-      sfree (act);
+      tr_meta_data_action_destroy (act);
       return (-ENOMEM);
     }
   }
@@ -283,7 +280,7 @@ static int tr_config_add_meta_action (tr_meta_data_action_t **dest, /* {{{ */
 } /* }}} int tr_config_add_meta_action */
 
 static int tr_action_invoke (tr_action_t *act_head, /* {{{ */
-    char *buffer_in, size_t buffer_in_size, int may_be_empty)
+    char *buffer_in, size_t buffer_in_size, _Bool may_be_empty)
 {
   int status;
   char buffer[DATA_MAX_NAME_LEN];
@@ -363,13 +360,14 @@ static int tr_meta_data_action_invoke ( /* {{{ */
     int value_type;
     int meta_data_status;
     char *value;
+    meta_data_t *result;
 
     value_type = meta_data_type (*dest, act->key);
     if (value_type == 0)  /* not found */
       continue;
     if (value_type != MD_TYPE_STRING)
     {
-      ERROR ("Target `replace': Attempting replace on metadata key `%s', "
+      WARNING ("Target `replace': Attempting replace on metadata key `%s', "
           "which isn't a string.",
           act->key);
       continue;
@@ -390,7 +388,10 @@ static int tr_meta_data_action_invoke ( /* {{{ */
         STATIC_ARRAY_SIZE (matches), matches,
         /* flags = */ 0);
     if (status == REG_NOMATCH)
+    {
+      sfree (value);
       continue;
+    }
     else if (status != 0)
     {
       char errbuf[1024] = "";
@@ -398,53 +399,56 @@ static int tr_meta_data_action_invoke ( /* {{{ */
       regerror (status, &act->re, errbuf, sizeof (errbuf));
       ERROR ("Target `replace': Executing a regular expression failed: %s.",
           errbuf);
+      sfree (value);
       continue;
     }
 
-    if (act->replacement != NULL)
+    if (act->replacement == NULL)
     {
-      meta_data_t *result;
+      /* no replacement; delete the key */
+      DEBUG ("target_replace plugin: tr_meta_data_action_invoke: "
+          "deleting `%s'", act->key);
+      meta_data_delete (*dest, act->key);
+      sfree (value);
+      continue;
+    }
 
-      subst_status = subst (temp, sizeof (temp), value,
-          (size_t) matches[0].rm_so, (size_t) matches[0].rm_eo,
+    subst_status = subst (temp, sizeof (temp), value,
+        (size_t) matches[0].rm_so, (size_t) matches[0].rm_eo, act->replacement);
+    if (subst_status == NULL)
+    {
+      ERROR ("Target `replace': subst (value = %s, start = %zu, end = %zu, "
+          "replacement = %s) failed.",
+          value, (size_t) matches[0].rm_so, (size_t) matches[0].rm_eo,
           act->replacement);
-      if (subst_status == NULL)
-      {
-        ERROR ("Target `replace': subst (value = %s, start = %zu, end = %zu, "
-            "replacement = %s) failed.",
-            value, (size_t) matches[0].rm_so, (size_t) matches[0].rm_eo,
-            act->replacement);
-        continue;
-      }
-
-      DEBUG ("target_replace plugin: tr_meta_data_action_invoke: `%s' "
-          "value `%s' -> `%s'", act->key, value, temp);
-
-      if ((result = meta_data_create()) == NULL)
-      {
-        ERROR ("Target `replace': failed to create metadata for `%s'.",
-            act->key);
-        return (-ENOMEM);
-      }
-
-      meta_data_status = meta_data_add_string (result, act->key, temp);
-
-      if (meta_data_status != 0)
-      {
-        ERROR ("Target `replace': Unable to set metadata value for `%s'.",
-            act->key);
-        return (meta_data_status);
-      }
-
-      meta_data_clone_merge (dest, result);
-      meta_data_destroy (result);
+      sfree (value);
+      continue;
     }
-    else  /* no replacement; delete the key */
+
+    DEBUG ("target_replace plugin: tr_meta_data_action_invoke: `%s' "
+        "value `%s' -> `%s'", act->key, value, temp);
+
+    if ((result = meta_data_create()) == NULL)
     {
-      DEBUG ("target_replace plugin: tr_meta_data_action_invoke: "
-          "deleting `%s'", act->key);
-      meta_data_delete (*dest, act->key);
+      ERROR ("Target `replace': failed to create metadata for `%s'.",
+          act->key);
+      sfree (value);
+      return (-ENOMEM);
     }
+
+    meta_data_status = meta_data_add_string (result, act->key, temp);
+    if (meta_data_status != 0)
+    {
+      ERROR ("Target `replace': Unable to set metadata value for `%s'.",
+          act->key);
+      meta_data_destroy (result);
+      sfree (value);
+      return (meta_data_status);
+    }
+
+    meta_data_clone_merge (dest, result);
+    meta_data_destroy (result);
+    sfree (value);
   } /* for (act = act_head; act != NULL; act = act->next) */
 
   return (0);