Before committing this, I'd like to hear from people who use augeas on
other OS's how they feel about a dependency on libxml2.
Some comments inline:
On Fri, 2011-03-04 at 17:32 +0100, Raphaël Pinson wrote:
> diff --git a/src/augeas.c b/src/augeas.c
> index 41e649e..f2923e5 100644
> --- a/src/augeas.c
> +++ b/src/augeas.c
> @@ -1427,6 +1428,140 @@ int aug_print(const struct augeas *aug, FILE *out,
> const char *pathin) {
> return -1;
> }
>
> +
> +static int to_xml_one(xmlNodePtr elem, const struct tree *tree,
> + const char *pathin) {
> + xmlNodePtr value;
> + xmlAttrPtr prop;
> +
> + prop = xmlSetProp(elem, (xmlChar*) "label", (xmlChar*) tree->label);
AFAIK, you're supposed to use BAD_CAST instead of casting directly to
(xmlChar*), i.e.
prop = xmlSetProp(elem, BAD_CAST "label", BAD_CAST tree->label);
> + if (prop == NULL)
> + goto error;
> +
> + if (tree->span) {
> + prop = xmlSetProp(elem, (xmlChar*) "file",
> + (xmlChar*) tree->span->filename->str);
> + if (prop == NULL)
> + goto error;
> + }
> +
> + if (pathin != NULL) {
> + prop = xmlSetProp(elem, (xmlChar*) "path", (xmlChar*) pathin);
> + if (prop == NULL)
> + goto error;
> + }
> + if (tree->value != NULL) {
> + value = xmlNewTextChild(elem, NULL, (xmlChar*) "value",
> + (xmlChar*) tree->value);
> + if (value == NULL)
> + goto error;
> + }
> + return 0;
> + error:
> + xmlFreeProp(prop);
Is that ever necessary ? I would assume that libxml takes care of
cleaning up after an error automatically.
> + return -1;
> +}
> +
> +
> +static int to_xml_rec(xmlNodePtr pnode, struct tree *start,
> + const char *pathin) {
> + int r;
> + char *path, *ppath = NULL;
You need to initialize path to NULL, too.
> + xmlNodePtr elem, nelem, added;
> +
> + elem = xmlNewNode(NULL, (xmlChar*) "node");
If you use xmlNewChild(pnode, NULL, BAD_CAST "node", NULL) here, there's
no danger we leak elem on error.
> + if (elem == NULL)
> + goto error;
> + r = to_xml_one(elem, start, pathin);
> + if (r < 0)
> + goto error;
> +
> + ppath = path_of_tree(start);
> + list_for_each(tree, start->children) {
> + if (TREE_HIDDEN(tree))
> + continue;
> + path = path_expand(tree, ppath);
> + if (path == NULL)
> + goto error;
> + nelem = xmlNewNode(NULL, (xmlChar*) "node");
> + if (nelem == NULL)
> + goto error;
> + r = to_xml_one(nelem, tree, NULL);
> + if (r < 0)
> + goto error;
> + r = to_xml_rec(elem, tree, NULL);
Doesn't the above call to_xml_one twice for tree ? Once here, and then
once more upon entry to to_xml_rec(elem, tree, NULL) ? As a matter of
fact, nelem is leaked.
> + free(path);
> + path = NULL;
Instead of the last two lines, use 'FREE(path)'
> + if (r < 0)
> + goto error;
> + }
> +
> + added = xmlAddChild(pnode, elem);
> + if (added == NULL)
> + return -1;
> +
> + free(ppath);
> + return 0;
> + error:
You either need to make sure that elem/nelem are always reachable from
pnode, or clean them up here. The former will be much easier.
> + free(path);
> + free(ppath);
> + return -1;
> +}
> +
> +
> +static int tree_to_xml(struct pathx *p, xmlNode **xml, const char *pathin) {
> + char *path = NULL;
> + struct tree *tree;
> + xmlAttrPtr attr;
> + int r;
> +
> + *xml = xmlNewNode(NULL, (xmlChar*) "augeas");
> + if (*xml == NULL)
> + goto error;
> + attr = xmlSetProp(*xml, (xmlChar*) "path", (xmlChar*) pathin);
This attribute should be called something else, since it's a path
expression; how about 'expr' ?
> + for (tree = pathx_first(p); tree != NULL; tree = pathx_next(p)) {
> + if (TREE_HIDDEN(tree))
> + continue;
> + path = path_of_tree(tree);
> + if (path == NULL)
> + goto error;
> + r = to_xml_rec(*xml, tree, path);
> + if (r < 0)
> + goto error;
> + free(path);
> + path = NULL;
Use 'FREE(path)'
> + }
> + return 0;
> + error:
> + free(path);
We should free the whole DOM *xml on error, and set *xml = NULL instead
of forcing the caller to do that.
> + return -1;
> +}
> +
> +
> +int aug_to_xml(struct augeas *aug, const char *pathin,
> + xmlNode **xmldoc) {
> + struct pathx *p;
> + int result;
> +
> + api_entry(aug);
> +
> + if (pathin == NULL || strlen(pathin) == 0) {
> + pathin = "/*";
> + }
> +
> + p = pathx_aug_parse(aug, aug->origin, pathin, true);
> + ERR_BAIL(aug);
> + result = tree_to_xml(p, xmldoc, pathin);
To properly indicate the error, you should do something like
ERR_THROW(result < 0, aug, AUG_ENOMEM, NULL);
If there's other reasons than an OOM for this to fail, you might want to
pass the struct augeas down to the helper functions, so that the can
directly set the error flags correctly for whatever error happened.
> + free_pathx(p);
> + api_exit(aug);
> +
> + return result;
> + error:
> + api_exit(aug);
> + return -1;
> +}
> +
> void aug_close(struct augeas *aug) {
> if (aug == NULL)
> return;
> diff --git a/src/augeas.h b/src/augeas.h
> index 813b7a6..77955b0 100644
> --- a/src/augeas.h
> +++ b/src/augeas.h
> @@ -21,6 +21,7 @@
> */
>
> #include <stdio.h>
> +#include <libxml/tree.h>
>
> #ifndef AUGEAS_H_
> #define AUGEAS_H_
> @@ -298,6 +299,15 @@ int aug_load(augeas *aug);
> */
> int aug_print(const augeas *aug, FILE *out, const char *path);
>
> +/* Function: aug_to_xml
> + *
> + * Turn the Augeas tree into a libxml2 xmlNodePtr structure.
> + *
> + * Returns:
> + * 0 on success, or a negative value on failure
> + */
> +int aug_to_xml(augeas *aug, const char *path, xmlNode **xmldoc);
Also explain what the convention for *xmlDoc is, especially on error.
I'd prefer if we'd say 'on error, *xmlDoc will be set to NULL'. Also, we
should say that *xmlDoc should be NULL on entry.
> diff --git a/src/augtool.c b/src/augtool.c
> index 8ee0920..6c7acd3 100644
> --- a/src/augtool.c
> +++ b/src/augtool.c
> @@ -32,6 +32,7 @@
> #include <limits.h>
> #include <ctype.h>
> #include <locale.h>
> +#include <libxml/tree.h>
>
> /* Global variables */
>
> @@ -856,6 +857,33 @@ static const struct command_def cmd_print_def = {
> .help = "Print entries in the tree. If PATH is given, printing starts
> there,\n otherwise the whole tree is printed"
> };
>
> +static void cmd_print_xml(struct command *cmd) {
> + const char *path = arg_value(cmd, "path");
> + xmlNodePtr xmldoc;
> +
> + aug_to_xml(aug, path, &xmldoc);
> + err_check(cmd);
> +
> + xmlElemDump(stdout, NULL, xmldoc);
> + printf("\n");
> +
> + xmlFreeNode(xmldoc);
> +}
> +
> +static const struct command_opt_def cmd_print_xml_opts[] = {
> + { .type = CMD_PATH, .name = "path", .optional = true,
> + .help = "print this subtree" },
> + CMD_OPT_DEF_LAST
> +};
What do you think of adding another optional argument 'file' so that
people can say 'print-xml /* /tmp/tree.xml' to dump straight into a
file ?
David
_______________________________________________
augeas-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/augeas-devel
