[augeas-devel] square lens review

[David Lutterkort]

On Sun, 2010-08-22 at 10:29 -0400, Francis Giraldeau wrote:
> Hi, 
> 
> I did modified the lens square patch: 
> 
> * renamed *tip to *square for clarity
> * code cleanup and formating
> * split from other changes
> * New changelog to explain changes
> * Squash patches into one
> 
> The code is in branch square-release : 
> 
> http://github.com/giraldeau/augeas/tree/square-release

This looks very good, almost ready to merge. There were a few problems I
found, and I attached a patch against your branch that corrects all but
one of them. In more detail:

(1) lns_make_square vs. memory management

Memory mgmt in Augeas is a bit of a mess, and I'd love to switch to a gc
for a lot of the backend memory mgmt; but that it's unlikely I'll get
around to it any time soon.

For reference counted structs, you should never explicitly call the
free_xxx routine, since that will complain if the ref count on the
struct is not zero yet. Instead, use unref; e.g., instead of calling
free_value(v) you should do unref(v, value)

The other issue is that you need to pay close attention where the
reference for newly created objects go. Most make routines consume the
reference, i.e. they incorporate the object without increasing its
refcount. The right pattern to use is

    lens = lns_make_prim(L_KEY, ref(info), ref(reg));
    if (some_error(lens))
      goto error;

    done:
      unref(info, info);
      unref(reg, regexp);
    
    error:
      unref(lens, lens);
      goto done;

A lot of the existing code gets that wrong, too. The idea is that
regardless of whether the function succeeds or errors out, we consume
exactly one reference for info and reg.

(2) Typechecking was turned off

Combinators like lns_make_concat take a flag to indicate whether they
should do typechecking or not. That flag needs to be passed through from
lns_square in builtin.c

(3) Some allocation error

When you run the test through valgrind, e.g, with

        libtool --mode=execute valgrind ./src/augparse --nostdinc tests/modules/pass_square.aug
        
it reports a bad memory access deep in the bowels of put - I haven't
been able to figure out where that comes from.

But with the attached patch, and a solution to (3), this will be ready
to merge.

David

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-a-few-problems-with-the-square-lens.patch
Type: text/x-patch
Size: 7757 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/augeas-devel/attachments/20100827/aa8ce6f8/attachment.bin>