[augeas-devel] [PATCH 3 of 4] Check for (some) allocation failures

[David Lutterkort]

On Fri, 2008-05-09 at 00:50 -0400, James Antill wrote:
> On Thu, 2008-05-08 at 18:33 -0700, David Lutterkort wrote:
> >  
> >  static struct re *make_re_rep(struct re *exp, int min, int max) {
> >      struct re *re = make_re(ITER);
> > -    re->exp = exp;
> > -    re->min = min;
> > -    re->max = max;
> > +    if (re) {
> > +        re->exp = exp;
> > +        re->min = min;
> > +        re->max = max;
> > +    }
> >      return re;
> >  }
> 
>  IMO you have to be really careful when you implicitly pass reference
> ownership, I think what you really want here (dito. make_re_binop) is:
> 
>  static struct re *make_re_rep(struct re *exp, int min, int max) {
>      struct re *re = make_re(ITER);
> -    re->exp = exp;
> -    re->min = min;
> -    re->max = max;
> +    if (re) {
> +        re->exp = ref(exp);
> +        re->min = min;
> +        re->max = max;
> +    }
> +    unref(exp, re); /* don't leak what was passed in on failure */
>      return re;
>  }

No, the usage is correct; the tortuous comment in ref.h about 'taking
ownership' and 'receiving ownership' tries to explain that: MAKE_RE_REP
receives ownership of EXP, meaning that after it returns EXP belongs to
whatever it did with it, not the caller of MAKE_RE_REP anymore.

The (badly documented) convention in the PARSE_* and MAKE_RE_* functions
in fa.c is that they receive ownership for the RE's in their arguments,
and give ownership of whatever they return to their caller.

> ...so the interface always consumes the reference, this makes the
> calling code correct:
> 
>     if (match(regexp, '?')) {
>         re = make_re_rep(re, 0, 1);
>     } else if (match(regexp, '*')) {
>         re = make_re_rep(re, 0, -1);
>     } else if (match(regexp, '+')) {
>         re = make_re_rep(re, 1, -1);
>     }

Preceding the above snippet in PARSE_REPEATED_EXP is
        struct re *re = parse_simple_exp(regexp, error);

Here, PARSE_SIMPLE_EXP returns an RE that is now owned by
PARSE_REPEATED_EXP; the block you qoute passes that ownership on to
MAKE_RE_REP, and gets a new RE back that is again owned by
PARSE_REPEATED_EXP; by and large, that ownership is passed up to the
caller of PARSE_REPEATED_EXP

> ...although the three different usages of the "re" symbol now, in that
> one function is a bit ... interesting :).

True; using the same symbol is meant to emphasize how ownership is
passed around, but I am just realizing that it's a nice leak when
make_re_rep fails. Ugh.

David