Re: Coverage for libltdl/slist.c and fallout

[Ralf Wildenhues]

Hi Gary,

* Gary V. Vaughan wrote on Mon, Nov 30, 2009 at 06:29:54PM CET:
> On 29 Nov 2009, at 16:27, Ralf Wildenhues wrote:
> > - 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?

There is none.  In your example, the callback, thus also slist_remove,
pass back an SList* already, which is good.  But the API also allows
the callback to return a pointer to the userdata only (at least that's
how I understand it).  That would still work alright with slist_find,
but with slist_remove, it necessarily causes a memory leak of the SList
structure pertaining to the data returned.  That's the case where the
user code "forgets" to use slist_unbox, namely because the item it gets
is already unboxed, and the box already leaked.

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

I'm not sure I agree that this is elegant, but maybe I just
misunderstand the code.

But all of this is so unimportant to the libltdl code quality that I
prefer we just keep the code we have, and *not* start introducing more
new code with more new bugs here.

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

No.  I don't propose that.  I only propose it for slist_remove, because
that's where otherwise, a memleak is inevitable.  That's just what my
patch does, by letting slist_remove return an SList *.

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

I don't believe you that it was really designed to do (i).  If it were,
then there were some code using it as such, which I would like to ask
you to submit as testsuite addition, so we can see whether, and *how*
it works at all, and the coverage may keep us from breaking it.  I don't
believe that it works.  Just like the sorting functions did not work.

If (i) does work, then likely my patch is broken after all; so I guess I
will have to wait with applying.

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

Oh well.  I'm not attached to the NULL patch.

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

Well I built it with g++ 4.2.4.

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

OK, thanks.  Well, REVISION needs a bump before the next release of
course.

Cheers,
Ralf