Re: Coverage for libltdl/slist.c and fallout

[Gary V. Vaughan]

Hallo Ralf!

On 29 Nov 2009, at 16:27, Ralf Wildenhues wrote:
> A first step at more libltdl coverage: better coverage for the slist
> API.

Excellent, and thank you.

>  Notes:
> 
> - I did find bugs in slist.c, albeit serious ones only in code not used
> elsewhere in libltdl.

Ick.  I think that libltdl might be the last live project still using
slist.c, so we can probably call this the master copy from here on in.

> - slist_foreach and slist_find are redundant, as they have the same
> semantics.

Oh, how come I never noticed that?  Maybe we can make slist_find into
a macro then... I like that the name of the call makes it obvious what
the client code is doing.  slist_foreach is also safe for deletion of
list items, whereas slist_find is not.

> - slist_remove should IMVHO return an SList *, because otherwise there
> is no way to avoid a memory leak.  APIs that force memleaks are bad.

I don't understand that assertion; where is the memory leak in the
following code?

/* Return the contents of a `boxed' ITEM, recycling the box itself.  */
void *
slist_unbox (SList *item)
{
  void *userdata = 0;

  if (item)
    {
      /* Strip the const, because responsibility for this memory
         passes to the caller on return.  */
      userdata = (void *) item->userdata;
      free (item);
    }

  return userdata;
}

...

static SList *loaders = 0;

static void *
loader_callback (SList *item, void *userdata)
{
  const lt_dlvtable *vtable = (const lt_dlvtable *) item->userdata;
  const char *      name    = (const char *) userdata;

  assert (vtable);

  return streq (vtable->name, name) ? (void *) item : NULL;
}

...
   return (lt_dlvtable *)
      slist_unbox ((SList *) slist_remove (&loaders, loader_callback, name));
...

I think the boxing/unboxing is a very elegant way to provide for clear
separation of concerns where the SList implementation handles the memory
for the list chaining wrapper structures (the boxes), where the client
code handles just the memory for the data being chained (a list of
dlloaders in this case), while also avoiding hand writing the glue code
to add the next pointers into structures that will be fed to SList.

Anyway, if you are proposing that SListCallback functions passed to
slist_remove should always return SList *, then that does seem like a
worthy addition to me. In that case, the following is cleaner, and
goes some way towards preventing misuse of the API by having the find
callback return something from the innards of an element (which is,
by the way still a valid use-case for other SListCallback occurences):

libltd/libltdl/slist.h:
typedef SList *  SListRemoveCallback   (SList *item, void *userdata);
...
LT_SCOPE void * slist_remove    (SList **phead, SListRemoveCallback *find,
                                 void *matchdata);

libltdl/slist.c:
SList *
slist_remove (SList **phead, SListRemoveCallback *find, void *matchdata)
{
  SList *stale = 0;
  SList *result = 0;

  ...rest of slist_remove is untouched...
}


> (Apart from that, I've never really understood the difference between
> boxed and unboxed stuff, basically you have to have a SList header in
> order to put something in a list, period.  But I didn't want to mangle
> the code beyond bug fixing really.)


SList is designed to manage two types of lists:

  (i) things that were specifically designed to be chained, in which
      case the first field of the structures to be chained has to be
      'next':

      struct notboxed {
         struct notboxed *next;
         <whatever real data needs to be stored in the element>
      };

 (ii) things that were not designed to be chained, but that we want
      to handle lists of in any case.  In this case, the SList api
      allows wrapping each item in an chainable element (boxing the
      item) and getting the original back out again (unboxing it).

      SList *open_filehandles = 0;
      ...
        FILE *fh = fopen ("/home/gary/input", "r");
     
        open_filehandles = slist_cons (slist_box (fh), open_filehandles);
      ...
        fclose (slist_unbox ((SList *) slist_remove (&open_filehandles,
                    match_cb, matchdata)));

      otherwise, we'd need to build our own wrapper struct to contain
      the chain pointer and the FILE pointer.

> - In the end I grew really lazy and added the new test to the old
> testsuite: that seemed the easiest way to integrate and catch all the
> compilation and include flags from toplevel Makefile.am in order to
> build and use a separate slist.o object.  If this is not ok with you,
> then please complain loudly.

I'm with Peter on phasing out the old testsuite.  But I accept that it
does offer coverage that AutoTest can't, so maybe we need to find a way
to make the old testsuite less painful to bootstrap.  In the end, I'd
rather have slist coverage in the old testsuite than not at all :)

> So I have three patches I would like to commit as part of this.
> The first one adds the test and fixes slist.c,

I recommend adding a new callback with SList* return type as I said
above.

> the second one is only
> stylistic and uses NULL instead of 0 throughout slist.c,

IIRC, it was trying to build libltdl with some old C++ compiler (maybe
Solaris 7 or so, but I don't recall clearly) which complained about the
use of NULL which drove me to using '0' in slist.c.  I googled for
NULL vs 0, and it seems there it has become a religious issue.

I'm not entirely comfortable with this change while we tout the feature
of being able to build libltdl with a C++ compiler.  I'd like to see
that C++ builds will still work correctly with explicit NULLs before we
settle on this patch.  But, if you're keen to commit but don't have time
to check, then I'll probably trip over it at some point for as long as we
have the C++ build requirement in HACKING.

> and the third
> one changes our public APIs lt_dlloader_remove and lt_dlloader_find to
> accept `const char *' arguments instead of `char *': that's what fits
> the purpose best, and what we document in the manual.  I'm not quite
> sure whether the last one constitutes a compatible API change or only
> a revision change, so versioning bump is still missing.

Implicit 'char *' to 'const char *' ought to work seamlessly IIUC, so
I'm not sure the version needs to change at all...

> Review appreciated.

By inspection only, but I hope it helps.

>    (slist_sort): Do not loop forever on one-item list.

Nice catch!  Thanks.

Cheers,
    Gary
-- 
Gary V. Vaughan (address@hidden)