incubator-lucy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marvin Humphrey <mar...@rectangular.com>
Subject Re: [lucy-dev] C TAP test harnesses
Date Tue, 17 May 2011 05:34:22 GMT
On Fri, May 13, 2011 at 11:33:55AM -0700, Joe Schaefer wrote:
> > Whoa, if the apreq harness can  work without APR that would be sweet.  I
> > looked at at.c and I see that  it makes heavy use of apr_snprintf().  Can
> > you fall back to sprintf()  safely?
> 
> Haven't looked closely, but I don't see a priori why the stdlib's formatting
> functions can't be used effectively here.
  
> > > It could be dropped without much effort, or these features  added to the
> > > existing Lucy C test apparatus.
> > 
> > Would apreq accept  patches removing the APR dependency?  And is at.h a public
> > API?   Ideally, we would want to bundle those files and use them  verbatim.
> > Maintaining our own fork for the sake of Charmonizer's tests is  less
> > desirable, unless we decide we want to go all out and publish a  supported C
> > TAP library ourselves.
> 
> Yes, of course that would be accepted/encouraged in apreq.  (The at.[ch] stuff
> is also designed to work within an httpd server C module (tho we don't make use
> of it in apreq) where the TAP output is sent out over HTTP.)  Getting rid of
> the apr dependency would be a welcome change, so long as we're careful to
> avoid leaks.

I put in a few hours on this over the weekend.  I made some "progress", but I
don't think any of that "progress" so far improves on the existing apreq code.  

Joe warned me in IRC that the hard part of this refactoring job was likely to
be the excision of the APR memory pool code -- and he was right.  Memory
allocation under a pool system has a distinct usage pattern, and since the
standard library doesn't provide pools, it doesn't support that usage pattern.
With the stdlib.h heap allocation functions, ordinarily you have to pair each
malloc() with a free().  With a memory pool, you don't have to do that -- you
can overwrite allocated pointers, and you can mix and match pointers to
static/data-segment memory with pointers to pool memory:

    /* Start off by allocating some memory from the apr_pool_t. */
    thing->ptr = (char*)apr_palloc(pool, 1);

    /* Later, we might overwrite that pool pointer with another pool pointer.
     * Note that we don't free() the existing pointer first. */
    if (foo != NULL) {
        thing->ptr = apr_pstrdup(pool, foo);
    }

    /* Later, we might overwrite that pointer to pool memory with a pointer to
     * some static data. */
    static char bar[] = "bar";
    thing->ptr = &bar;

    /* When the cleanup finally happens, it doesn't matter what's in
     * thing->ptr, because the *pool* keeps track of what memory needs to be
     * freed.  We never call free() on individual pointers.  */
    apr_pool_clear(pool);

Additionally, the apreq harness takes advantage of the fact that you can
register pseudo-destructors that are triggered by apr_pool_clear() and get run
just before the pool memory gets deallocated.

        apr_pool_cleanup_register(p, q, report_local_cleanup,
                                        report_local_cleanup);

The localizer functionality in the apreq test harness depends on these
pseudo-destructors. (And unusually, it's the user's responsibility to invoke
apr_pool_clear() directly and cause the subtest cleanup as a side effect, as
opposed to invoking something like "AT_subtest_done()" which cleans up memory
as an implementation detail.) During this destructor, certain information gets
transfered from the child test harness object to the parent in a way which is
hard to track under a malloc/free memory model.  It all works fine when you're
playing by the loose rules of the APR pool system, but things get complicated
when you try to apply the constraints imposed by malloc/free.

I don't see how we can make the localizer feature work once APR is removed
without substantial rearchitecting.  And there's a bunch of other stuff that
has to happen too before the harness is Lucy-ready, all of it churn for zero
positive impact or mildly negative impact from the apreq perspective, such as
replacing apr_snprintf with a fixed up version of _snprintf under MSVC or
eliminating APR status codes.

It doesn't seem to me like it's in Lucy's interest to proceed further down
this path, nor in apreq's.  I don't like the idea of going to apreq-dev and
saying, "Here a series of patches that make your code crappier.  It's a
significant rearchitecting with a lot of churn, so it almost certainly
introduces new bugs that you're going to have to deal with.  And I'm doing
this because I want to take a private implementation detail of your project
and turn it into a public API that you have to support.  We'll be counting on
you!"

If the apreq community were to decide spontaneously to remove APR as a
dependency from at.c/at.h and start publishing it as a public API with full
backwards compatibility promises, that would be great.  But it doesn't make
sense for apreq to accept these changes, and it doesn't make sense for Lucy to
depend on apreq code when it isn't in apreq's interest to tailor it to Lucy's
needs.  

If we're going to put in this much effort making a test harness work for us,
we should either fork it and own the code, or we should be contributing to a
common project with an official public API. 

Marvin Humphrey


Mime
View raw message