Re: 329-gary-allow-RTLD_GLOBAL

[Gary V. Vaughan]

On 6 May 2007, at 21:57, Ralf Wildenhues wrote:
> Hi Gary,

Hallo Ralf,

Thanks for the review :-)  Patch now committed to CVS.

> * Gary V. Vaughan wrote on Tue, May 01, 2007 at 04:33:37PM CEST:
> > Sorry for the delay.
>
> Don't worry, and likewise.  Thanks for the patch.  Please ensure  
> next time
> that your mailer does not wrap long lines;

Unfortunately, my laptop is in repair, and I don't want to spend the  
time
setting up postfix on my desktop, then configuring my patch posting  
scripts
to work in that environment.

> I should apologize in advance that my mailer may do the same now.

No problem.  I'm afraid that my desktop setup will continue to wrap long
lines too, although I should have my laptop back in a day or two.

> > I've added a test, which tries to be sympathetic to architectures  
> > that
> have
> > no loaders that listen to advise, but will probably take a few
> iterations
> > to shake out spurious failures on some hosts.
>
> Sure.  Let's get the remaining stuff out once this is in.
> Which hosts have you done testing on already?

Just Ubuntu and OS X.  Though, after my next couple of patches I think
we'll be in good shape for an alpha release, which I'll test extensively
on all the architectures I have access to.

> > Okay to commit?
>
> Yes.  Below is a bunch of nits.  Please address as many as you can and
> indicate which ones you left out, and apply.  Thanks.

Will do.

> >         (lt_dladvise_init, lt_dladvise_destroy): New functions to
> >         initialize and destroy an advise type hint.
> >         (lt_dladvise_ext, lt_dladvise_resident, lt_dladvise_local)
> >         (lt_dladvise_global): Set hints on an advise type.
>
> Upon reading, I wondered whether  s/advise type/advice/  would  
> benefit or
> hurt the text and the code.  No idea really.

I went back and forth on this myself.  Arguably we could use verbs for
functions (advise) and nouns for types (advice), but in the end I think
the consistency of advise saves having to remember what word to use at
each point in the API.  I don't have any particularly strong feelings
about it though incase someone would like to patch for `advice' in some
or all of the API after this commit.

FYA(musement) I took inspiration for the name from common lisp's advise
function, which wraps some other function with pre and post  
processing at
run time.

> >    +  - New lt_dlopenadvise takes a new lt_dladvise type argument,  
> > which
> +    lets the caller request local or global symbol visibility from
> the
> >   +    module loader with lt_dladvise_local and lt_dladvise_global +
> respectively.  If neither is given, or if lt_dlopen (or
> > lt_dlopenext)
> >   +    are called, then the system default module symbol  
> > visibility is
> > used.
> >   +  - The new lt_dladvise_init based APIs also allow caller requests
> for
> >   +    a filename extension search with lt_dladvise_ext, and for  
> > marking
> a
> >   +    module unloadable with lt_dladvise_resident.
>
> Should all new interfaces (all functions) be listed in NEWS?  I'd  
> prefer
> that, even if you don't give any explanation for their semantics.
> Autoconf does this, too.

Yes, and I thought I had done that.  Turns out I didn't update NEWS  
after
I included lt_dladvise_destroy.  Now fixed.

> >   address@hidden int lt_dladvise_init (lt_dladvise address@hidden)
> >   +The @var{advise} parameter can be used to pass hints to the module
> +loader when using @code{lt_dlopenadvise} to perform the loading. +The
> @var{advise} paramater needs to be initialised by this function
>
> s/paramater/parameter/

Thanks.  Done.

> >   +before it can be used.  Any memory used by @var{advise} is
> automatically
> >   +recycled when it is passed to @code{lt_dlopenadvise}.
>
> Is that a good idea?  What if I want to advise hundreds of libraries?
> Also, I think this bit of documentation does not match the
> implementation, nor the my_dlopenext example below: lt_dlopenadvice  
> does
> not free the storage that `advise' uses.  So I guess rather point to
> lt_dladvise_destroy here, or, as it's described as very next, just do
> without a pointer.

ACK.  I started out with it implemented as described in the  
documentation,
but, as you noticed, realised that it wasn't a good idea.  I forgot to
update the docs after changing the implementation.  Now fixed.

> >   +The following example is equivalent to calling
> >   address@hidden (filename)}:
> >   +
> >   address@hidden
> >   +lt_dlhandle
> >   +my_dlopenext (const char *filename)
> >   address@hidden
> >   +  lt_dlhandle handle = 0;
> >   +  lt_dladvise advise;
> >   +
> >   +  if (!lt_dladvise_init (&advise) && !lt_dladvise_ext (&advise)) +
> handle = lt_dlopenadvise (filename, &advise);
> >   +
> >   +  lt_dladvise_destroy (&advise);
> >   +
> >   +  return handle;
> >   address@hidden
> >   address@hidden example
>
> It would be cool if this example matched exactly a bit of testsuite  
> code.
> As of now, there are small differences to hint_ext in the test you  
> added.

The current test is a minimal effort bad example of coding that is  
designed
to stress test the api, where the documentation is trying to show a  
minimal
example with relatively clean code.  I'd either have to expose my  
lack of
error checking and non-expandable code from the test, or else change the
way the tests are written to make them identical.  There's no reason we
can't extract the example code into a separate test later though.

> >   +On failure, @code{lt_dladvise_ext} returns non-zero and sets an  
> > error
> +message that can be retrieved with @code{lt_dlerror}.
>
> Also, it would be cool if this functionality (failure to open) were
> exposed in the testsuite; likewise for the other new functions.

The only way it can fail at the moment is with an out of memory error in
lt_dladvise_init, in which case getting the test to run to completion  
will
be a challenge.  More tests are always welcome though.

> >   address@hidden int lt_dladvise_local (lt_dladvise address@hidden) +Set
> the @code{symlocal} hint on @var{advise}.  Passing an
> @var{advise}
> >   +parameter to @code{lt_dlopenadvise} with this hint set causes  
> > it to
> try
> >   +to keep the loaded module's symbols hidden so that they are not
> +visible to subsequently loaded modules.
> >   +
> >   +On failure, @code{lt_dladvise_local} returns non-zero and sets an
> error
> >   +message that can be retrieved with @code{lt_dlerror}.
>
> Should the hint about lt_dlgetinfo be repeated here?  Otherwise,  
> the user
> may think that a non-failure of lt_dladvise_local may indicate that  
> the
> host /can/ localize/hide symbols.  WDYT?

Good call.  Done.

> >   @@ -3853,17 +3944,16 @@ Some of the internal information about e
> >    maintained by libltdl is available to the user, in the form of  
> > this
> structure:
> >   address@hidden {Type} {struct} lt_dlinfo @{ @w{char address@hidden;}
> > @w{char address@hidden;} @w{int @var{ref_count};} @w{lt_module
> > @var{module};address@hidden
> >   address@hidden {Type} {struct} lt_dlinfo @{ @w{char address@hidden;}
> > @w{char address@hidden;} @w{int @var{ref_count};} @w{int
> @var{is_resident};}
> > @w{int @var{is_symglobal};} @w{int @var{is_symlocal};address@hidden
>
> Hmm.  If lt_dlinfo is part of the public interface, and looking at  
> 1.5.22
> it seems is it, then this is an incompatible change that requires a  
> major
> version bump.

Yes indeed.  Actually, the module field had already been moved out of
lt_dlinfo before I changed the docs, so we're past due the major version
bump.

> We could discourage use of lt_dlinfo without going through pointer  
> access
> and malloc, in order to avoid this next time.

I don't follow.  What would be malloced?  We already state that this  
is a
read-only structure.  Although I have no clue why we exposed the  
internal
module field originally :-/

> >    /* lt_dlloader.c -- dynamic library loader interface
> >   -   Copyright (C) 2004 Free Software Foundation, Inc.
> >   +   Copyright (C) 2004, 2007 Free Software Foundation, Inc.
>
> Any reason for this change?

Forgot to revert it after I undid a change to that file.  Fixed.


> >   +/* Try all dlloaders for FILENAME.  If the library is not
> successfully
> >   +   loaded, return non-zero.  Otherwise, the dlhondle is stored  
> > at the
>
> s/dlhondle/dlhandle/

The joys of dvorak keymapping vs a small font :-)  Good catch.  Thanks.

> >   +static lt__advise *
> >   +advise_dup (lt__advise *advise)
> >   +{
> >   +  lt__advise *dup = (lt__advise *) lt__zalloc (sizeof  
> > (lt__advise));
> +  return memcpy (dup, advise, sizeof (lt__advise));
> >   +}
> >   +
> >   +/* Libtool-1.5x interface for loading a new module named FILENAME.
> */
>
> s/1\.5x/1.5.x/

Thanks.

> >      char *     name;           /* module name */
> >      int                ref_count;      /* number of times lt_dlopened minus
> >                                    number of times lt_dlclosed. */
> >   +  int                is_resident:1;  /* module can't be unloaded. */
> >   +  int                is_symglobal:1; /* module symbols can satisfy
> >   +                                subsequently loaded modules.  */
> >   +  int                is_symlocal:1;  /* module symbols are only available
> >   +                                locally. */
>
> Are we certain that bitfields are portable enough?  I suppose with  
> C89 as
> prerequisite that should be no issue.

I don't know, but I didn't get any feedback when I posed that  
question with my
original patch proposal.  I think they ought to be, and we'll find  
out soon enough
during alpha testing.

> >   +void
> >   +hint_ext (void)
> >   +{
> >   +  lt_dlhandle handle;
> >   +  lt_dladvise advise;
> >   +
> >   +  if (lt_dladvise_init (&advise) || lt_dladvise_ext (&advise)) +
> complain ("error setting advise ext");
> >   +
> >   +  handle = moduleopen ("moddepend", advise);
> >   +
> >   +  if (handle)
> >   +    printf ("depend: %d\n", moduletest (handle, "g", "j"));
> >   +
> >   +  lt_dladvise_destroy (&advise);
> >   +}
>
> handle is never closed here.  This is important: some failures are  
> only
> caught at module closing time.

in main I've written:

  if (lt_dlexit () != 0)
    complain ("error during exit");

Is there something this won't catch?

> >   +$CC $CPPFLAGS $CFLAGS -c main.c
> >   +for file in modresident modlocal modglobal moddepend; do
> >   +  $LIBTOOL --mode=compile $CC $CPPFLAGS $CFLAGS -c $file.c
> >   +  AT_CHECK([$LIBTOOL --mode=link $CC -module $CFLAGS $LDFLAGS -o
> > $file.la \
> >   +            $file.lo -rpath /foo -avoid-version], [], [ignore],
>
> What about -no-undefined, for w32?

That would defeat building moddepend with unresolved symbols at link  
time,
so that we can test that it correctly calls the matching symbols from
modglobal after load time symbol resolution :-(  Known issue, but I  
don't
know how to fix it.  I guess we'll need to skip this test on Windows and
AIX due to linker brain damage?

Cheers,
        Gary
--
  ())_.              Email me: address@hidden
  ( '/           Read my blog: http://blog.azazil.net
  / )=         ...and my book: http://sources.redhat.com/autobook
`(_~)_ Join my AGLOCO Network: http://www.agloco.com/r/BBBS7912

PGP.sig
Description: This is a digitally signed message part