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

[David Lutterkort]

Hi Jim,

On Mon, 2008-04-21 at 18:02 +0200, Jim Meyering wrote:
> I'll be happy to look.

thanks a ton for looking over this.

> On the Copyright line: add 2008

Good catch - fixed.

> For the file-spanning #ifndef,
>   #ifndef __AUGEAS_H
> I prefer to use a trailing "_".
> Leading ones are reserved, technically.  No big deal.

Fixed.

> This is important: do not hide a pointer in a typedef:
> 
>   typedef struct augeas *augeas_t;
> 
> You can even do it with just one name.  I.e., do this instead:
> (impinge less on user name-space)
> 
>   typedef struct augeas augeas;
> 
> Even the *_t name-space is reserved, technically,
> but only language lawyers care if you violate that one.

Fixed  .. yeah, I didn't think about const correctness there. I
initially did that to emphasize that struct augeas is opaque for API
users, but doing that the way you suggest achieves the same thing.

> aug_print can fail, so it must return an indication of success or failure.

Good catch .. fixed that.

> Most of the above is pretty shallow.  I haven't even really studied
> how these functions are used or how the underlying data structures are
> arranged, so please consider this a first pass.

This was already very good. If you have time to review more, I'd
appreciate it a lot. I know that there is a lot of error checking in the
guts of Augeas that is still missing.

> 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.

> 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)

>   Even reading a normal file can cause trouble if
> it's too large relative to the size of available virtual memory, hence
> that function's "max_len" parameter:

Good point; since I have no good idea what a reasonable maximum file
size if, I capped it at 32MB, which is way bigger than any config file
I've ever seen.

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

> diff --git a/src/augeas.c b/src/augeas.c
> --- a/src/augeas.c
> +++ b/src/augeas.c
> @@ -121,13 +121,13 @@
>   * 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);
>      if (result == NULL)
>          return NULL;
> -    result->root = root;
> +    result->root = (struct tree *) root;

I started declaring various fields in internal structs as const
pointers, but invariably I wind up doing casts like the above, which
just doesn't feel right. A common case is where a struct contains a
string that is almost always read-only, but when you free the struct you
need to write something ugly like 'free((char*) state->string)'

David