incubator-lucy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Schaefer <joe_schae...@yahoo.com>
Subject Re: [lucy-dev] C TAP test harnesses
Date Wed, 18 May 2011 03:52:59 GMT
----- Original Message ----

> From: Marvin Humphrey <marvin@rectangular.com>
> To: lucy-dev@incubator.apache.org
> Sent: Tue, May 17, 2011 1:34:22 AM
> Subject: Re: [lucy-dev] C TAP test harnesses
> 
> 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!"

Two hours of hacking on your pull request and I've gotten at.[ch] to the point
where the apr dependence is gone and the apreq tests (tweaked by about 10 LOC)
all pass.  There are some warnings issued under maintainer mode, but I probably
will just ignore those and change apreq to use this de-apr'd code instead.

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

If we can get lucy using the code at http://github.com/joesuf4/at then that can
be the upstream location for both projects.

WDYT?

Mime
View raw message