[augeas-devel] Re: Review of the Augeas API

[Jim Meyering]

David Lutterkort <dlutter at redhat.com> wrote:
> On Mon, 2008-04-21 at 18:02 +0200, Jim Meyering wrote:
...
>> One possibility for a slightly deeper suggestion: provide a "do_for_each"
>> style iterator function.  Currently you can do that, but only by first
>> allocating/filling space for the list of all matches and then iterating
>> through that list.  With a do_for_each function, you could do the same
>> job without the cost (space) of allocating that list.
>
> Agreed; my aim was to keep the first cut at the API as dirt simple as
> possible. Adding an iterator requires that the API uses another type to
> store iteration information. But I will add that probably pretty soon.

If you want to keep it simple, you can provide an interface that does
not require an iterator type or any public iterator-related data; instead,
just provide a do_for_each function with a callback function parameter.

>> Not in the header, but I suggest you change aug_read_file to something
>> like libvirt's fread_file_lim function (to be added to gnulib when I
>> find time).
>>   Otherwise, reading a non-regular file will cause trouble
>> (like OOM, e.g., on some directory/systems, or unnecessary failure when
>> given a named pipe).
>
> I added a check to avoid reading directories. This probably needs to be
> clamped down even further (there's not really much need for reading from
> anythin but regular files and symlinks in Augeas' case, and symlinks
> need to be handled with some care to avoid mapping the same file into
> the tree in different places)

But that still misbehaves on non-regular, non-directory files.
Maybe that's not a big deal, but it also has a race condition
when the file changes size between the fstat and the fread.
That can result in an unnecessary failure.

If you use the fread_file_lim function, you avoid all that.

> Longer term, a lot of the file reading should be mmap'd.

I wouldn't worry too much, unless profiling says to,
since mmap is inherently less portable.

There's a problem in pathjoin: upon failed malloc,
it would dereference NULL and probably segfault.
Here's a patch that also adds some more "const" attributes in the area.

diff --git a/src/augeas.c b/src/augeas.c
--- a/src/augeas.c
+++ b/src/augeas.c
@@ -121,7 +121,7 @@
  * SEGMENT = STRING ('[' N ']') ? | '*'
  * where STRING is any string not containing '/' and N is a positive number
  */
-static struct path *make_path(struct tree *root, const char *path) {
+static struct path *make_path(const struct tree *root, const char *path) {
     struct path *result;

     CALLOC(result, 1);
@@ -906,8 +906,8 @@
     return -1;
 }

-int print_tree(struct tree *start, FILE *out, const char *pathin,
-                int pr_hidden) {
+int print_tree(const struct tree *start, FILE *out, const char *pathin,
+               int pr_hidden) {

     struct path *p = make_path(start, pathin);
     char *path = NULL;
diff --git a/src/internal.c b/src/internal.c
--- a/src/internal.c
+++ b/src/internal.c
@@ -52,7 +52,8 @@
                 seg += 1;
             strcat(*path, seg);
         } else {
-            *path = malloc(len);
+            if ((*path = malloc(len)) == NULL)
+                return -1;
             strcpy(*path, seg);
         }
     }
diff --git a/src/internal.h b/src/internal.h
--- a/src/internal.h
+++ b/src/internal.h
@@ -201,7 +201,8 @@
 int tree_rm(struct tree **tree, const char *path);
 struct tree *tree_set(struct tree *tree, const char *path, const char *value);
 int free_tree(struct tree *tree);
-int print_tree(struct tree *tree, FILE *out, const char *path, int pr_hidden);
+int print_tree(const struct tree *tree, FILE *out, const char *path,
+               int pr_hidden);
 int tree_equal(const struct tree *t1, const struct tree *t2);

 #endif