2011/3/5 David Lutterkort <[email protected]>:
> 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.
>
Definitely. Like I said on the IRC, maybe if other OSes have problems
with libxml2, we might make this an optional compile flag.
> 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.
>
All of the above was fixed in
https://github.com/raphink/Augeas/commit/5f27b6bb75f1d97ac67029847af8379da6707374
Thanks for the great insights.
>> 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 ?
That could be interesting when you want to export several trees
without instantiating augtool several times. For a simple use though,
redirection using the shell is good enough in most cases.
Raphaël
_______________________________________________
augeas-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/augeas-devel
