Re: [exim-dev] [Bug 139] Dynamically loadable lookup modules

On Wed, 2008-05-14 at 14:57 +0100, Tony Finch wrote:
> --- Comment #20 from Tony Finch <dot@???> 2008-05-14 14:57:21 ---
> Some comments based on a fairly cursory read-through of the patch...

Thanks for the review.

> I'm not sure that configuring the available lookups by installing files in
> a library directory is very nice. I can see it makes sense from a Linux
> distribution point of view, but when building from source or creating a
> unified package it makes sense to install all the lookups then load only
> the necessary ones at run time. I'm not sure if this would lead to a
> chicken/egg problem.

Hm, I haven't really given much thought to use cases _other_ than that
of software distributors who want to make it _possible_, but not
mandatory, to use certain lookups in their prebuilt version.

When you're building from source, why would you ever build modules
dynamically? You can just turn off the ones you don't want (or don't
have the necessary development environment for).

When you say "creating a unified package", you mean a package such as
the existing distro packages, with all the lookups present -- except
that they're as modules, not built-in? That kind of defeats the object
of having them modular, surely?

Certainly, it would be possible to set stuff up so that you have to list
the modules you want loaded, rather than having them get loaded
automatically. I don't really understand the use case where that would
be preferable, though. The reason I _don't_ like it is because it means
you have to edit your config when you (or your distro) switch to
building with dynamically-loaded modules.

I was thinking of moving the contents of the existing 'exim' package to
an 'exim-core' package, and having an empty 'exim' package which pulls
in that _and_ the lookup modules which we used to build statically. That
way, it would have no effect on upgrades. People can still remove the
'exim', exim-pgsql' and 'exim-mysql' packages if they want to save
space, but configurations don't break by default.

> drtables.c:
>
> The #include <dlfcn.h> should be removed for portability to
> systems without that header. Instead add defined(LOOKUP_MODULE_DIR) to
> the #if on line 463 of exim.h which guards another #include <dlfcn.h>.

Fixed (in {http,git}://git.infradead.org/users/dwmw2/exim-dynlookup.git)

> lookupapi.h:
>
> This file is missing the copyright and end-of-file rubric.

University of Cambridge 1995-2007?

I suspect that the end-of-file rubric might be something we could start
to dispense with. (One of these days I might even be brave enough to
suggest running the whole thing through indent, too. It makes my brain
hurt :)

> The LMM1 comment should be less cryptic.

Can you be a little more specific about what you want it to say?
0x4c4d4d41 is "LMM1"... that seems simple enough. Maybe it wants a
comment saying how it's used, but that's a different issue.

> lookups/Makefile:
>
> This file is gmake-only which is a nasty regression. Exim's build system
> has stuff in the scripts directory that constructs the actual Makefile
> based on EDITME. I would greatly prefer it if that mechanism were adapted
> to support the new functionality so that we don't have to add another
> build dependency.
>
> The .c.so rule is non-portable. The shared-library command line options
> need to be made into an EDITME option.

Will play with that; thanks. Which bits are gmake-only?

--
dwmw2