[Pkg-openldap-devel] Problems with current slapd postinst

[Steve Langasek]

--J2SCkAp4GZ/dPZZf
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi folks,

I'm looking over the current slapd maintainer scripts, and I'm seeing a
number of issues that I think are bugs that I'd like to discuss here before
I start making changes and committing them to svn; apologies for the long
mail, but this is something of a brain dump of all the issues I'm seeing.

First, there's bug #304840, which is that 2.2.23-2 tries to load an ldif
unconditionally, even if the preinst had no reason to write one out.
Obviously we would want to keep the dumping/loading in sync, so that one
isn't happening without the other; but I think the larger issue in trying to
do that is that there's unnecessary user interaction here, or at least we're
asking the wrong question.

  Template: slapd/dump_database
  Type: select
  Choices: always, when needed, never
  Default: when needed
  _Description: Dump databases to file on upgrade:
   Before upgrading to a new version of the OpenLDAP server the data of=20
   your LDAP directories can be dumped to plain text files (LDIF format)
   which is a standardized description of that data (LDIF stands for
   LDAP Data Interchange Format).
   .
   Selecting "always" will make the maintainer scripts dump your
   databases before upgrading unconditionally. Selecting "when needed"
   will only dump the database if the new version is incompatible with
   the old database format and it has to be reimported. The "never"
   choice will just go ahead without ever dumping your database.

*why* would anyone want the package to dump the database for them when it's
not needed?  This makes the upgrade slow and error prone, and eats up disk
space with both the redundant binary copies of the database and the
redundant ldifs.  I always keep my own LDIFs around for backup purposes; why
would the package need to create another one for me?

If there is a use case for always creating an LDIF, I think that we at least
shouldn't reload the database just for this reason.  Thus, I think the check
in the postinst should be:

  if database_format_changed; then
  	move_incompatible_databases_away
  	load_databases
  fi

which should be fairly idempotent actually, since
move_incompatible_databases_away checks for an empty source directory and
moves on, while bombing out if there are files at both the source and target
locations.

However, this code doesn't deal with the case when the user answered to
'never' dump databases, because then the preinst does:

dump_databases() {                                                      # {=
{{
# If the user wants us to dump the databases they are dumped to the
# configured directory.

        local db suffix file dir failed

        database_dumping_enabled || return 0

	[...]
}

and thus returns success from a function that hasn't dumped anything simply
because the user said not to.  Why would we want to allow the user to do
this?  We *know* that the user will end up with an unusable install if
upgrading from an incompatible old version, and that if they do have an LDIF
for backup purposes, we don't know where it is.  They *may* (if they don't
have a good backup policy) be left with no LDIF, and no tools on the system
that they can use to create one.  Does it make sense to let the user break
the package this way?  (It will break -- the best case scenario is that
the upgrade will die in the postinst, and they'll have to dig through the
maintainer scripts to see where they have to put their LDIF to have it
auto-loaded.)

I think the best thing here is to drop the slapd/dump_database question; the
second-best thing is to no longer give "never" as an option.  Since this
question is only asked at medium priority anyway, it's pretty clear to me
that having control over this isn't actually all that important.  If users
need to have the option to not do a dump, then I think this really needs to
be turned into an "abort upgrade" option instead.

Dropping the question would also partially solve another problem with the
preinst script, which is that it does

  if [ -e "/usr/share/debconf/confmodule" ]; then
         . /usr/share/debconf/confmodule || true
  fi

at the top, but then goes on to call functions that depend on debconf and
*will* fail if it's not present.  (Both the check for whether to dump, and
the check for where to dump, fail to handle the case when the debconf
package is not in an installed state.)  So effectively we have an undeclared
pre-depends on debconf here, which it would be nice to get away from, IMHO.
Alternatively, it is valid to Pre-Depend on debconf -- there are 71 packages
in unstable which already do so.

Other minor comments:

- database_dumping_destdir pulls a value from debconf; this value could
  change between the preinst and the postinst.  Should that be handled
  somehow?

- /usr/share/doc/slapd/examples/DB_CONFIG -- I realize this is wrapped with
  checks for whether the directory exists, but users who choose not to have
  /usr/share/doc installed on their system shouldn't be penalized by getting
  a broken bdb setup.  /usr/share/slapd would be much better.  (we already
  have /usr/share/slapd/slapd.conf, after all.)  linking
  /usr/share/doc/slapd/examples/ to /usr/share/slapd/ is explicitly allowed,
  btw.

- alert_user -- if it's really that important that the user see it, use
  db_input critical, not db_input high; in any case, I don't think it's
  appropriate to spit a replacement message out on stderr just because the
  admin has configured debconf not to display the message.

- In general, the maintainer scripts are a lot noisier than I think they
  should be, in keeping with Policy section 3.10.  I hope some of these
  echo's can be phased out as we gain more confidence that the maintainer
  scripts are doing the right thing.

- update_access_config_directives -- eew, no.  Torsten, I've already talked
  to you about this, I'll probably cook up a replacement this weekend. :)

- # Better back up the config file in any case
  echo -n "  Backing up $SLAPD_CONF in =04atabase_dumping_destdir... " >&2
  backup_config_once
  echo done. >&2

  Hmm, well, once update_access_config_directives is fixed, I disagree. :)
  Among other things, this seems redundant given that each of the functions
  that *does* update the slapd.conf calls install_new_slapd_conf, so this
  chunk will create a backup even when we know we don't need one.


If anyone wants to tell me why I'm wrong about one of the above items,
please do; otherwise I'll probably start trying to commit a bunch of these
fixes this weekend. :)  I would like to hear others' thoughts on what form
the slapd/dump_database question should take.

Also, expect some updates to (and translations of) the new debconf
templates, once I have a chance to go over them with Christian Perrier.

Cheers,
--=20
Steve Langasek
postmodern programmer

--J2SCkAp4GZ/dPZZf
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)

iD8DBQFCYbe6KN6ufymYLloRAkBwAJwLC406FM7qqIPCKzgr+s5cb7lm1ACgqw0V
KsTD/gzP4Mo23dXfnDCT9qw=
=xuZQ
-----END PGP SIGNATURE-----

--J2SCkAp4GZ/dPZZf--