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

[David Lutterkort]

On Tue, 2008-04-22 at 20:38 +0200, Jim Meyering wrote:
> > 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.

Yes, that's another option, though slightly less friendly for binding in
other languages; I completely agree with you that the current aug_match
interface is not very well-suited for pulling out large (or even
moderately-sized) amounts of data. But I'd rather wait for a real-world
use where somebody does try to match large numbers of entries before
adding an iterator-style or callback (or both) interface to address that
waste.

> But that still misbehaves on non-regular, non-directory files.

I don't see a legitimate reason why Augeas would read anything but
regular files or through symbolic links; keep in mind that for Augeas'
purposes, after reading a file, it also needs to be able to write a
modified version back.

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

Good point. Concurrent file modification (Augeas reads file, somebody
else changes it, Augeas tries to write file) is another area that needs
a lot more error checking.

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

Ok .. you convinced me. I'll change read_file to use fread_file_lim.

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

That's why I haven't done that ;) But Augeas has the potential to read a
_lot_ of files on startup. But until it does, and that leads to
performance problems, I am not going to try mmap or anything else.

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

Cool .. committed.

David