Re: rewrite try_dlopen (2)

[Gary V. Vaughan]

Hi Ralf!

Ralf Wildenhues wrote:
> Ok, I've already threatened I'd further rewrite the .la file
> parsing.  Here it is.  I haven't had time to split this patch in
> nice little pieces, so if you want me to do that, please say so.

The only requirement is that each commit should address a single
logical change.  As long as you don't try to do two different things
in a single changeset, then I (for one) am happy.

> This splits pars_dotla_file in a bunch of small functions, each of
> which does one thing.  It introduces fairly strict checking of the
> .la file syntax (not its semantics), adds an error reporting a
> malformed .la file, groups .la file information into an internal
> structure, which is also used by find_module.  The error reporting
> could easily be improved now (e.g., report where the parse error
> occured and what caused it), but that would probably imply some sort
> of ltdl interface change, so I'd like to solicit if that is wanted.

Definitely of interest.  I don't think it needs to change the
interface in a backwards incompatible way, for example we could
just add a lt_dlerror_position() entry point (not that I've given
it much thought mind).

> My splitting between parsing and evaluating might be a little too
> strict, but I think the code is easier to understand that way.

I'm big on maintainability above all else (including efficiency!),
so that is a win for me.

> The getline clone get_line that fell out of this change could in
> principle be replaced by the glibc one (and LIBOBJed on non-glibc
> systems), and its current implementation is still ugly, but I did
> not want to work on this now.  I'd rather do that together with
> switching ltdl from stdio to fd IO sometime soon (as requested).

If you are not planning on tackling that next, then please add a
TODO item as part of the commit incase anyone else wants to implement
it while you are working on other things.

> As as related side note, I have not made use of *scanf on purpose.  
> Together with fd IO I'd really like to also remove any usage of
> *scanf() or *printf().  This really saves on static library size,
> and libltdl IMHO deserves to be a small library.

I'm undecided on whether removing the use of stdio from libltd is a
worthwhile endeavour.  In addition to Bob's arguments, note that
almost all clients of libltdl will use stdio themselves.  I'll
mull it over and cast an opinion if you and Bob don't reach a
concensus :-)

> What's missing from this patch is a necessary change in the library
> version of libltdl.  That depends on whether the error message
> addition is backwards-compatible or not (a question I pointed out in
> some other thread already).

Okay.  I believe the error message to be a backwards compatible
change as long as we only add messages on to the end of the list;
the array is not part of the API per-se, it just needs to be
declared there so that the macros in lt_error.h can work.  That
aside, I'm wary of reving the soname for every interface change --
I think we need a way of setting a dirty flag (maybe a comment
in configure.ac?) and reving as part of the release process (alpha
releases too) if interfaces have changed since the last release...

> Further missing bits:
> - Currently no check is made for any garbage after an assignment.
> E.g.,
>    foo1=bar foo2=baz
> would only pick up the first assignment.  A check could be added.

Good idea.

> - Trailing white space in assignment values is not checked for.
> Most notably,
>    library_names='a b c '
> would fail to do the right thing.  One could remove leading and
> trailing white space in the value (disallowing is not possible
> due to existing use).
> BTW, the code now ignores white space before assignment.

This is something that will need to be tested rigorously.  I have
a sneaking suspicion that some of that whitespace is significant.
On the other hand some of it is also definitely an accident of the
shell code that produced it.  We should add a func_list_add to
general.m4sh that doesn't leave whitespace droppings...

> Next step would be a further tackle of the rest of try_dlopen, still
> ugly 300 lines of code.  Oh yeah, the documentation of the .la file
> syntax..

All good.

As a next step it would be good if you could use some macro magic (a la
lt_error.h) to tabulate and generate the *_module_parm (*_lalib_parm?
See below) functions.  Then adding a new value to the structure and
parser will be just a matter of adding a line into the table.

> If this patch pleases the audience, I'll write a (or several) nice
> ChangeLog entry and commit this.  For better reading, the patch is
> separated into the part removing the old cruft and the part adding
> the new cruft ;-)

It does.  Please commit, pending addressing the comments herein.

> --- libltdl/ltdl.c    2004-10-05 15:05:30.000000000 +0200
> +++ libltdl/ltdl.c-changed    2004-10-05 15:11:21.000000000 +0200
> @@ -82,6 +82,14 @@
>  
>  /* --- DYNAMIC MODULE LOADING --- */
>  
> +/* The contents of a .la file interesting to ltdl. */
> +typedef struct {
> +  char *dlname;
> +  char *libdir;
> +  char *deplibs;
> +  char *old_name;
> +  int installed;
> +} lt_module_data;

I don't like the use of `module'.  The dlloaders use lt_module to refer
to the system module handle inside an lt_dlhandle.  What about lt_lalib_data?

>  static       int     find_module           (lt_dlhandle *handle, const char 
> *dir,
> -                                    const char *libdir, const char *dlname,
> -                                    const char *old_name, int installed);

Ah.  Okay.  This is the core ltdl code I haven't refactored.  By the same
argument this function should be find_lalib.  There are probably others too.

>         if (tryall_dlopen_module (handle,
> -                                 (const char *) 0, libdir, dlname) == 0)
> +                                 (const char *) 0, parm->libdir, 
> parm->dlname) == 0)

tryall_dlopen_lalib?

> +    LT_ERROR(INVALID_FILE,          "malformed library file")

INVALID_LALIB?   _FILE is too ambiguous.

Cheers,
        Gary.
-- 
Gary V. Vaughan      ())_.  address@hidden,gnu.org}
Research Scientist   ( '/   http://tkd.kicks-ass.net
GNU Hacker           / )=   http://www.gnu.org/software/libtool
Technical Author   `(_~)_   http://sources.redhat.com/autobook

signature.asc
Description: OpenPGP digital signature