On Mon, Jan 11, 2010 at 9:26 PM, Florian Forster <o...@verplant.org> wrote:
> Hi Amit,
>
> thank you very much for your patch :)
>
Thanks for applying the patch :)
> I did the following changes:
>
> * Re-order all of the functions to get rid of the forward declarations.
> * Replace the string and boolean handling functions for config options
> with functions declared in src/configfile.h. This way the
> functionality is not duplicated.
> * Put the type_instance setting and value parsing into a separate
> functions. This makes “cx_submit_xpath_values” considerably bit
> shorter.
>
> Please let me know if my changes broke anything – unfortunately I didn't
> have the possibility to test them :/
>
I did a sanity testing and the plugin seems to be working fine.
> > #define CX_KEY_MAGIC 0x43484b59UL /* CHKY */
> > #define CX_IS_KEY(key) (key)->magic == CX_KEY_MAGIC
>
> What do we need this hack for? I've found only one place where
> “c_avl_insert” is called, so the elements in the tree should all be of
> the same type, right? I'd be great if you could remove or document this.
>
There is no need of this magic key. It just happens to be there since I
borrowed the code from curl_json :). I will clean this up.
>
> Maybe the entire AVL tree should be removed: As far as I see it is only
> used to iterate over the values (rather than searching for a specific
> key), so a linked list is probably more appropriate.
>
Yeah I agree. I have replaced avl tree with a linked list.
>
> Both, “cx_check_type” and “cx_submit_xpath_values” call “plugin_get_ds”.
> I think one call could be optimized away.
>
This is also done.
Do find the updated patch attached.
Regards
Amit
>
> Best regards,
> —octo
>
> [0] <
> http://git.verplant.org/?p=collectd.git;a=blob_plain;f=src/curl_xml.c;hb=refs/heads/ag/curl_xml
> >
> --
> Florian octo Forster
> Hacker in training
> GnuPG: 0x91523C3D
> http://verplant.org/
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
>
> iD8DBQFLS0olHdggu3Q05IYRAuMXAJ440LTtkMUZaDb7MUq4EdnXw8SXTgCfS3gS
> Pat48nthKsGvE3YMI6rIOh0=
> =VgpJ
> -----END PGP SIGNATURE-----
>
>
--- collectd-latest/src/curl_xml.c Wed Jan 13 16:23:42 2010
+++ collectd-latest-mine/src/curl_xml.c Wed Jan 13 15:39:22 2010
@@ -23,7 +23,7 @@
#include "common.h"
#include "plugin.h"
#include "configfile.h"
-#include "utils_avltree.h"
+#include "utils_llist.h"
#include <libxml/parser.h>
#include <libxml/tree.h>
@@ -32,8 +32,6 @@
#include <curl/curl.h>
#define CX_DEFAULT_HOST "localhost"
-#define CX_KEY_MAGIC 0x43484b59UL /* CHKY */
-#define CX_IS_KEY(key) (key)->magic == CX_KEY_MAGIC
/*
* Private data structures
@@ -79,7 +77,7 @@
size_t buffer_size;
size_t buffer_fill;
- c_avl_tree_t *tree; /* tree of xpath blocks */
+ llist_t *list; /* list of xpath blocks */
};
typedef struct cx_s cx_t; /* }}} */
@@ -138,25 +136,26 @@
sfree (xpath);
} /* }}} void cx_xpath_free */
-static void cx_tree_free (c_avl_tree_t *tree) /* {{{ */
+static void cx_list_free (llist_t *list) /* {{{ */
{
- char *name;
- void *value;
+ llentry_t *le;
- while (c_avl_pick (tree, (void *) &name, (void *) &value) == 0)
+ le = llist_head (list);
+ while (le != NULL)
{
- cx_xpath_t *key = (cx_xpath_t *)value;
+ llentry_t *le_next;
- if (CX_IS_KEY(key))
- cx_xpath_free (key);
- else
- cx_tree_free ((c_avl_tree_t *)value);
+ le_next = le->next;
- sfree (name);
+ sfree (le->key);
+ cx_xpath_free (le->value);
+
+ le = le_next;
}
- c_avl_destroy (tree);
-} /* }}} void cx_tree_free */
+ llist_destroy (list);
+ list = NULL;
+} /* }}} void cx_list_free */
static void cx_free (void *arg) /* {{{ */
{
@@ -173,9 +172,8 @@
curl_easy_cleanup (db->curl);
db->curl = NULL;
- if (db->tree != NULL)
- cx_tree_free (db->tree);
- db->tree = NULL;
+ if (db->list != NULL)
+ cx_list_free (db->list);
sfree (db->buffer);
sfree (db->instance);
@@ -190,11 +188,8 @@
sfree (db);
} /* }}} void cx_free */
-static int cx_check_type (cx_xpath_t *xpath) /* {{{ */
+static int cx_check_type (const data_set_t *ds, cx_xpath_t *xpath) /* {{{ */
{
- const data_set_t *ds;
-
- ds = plugin_get_ds (xpath->type);
if (!ds)
{
WARNING ("curl_xml plugin: DataSet `%s' not defined.", xpath->type);
@@ -373,7 +368,7 @@
if ( (tmp_size == 0) && (is_table) )
{
WARNING ("curl_xml plugin: "
- "relative xpath expression for 'Instance' \"%s\" doesn't match "
+ "relative xpath expression for 'InstanceFrom' \"%s\" doesn't match "
"any of the nodes. Skipping the node.", xpath->instance);
xmlXPathFreeObject (instance_node_obj);
return (-1);
@@ -382,7 +377,7 @@
if (tmp_size > 1)
{
WARNING ("curl_xml plugin: "
- "relative xpath expression for 'Instance' \"%s\" is expected "
+ "relative xpath expression for 'InstanceFrom' \"%s\" is expected "
"to return only one text node. Skipping the node.", xpath->instance);
xmlXPathFreeObject (instance_node_obj);
return (-1);
@@ -425,7 +420,7 @@
} /* }}} int cx_handle_instance_xpath */
static int cx_handle_base_xpath (char *plugin_instance, /* {{{ */
- xmlXPathContextPtr xpath_ctx,
+ xmlXPathContextPtr xpath_ctx, const data_set_t *ds,
char *base_xpath, cx_xpath_t *xpath)
{
int total_nodes;
@@ -435,7 +430,6 @@
xmlNodeSetPtr base_nodes = NULL;
value_list_t vl = VALUE_LIST_INIT;
- const data_set_t *ds;
base_node_obj = cx_evaluate_xpath (xpath_ctx, BAD_CAST base_xpath);
if (base_node_obj == NULL)
@@ -447,7 +441,8 @@
if (total_nodes == 0)
{
ERROR ("curl_xml plugin: "
- "xpath expression \"%s\" doesn't match any of the node. Skipping...", base_xpath);
+ "xpath expression \"%s\" doesn't match any of the nodes. "
+ "Skipping the xpath block...", base_xpath);
xmlXPathFreeObject (base_node_obj);
return -1;
}
@@ -457,13 +452,12 @@
if (total_nodes > 1 && xpath->instance == NULL)
{
ERROR ("curl_xml plugin: "
- "Instance is must in xpath block since the base xpath expression \"%s\" "
+ "InstanceFrom is must in xpath block since the base xpath expression \"%s\" "
"returned multiple results. Skipping the xpath block...", base_xpath);
return -1;
}
/* set the values for the value_list */
- ds = plugin_get_ds (xpath->type);
vl.values_len = ds->ds_num;
sstrncpy (vl.type, xpath->type, sizeof (vl.type));
sstrncpy (vl.plugin, "curl_xml", sizeof (vl.plugin));
@@ -496,21 +490,26 @@
static int cx_handle_parsed_xml(xmlDocPtr doc, /* {{{ */
xmlXPathContextPtr xpath_ctx, cx_t *db)
{
- c_avl_iterator_t *iter;
- char *key;
- cx_xpath_t *value;
+ llentry_t *le;
+ const data_set_t *ds;
+ cx_xpath_t *xpath;
int status=-1;
- iter = c_avl_get_iterator (db->tree);
- while (c_avl_iterator_next (iter, (void *) &key, (void *) &value) == 0)
+
+ le = llist_head (db->list);
+ while (le != NULL)
{
- if (cx_check_type(value) == -1)
- continue;
+ /* get the ds */
+ xpath = (cx_xpath_t *) le->value;
+ ds = plugin_get_ds (xpath->type);
- if (cx_handle_base_xpath(db->instance, xpath_ctx, key, value) == 0)
+ if ( (cx_check_type(ds, xpath) == 0) &&
+ (cx_handle_base_xpath(db->instance, xpath_ctx, ds, le->key, xpath) == 0) )
status = 0; /* we got atleast one success */
- } /* while (c_avl_iterator_next) */
+ le = le->next;
+ } /* while (le != NULL) */
+
return status;
} /* }}} cx_handle_parsed_xml */
@@ -602,7 +601,7 @@
if (ci->values_num < 1)
{
- WARNING ("curl_xml plugin: `Values' needs at least one argument.");
+ WARNING ("curl_xml plugin: `ValuesFrom' needs at least one argument.");
return (-1);
}
@@ -609,7 +608,7 @@
for (i = 0; i < ci->values_num; i++)
if (ci->values[i].type != OCONFIG_TYPE_STRING)
{
- WARNING ("curl_xml plugin: `Values' needs only string argument.");
+ WARNING ("curl_xml plugin: `ValuesFrom' needs only string argument.");
return (-1);
}
@@ -631,11 +630,6 @@
return (0);
} /* }}} cx_config_add_values */
-static c_avl_tree_t *cx_avl_create(void) /* {{{ */
-{
- return c_avl_create ((int (*) (const void *, const void *)) strcmp);
-} /* }}} cx_avl_create */
-
static int cx_config_add_xpath (cx_t *db, /* {{{ */
oconfig_item_t *ci)
{
@@ -643,14 +637,6 @@
int status;
int i;
- if ((ci->values_num != 1)
- || (ci->values[0].type != OCONFIG_TYPE_STRING))
- {
- WARNING ("curl_xml plugin: The `xpath' block "
- "needs exactly one string argument.");
- return (-1);
- }
-
xpath = (cx_xpath_t *) malloc (sizeof (*xpath));
if (xpath == NULL)
{
@@ -658,21 +644,19 @@
return (-1);
}
memset (xpath, 0, sizeof (*xpath));
- xpath->magic = CX_KEY_MAGIC;
- if (strcasecmp ("xpath", ci->key) == 0)
+ status = cf_util_get_string (ci, &xpath->path);
+ if (status != 0)
{
- status = cf_util_get_string (ci, &xpath->path);
- if (status != 0)
- {
- sfree (xpath);
- return (status);
- }
+ sfree (xpath);
+ return (status);
}
- else
+
+ /* error out if xpath->path is an empty string */
+ if (*xpath->path == 0)
{
- ERROR ("curl_xml plugin: cx_config: "
- "Invalid key: %s", ci->key);
+ ERROR ("curl_xml plugin: invalid xpath. "
+ "xpath value can't be an empty string");
return (-1);
}
@@ -685,10 +669,10 @@
status = cf_util_get_string (child, &xpath->type);
else if (strcasecmp ("InstancePrefix", child->key) == 0)
status = cf_util_get_string (child, &xpath->instance_prefix);
- else if (strcasecmp ("Instance", child->key) == 0)
+ else if (strcasecmp ("InstanceFrom", child->key) == 0)
status = cf_util_get_string (child, &xpath->instance);
- else if (strcasecmp ("Values", child->key) == 0)
- status = cx_config_add_values ("Values", xpath, child);
+ else if (strcasecmp ("ValuesFrom", child->key) == 0)
+ status = cx_config_add_values ("ValuesFrom", xpath, child);
else
{
WARNING ("curl_xml plugin: Option `%s' not allowed here.", child->key);
@@ -699,35 +683,42 @@
break;
} /* for (i = 0; i < ci->children_num; i++) */
- while (status == 0)
+ if (status == 0 && xpath->type == NULL)
{
- if (xpath->type == NULL)
- {
- WARNING ("curl_xml plugin: `Type' missing in `xpath' block.");
- status = -1;
- }
+ WARNING ("curl_xml plugin: `Type' missing in `xpath' block.");
+ status = -1;
+ }
- break;
- } /* while (status == 0) */
-
if (status == 0)
{
char *name;
- c_avl_tree_t *tree;
+ llentry_t *le;
- if (db->tree == NULL)
- db->tree = cx_avl_create();
+ if (db->list == NULL)
+ {
+ db->list = llist_create();
+ if (db->list == NULL)
+ {
+ ERROR ("curl_xml plugin: list creation failed.");
+ return (-1);
+ }
+ }
- tree = db->tree;
- name = xpath->path;
+ name = strdup(xpath->path);
+ if (name == NULL)
+ {
+ ERROR ("curl_xml plugin: strdup failed.");
+ return (-1);
+ }
- if (*name)
- c_avl_insert (tree, strdup(name), xpath);
- else
+ le = llentry_create (name, xpath);
+ if (le == NULL)
{
- ERROR ("curl_xml plugin: invalid key: %s", xpath->path);
- status = -1;
+ ERROR ("curl_xml plugin: llentry_create failed.");
+ return (-1);
}
+
+ llist_append (db->list, le);
}
return (status);
@@ -850,7 +841,7 @@
if (status == 0)
{
- if (db->tree == NULL)
+ if (db->list == NULL)
{
WARNING ("curl_xml plugin: No (valid) `Key' block "
"within `URL' block `%s'.", db->url);
_______________________________________________
collectd mailing list
collectd@verplant.org
http://mailman.verplant.org/listinfo/collectd
