[PATCH 0/6] Second call for review: conffile tracking / merging

[sean finney]

This is the next iteration of the same patch series i've previously posted.

The changes since the last version are largely of an aesthetic or
documentation-oriented nature.  For the sake of sanity I'll itemize
the changes below.

At this point it's becoming quite cumbersome to track the changes in
this fashion, so I would really appreciate getting at least Patches 1-3
committed, (everything up to the part where the hooks are added into
dpkg's operations), or otherwise hearing what needs to be done in order to
make them acceptable.  Note that these first patches not have any effect
on dpkg apart from compiling in and linking in some unused code, but
it will allow me to more easily submit future modifications and stop
needing to rebase as much.

I also have (unsubmitted here, but in my git p.d.o git repo) the beginnings
of a cmdline utility for inspection/interaction with the conffiledb.  It
isn't really worthy of review at this point beyond the RFC I posted earlier.

Notable changes:

 * rebased to master (c057025d8)
 * rename conffiles.{c,h} to conffiledb.{c,h} to be more descriptive
 * LOTS AND LOTS of (doxygen style) comments/documentation
 * formalized terminology used in comments and code (the "glossary" is
   included in the comments/docs).
 * change pathname convention from
        <admindir>/conffiles/<pkg>/<base>/<hash>
   to
        <admindir>/conffiles/<base>/<pkg>/<hash>
   it may seem superficial but it in fact simplifies a lot of code.
 * change the names of macros, functions, enumerated types, etc to
   be generally more descriptive
 * formalize parameter/variable naming to be consistant for all functions
 * also change the names to contain a leading "conffiledb_" to follow
   the conventions elsewhere in dpkg
 * remove the "basedir" path definitions from dpkg.h and leave only
   the conffile db root dir there.  the basedirs are now defined by
   a static char array which is indexed to the respective enum values
   (which makes for simple code here and later in the cmdline program)
 * expose conff_db_path/conffiledb_path as non-static, as it's useful
   to the dpkg-conffile cmdline program as well
 * small bugfix in conff_reg_fd/conffiledb_register_new_conffile to ensure
   the target conffiledb path is always available.
 * conff_commit_new is replaced with slightly more general conffiledb_commit
 * new conffiledb_diff function
 * new base reference type "resolved" for storing successful merges
 * threw in some documentation for promptconfaction while i was there
 * expose a few more static functions, moving them from help.c to a new util.c

Sean Finney (6):
  Update promptconfaction() to also require package name
  Add doxygen comments for promptconfaction
  Conffile database handling functions
  Hook conffile database into dpkg unpack/configure/remove stages
  Implement 'm' option for conffile merging during conflict resolution
  Split some useful functions from help.c to (new) util.c

 lib/dpkg/dpkg.h  |    3 +
 src/Makefile.am  |    4 +-
 src/archives.c   |   18 +++-
 src/conffiledb.c |  292 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/conffiledb.h |  166 +++++++++++++++++++++++++++++++
 src/configure.c  |   52 +++++++++-
 src/help.c       |   72 -------------
 src/processarc.c |    4 +
 src/remove.c     |    3 +
 src/util.c       |  111 +++++++++++++++++++++
 10 files changed, 644 insertions(+), 81 deletions(-)
 create mode 100644 src/conffiledb.c
 create mode 100644 src/conffiledb.h
 create mode 100644 src/util.c


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[sean finney]

In order to provide an interface into the conffiles DB API, it's required
to know the package name that owns the conffile. Since this is a static
function and the package name is available in all places that the function
is used, this is a fairly easy fix.
---
 src/configure.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/configure.c b/src/configure.c
index 72d15de..1aa8b6a 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -61,7 +61,7 @@ static int conffoptcells[2][2] = {
 static void md5hash(struct pkginfo *pkg, char *hashbuf, const char *fn);
 static void showdiff(const char *old, const char *new);
 static void suspend(void);
-static enum conffopt promptconfaction(const char *cfgfile,
+static enum conffopt promptconfaction(const char *pkg, const char *cfgfile,
                                       const char *realold, const char *realnew,
                                       int useredited, int distedited,
                                       enum conffopt what);
@@ -152,7 +152,7 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
       "deferred_configure '%s' (= '%s') useredited=%d distedited=%d what=%o",
       usenode->name, cdr.buf, useredited, distedited, what);
 
- what = promptconfaction(usenode->name, cdr.buf, cdr2.buf,
+ what = promptconfaction(pkg->name, usenode->name, cdr.buf, cdr2.buf,
                         useredited, distedited, what);
 
  switch (what & ~(cfof_isnew | cfof_userrmd)) {
@@ -554,8 +554,9 @@ suspend(void)
  * Select what to do with a configuration file.
  */
 static enum conffopt
-promptconfaction(const char *cfgfile, const char *realold, const char *realnew,
-                 int useredited, int distedited, enum conffopt what)
+promptconfaction(const char *pkg, const char *cfgfile, const char *realold,
+                 const char *realnew, int useredited, int distedited,
+                 enum conffopt what)
 {
  const char *s;
  int c, cc;
--
1.6.4.3


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[sean finney]

---
 src/configure.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/src/configure.c b/src/configure.c
index 1aa8b6a..d01a856 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -61,6 +61,29 @@ static int conffoptcells[2][2] = {
 static void md5hash(struct pkginfo *pkg, char *hashbuf, const char *fn);
 static void showdiff(const char *old, const char *new);
 static void suspend(void);
+
+/** Prompt the user for how to resolve a conffile conflict
+ *
+ * When encountering a conffile conflict during configuration, the user will
+ * normally be presented with a textual menu of possible actions. This
+ * behavior is modified via various --force flags and perhaps on whether
+ * or not a terminal is available to do the prompting.
+ *
+ * @param pkg The package owning the conffile
+ * @param cfgfile The conffile
+ * @param realold The location of the old conffile (dereferenced in the case
+ *        of a symlink).
+ * @param realnew The location of the new conffile (dereferenced in the case
+ *        of a symlink).
+ * @param useredited A flag to indicate whether the file has been edited
+ *        locally. Set to nonzero to indicate that the file has been modified.
+ * @param distedited A flag to indicate whether the file has been updated
+ *        between package versions.  Set to nonzero to indicate that
+ *        the file has been updated.
+ * @param what Hints on what action should be taken by defualt.
+ * @return The action which should be taken based on user input and/or the
+ *         default actions as configured by cmdline/configuration options.
+ */
 static enum conffopt promptconfaction(const char *pkg, const char *cfgfile,
                                       const char *realold, const char *realnew,
                                       int useredited, int distedited,
--
1.6.4.3


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[sean finney]

(note: this information is largely duplicative of the comments in conffiledb.h)

The files conffiledb.{c,h} define a set of routines for adding and removing
stored versions of the pristine conffiles as shipped in their respective
packages.

Such an implementation allows for some neat features in the future,
such as:

 * 3-way merging of conffile changes
 * showing the old->new delta for the "dist" conffiles
 * dynamic registering of conffiles (à la ucf)
 * a general dpkg/conffile cmdline interface for all of this

The layout pattern for the conffile database is:

        <admindir>/conffiles/<base>/<pkg>/<hash>

Where:

 * <admindir> is the standard dpkg administrative dir (i.e. /var/lib/dpkg).
 * <base> is a short string corresponding to which type of base reference
          is stored underneath (see source code comments for more info).
 * <pkg> is the conffile database directory for a package.
 * <hash> is the md5 hash of the installed location of a package's conffile.
          A hash is used to keep the directory structure flat and balanced.

It's assumed that this database is not meant for direct user consumption.
However, a "dpkg-conffile" cmdline interface will be provided that will
allow the user to perform various forms of inspection with regards to
installed conffiles vs the versions in this database.

conffiledb
---
 lib/dpkg/dpkg.h  |    1 +
 src/Makefile.am  |    1 +
 src/conffiledb.c |  218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/conffiledb.h |  155 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 375 insertions(+), 0 deletions(-)
 create mode 100644 src/conffiledb.c
 create mode 100644 src/conffiledb.h

diff --git a/lib/dpkg/dpkg.h b/lib/dpkg/dpkg.h
index afe650f..9b06c73 100644
--- a/lib/dpkg/dpkg.h
+++ b/lib/dpkg/dpkg.h
@@ -74,6 +74,7 @@ DPKG_BEGIN_DECLS
 #define STATOVERRIDEFILE  "statoverride"
 #define UPDATESDIR        "updates/"
 #define INFODIR           "info/"
+#define CONFFILEDBDIRROOT "conffiles/" /**< @ref conffilesdb root location */
 #define TRIGGERSDIR       "triggers/"
 #define TRIGGERSFILEFILE  "File"
 #define TRIGGERSDEFERREDFILE "Unincorp"
diff --git a/src/Makefile.am b/src/Makefile.am
index a29b629..24583de 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -20,6 +20,7 @@ bin_PROGRAMS = \
 dpkg_SOURCES = \
  archives.c archives.h \
  cleanup.c \
+ conffiledb.c conffiledb.h \
  configure.c \
  depcon.c \
  enquiry.c \
diff --git a/src/conffiledb.c b/src/conffiledb.c
new file mode 100644
index 0000000..46cbfaf
--- /dev/null
+++ b/src/conffiledb.c
@@ -0,0 +1,218 @@
+#include "conffiledb.h"
+
+#include <config.h>
+
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <errno.h>
+
+#include <dpkg/dpkg.h>
+#include <dpkg/i18n.h>
+#include <dpkg/buffer.h>
+#include <dpkg/path.h>
+#include <dpkg/dpkg-db.h>
+#include "main.h"
+
+const char *conffiledb_base_dirs[] = { "current", "new", "resolved", "tmp" };
+
+char*
+conffiledb_path(const char *pkg, const char *path, conffiledb_base base)
+{
+ char *full_path = NULL, *hash = NULL;
+ const char *basedir = NULL;
+ size_t basedir_sz = 0, conffiledb_sz = 0, full_path_sz = 0;
+
+ /* sanity check for a valid base dir */
+ if (base >= conffiledb_base_COUNT)
+ ohshit("conffiledb_path called with unsupported base %x", base);
+
+ basedir = conffiledb_base_dirs[base];
+
+ /* the root path is <admindir>/<#CONFFILEDBDIRROOT/><basedir>/
+ * (note that the '/' after #CONFFILEDBDIRROOT is implicitly included
+ * in the #define'd string). The null byte is also accounted for
+ * at this point. */
+ basedir_sz = strlen(admindir) + 1 + strlen(CONFFILEDBDIRROOT) +
+             strlen(basedir) + 2;
+ full_path_sz = conffiledb_sz = basedir_sz;
+
+ /* we will add "<pkg>/" to the string later if <pkg> is not null */
+ if (pkg != NULL)
+ full_path_sz = conffiledb_sz += strlen(pkg) + 1;
+
+ /* and if a conffile is being requested (not just the db root)... */
+ if (path != NULL)
+ full_path_sz += CONFFILEDB_MD5SUM_LEN;
+
+ /* this is the path to the conffile db for the given base/pkg */
+ full_path = m_malloc(full_path_sz);
+ snprintf(full_path, basedir_sz, "%s/%s%s/", admindir,
+         CONFFILEDBDIRROOT, basedir);
+
+ /* append "<pkg>/" if needed */
+ if (pkg != NULL)
+ sprintf(full_path + basedir_sz - 1, "%s/", pkg);
+
+ /* append the pathname's hash if relevant */
+ if (path != NULL) {
+ hash = full_path + conffiledb_sz - 1;
+ buffer_md5(path, hash, strlen(path));
+ }
+
+ debug(dbg_conffdetail, "confffiledb_path(%s, %s, %x) = %s\n", pkg,
+      path, base, full_path);
+
+ return full_path;
+}
+
+/** Ensure that a particular conffiledb dir exists.
+ *
+ * @param pkg The package owning the conffiledb dir.
+ * @param base Which conffiledb basedir to use.
+ */
+static void
+conffiledb_ensure_db(const char *pkg, conffiledb_base base)
+{
+ struct stat s;
+ short i = 0;
+ char *dbdir = NULL;
+ char *separators[3];
+
+ dbdir = conffiledb_path(pkg, NULL, base);
+ /* temporarily truncate the string to cheaply traverse up to the
+ * the parent and its parent. store the locations so we can cheaply
+ * get back to them later. */
+ for (i = 0; i < 3; i++) {
+ separators[i] = rindex(dbdir, '/');
+ *separators[i] = '\0';
+ }
+
+ /* now ensure each directory exists while reconstructing the path */
+ for (i = 2; i >= 0; i--) {
+ if (stat(dbdir, &s)) {
+ debug(dbg_conffdetail,
+      "conffiledb_ensure_db: creating %s\n", dbdir);
+ if (errno != ENOENT)
+ ohshite("conffiledb_ensure_db: stat(%s)",
+        dbdir);
+ if (mkdir(dbdir, S_IRWXU))
+ ohshite("conffiledb_ensure_db: mkdir(%s)",
+        dbdir);
+ }
+ *separators[i] = '/';
+ }
+
+ free(dbdir);
+}
+
+void
+conffiledb_register_new_conffile(const char *pkg, const char *path, int fd,
+                                 size_t sz)
+{
+ int cfgfd;
+ /* get the path to where the registered file goes */
+ char *p = conffiledb_path(pkg, path, conffiledb_base_new);
+ char fnamebuf[256];
+
+ conffiledb_ensure_db(pkg, conffiledb_base_new);
+ debug(dbg_conff, "conffile_reg_fd: %s, %s, %s\n", pkg, path, p);
+ /* make a mode 600 copy of file to p */
+ cfgfd = open(p, (O_CREAT|O_EXCL|O_WRONLY), (S_IRUSR|S_IWUSR));
+ if (cfgfd < 0)
+ ohshite(_("unable to create `%.255s'"),p);
+ fd_fd_copy(fd, cfgfd, sz, _("backend dpkg-deb during `%.255s'"),
+           path_quote_filename(fnamebuf, p, 256));
+ if (close(cfgfd))
+ ohshite("can't close %s", p);
+
+ free(p);
+}
+
+int
+conffiledb_get_conffile(const char *pkg, const char *path, conffiledb_base base)
+{
+ int fd = -1;
+ char *p = conffiledb_path(pkg, path, base);
+
+ fd = open(p, O_RDONLY);
+ if (fd == -1)
+ ohshite("error opening conffile registered at %s", p);
+
+ free(p);
+ return fd;
+}
+
+void
+conffiledb_commit(const char *pkg, conffiledb_base from_base,
+                  conffiledb_base to_base)
+{
+ /* update the conffiles "db" dir.  this consists of the following steps:
+ * - nuke the tmp dir if it exists
+ * - rename to_base to the tmp dir
+ * - rename from_base to to_base
+ * - nuke the tmp dir
+ */
+ char *cfdb_to = NULL, *cfdb_from = NULL, *cfdb_tmp = NULL;
+ debug(dbg_conff, "conffiledb_commit(%s, %d, %d)", pkg, from_base,
+      to_base);
+
+ cfdb_from = conffiledb_path(pkg, NULL, from_base);
+ cfdb_to = conffiledb_path(pkg, NULL, to_base);
+ cfdb_tmp = conffiledb_path(pkg, NULL, conffiledb_base_tmp);
+
+ /* Make sure all leading components of the tmp conffiledb exist */
+ conffiledb_ensure_db(pkg, conffiledb_base_tmp);
+ /* Then nuke the tmp conffiledb if it exists */
+ conffiledb_remove(pkg, conffiledb_base_tmp);
+ /* Make sure all leading components of the target conffiledb exist */
+ conffiledb_ensure_db(pkg, to_base);
+ /* Rename the target conffiledb to the tmp one */
+ if (rename(cfdb_to, cfdb_tmp))
+ ohshite("conffiledb_commit: rename(%s,%s)", cfdb_to, cfdb_tmp);
+ /* Make sure all leading components of the source conffiledb exist */
+ conffiledb_ensure_db(pkg, from_base);
+ /* Rename from_base to to_base */
+ if (rename(cfdb_from, cfdb_to))
+ ohshite("conffiledb_commit: rename(%s,%s)", cfdb_from, cfdb_to);
+ /* Nuke the tmp version if it exists */
+ conffiledb_remove(pkg, conffiledb_base_tmp);
+
+ free(cfdb_from);
+ free(cfdb_to);
+ free(cfdb_tmp);
+}
+
+void
+conffiledb_remove(const char *pkg, conffiledb_base base)
+{
+ char *cfdb = conffiledb_path(pkg, NULL, base);
+
+ debug(dbg_conff, "conffiledb_remove(%s): removing %s\n", pkg, cfdb);
+ ensure_pathname_nonexisting(cfdb);
+
+ free(cfdb);
+}
+
+int
+conffiledb_diff (const char *pkg, const char *path, conffiledb_base base)
+{
+ int status;
+ char *src = conffiledb_path(pkg, path, base);
+ char *cmd = NULL;
+ const char *opts = "-u";
+ /* DIFF + ' ' + <opts> + ' ' + <srcfile> + ' ' + <dstfile> + '\0' */
+ size_t cmd_sz = strlen(DIFF) + 1 + strlen(opts) + 1 + strlen(src) +
+                1 + strlen(path) + 1;
+
+ cmd = m_malloc(cmd_sz);
+ sprintf(cmd, "%s %s %s %s", DIFF, opts, src, path);
+ status = system(cmd);
+
+ free(src);
+ free(cmd);
+ return WEXITSTATUS(status);
+}
diff --git a/src/conffiledb.h b/src/conffiledb.h
new file mode 100644
index 0000000..9278cf1
--- /dev/null
+++ b/src/conffiledb.h
@@ -0,0 +1,155 @@
+/**
+ * @file conffiledb.h
+ *
+ * Functions for tracking referencable versions of packages' conffiles.
+ *
+ * This library allows for tracking the pristine versions (and in some specific
+ * cases a few other versions) of packages' conffiles.  This allows for a
+ * number of desirable features, such as conffile merging and more
+ * featureful reporting of the differences between conffiles on package
+ * upgrade even when the local admin may have modified the installed
+ * version of the file.
+ *
+ * The internal database of conffiles follows the convention:
+ *
+ * #CONFFILEDBDIRROOT/\<base\>/\<pkg\>/\<md5\>
+ *
+ * where:
+ * - @b \<base\> is a short string which corresponds to the base reference
+ *   type in question. See the documentation for conffiledb_base for the
+ *   enumerated values and conffiledb_base_dirs for the corresponding
+ *   string values.
+ * - @b \<pkg\> is the conffile database directory for an individual package.
+ * - @b \<md5\> is the md5 hash of the location of a package's conffile.  A
+ *   hash is used to keep a flat and balanced directory structure, and has
+ *   the added benefit of a simpler implementation.
+ *
+ * Some further terminology to keep things clear later on:
+ *
+ * - @b "conffiledb root" refers to the top-most directory containing
+ *   all conffile databases, and is defined by #CONFFILEDBDIRROOT.
+ * - @b "conffiledb basedir" refers to the subdirectory directly
+ *   underneath the conffiledb root directory, which divides the the
+ *   different conffiledbs by different "reference types" (i.e. the
+ *   original conffile provided by the currently installed package vs.
+ *   a newly unpacked version). The available basedirs are defined in
+ *   conffiledb_base_dirs, which is indexed on the respective enumerated
+ *   values from conffiledb_base.
+ * - @anchor conffiledb @b "conffile database" or @b "conffiledb" refers
+ *   to a directory specific to a conffiledb_base + package tuple.  Such a
+ *   directory will contain copies of the conffiles named after the hash
+ *   of their respective installed locations.
+ * - @b "conffiledb file" or @b "conffiledb entry" refers to a file whose
+ *   contents correspond to a particular conffiledb_base/package/conffile
+ *   triplet.
+ */
+#ifndef CONFFILES_H
+#define CONFFILES_H
+
+#include <sys/types.h>
+
+/** The different base reference types underneath the conffile db. */
+typedef enum {
+ conffiledb_base_current, /**< The currently installed version */
+ conffiledb_base_new, /**< A newly unpacked version */
+ conffiledb_base_resolved, /**< The most recently resolved conflict */
+ conffiledb_base_tmp, /**< A temporary version for internal use */
+ conffiledb_base_COUNT /**< The count of possible values for this type */
+} conffiledb_base;
+
+/**
+ * The directory names of the conffiledb base dirs, indexed by the respective
+ * conffiledb_base value.
+ */
+extern const char *conffiledb_base_dirs[conffiledb_base_COUNT];
+
+/** Length of the MD5 string used in conffile path hashing (NULL not counted) */
+#define CONFFILEDB_MD5SUM_LEN 32
+
+/** Return the canonical path for a conffiledb entry or other location.
+ *
+ * This is a helper function to resolve the full path to a specific location
+ * within the conffile database. The primary use is to find the location
+ * of a conffiledb file, but it can also be used to find the location
+ * of the @ref conffiledb for a particular conffiledb_base + package,
+ * as well as the parent conffiledb basedir corresponding to a particular
+ * conffiledb_base.
+ *
+ * @param pkg The package name. If NULL, the returned value will be
+ *        the conffiledb basedir corresponding to base.
+ * @param path The absolute path to the conffile. If NULL, the returned
+ *        value will be the conffiledb directory which should contain all
+ *        conffiledb files for the combination of pkg and base.
+ * @param base Which conffiledb basedir is requested.
+ * @return An allocated string with the path (your job to free it).
+ */
+char* conffiledb_path(const char *pkg, const char *path, conffiledb_base base);
+
+/** Register a new conffiledb file for a package.
+ *
+ * Create a conffiledb entry in the conffiledb dir for the::conffiledb_base_new
+ * version of a particular package. The conffile will be later processed in the
+ * package configuration stage, after which there should be a call to
+ * conffiledb_commit_new before configuration can be considered successful.
+ *
+ * The use of a filedescriptor is due to the fact that the new conffile is
+ * intercepted very early during the unpack phase, before it exists on disk.
+ * Therefore this interface is designed to hook cleanly into what's available
+ * at that point, which is a filedescriptor generated from a struct TarInfo
+ * (see tarobject in archives.c).
+ *
+ * @param pkg The package owning the conffile.
+ * @param path The installed location of the conffile.
+ * @param fd A file descriptor holding the contents of the conffile.
+ * @param sz The size of the conffile.
+ */
+void conffiledb_register_new_conffile(const char *pkg, const char *path, int fd,
+                                      size_t sz);
+
+/** Get a readable filehandle to the contents of a conffiledb entry.
+ *
+ * @param pkg The package owning the conffile.
+ * @param path The installed location of the conffile.
+ * @param base Which conffiledb basedir to use.
+ * @return A read-only file descriptor for the registered conffile.
+ */
+int conffiledb_get_conffile(const char *pkg, const char *path,
+                            conffiledb_base base);
+
+/** Remove a conffiledb directory for a package.
+ *
+ * This is designed to be used in two use cases:
+ * - package removal and cleanup
+ * - after one conffiledb directory has been replaced by another.
+ *
+ * @param pkg The package owning the conffile db to be removed.
+ * @param base Which conffiledb basedir to use with respect to pkg.
+ */
+void conffiledb_remove(const char *pkg, conffiledb_base base);
+
+/** Commit one conffiledb as another, replacing any existing one.
+ *
+ * This function takes the conffiledb defined by pkg and from_base and
+ * commits it as being defined by pkg and to_base. Any existing conffiledb
+ * for pkg / to_base is first renamed as conffiledb_base_tmp and then
+ * removed after the successful "commit" (to keep things as atomic as
+ * possible).
+ *
+ * @param pkg The package owning the conffile database
+ * @param from_base The current conffiledb basedir which should be renamed.
+ * @param to_base The conffiledb basedir which should be replaced by the
+ *        version in from_base.
+ */
+void conffiledb_commit(const char *pkg, conffiledb_base from_base,
+                       conffiledb_base to_base);
+
+/** Show the diff between a conffiledb entry and a corresponding conffile.
+ *
+ * @param pkg The package that owns the conffile.
+ * @param path The pathname to the installed version of the conffile.
+ * @param base Which conffiledb basedir to use.
+ * @return The exit status of the underlying call to diff(1).
+ */
+int conffiledb_diff(const char *pkg, const char *path, conffiledb_base base);
+
+#endif /* CONFFILES_H */
--
1.6.4.3


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[sean finney]

In process_archive() (src/processarc.c), clean out the staged ("-new")
version of the package's conffile database.

In tarobject() (src/archives.c), some slight of hand is performed with
the backend pipe of the struct tarcontext to intercept conffiles and
place a copy in the staged ("new") conffile database for the package
before extracting it as usual.

In deferred_configure() (src/configure.c), commit the package's staged
conffile database is committed.

And finally, in checkforremoval() (src/remove.c), remove the package's
conffile database.
---
 src/archives.c   |   18 +++++++++++++++---
 src/configure.c  |    5 +++++
 src/processarc.c |    4 ++++
 src/remove.c     |    3 +++
 4 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/src/archives.c b/src/archives.c
index 48221a7..692c4ba 100644
--- a/src/archives.c
+++ b/src/archives.c
@@ -58,6 +58,7 @@ static security_context_t scontext    = NULL;
 #include "filesdb.h"
 #include "main.h"
 #include "archives.h"
+#include "conffiledb.h"
 
 #define MAXCONFLICTORS 20
 
@@ -360,7 +361,7 @@ int tarobject(struct TarInfo *ti) {
 
   struct conffile *conff;
   struct tarcontext *tc= (struct tarcontext*)ti->UserData;
-  int statr, i, existingdirectory, keepexisting;
+  int statr, i, existingdirectory, keepexisting, copyfd;
   ssize_t r;
   struct stat stab, stabtmp;
   char databuf[TARBLKSZ];
@@ -623,10 +624,21 @@ int tarobject(struct TarInfo *ti) {
   }
 #endif /* WITH_SELINUX */
 
-
+  /* in the case of a conffile we may want to extract the file somewhere
+   * else first, thus we use a "middleman" variable to keep the code clean
+   * and generalizable
+   */
+  copyfd = tc->backendpipe;
   /* Extract whatever it is as .dpkg-new ... */
   switch (ti->Type) {
   case NormalFile0: case NormalFile1:
+    /* if the file is a conffile... */
+    if (nifd->namenode->flags & fnnf_new_conff) {
+      conffiledb_register_new_conffile(tc->pkg->name, fnamevb.buf, copyfd,
+                                       ti->Size);
+      copyfd = conffiledb_get_conffile(tc->pkg->name, fnamevb.buf,
+                                       conffiledb_base_new);
+    }
     /* We create the file with mode 0 to make sure nobody can do anything with
      * it until we apply the proper mode, which might be a statoverride.
      */
@@ -637,7 +649,7 @@ int tarobject(struct TarInfo *ti) {
     debug(dbg_eachfiledetail,"tarobject NormalFile[01] open size=%lu",
           (unsigned long)ti->Size);
     { char fnamebuf[256];
-    fd_fd_copy(tc->backendpipe, fd, ti->Size,
+    fd_fd_copy(copyfd, fd, ti->Size,
                _("backend dpkg-deb during `%.255s'"),
                path_quote_filename(fnamebuf, ti->Name, 256));
     }
diff --git a/src/configure.c b/src/configure.c
index d01a856..8d69c66 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -50,6 +50,7 @@
 #include <dpkg/file.h>
 
 #include "filesdb.h"
+#include "conffiledb.h"
 #include "main.h"
 
 static int conffoptcells[2][2] = {
@@ -353,6 +354,10 @@ deferred_configure(struct pkginfo *pkg)
  for (conff = pkg->installed.conffiles; conff; conff = conff->next)
  deferred_configure_conffile(pkg, conff);
 
+ /* commit the new conffiledb for this package */
+ conffiledb_commit(pkg->name, conffiledb_base_new,
+                  conffiledb_base_current);
+
  pkg->status = stat_halfconfigured;
  }
 
diff --git a/src/processarc.c b/src/processarc.c
index 2d6f320..15dd93a 100644
--- a/src/processarc.c
+++ b/src/processarc.c
@@ -48,6 +48,7 @@
 #include "filesdb.h"
 #include "main.h"
 #include "archives.h"
+#include "conffiledb.h"
 
 void process_archive(const char *filename) {
   static const struct TarFunctions tf = {
@@ -316,6 +317,9 @@ void process_archive(const char *filename) {
    * OK, we're going ahead.
    */
 
+  /* make sure the new conffiles dir does not exist at this point */
+  conffiledb_remove(pkg->name, conffiledb_base_new);
+
   trig_activate_packageprocessing(pkg);
   strcpy(cidirrest, TRIGGERSCIFILE);
   trig_parse_ci(cidir, NULL, trig_cicb_statuschange_activate, pkg);
diff --git a/src/remove.c b/src/remove.c
index 44ff139..12e085e 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -42,6 +42,7 @@
 #include <dpkg/myopt.h>
 
 #include "filesdb.h"
+#include "conffiledb.h"
 #include "main.h"
 
 static void checkforremoval(struct pkginfo *pkgtoremove,
@@ -490,6 +491,8 @@ static void removal_bulk_remove_configfiles(struct pkginfo *pkg) {
       pop_cleanup(ehflag_normaltidy); /* closedir */
     
     }
+    /* remove the current conffile db for this package */
+    conffiledb_remove(pkg->name, conffiledb_base_current);
     
     pkg->installed.conffiles = NULL;
     modstatdb_note(pkg);
--
1.6.4.3


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[sean finney]

The conflict resolution prompt has been updated with a new option for
automatic merging:

Configuration file `<file>'
 ==> Modified (by you or by a script) since installation.
 ==> Package distributor has shipped an updated version.
   What would you like to do about it ?  Your options are:
    Y or I  : install the package maintainer's version
    N or O  : keep your currently-installed version
      D     : show the differences between the versions
      M     : attempt to automatically merge the differences
      Z     : background this process to examine the situation
 The default action is to keep your current version.
*** foo.cfg (Y/I/N/O/D/M/Z) [default=N] ?

This functions by performing a 3-way diff using the new conffile, the
currently-installed conffile, and the pristine version of the conffile
shipped in the currently installed package (in the conffile database).

The merge output is stored into a temporary file (<file>.dpkg-merge).
On merge failure, the file is unlinked, the user is informed of the
failure, and the prompt is repeated. On success, the merge output is
renamed on top of the installed conffile, and then configuration
proceeds as though the user selected to keep the currently-installed
version.

Future implementations can extend this to allow for interactive inspection
of failed merges, piping the diff to a pager, or perhaps even alternative
diff3 implementations when available.
---
 lib/dpkg/dpkg.h  |    2 +
 src/conffiledb.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/conffiledb.h |   11 ++++++++
 src/configure.c  |   15 ++++++++++-
 4 files changed, 101 insertions(+), 1 deletions(-)

diff --git a/lib/dpkg/dpkg.h b/lib/dpkg/dpkg.h
index 9b06c73..425d1e4 100644
--- a/lib/dpkg/dpkg.h
+++ b/lib/dpkg/dpkg.h
@@ -58,6 +58,7 @@ DPKG_BEGIN_DECLS
 #define DPKGNEWEXT         ".dpkg-new"
 #define DPKGOLDEXT         ".dpkg-old"
 #define DPKGDISTEXT        ".dpkg-dist"
+#define DPKGMERGEEXT        ".dpkg-merge"
 
 #define CONTROLFILE        "control"
 #define CONFFILESFILE      "conffiles"
@@ -115,6 +116,7 @@ DPKG_BEGIN_DECLS
 #define RM "rm"
 #define FIND "find"
 #define DIFF "diff"
+#define DIFF3 "diff3"
 
 #define FIND_EXPRSTARTCHARS "-(),!"
 
diff --git a/src/conffiledb.c b/src/conffiledb.c
index 46cbfaf..00feee8 100644
--- a/src/conffiledb.c
+++ b/src/conffiledb.c
@@ -9,11 +9,13 @@
 #include <fcntl.h>
 #include <stdlib.h>
 #include <errno.h>
+#include <sys/wait.h>
 
 #include <dpkg/dpkg.h>
 #include <dpkg/i18n.h>
 #include <dpkg/buffer.h>
 #include <dpkg/path.h>
+#include <dpkg/file.h>
 #include <dpkg/dpkg-db.h>
 #include "main.h"
 
@@ -216,3 +218,75 @@ conffiledb_diff (const char *pkg, const char *path, conffiledb_base base)
  free(cmd);
  return WEXITSTATUS(status);
 }
+
+int conffiledb_automerge(const char *pkg, const char *path)
+{
+ int res, merge_fd, resolved_fd, path_fd;
+ size_t cmd_sz = 0, merge_output_fname_sz = 0;
+ char *cmdbuf = NULL, *cf_old = NULL, *cf_new = NULL;
+ char *cf_resolved = NULL, *merge_output_fname = NULL;
+
+ /* i.e. <path> + ".dpkg-merge" + '\0' */
+ merge_output_fname_sz = strlen(path) + strlen(DPKGMERGEEXT) + 1;
+ merge_output_fname = m_malloc(merge_output_fname_sz);
+ sprintf(merge_output_fname, "%s%s", path, DPKGMERGEEXT);
+
+ /* the 'old' version is actually the current dist version in the db */
+ cf_old = conffiledb_path(pkg, path, conffiledb_base_current);
+ cf_new = conffiledb_path(pkg, path, conffiledb_base_new);
+
+ /* create a file for merge output, ensuring it does not already exist */
+ merge_fd = open(merge_output_fname, O_CREAT|O_WRONLY|O_EXCL);
+ if (merge_fd == -1 )
+ ohshite("conffiledb_automerge: can't open %s for merge output",
+        merge_output_fname);
+ else if (close(merge_fd))
+ ohshite("conffiledb_automerge: close failed");
+
+ /* copy file permissions from the original file */
+ file_copy_perms(path, merge_output_fname);
+
+ /* DIFF3 + " -m " + new + " " + old + " " + cur + " > " + out + '\0' */
+ cmd_sz = strlen(DIFF3) + 4 + strlen(cf_new) + 1 + strlen(cf_old) + 1 +
+         strlen(path) + 3 + merge_output_fname_sz + 1;
+ cmdbuf = m_malloc(cmd_sz);
+ snprintf(cmdbuf, cmd_sz, "%s -m %s %s %s > %s", DIFF3, cf_new, cf_old,
+         path, merge_output_fname);
+
+ /* let's give it a go! */
+ res = system(cmdbuf);
+ if (WEXITSTATUS(res) == 0) {
+ /* rename the merge output to the final destination */
+ if (rename(merge_output_fname, path) == -1)
+ ohshite("conffiledb_automerge: can not rename %s",
+        merge_output_fname);
+ /* and the also register the successful merge in the
+ * "resolved" conffiles db, as another possible ancestor for
+ * future merges */
+ path_fd = open(path, O_RDONLY);
+ if (path_fd < 0)
+ ohshite("conffiledb_automerge: can not open %s", path);
+ conffiledb_ensure_db(pkg, conffiledb_base_resolved);
+ cf_resolved = conffiledb_path(pkg, path,
+                              conffiledb_base_resolved);
+ resolved_fd = open(cf_resolved, O_WRONLY|O_CREAT|O_TRUNC);
+ if (resolved_fd < 0)
+ ohshite("conffiledb_automerge: can not open %s",
+        cf_resolved);
+ fd_fd_copy(path_fd, resolved_fd, -1, "conffiledb_automerge");
+ } else {
+ /* oh noes, merge failed :( */
+ if (unlink(merge_output_fname))
+ ohshite("conff_automerge: can not unlink %s",
+        merge_output_fname);
+ }
+
+ free(merge_output_fname);
+ free(cmdbuf);
+ free(cf_new);
+ free(cf_old);
+ if (cf_resolved != NULL)
+ free(cf_resolved);
+
+ return res;
+}
diff --git a/src/conffiledb.h b/src/conffiledb.h
index 9278cf1..6738b55 100644
--- a/src/conffiledb.h
+++ b/src/conffiledb.h
@@ -152,4 +152,15 @@ void conffiledb_commit(const char *pkg, conffiledb_base from_base,
  */
 int conffiledb_diff(const char *pkg, const char *path, conffiledb_base base);
 
+/** Attempt to automagically merge a 3-way delta for a conffile.
+ *
+ * On a successful merge, the merged copy is also registered under the
+ * ::conffiledb_base_resolved basedir for pkg as a "resolved" version.
+ *
+ * @param pkg The package owning the conffile
+ * @param path The path to the installed conffile
+ * @return The exit status of the underlying call to diff3(1)
+ */
+int conffiledb_automerge(const char *pkg, const char *path);
+
 #endif /* CONFFILES_H */
diff --git a/src/configure.c b/src/configure.c
index 8d69c66..9b6a12a 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -654,6 +654,7 @@ promptconfaction(const char *pkg, const char *cfgfile, const char *realold,
           "    Y or I  : install the package maintainer's version\n"
           "    N or O  : keep your currently-installed version\n"
           "      D     : show the differences between the versions\n"
+          "      M     : attempt to automatically merge the differences\n"
           "      Z     : background this process to examine the situation\n"));
 
  if (what & cfof_keep)
@@ -664,7 +665,7 @@ promptconfaction(const char *pkg, const char *cfgfile, const char *realold,
  s = strrchr(cfgfile, '/');
  if (!s || !*++s)
  s = cfgfile;
- fprintf(stderr, "*** %s (Y/I/N/O/D/Z) %s ? ",
+ fprintf(stderr, "*** %s (Y/I/N/O/D/M/Z) %s ? ",
         s,
         (what & cfof_keep) ? _("[default=N]") :
         (what & cfof_install) ? _("[default=Y]") :
@@ -698,6 +699,18 @@ promptconfaction(const char *pkg, const char *cfgfile, const char *realold,
  if (cc == 'd')
  showdiff(realold, realnew);
 
+ if (cc == 'm') {
+ fprintf(stderr, _("attempting automatic merge of %s: "), cfgfile);
+ if( conffiledb_automerge(pkg, cfgfile) == 0){
+ fprintf(stderr, _("success.\n"));
+ /* so let's just pretend they said "keep my
+ * version" ;) */
+ cc = 'n';
+ } else {
+ fprintf(stderr, _("failed.\n"));
+ }
+ }
+
  if (cc == 'z')
  suspend();
  } while (!strchr("yino", cc));
--
1.6.4.3


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[sean finney]

A small number of functions from help.c are required for the new
dpkg-conffile cmdline program, but it's not currently possible
to link help.c into other programs (without defining a large quantity
of unused global variables to satisfy all the extern's in main.h).

Therefore, the following functions have been moved to a new util.c file:

 * void debug(int which, const char *fmt, ...)
 * void ensure_pathname_nonexisting(const char *pathname)
 * int secure_unlink(const char *pathname)
 * int secure_unlink_statted(const char *pathname, const struct stat *stab)

Note that debug still references an external int f_debug, which must be
provided by a cmdline program.  It's left for discussion and/or future
implementation to fix that.

The Makefile.am automake template has also been updated to include this file
in dpkg_SOURCES and no further changes are required to dpkg source code.
---
 src/Makefile.am |    3 +-
 src/help.c      |   72 -----------------------------------
 src/util.c      |  111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+), 73 deletions(-)
 create mode 100644 src/util.c

diff --git a/src/Makefile.am b/src/Makefile.am
index 24583de..9c1fc15 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -36,7 +36,8 @@ dpkg_SOURCES = \
  remove.c \
  select.c \
  trigproc.c \
- update.c
+ update.c \
+ util.c
 
 dpkg_LDADD = \
  ../lib/dpkg/libdpkg.a \
diff --git a/src/help.c b/src/help.c
index 62221ce..db7b22d 100644
--- a/src/help.c
+++ b/src/help.c
@@ -461,16 +461,6 @@ void clear_istobes(void) {
   iterpkgend(it);
 }
 
-void debug(int which, const char *fmt, ...) {
-  va_list ap;
-  if (!(f_debug & which)) return;
-  fprintf(stderr,"D0%05o: ",which);
-  va_start(ap,fmt);
-  vfprintf(stderr,fmt,ap);
-  va_end(ap);
-  putc('\n',stderr);
-}
-
 /*
  * Returns true if the directory contains conffiles belonging to pkg,
  * false otherwise.
@@ -535,68 +525,6 @@ void oldconffsetflags(const struct conffile *searchconff) {
   }
 }
 
-/*
- * If the pathname to remove is:
- *
- * 1. a sticky or set-id file, or
- * 2. an unknown object (i.e., not a file, link, directory, fifo or socket)
- *
- * we change its mode so that a malicious user cannot use it, even if it's
- * linked to another file.
- */
-int
-secure_unlink(const char *pathname)
-{
-  struct stat stab;
-
-  if (lstat(pathname,&stab)) return -1;
-
-  return secure_unlink_statted(pathname, &stab);
-}
-
-int
-secure_unlink_statted(const char *pathname, const struct stat *stab)
-{
-  if (S_ISREG(stab->st_mode) ? (stab->st_mode & 07000) :
-      !(S_ISLNK(stab->st_mode) || S_ISDIR(stab->st_mode) ||
- S_ISFIFO(stab->st_mode) || S_ISSOCK(stab->st_mode))) {
-    if (chmod(pathname, 0600))
-      return -1;
-  }
-  if (unlink(pathname)) return -1;
-  return 0;
-}
-
-void ensure_pathname_nonexisting(const char *pathname) {
-  int c1;
-  const char *u;
-
-  u = path_skip_slash_dotslash(pathname);
-  assert(*u);
-
-  debug(dbg_eachfile,"ensure_pathname_nonexisting `%s'",pathname);
-  if (!rmdir(pathname)) return; /* Deleted it OK, it was a directory. */
-  if (errno == ENOENT || errno == ELOOP) return;
-  if (errno == ENOTDIR) {
-    /* Either it's a file, or one of the path components is.  If one
-     * of the path components is this will fail again ...
-     */
-    if (secure_unlink(pathname) == 0)
-      return; /* OK, it was */
-    if (errno == ENOTDIR) return;
-  }
-  if (errno != ENOTEMPTY && errno != EEXIST) { /* Huh ? */
-    ohshite(_("unable to securely remove '%.255s'"), pathname);
-  }
-  c1= m_fork();
-  if (!c1) {
-    execlp(RM, "rm", "-rf", "--", pathname, NULL);
-    ohshite(_("failed to exec rm for cleanup"));
-  }
-  debug(dbg_eachfile,"ensure_pathname_nonexisting running rm -rf");
-  waitsubproc(c1,"rm cleanup",0);
-}
-
 void log_action(const char *action, struct pkginfo *pkg) {
   log_message("%s %s %s %s", action, pkg->name,
       versiondescribe(&pkg->installed.version, vdew_nonambig),
diff --git a/src/util.c b/src/util.c
new file mode 100644
index 0000000..bade26c
--- /dev/null
+++ b/src/util.c
@@ -0,0 +1,111 @@
+/*
+ * util.c - misc utility functions
+ *
+ * Copyright © 1995 Ian Jackson <ian@...>
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2,
+ * or (at your option) any later version.
+ *
+ * This is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with dpkg; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#include <config.h>
+#include <compat.h>
+
+#include <assert.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "main.h"
+
+#include "dpkg/dpkg.h"
+#include "dpkg/i18n.h"
+#include "dpkg/path.h"
+#include "dpkg/subproc.h"
+
+void
+debug(int which, const char *fmt, ...)
+{
+ va_list ap;
+ if (!(f_debug & which)) return;
+ fprintf(stderr,"D0%05o: ",which);
+ va_start(ap,fmt);
+ vfprintf(stderr,fmt,ap);
+ va_end(ap);
+ putc('\n',stderr);
+}
+
+void    
+ensure_pathname_nonexisting(const char *pathname)
+{
+ int c1;
+ const char *u;
+
+ u = path_skip_slash_dotslash(pathname);
+ assert(*u);
+
+ debug(dbg_eachfile,"ensure_pathname_nonexisting `%s'",pathname);
+ if (!rmdir(pathname)) return; /* Deleted it OK, it was a directory. */
+ if (errno == ENOENT || errno == ELOOP) return;
+ if (errno == ENOTDIR) {
+ /* Either it's a file, or one of the path components is.  If one
+ * of the path components is this will fail again ...
+ */
+ if (secure_unlink(pathname) == 0)
+ return; /* OK, it was */
+ if (errno == ENOTDIR) return;
+ }
+ if (errno != ENOTEMPTY && errno != EEXIST) { /* Huh ? */
+ ohshite(_("unable to securely remove '%.255s'"), pathname);
+ }
+ c1= m_fork();
+ if (!c1) {
+ execlp(RM, "rm", "-rf", "--", pathname, NULL);
+ ohshite(_("failed to exec rm for cleanup"));
+ }
+ debug(dbg_eachfile,"ensure_pathname_nonexisting running rm -rf");
+ waitsubproc(c1,"rm cleanup",0);
+}
+
+/*
+ * If the pathname to remove is:
+ *
+ * 1. a sticky or set-id file, or
+ * 2. an unknown object (i.e., not a file, link, directory, fifo or socket)
+ *
+ * we change its mode so that a malicious user cannot use it, even if it's
+ * linked to another file.
+ */
+int
+secure_unlink(const char *pathname)
+{
+ struct stat stab;
+
+ if (lstat(pathname,&stab))
+ return -1;
+
+ return secure_unlink_statted(pathname, &stab);
+}
+
+int
+secure_unlink_statted(const char *pathname, const struct stat *stab)
+{
+ if (S_ISREG(stab->st_mode) ? (stab->st_mode & 07000) :
+ !(S_ISLNK(stab->st_mode) || S_ISDIR(stab->st_mode) ||
+  S_ISFIFO(stab->st_mode) || S_ISSOCK(stab->st_mode))) {
+ if (chmod(pathname, 0600))
+ return -1;
+ }
+ if (unlink(pathname)) return -1;
+ return 0;
+}
--
1.6.4.3


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Jonathan Nieder]

Hi Sean,

Sean Finney wrote:

> In order to provide an interface into the conffiles DB API, it's required
> to know the package name that owns the conffile. Since this is a static
> function and the package name is available in all places that the function
> is used, this is a fairly easy fix.

Looks good and sane.

> diff --git a/src/configure.c b/src/configure.c
> index 72d15de..1aa8b6a 100644
> --- a/src/configure.c
> +++ b/src/configure.c
> @@ -554,8 +554,9 @@ suspend(void)
>   * Select what to do with a configuration file.
>   */
>  static enum conffopt
> -promptconfaction(const char *cfgfile, const char *realold, const char *realnew,
> -                 int useredited, int distedited, enum conffopt what)
> +promptconfaction(const char *pkg, const char *cfgfile, const char *realold,
> +                 const char *realnew, int useredited, int distedited,
> +                 enum conffopt what)

There is some trailing whitespace here.  If you have time, checking
patches with the sample pre-commit hook (.git/hooks/pre-commit.sample)
or git diff --check can be a help.

But that’s just a nitpick.

Reviewed-by: Jonathan Nieder <jrnieder@...>

For convenience, the same patch follows, as mangled by "git rebase -f
--whitespace=fix origin/master".

-- %< --
From: Sean Finney <seanius@...>
Date: Wed, 14 Oct 2009 21:23:04 +0200
Subject: [PATCH] Update promptconfaction() to also require package name

In order to provide an interface into the conffiles DB API, it's required
to know the package name that owns the conffile. Since this is a static
function and the package name is available in all places that the function
is used, this is a fairly easy fix.
---
 src/configure.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/configure.c b/src/configure.c
index 72d15de..9ef7ad6 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -61,7 +61,7 @@ static int conffoptcells[2][2] = {
 static void md5hash(struct pkginfo *pkg, char *hashbuf, const char *fn);
 static void showdiff(const char *old, const char *new);
 static void suspend(void);
-static enum conffopt promptconfaction(const char *cfgfile,
+static enum conffopt promptconfaction(const char *pkg, const char *cfgfile,
                                       const char *realold, const char *realnew,
                                       int useredited, int distedited,
                                       enum conffopt what);
@@ -152,7 +152,7 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
       "deferred_configure '%s' (= '%s') useredited=%d distedited=%d what=%o",
       usenode->name, cdr.buf, useredited, distedited, what);
 
- what = promptconfaction(usenode->name, cdr.buf, cdr2.buf,
+ what = promptconfaction(pkg->name, usenode->name, cdr.buf, cdr2.buf,
                         useredited, distedited, what);
 
  switch (what & ~(cfof_isnew | cfof_userrmd)) {
@@ -554,8 +554,9 @@ suspend(void)
  * Select what to do with a configuration file.
  */
 static enum conffopt
-promptconfaction(const char *cfgfile, const char *realold, const char *realnew,
-                 int useredited, int distedited, enum conffopt what)
+promptconfaction(const char *pkg, const char *cfgfile, const char *realold,
+                 const char *realnew, int useredited, int distedited,
+                 enum conffopt what)
 {
  const char *s;
  int c, cc;
--
1.6.5.rc1.199.g596ec


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Jonathan Nieder]

Sean Finney wrote:

> --- a/src/configure.c
> +++ b/src/configure.c
> @@ -61,6 +61,29 @@ static int conffoptcells[2][2] = {
>  static void md5hash(struct pkginfo *pkg, char *hashbuf, const char *fn);
>  static void showdiff(const char *old, const char *new);
>  static void suspend(void);
> +
> +/** Prompt the user for how to resolve a conffile conflict

There are not many of these documentation comments, but adding some
seems like a good idea, especially if these functions are to
eventually be usable by other frontends.

Will documentation processing tools choke on the string /*** used
elsewhere?  (Just curious.)

> + *
> + * When encountering a conffile conflict during configuration, the user will
> + * normally be presented with a textual menu of possible actions. This
> + * behavior is modified via various --force flags and perhaps on whether
> + * or not a terminal is available to do the prompting.
> + *
> + * @param pkg The package owning the conffile
> + * @param cfgfile The conffile
> + * @param realold The location of the old conffile (dereferenced in the case
> + *        of a symlink).
> + * @param realnew The location of the new conffile (dereferenced in the case
> + *        of a symlink).

Some pedantic clarifications:

        @param pkg The name of the package

        @param cfgfile Name of the conffile to be updated

        @param realold Name of a file with the configuration file's
               previous contents, possibly modified by the
               administrator.  Can be a symlink, in which case it will
               be dereferenced.

        @param realnew Name of a file with the configuration file's
               updated default contents.  If a symlink, will be
               dereferenced.

Am I understanding correctly?

> + * @param what Hints on what action should be taken by defualt.

These contain not just hints but required information, right?

Maybe I am being too pedantic here, since most functions don’t have
documentation at all!  So feel free to ignore. :)

Thanks for the interesting read.

Regards,
Jonathan


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Jonathan Nieder]

Sean Finney wrote:

> The files conffiledb.{c,h} define a set of routines for adding and removing
> stored versions of the pristine conffiles as shipped in their respective
> packages.
>
> Such an implementation allows for some neat features in the future,
> such as:
>
>  * 3-way merging of conffile changes
>  * showing the old->new delta for the "dist" conffiles
>  * dynamic registering of conffiles (à la ucf)
>  * a general dpkg/conffile cmdline interface for all of this

I am really looking forward to this.

> The layout pattern for the conffile database is:
>
> <admindir>/conffiles/<base>/<pkg>/<hash>
>
> Where:
>
>  * <admindir> is the standard dpkg administrative dir (i.e. /var/lib/dpkg).
>  * <base> is a short string corresponding to which type of base reference
>           is stored underneath (see source code comments for more info).

This is current, new, resolved, or tmp, right?  It seems to almost
correspond to the three stages of the git index and the working tree,
but I haven’t worked this out all the way.  Which contains the merge
base in a three-way merge?

>  * <pkg> is the conffile database directory for a package.

This is just the package name, right?

> --- /dev/null
> +++ b/src/conffiledb.c
> @@ -0,0 +1,218 @@
[...]
> +char*

I think the asterisk and space should be swapped here.

> +conffiledb_path(const char *pkg, const char *path, conffiledb_base base)
> +{
[...]
> + /* the root path is <admindir>/<#CONFFILEDBDIRROOT/><basedir>/
> + * (note that the '/' after #CONFFILEDBDIRROOT is implicitly included
> + * in the #define'd string). The null byte is also accounted for
> + * at this point. */
> + basedir_sz = strlen(admindir) + 1 + strlen(CONFFILEDBDIRROOT) +
> +             strlen(basedir) + 2;
> + full_path_sz = conffiledb_sz = basedir_sz;

This string size accounting makes me nervous about future
modifications to the code.  Couldn’t you use struct varbuf?

Example:

        struct varbuf full_path = VARBUF_INIT;

        varbufprintf(&full_path, "%s/%s%s/",
                admindir, CONFFILEDBDIRROOT, basedir);

        if (pkg != NULL)
                varbufprintf(&full_path, "%s/", pkg);
        if (path != NULL) {
                varbuf_grow(&full_path, CONFFILEDB_MD5SUM_LEN + 1);
                buffer_md5(path, full_path.buf + full_path.used,
                        strlen(path));
                full_path.used += CONFFILEDB_MD5SUM_LEN;
        }

        return full_path.buf;

[...]

> +/** Ensure that a particular conffiledb dir exists.
> + *
> + * @param pkg The package owning the conffiledb dir.
> + * @param base Which conffiledb basedir to use.
> + */
> +static void
> +conffiledb_ensure_db(const char *pkg, conffiledb_base base)
> +{
> + struct stat s;
> + short i = 0;
> + char *dbdir = NULL;
> + char *separators[3];
> +
> + dbdir = conffiledb_path(pkg, NULL, base);
> + /* temporarily truncate the string to cheaply traverse up to the
> + * the parent and its parent. store the locations so we can cheaply
> + * get back to them later. */
> + for (i = 0; i < 3; i++) {
> + separators[i] = rindex(dbdir, '/');
> + *separators[i] = '\0';
> + }

I was going to suggest a silly micro-optimization:

        char *end;

        end = dbdir + strlen(dbdir);
        for (i = 0; i < 3; i++) {
                end = separators[i] = memrchr(dbdir, '/', end - dbdir);
                assert(end != NULL);
                *end = '\0';
        }

But memrchr is not portable, so this is almost certainly not worth it
(unless there is already a ready-made implementation to put in
libcompat).

It might be worth documenting that conffiledb_path needs to return a
path with at least 4 components.

[...]
> +void
> +conffiledb_register_new_conffile(const char *pkg, const char *path, int fd,
> +                                 size_t sz)
> +{

This function just copies a configuration file into the “new” area of
the conffiledb, as preparation before storing it in /etc, right?  It’s
hard to see that at a glance.  Maybe it would be good to add a comment
to that effect, even if it would be redundant next to the more
longer comment in conffiledb.h.

This function is dominated by error handling, so I am tempted to say
there should be separate xopen() and xclose() functions that take care
of their own error handling (quitting with a message prefixed by a
caller-specified string when appropriate).  I dunno.

[...]
> + if (cfgfd < 0)
> + ohshite(_("unable to create `%.255s'"),p);

Straight quotes are preferred (see doc/coding-style.txt).

[...]
> +int
> +conffiledb_get_conffile(const char *pkg, const char *path, conffiledb_base base)
[...]

Maybe calling it conffiledb_open_conffile would make it more obvious
the result is a file descriptor.  Not sure.

> +void
> +conffiledb_commit(const char *pkg, conffiledb_base from_base,
> +                  conffiledb_base to_base)
> +{
> + /* update the conffiles "db" dir.  this consists of the following steps:
> + * - nuke the tmp dir if it exists
> + * - rename to_base to the tmp dir
> + * - rename from_base to to_base
> + * - nuke the tmp dir
> + */

Better to describe a function outside its body.  Even after reading
the doxygen comment in conffiledb.h, I am not sure what this function
is for.  Does it just copy from the “from” dir to the “to” dir with an
opportunity to roll back?  When does this need arise, on ENOSPC?

[...]

> + /* Make sure all leading components of the tmp conffiledb exist */
> + conffiledb_ensure_db(pkg, conffiledb_base_tmp);
> + /* Then nuke the tmp conffiledb if it exists */
> + conffiledb_remove(pkg, conffiledb_base_tmp);
> + /* Make sure all leading components of the target conffiledb exist */
> + conffiledb_ensure_db(pkg, to_base);
> + /* Rename the target conffiledb to the tmp one */
> + if (rename(cfdb_to, cfdb_tmp))
> + ohshite("conffiledb_commit: rename(%s,%s)", cfdb_to, cfdb_tmp);
> + /* Make sure all leading components of the source conffiledb exist */
> + conffiledb_ensure_db(pkg, from_base);
> + /* Rename from_base to to_base */
> + if (rename(cfdb_from, cfdb_to))
> + ohshite("conffiledb_commit: rename(%s,%s)", cfdb_from, cfdb_to);
> + /* Nuke the tmp version if it exists */
> + conffiledb_remove(pkg, conffiledb_base_tmp);

The heavy comments are distracting.  Much more interesting than _what_
the code does is why it does it.  In particular, the atomiticity here
is hard to understand, at least for me.  Atomicity usually provides a
guarantee of the form, “if I interrupt the code at any point, ____
will always hold.” What is the invariant being maintained here?

[...]
> +int
> +conffiledb_diff (const char *pkg, const char *path, conffiledb_base base)
> +{
[...]
> + /* DIFF + ' ' + <opts> + ' ' + <srcfile> + ' ' + <dstfile> + '\0' */
> + size_t cmd_sz = strlen(DIFF) + 1 + strlen(opts) + 1 + strlen(src) +
> +                1 + strlen(path) + 1;

struct varbuf seems like a good fit here.

> --- /dev/null
> +++ b/src/conffiledb.h
> @@ -0,0 +1,155 @@
[...]
> +/** Register a new conffiledb file for a package.
> + *
> + * Create a conffiledb entry in the conffiledb dir for the::conffiledb_base_new
> + * version of a particular package. The conffile will be later processed in the
> + * package configuration stage, after which there should be a call to
> + * conffiledb_commit_new before configuration can be considered successful.
      ^^^
conffiledb_commit?

A simple example of use of this API would be really nice.  Maybe even
a test (see <dpkg/test.h> for some helpers).

I hope to read your remaining patches later this week, but sadly there
is not enough time now.  I hope my comments are of some use anyway.
As it is now, each local modification to a configuration file is a
burden to be painfully maintained, so I am glad you seem to be moving
towards a fix.

Regards,
Jonathan


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Guillem Jover]

Hi!

On Wed, 2009-10-14 at 21:23:04 +0200, Sean Finney wrote:
> In order to provide an interface into the conffiles DB API, it's required
> to know the package name that owns the conffile. Since this is a static
> function and the package name is available in all places that the function
> is used, this is a fairly easy fix.

Pushed with a slight change, thanks!

> diff --git a/src/configure.c b/src/configure.c
> index 72d15de..1aa8b6a 100644
> --- a/src/configure.c
> +++ b/src/configure.c
> @@ -61,7 +61,7 @@ static int conffoptcells[2][2] = {
>  static void md5hash(struct pkginfo *pkg, char *hashbuf, const char *fn);
>  static void showdiff(const char *old, const char *new);
>  static void suspend(void);
> -static enum conffopt promptconfaction(const char *cfgfile,
> +static enum conffopt promptconfaction(const char *pkg, const char *cfgfile,
>                                        const char *realold, const char *realnew,
>                                        int useredited, int distedited,
>                                        enum conffopt what);

Passed “struct pkginfo *” instead so that we can greacfully handle the
multiarch cases in the future.

regards,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Guillem Jover]

Hi!

Cleaned up the doxygen file I had around and infrastructure changes
needed and pushed those.

Also pushed this patch with slight changes, thanks!

On Wed, 2009-10-14 at 21:23:05 +0200, Sean Finney wrote:

> diff --git a/src/configure.c b/src/configure.c
> index 1aa8b6a..d01a856 100644
> --- a/src/configure.c
> +++ b/src/configure.c
> @@ -61,6 +61,29 @@ static int conffoptcells[2][2] = {
>  static void md5hash(struct pkginfo *pkg, char *hashbuf, const char *fn);
>  static void showdiff(const char *old, const char *new);
>  static void suspend(void);
> +
> +/** Prompt the user for how to resolve a conffile conflict
> + *

The first line should be empty, followed by the brief description,

/**
 * Brief description.
 *

> + * When encountering a conffile conflict during configuration, the user will
> + * normally be presented with a textual menu of possible actions. This
> + * behavior is modified via various --force flags and perhaps on whether
> + * or not a terminal is available to do the prompting.
> + *
> + * @param pkg The package owning the conffile
> + * @param cfgfile The conffile
> + * @param realold The location of the old conffile (dereferenced in the case
> + *        of a symlink).
> + * @param realnew The location of the new conffile (dereferenced in the case
> + *        of a symlink).
> + * @param useredited A flag to indicate whether the file has been edited
> + *        locally. Set to nonzero to indicate that the file has been modified.
> + * @param distedited A flag to indicate whether the file has been updated
> + *        between package versions.  Set to nonzero to indicate that
> + *        the file has been updated.
> + * @param what Hints on what action should be taken by defualt.

Reworded some of the @params to try to make them more clear.

A blank line before @return.

> + * @return The action which should be taken based on user input and/or the
> + *         default actions as configured by cmdline/configuration options.
> + */

>  static enum conffopt promptconfaction(const char *pkg, const char *cfgfile,
>                                        const char *realold, const char *realnew,
>                                        int useredited, int distedited,

JavaDoc comments should go closer to the definition of the code, except
for type declarations (enums, structs, typedefs, etc), which should go
closer to the declaration, usually in the header files.

I should probably add this to the coding-style.txt.

regards,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Jonathan Nieder]

Hi Sean,

I finally got around to reviewing your patch to maintain the conffile
database during unpack/configure/remove stages.  I am still worried
that the “commit” operation does not seem to be atomic like it should
be.  Could you look at my description of your patch and let me know
where I go wrong?

As is, of course, your code should be low-risk, since the conffile
database is not used except as a staging area during unpacking at this
point.  We just have to check that the database is well enough
maintained that future patches can rely on it.

Okay, so here’s what your patch does:

 * Before unpacking, clean out the staged ("new") version of a
   package’s conffile database.

 * When unpacking archives, let conffiles pass through the staged
   (new) conffile database before copying to their installed location
   elsewhere in the filesystem.

 * When configuring, “commit” the package’s staged conffile database
   (rename it to "current").

 * When purging a package’s configuration files, remove its conffile
   database (right after removing the conffiles themselves).

This all seems sane and simple --- good.

Invariants maintained (ignoring a possible tmp conffile db):

 * When not-installed, a package has no conffile db.

 * In config-files state, a package has a "current" conffile db
   and no other.

 * When half-installed, a package has no conffile db.

 * Once unpacked, a package may or may not have a "current"
   conffile db, which remembers the distributed configuration files
   for the last configured version; and a package will certainly have
   a "new" conffile db, which holds the distributed configuration
   files for the unpacked version.

 * In half-configured state, everything breaks.  It is possible
   to forget the "current" conffile db here.  One problem is that
   committing the new conffile db requires multiple steps:

        rm -r tmp/pkg
        mv current/pkg tmp/pkg
        mv new/pkg current/pkg
        rm -r tmp/pkg

   If dpkg is interrupted in the middle, the "current" configuration
   can be lost.

        $ dpkg --configure pkg
        rm -r tmp/pkg
        mv current/pkg tmp/pkg
        <dpkg is interrupted here>
        $ # okay, better try again.
        $ dpkg --configure pkg
        rm -r tmp/pkg
        ...

    If dpkg is interrupted after committing the new conffile db but
    before the package leaves half-configured state, the "new"
    configuration can be lost.

        $ dpkg --configure pkg
        rm -r tmp/pkg
        mv current/pkg tmp/pkg
        mv new/pkg current/pkg
        rm -r tmp/pkg
        <dpkg is interrupted here>
        $ # okay, better try again.
        $ dpkg --configure pkg
        rm -r tmp/pkg
        mv current/pkg tmp/pkg
        [dpkg could notice something wrong here: why is new missing?]
        mv new/pkg current/pkg
        rm -r tmp/pkg
        <dpkg is interrupted>
        $ # okay, try again
        rm -r tmp/pkg
        ...

   One (crazy?) idea would be to overwrite entries in current/pkg
   until they all match new/.  That is, run the equivalent of

        ln -f new/pkg/afd9834287dc... current/pkg/afd9834287dc...

   for each configuration file until their stat information matches.

   Upside: I imagine something similar to this would be useful anyway
   for remembering which configuration files have been brought up to
   date.  Downside: after this operation, the old distributed
   configuration file is gone.  There is no turning back and deciding
   to upgrade the conffile a different way.

 - Once installed, the "current" conffile db should remember
   the distributed configuration files and there should be no
   "new" conffile db.

 - triggers-awaited and triggers-pending are just like the
   installed state here.

What do you think?

Thanks for the interesting read.  Hopefully, others can chime in with
some more useful thoughts.

Regards,
Jonathan


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[sean finney]

hi jonathan,

thanks for your feedback on this and the previous patches.  

before going any further, i'd like to point out that i'm still waiting
for feedback from guillem, and i don't plan on doing any further work
on this until i hear at least something from him indicating that he's
interested in moving forward with this.  

to put things as politely as possible i find the current situation
"demotivating" with regards to the amount of time and effort i've put
in and the level of feedback recieved.  but i'll leave that there for now.

On Wed, Nov 04, 2009 at 03:17:00AM -0600, Jonathan Nieder wrote:
> I finally got around to reviewing your patch to maintain the conffile
> database during unpack/configure/remove stages.  I am still worried
> that the “commit” operation does not seem to be atomic like it should
> be.  Could you look at my description of your patch and let me know
> where I go wrong?

i think most of what you say below is correct.

> Okay, so here’s what your patch does:
>
>  * Before unpacking, clean out the staged ("new") version of a
>    package’s conffile database.
>
>  * When unpacking archives, let conffiles pass through the staged
>    (new) conffile database before copying to their installed location
>    elsewhere in the filesystem.
>
>  * When configuring, “commit” the package’s staged conffile database
>    (rename it to "current").
>
>  * When purging a package’s configuration files, remove its conffile
>    database (right after removing the conffiles themselves).

this is all correct.

> Invariants maintained (ignoring a possible tmp conffile db):
>
>  * When not-installed, a package has no conffile db.
>
>  * In config-files state, a package has a "current" conffile db
>    and no other.

correct.  there might be a little "*" attached to this invariant though
that it might be possible that a config-files or purged package may be
left with an empty conffile db... at least it was possible at some point
in the evolution of these changes but i can't say for sure whether it's
still the case.

>  * When half-installed, a package has no conffile db.

not completely sure on this.  i should say that i haven't fully explored
the various corner cases of the package configuration workflow, which is
what i was hoping to get from discussions like these.

>  * Once unpacked, a package may or may not have a "current"
>    conffile db, which remembers the distributed configuration files
>    for the last configured version; and a package will certainly have
>    a "new" conffile db, which holds the distributed configuration
>    files for the unpacked version.

correct.

>  * In half-configured state, everything breaks.  It is possible
>    to forget the "current" conffile db here.  One problem is that
>    committing the new conffile db requires multiple steps:
>
> rm -r tmp/pkg
> mv current/pkg tmp/pkg
> mv new/pkg current/pkg
> rm -r tmp/pkg

yeah, this is the only place afaik where the atomicity breaks down, largely
due to not being able to atomically replace one directory with another.  i'm
not sure what the best alternative here is...

>    If dpkg is interrupted in the middle, the "current" configuration
>    can be lost.
>
> $ dpkg --configure pkg
> rm -r tmp/pkg
> mv current/pkg tmp/pkg
> <dpkg is interrupted here>
> $ # okay, better try again.
> $ dpkg --configure pkg
> rm -r tmp/pkg
> ...

it's a pretty small window for failure, and i designed it to do the least
amount of stuff possible during said window of non-atomicity (i.e. one
directory rename).  but yes it's hypothetically possible that a power outage
some other externality (unrelated memory errors, i/o errors, etc) could
trigger what you describe.  one remedy not requiring any significant changes
would be to have a final "flag" file of some kind to indicate that it's
been successful, which doesn't solve the atomicity problem but could solve
the "how to detect when we previously screwed up" matter.

>    One (crazy?) idea would be to overwrite entries in current/pkg
>    until they all match new/.  That is, run the equivalent of
>
> ln -f new/pkg/afd9834287dc... current/pkg/afd9834287dc...
>
>    for each configuration file until their stat information matches.
>
>    Upside: I imagine something similar to this would be useful anyway
>    for remembering which configuration files have been brought up to
>    date.  Downside: after this operation, the old distributed
>    configuration file is gone.  There is no turning back and deciding
>    to upgrade the conffile a different way.

it's also quite a bit more complicated, and more stuff could go wrong
during this time (i.e. running out of inodes, etc).  with regards to losing
the old files, if those were similarly link-shuffled off to a tmp location
it might be possible to keep them in case of error, though.

another idea:  what if we just abandoned the concept of "current"/etc entirely
and used package versions instead, or maybe had them as symlinks?  a symlink
can be updated atomically, and if there were some kind of "byversion" dir
then they wouldn't need to cross paths on the filesystem at all.

>  - Once installed, the "current" conffile db should remember
>    the distributed configuration files and there should be no
>    "new" conffile db.
>
>  - triggers-awaited and triggers-pending are just like the
>    installed state here.

correct.



thanks,
        sean

[Jonathan Nieder]

Hi Sean,

Thanks for the thoughts and clarifications.

Until I read how it gets used (i.e. the remaining patches), I don’t
think I’m in a position to say much definitive about how the conffile
db should work.  So feel free to skip this message if you lack time.
I’ll write more when I get there.

sean finney wrote:

> it's a pretty small window for failure, and i designed it to do the least
> amount of stuff possible during said window of non-atomicity (i.e. one
> directory rename).  but yes it's hypothetically possible that a power outage
> some other externality (unrelated memory errors, i/o errors, etc) could
> trigger what you describe.  one remedy not requiring any significant changes
> would be to have a final "flag" file of some kind to indicate that it's
> been successful, which doesn't solve the atomicity problem but could solve
> the "how to detect when we previously screwed up" matter.

It’s a small window of time, but it makes me nervous.  dpkg generally
has very nice error handling, and it seems worthwhile to make sure
additions continue in the same vein.

The conffile db does not contain any precious data, but if we drop the
conffiles from status eventually (is that part of the plan?) then
removing a package’s conffile db would mean either pestering the user
about its every conffile installed for the next upgrade or blindly
using the old versions.

>>    One (crazy?) idea would be to overwrite entries in current/pkg
>>    until they all match new/.  That is, run the equivalent of
>>
>> ln -f new/pkg/afd9834287dc... current/pkg/afd9834287dc...
>>
>>    for each configuration file until their stat information matches.
[...]
> it's also quite a bit more complicated, and more stuff could go wrong
> during this time (i.e. running out of inodes, etc).

Yes, mv would be more appropriate.

> with regards to losing
> the old files, if those were similarly link-shuffled off to a tmp location
> it might be possible to keep them in case of error, though.

Anything in tmp is as good as gone, in my opinion, unless dpkg starts
rolling back previous failed transactions at the start of the next
command.  That sounds a bit scarily complicated.

> another idea:  what if we just abandoned the concept of "current"/etc entirely
> and used package versions instead, or maybe had them as symlinks?  a symlink
> can be updated atomically, and if there were some kind of "byversion" dir
> then they wouldn't need to cross paths on the filesystem at all.

Both methods sound sane.

I think I will need to read the next patch in the series to learn what
is most convenient here. :)

Until then, let me compare the analogous moment in the current
configure phase. (deferred_configure_conffile(), in configure.c)

        1. if .dpkg-new file is missing, skip to the next conffile
        2. compute .dpkg-new file hash
        3. prompt the user for action
        4. with user’s help, bring installed configuration file up to date:
                - if doing something complicated, let the user modify
                  his own version. We’re not responsible for this. :)
                - if keeping the user’s version, remove the .dpkg-new
                - if using the package’s version, rename the .dpkg-new
                  to replace the conffile
        5. atomically write the saved .dpkg-new file hash to
                        /var/lib/dpkg/status

If dpkg is interrupted between steps 4 and 5, the .dpkg-new file hash
is gone, and the conffile hash saved in status is the old one, meaning
the user will be asked about the conffile in the next run.  I’m not
sure why the code is written this way.  Why not save the hash between
steps 3 and 4?

So as long as your commit is atomic, you’re at least not making things
worse.

Atomic here means from the point of view of dpkg; step 5 does not
require status to be written out in full each time.  (Either dpkg stops
and status is unchanged, or dpkg bets everything on finishing updating
status.  For example, if there is no room on disk for an updated
status file, dpkg will become a lame duck until someone makes room,
after which execing dpkg will make it write the status file and
proceed happily again.)

Step 1 is important to make this idempotent.

Hmm.

Thanks,
Jonathan


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Jonathan Nieder]

Jonathan Nieder wrote:
> If dpkg is interrupted between steps 4 and 5, the .dpkg-new file hash
> is gone, and the conffile hash saved in status is the old one, meaning
> the user will be asked about the conffile in the next run.  I’m not

Erm, s/next run/next upgrade/.  Sorry for the noise.

Good night,
Jonathan


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Guillem Jover]

Hi!

Some of the comments from Jonathan's review apply, I'll reply to his
mail instead.

On Wed, 2009-10-14 at 21:23:06 +0200, Sean Finney wrote:

> The layout pattern for the conffile database is:
>
> <admindir>/conffiles/<base>/<pkg>/<hash>
>
> Where:
>
>  * <admindir> is the standard dpkg administrative dir (i.e. /var/lib/dpkg).
>  * <base> is a short string corresponding to which type of base reference
>           is stored underneath (see source code comments for more info).
>  * <pkg> is the conffile database directory for a package.
>  * <hash> is the md5 hash of the installed location of a package's conffile.
>           A hash is used to keep the directory structure flat and balanced.

Hmmm, I think it'd be better to have the <base> part as an extension
to the conffile, similar to how the conffiles are handled right now on
the file system. It would also guarantee that the different
directories are on the same file system and rename(2) will always
succeed, even though putting them in different file systems would be
silly.

So something like “<admindir>/conffiles/<pkg>/<hash>[.<base>]”, where
<base> could be one of the already existing extensions “.dpkg-<ext>”.

> diff --git a/lib/dpkg/dpkg.h b/lib/dpkg/dpkg.h
> index afe650f..9b06c73 100644
> --- a/lib/dpkg/dpkg.h
> +++ b/lib/dpkg/dpkg.h
> @@ -74,6 +74,7 @@ DPKG_BEGIN_DECLS
>  #define STATOVERRIDEFILE  "statoverride"
>  #define UPDATESDIR        "updates/"
>  #define INFODIR           "info/"
> +#define CONFFILEDBDIRROOT "conffiles/" /**< @ref conffilesdb root location */
>  #define TRIGGERSDIR       "triggers/"
>  #define TRIGGERSFILEFILE  "File"
>  #define TRIGGERSDEFERREDFILE "Unincorp"

Place this in the conffiledb.c src/, the only place where it's going
to be used.

> diff --git a/src/conffiledb.h b/src/conffiledb.h
> new file mode 100644
> index 0000000..9278cf1
> --- /dev/null
> +++ b/src/conffiledb.h
> @@ -0,0 +1,155 @@

Missing license and copyright comment header.

Comments about the doxygen stuff:

  <http://lists.debian.org/debian-dpkg/2009/10/msg00146.html>

> +/**
> + * @file conffiledb.h
> + *
[...]
> + */

Missing blank line

> +#ifndef CONFFILES_H
> +#define CONFFILES_H

Namespace those with DPKG_ for stuff under src/, and use the header
name, so DPKG_CONFFILEDB_H.

> +#include <sys/types.h>
> +
> +/** The different base reference types underneath the conffile db. */
> +typedef enum {
> + conffiledb_base_current, /**< The currently installed version */
> + conffiledb_base_new, /**< A newly unpacked version */
> + conffiledb_base_resolved, /**< The most recently resolved conflict */
> + conffiledb_base_tmp, /**< A temporary version for internal use */
> + conffiledb_base_COUNT /**< The count of possible values for this type */
> +} conffiledb_base;

No typedefs for enums, structs, unions.

> +/**
> + * The directory names of the conffiledb base dirs, indexed by the respective
> + * conffiledb_base value.
> + */
> +extern const char *conffiledb_base_dirs[conffiledb_base_COUNT];

This does not seem to be needed as extern.

> +/** Length of the MD5 string used in conffile path hashing (NULL not counted) */
> +#define CONFFILEDB_MD5SUM_LEN 32

There's already MD5HASHLEN in <dpkg/dpkg.h>.

> +char* conffiledb_path(const char *pkg, const char *path, conffiledb_base base);

Does not seem to be needed as extern either. Pass ‘struct pkginfo *’
instead of ‘const char *’ whenever you need the package name, this will
be needed for multi-arch, later on.

> +void conffiledb_register_new_conffile(const char *pkg, const char *path, int fd,
> +                                      size_t sz);

sz should be off_t. off_t is for files, size_t for memory.

> +void conffiledb_remove(const char *pkg, conffiledb_base base);

> +void conffiledb_commit(const char *pkg, conffiledb_base from_base,
> +                       conffiledb_base to_base);

These two should be conffile based and not pkg or base based. This way
we can guarantee atomicity, which we cannot when managing directories
as a whole.

> diff --git a/src/conffiledb.c b/src/conffiledb.c
> new file mode 100644
> index 0000000..46cbfaf
> --- /dev/null
> +++ b/src/conffiledb.c
> @@ -0,0 +1,218 @@

Missing license and copyright comment header.

> +#include "conffiledb.h"

Place this the last with the local includes.

> +#include <config.h>

Missing <compat.h>

> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +
> +#include <dpkg/dpkg.h>
> +#include <dpkg/i18n.h>
> +#include <dpkg/buffer.h>
> +#include <dpkg/path.h>
> +#include <dpkg/dpkg-db.h>

Missing blank line.

> +#include "main.h"
> +
> +const char *conffiledb_base_dirs[] = { "current", "new", "resolved", "tmp" };

Why are you storing resolved (in the merge patch), how are you planning
to use it afterwards?

> +char*
> +conffiledb_path(const char *pkg, const char *path, conffiledb_base base)
> +{
> + char *full_path = NULL, *hash = NULL;
> + const char *basedir = NULL;
> + size_t basedir_sz = 0, conffiledb_sz = 0, full_path_sz = 0;
> +
> + /* sanity check for a valid base dir */
> + if (base >= conffiledb_base_COUNT)
> + ohshit("conffiledb_path called with unsupported base %x", base);

Use internerr here, as this would be a programming error, and we
should act like in an assert.

> +}

> +/** Ensure that a particular conffiledb dir exists.
> + *
> + * @param pkg The package owning the conffiledb dir.
> + * @param base Which conffiledb basedir to use.
> + */
> +static void
> +conffiledb_ensure_db(const char *pkg, conffiledb_base base)
> +{
> + struct stat s;
> + short i = 0;

Just use int, as the preferred native integer type. Also no need to
initialize as that's already done in the loop.

> + char *dbdir = NULL;
> + char *separators[3];
> +
> + dbdir = conffiledb_path(pkg, NULL, base);
> + /* temporarily truncate the string to cheaply traverse up to the
> + * the parent and its parent. store the locations so we can cheaply
> + * get back to them later. */
> + for (i = 0; i < 3; i++) {
> + separators[i] = rindex(dbdir, '/');
> + *separators[i] = '\0';
> + }
> +
> + /* now ensure each directory exists while reconstructing the path */
> + for (i = 2; i >= 0; i--) {
> + if (stat(dbdir, &s)) {
> + debug(dbg_conffdetail,
> +      "conffiledb_ensure_db: creating %s\n", dbdir);
> + if (errno != ENOENT)
> + ohshite("conffiledb_ensure_db: stat(%s)",
> +        dbdir);
> + if (mkdir(dbdir, S_IRWXU))
> + ohshite("conffiledb_ensure_db: mkdir(%s)",
> +        dbdir);
> + }
> + *separators[i] = '/';
> + }

Instead of this, I'd rather ship the <admindir>/conffiles/ in the
dpkg.deb itself, and with using extensions, there's only need to
ensure the <pkg> directory, which should simplify this.

> + free(dbdir);
> +}

> +void
> +conffiledb_register_new_conffile(const char *pkg, const char *path, int fd,
> +                                 size_t sz)
> +{
> + int cfgfd;
> + /* get the path to where the registered file goes */
> + char *p = conffiledb_path(pkg, path, conffiledb_base_new);
> + char fnamebuf[256];
> +
> + conffiledb_ensure_db(pkg, conffiledb_base_new);
> + debug(dbg_conff, "conffile_reg_fd: %s, %s, %s\n", pkg, path, p);
> + /* make a mode 600 copy of file to p */

Instead of the comment, just use 0600 in the open call.

> + cfgfd = open(p, (O_CREAT|O_EXCL|O_WRONLY), (S_IRUSR|S_IWUSR));
> + if (cfgfd < 0)
> + ohshite(_("unable to create `%.255s'"),p);
> + fd_fd_copy(fd, cfgfd, sz, _("backend dpkg-deb during `%.255s'"),

This string seems out of place (probably copy-pasted from
src/archive.c :).

> +           path_quote_filename(fnamebuf, p, 256));

Use fsync before the close.

> + if (close(cfgfd))
> + ohshite("can't close %s", p);
> +
> + free(p);
> +}
> +
> +int
> +conffiledb_get_conffile(const char *pkg, const char *path, conffiledb_base base)
> +{
> + int fd = -1;

No need to initialize, it's done unconditionally on open.

> + char *p = conffiledb_path(pkg, path, base);
> +
> + fd = open(p, O_RDONLY);
> + if (fd == -1)
> + ohshite("error opening conffile registered at %s", p);
> +
> + free(p);
> + return fd;
> +}

> +int
> +conffiledb_diff (const char *pkg, const char *path, conffiledb_base base)
> +{
> + int status;
> + char *src = conffiledb_path(pkg, path, base);
> + char *cmd = NULL;
> + const char *opts = "-u";
> + /* DIFF + ' ' + <opts> + ' ' + <srcfile> + ' ' + <dstfile> + '\0' */
> + size_t cmd_sz = strlen(DIFF) + 1 + strlen(opts) + 1 + strlen(src) +
> +                1 + strlen(path) + 1;
> +
> + cmd = m_malloc(cmd_sz);
> + sprintf(cmd, "%s %s %s %s", DIFF, opts, src, path);
> + status = system(cmd);
> +
> + free(src);
> + free(cmd);
> + return WEXITSTATUS(status);
> +}

This is a reimplementation of showdiff, but it does not use a pager
for example.

thanks,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Guillem Jover]

Hi!

On Sun, 2009-10-25 at 16:46:04 -0500, Jonathan Nieder wrote:
> Sean Finney wrote:
> > --- /dev/null
> > +++ b/src/conffiledb.c
> > @@ -0,0 +1,218 @@
> [...]
> > +char*
>
> I think the asterisk and space should be swapped here.

Right. There's other trialing spaces around.

> > +conffiledb_path(const char *pkg, const char *path, conffiledb_base base)
> > +{
> [...]
> > + /* the root path is <admindir>/<#CONFFILEDBDIRROOT/><basedir>/
> > + * (note that the '/' after #CONFFILEDBDIRROOT is implicitly included
> > + * in the #define'd string). The null byte is also accounted for
> > + * at this point. */
> > + basedir_sz = strlen(admindir) + 1 + strlen(CONFFILEDBDIRROOT) +
> > +             strlen(basedir) + 2;
> > + full_path_sz = conffiledb_sz = basedir_sz;
>
> This string size accounting makes me nervous about future
> modifications to the code.  Couldn’t you use struct varbuf?
>
> Example:
>
> struct varbuf full_path = VARBUF_INIT;
>
> varbufprintf(&full_path, "%s/%s%s/",
> admindir, CONFFILEDBDIRROOT, basedir);
>
> if (pkg != NULL)
> varbufprintf(&full_path, "%s/", pkg);
> if (path != NULL) {
> varbuf_grow(&full_path, CONFFILEDB_MD5SUM_LEN + 1);
> buffer_md5(path, full_path.buf + full_path.used,
> strlen(path));
> full_path.used += CONFFILEDB_MD5SUM_LEN;
> }
>
> return full_path.buf;

Yeah, better like this, now that we have a saner varbufprintf, there's
no reason to not use it, at least on code paths that are not critical.

> [...]
> > + if (cfgfd < 0)
> > + ohshite(_("unable to create `%.255s'"),p);
>
> Straight quotes are preferred (see doc/coding-style.txt).

Only as long as this does not introduce a gratuituous new string. In
this case this exact same string appears elsewhere so it gets reused.

> [...]
> > +int
> > +conffiledb_get_conffile(const char *pkg, const char *path, conffiledb_base base)
> [...]
>
> Maybe calling it conffiledb_open_conffile would make it more obvious
> the result is a file descriptor.  Not sure.

Yeah, open better than get.

> > +void
> > +conffiledb_commit(const char *pkg, conffiledb_base from_base,
> > +                  conffiledb_base to_base)
> > +{
> [...]
> > + /* Make sure all leading components of the tmp conffiledb exist */
> > + conffiledb_ensure_db(pkg, conffiledb_base_tmp);
> > + /* Then nuke the tmp conffiledb if it exists */
> > + conffiledb_remove(pkg, conffiledb_base_tmp);
> > + /* Make sure all leading components of the target conffiledb exist */
> > + conffiledb_ensure_db(pkg, to_base);
> > + /* Rename the target conffiledb to the tmp one */
> > + if (rename(cfdb_to, cfdb_tmp))
> > + ohshite("conffiledb_commit: rename(%s,%s)", cfdb_to, cfdb_tmp);
> > + /* Make sure all leading components of the source conffiledb exist */
> > + conffiledb_ensure_db(pkg, from_base);
> > + /* Rename from_base to to_base */
> > + if (rename(cfdb_from, cfdb_to))
> > + ohshite("conffiledb_commit: rename(%s,%s)", cfdb_from, cfdb_to);
> > + /* Nuke the tmp version if it exists */
> > + conffiledb_remove(pkg, conffiledb_base_tmp);
>
> The heavy comments are distracting.  Much more interesting than _what_
> the code does is why it does it.  In particular, the atomiticity here
> is hard to understand, at least for me.

Right.

> Atomicity usually provides a
> guarantee of the form, “if I interrupt the code at any point, ____
> will always hold.” What is the invariant being maintained here?

Yeah, if as suggested it gets switched from dir based to conffile base
it should be possible to guarantee atomicity, and rollback

> [...]
> > +int
> > +conffiledb_diff (const char *pkg, const char *path, conffiledb_base base)
> > +{
> [...]
> > + /* DIFF + ' ' + <opts> + ' ' + <srcfile> + ' ' + <dstfile> + '\0' */
> > + size_t cmd_sz = strlen(DIFF) + 1 + strlen(opts) + 1 + strlen(src) +
> > +                1 + strlen(path) + 1;
>
> struct varbuf seems like a good fit here.

Yeah.

> A simple example of use of this API would be really nice.  Maybe even
> a test (see <dpkg/test.h> for some helpers).

That'd be nice, although not a requirement for merging.

thanks,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Jonathan Nieder]

Hi,

For reference when looking at other patches, I made a bunch of
cosmetic changes to make this patch a little easier to read.  This
does not address Guillem’s comments, nor even all of my own.  The
changes have not been considered carefully.  But maybe it can be of
some use nevertheless.

Changes from Sean’s original:

 * clarify commit message
 * use struct pkginfo instead of strings to indicate package
 * add one-line comment summarizing each function’s operation in
    conffiledb.c (to complement the more comprehensive interface
    documentation in the header file)
 * be a little paranoid about null arguments (but I didn’t go all the
    way for some reason: if pkg is non-null, pkg->name is assumed to be
    as well)
 * make sure conffiledb_ensure_db behaves even if conffiledb_path
    returns a path with too few components, by open-coding the
    version using memrchr
 * use fork() and exec() instead of system() to run 'diff'
 * new PROCSAVESTATUS flag for libdpkg subproc library, to support
    fork()/exec()-ing diff.
 * some API documentation clarification
 * various whitespace tweaks
 * mark error messages as candidates for translation
 * escape filename characters with high bit set in more error messages
    (This just helped me figure out path_quote_filename().  It might be
    useful to do this generally if path_quote_filename() leans to
    quote control characters such as newline, but until then I don’t
    think it’s worth adopting.)

I am sending this since I have it around already (to avoid duplicate
effort), but I doubt my changes are suitable lumped together like
this.  Please let me know if any of these changes are actually useful,
and I can write a proper patch.

An interdiff will follow under separate cover.

-- %< --
From: Sean Finney <seanius@...>
Date: Wed, 14 Oct 2009 21:23:06 +0200
Subject: [PATCH] Conffile database handling functions

Currently, each Debian package that wants it (and only some) has
to separately carry out the following tasks:

 * 3-way merging of configuration file changes (updating modified
   conffiles without user intervention)
 * showing the old->new delta for distributed conffiles

To do these itself, dpkg needs a working space to store the old
distributed conffile and manipulate the new one before installing
it.  Add some routines to manipulate these files.

Each stored configuration file has the filename

        <admindir>/conffiles/<base>/<pkg>/<hash>

where

 * <admindir> is the standard dpkg administrative dir
              (i.e., /var/lib/dpkg).
 * <base> is a short string corresponding to which version of the
          conffiles is stored underneath
          (current, new, resolved, or tmp)
 * <pkg> is a package name
 * <hash> is the md5 hash of the installed location of a
          package's conffile.  A hash is used to keep the
          directory structure flat and balanced.

This database is not meant for direct user consumption.  However,
a "dpkg-conffile" command-line interface would allow the user to
perform various forms of inspection with regards to installed
conffiles vs the versions in this database.

Using a configuration file database instead of the current
/var/lib/dpkg/info/*.conffiles lists would also allow for
dynamic registration of configuration files, à la ucf.
---
 lib/dpkg/dpkg.h    |    1 +
 lib/dpkg/subproc.c |   34 ++++----
 lib/dpkg/subproc.h |    1 +
 src/Makefile.am    |    1 +
 src/conffiledb.c   |  247 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/conffiledb.h   |  182 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 449 insertions(+), 17 deletions(-)
 create mode 100644 src/conffiledb.c
 create mode 100644 src/conffiledb.h

diff --git a/lib/dpkg/dpkg.h b/lib/dpkg/dpkg.h
index 78a2fe6..abbbd1f 100644
--- a/lib/dpkg/dpkg.h
+++ b/lib/dpkg/dpkg.h
@@ -73,6 +73,7 @@ DPKG_BEGIN_DECLS
 #define STATOVERRIDEFILE  "statoverride"
 #define UPDATESDIR        "updates/"
 #define INFODIR           "info/"
+#define CONFFILEDBDIRROOT "conffiles/" /**< @ref conffilesdb root location */
 #define TRIGGERSDIR       "triggers/"
 #define TRIGGERSFILEFILE  "File"
 #define TRIGGERSDEFERREDFILE "Unincorp"
diff --git a/lib/dpkg/subproc.c b/lib/dpkg/subproc.c
index 5a42bf8..80994e4 100644
--- a/lib/dpkg/subproc.c
+++ b/lib/dpkg/subproc.c
@@ -73,22 +73,24 @@ cu_subproc_signals(int argc, void **argv)
 int
 checksubprocerr(int status, const char *description, int flags)
 {
- int n;
+ if (flags & PROCWARN)
+ flags |= PROCNOERR;
 
  if (WIFEXITED(status)) {
- n = WEXITSTATUS(status);
+ int n = WEXITSTATUS(status);
+
  if (!n)
  return 0;
- if (flags & PROCNOERR)
- return -1;
  if (flags & PROCWARN)
  warning(_("%s returned error exit status %d"),
         description, n);
- else
- ohshit(_("subprocess %s returned error exit status %d"),
-       description, n);
+ if (flags & PROCNOERR)
+ return flags & PROCSAVESTATUS ? n : -1;
+ ohshit(_("subprocess %s returned error exit status %d"),
+       description, n);
  } else if (WIFSIGNALED(status)) {
- n = WTERMSIG(status);
+ int n = WTERMSIG(status);
+
  if (!n)
  return 0;
  if ((flags & PROCPIPE) && n == SIGPIPE)
@@ -97,16 +99,14 @@ checksubprocerr(int status, const char *description, int flags)
  warning(_("%s killed by signal (%s)%s"),
         description, strsignal(n),
         WCOREDUMP(status) ? _(", core dumped") : "");
- else
- ohshit(_("subprocess %s killed by signal (%s)%s"),
-       description, strsignal(n),
-       WCOREDUMP(status) ? _(", core dumped") : "");
- } else {
- ohshit(_("subprocess %s failed with wait status code %d"),
-       description, status);
+ if (flags & PROCNOERR)
+ return -1;
+ ohshit(_("subprocess %s killed by signal (%s)%s"),
+       description, strsignal(n),
+       WCOREDUMP(status) ? _(", core dumped") : "");
  }
-
- return -1;
+ ohshit(_("subprocess %s failed with wait status code %d"),
+       description, status);
 }
 
 int
diff --git a/lib/dpkg/subproc.h b/lib/dpkg/subproc.h
index d1a0748..6037863 100644
--- a/lib/dpkg/subproc.h
+++ b/lib/dpkg/subproc.h
@@ -34,6 +34,7 @@ void cu_subproc_signals(int argc, void **argv);
 #define PROCPIPE 1
 #define PROCWARN 2
 #define PROCNOERR 4
+#define PROCSAVESTATUS 8
 
 int checksubprocerr(int status, const char *desc, int flags);
 int waitsubproc(pid_t pid, const char *desc, int flags);
diff --git a/src/Makefile.am b/src/Makefile.am
index a29b629..24583de 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -20,6 +20,7 @@ bin_PROGRAMS = \
 dpkg_SOURCES = \
  archives.c archives.h \
  cleanup.c \
+ conffiledb.c conffiledb.h \
  configure.c \
  depcon.c \
  enquiry.c \
diff --git a/src/conffiledb.c b/src/conffiledb.c
new file mode 100644
index 0000000..301abce
--- /dev/null
+++ b/src/conffiledb.c
@@ -0,0 +1,247 @@
+#include "conffiledb.h"
+
+#include <config.h>
+
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <errno.h>
+
+#include <dpkg/dpkg.h>
+#include <dpkg/i18n.h>
+#include <dpkg/varbuf.h>
+#include <dpkg/buffer.h>
+#include <dpkg/path.h>
+#include <dpkg/subproc.h>
+#include <dpkg/dpkg-db.h>
+#include "main.h"
+
+const char *conffiledb_base_dirs[] = { "current", "new", "resolved", "tmp" };
+
+/* Find a conffiledb entry, conffiledb, or conffiledb base dir. */
+char *
+conffiledb_path(const struct pkginfo *pkg, const char *path,
+ conffiledb_base base)
+{
+ struct varbuf full_path = VARBUF_INIT;
+ const char *basedir;
+
+ /* sanity check */
+ if (base >= conffiledb_base_COUNT)
+ ohshit(_("conffiledb_path called with unsupported base %x"),
+ base);
+
+ basedir = conffiledb_base_dirs[base];
+ varbufprintf(&full_path, "%s/%s%s/",
+ admindir, CONFFILEDBDIRROOT, basedir);
+
+ if (pkg == NULL)
+ goto done;
+ varbufprintf(&full_path, "%s/", pkg->name);
+
+ if (path == NULL)
+ goto done;
+ varbuf_grow(&full_path, CONFFILEDB_MD5SUM_LEN + 1);
+ buffer_md5(path, full_path.buf + full_path.used,
+ strlen(path));
+ full_path.used += CONFFILEDB_MD5SUM_LEN;
+
+done:
+ debug(dbg_conffdetail, "confffiledb_path(%s, %s, %x) = %s\n",
+ pkg ? pkg->name : "(null)", path ? path : "(null)", base,
+ full_path.buf);
+ return full_path.buf;
+}
+
+/**
+ * Ensure that a particular conffiledb dir exists.
+ *
+ * mkdir -p $CONFFILEDBDIRROOT/$base/$pkg
+ *
+ * @param pkg The package owning the conffiledb dir.
+ * @param base Which conffiledb basedir to use.
+ */
+static void
+conffiledb_ensure_db(const struct pkginfo *pkg, conffiledb_base base)
+{
+ struct stat s;
+ int i;
+ char *dbdir, *end, *separators[3];
+
+ dbdir = conffiledb_path(pkg, NULL, base);
+
+ /* sanity check */
+ if (*dbdir != '/')
+ ohshit(_("conffiledb_path returned a relative path"));
+
+ /*
+ * Temporarily truncate the string to cheaply traverse up to the
+ * the parent and its parent. store the locations so we can cheaply
+ * get back to them later.
+ */
+ end = dbdir + strlen(dbdir);
+ for (i = 0; i < 3; i++) {
+ while (*end != '/' && end != dbdir)
+ end--;
+ *end = '\0';
+ separators[i] = end;
+ }
+
+ /* Now ensure each directory exists while reconstructing the path. */
+ for (i = 2; i >= 0; i--) {
+ if (stat(dbdir, &s)) {
+ debug(dbg_conffdetail,
+      "conffiledb_ensure_db: creating %s\n", dbdir);
+ if (errno != ENOENT)
+ ohshite("conffiledb_ensure_db: stat(%s)",
+        dbdir);
+ if (mkdir(dbdir, S_IRWXU))
+ ohshite("conffiledb_ensure_db: mkdir(%s)",
+        dbdir);
+ }
+ *separators[i] = '/';
+ }
+
+ free(dbdir);
+}
+
+/* Add a new entry to $CONFFILEDBDIRROOT/new/$pkg. */
+void
+conffiledb_register_new_conffile(const struct pkginfo *pkg,
+ const char *path, int fd, size_t sz)
+{
+ int cfgfd;
+ char *dest;
+ char msg_dest[256];
+
+ /* sanity check */
+ if (path == NULL || pkg == NULL)
+ ohshit(_("conffiledb_register_new_conffile called "
+ "with NULL arguments"));
+
+ dest = conffiledb_path(pkg, path, conffiledb_base_new);
+ path_quote_filename(msg_dest, dest, sizeof(msg_dest));
+ debug(dbg_conff, "conffile_register_new_conffile: %s, %s, %s\n",
+ pkg->name, path, msg_dest);
+
+ conffiledb_ensure_db(pkg, conffiledb_base_new);
+
+ cfgfd = open(dest, O_CREAT|O_EXCL|O_WRONLY, S_IRUSR|S_IWUSR);
+ if (cfgfd < 0)
+ ohshite(_("unable to create `%.255s'"), msg_dest);
+ fd_fd_copy(fd, cfgfd, sz, _("new conffiledb entry '%.255s'"),
+           msg_dest);
+
+ if (close(cfgfd))
+ ohshite("can't close %.255s", msg_dest);
+ free(dest);
+}
+
+/* Open a conffiledb entry for reading. */
+int
+conffiledb_get_conffile(const struct pkginfo *pkg,
+ const char *path, conffiledb_base base)
+{
+ int fd;
+ char *p = conffiledb_path(pkg, path, base);
+
+ fd = open(p, O_RDONLY);
+ if (fd == -1) {
+ char msg_p[256];
+ ohshite(_("error opening conffile registered at %.255s"),
+ path_quote_filename(msg_p, p, sizeof(msg_p)));
+ }
+
+ free(p);
+ return fd;
+}
+
+/*
+ * Update the conffiles "db" dir.  This consists of the following steps:
+ * - nuke the tmp dir if it exists
+ * - rename to_base to the tmp dir
+ * - rename from_base to to_base
+ * - nuke the tmp dir
+ */
+void
+conffiledb_commit(const struct pkginfo *pkg, conffiledb_base from_base,
+                  conffiledb_base to_base)
+{
+ char *cfdb_to = NULL, *cfdb_from = NULL, *cfdb_tmp = NULL;
+
+ /* sanity check */
+ if (pkg == NULL)
+ ohshit(_("conffiledb_commit called for NULL package"));
+
+ debug(dbg_conff, "conffiledb_commit(%s, %d, %d)",
+ pkg->name, from_base, to_base);
+
+ cfdb_from = conffiledb_path(pkg, NULL, from_base);
+ cfdb_to = conffiledb_path(pkg, NULL, to_base);
+ cfdb_tmp = conffiledb_path(pkg, NULL, conffiledb_base_tmp);
+
+ conffiledb_ensure_db(pkg, conffiledb_base_tmp);
+ conffiledb_remove(pkg, conffiledb_base_tmp);
+
+ conffiledb_ensure_db(pkg, to_base);
+ if (rename(cfdb_to, cfdb_tmp))
+ ohshite(_("conffiledb_commit: rename(%s, %s)"),
+ cfdb_to, cfdb_tmp);
+ conffiledb_ensure_db(pkg, from_base);
+ if (rename(cfdb_from, cfdb_to))
+ ohshite(_("conffiledb_commit: rename(%s, %s)"),
+ cfdb_from, cfdb_to);
+
+ conffiledb_remove(pkg, conffiledb_base_tmp);
+
+ free(cfdb_from);
+ free(cfdb_to);
+ free(cfdb_tmp);
+}
+
+/* Remove a conffiledb with all of its entries. */
+void
+conffiledb_remove(const struct pkginfo *pkg, conffiledb_base base)
+{
+ char *cfdb;
+
+ /* sanity check */
+ if (pkg == NULL)
+ ohshit(_("conffiledb_remove called for NULL package"));
+
+ cfdb = conffiledb_path(pkg, NULL, base);
+ debug(dbg_conff, "conffiledb_remove(%s): removing %s\n",
+ pkg->name, cfdb);
+ ensure_pathname_nonexisting(cfdb);
+
+ free(cfdb);
+}
+
+/* Spawn 'diff -u <conffiledb entry> <installed conffile>'. */
+int
+conffiledb_diff(const struct pkginfo *pkg, const char *path,
+ conffiledb_base base)
+{
+ int child, result;
+ char *src;
+
+ /* sanity check */
+ if (pkg == NULL || path == NULL)
+ ohshit(_("conffiledb_diff called for NULL conffile"));
+
+ src = conffiledb_path(pkg, path, base);
+
+ if (!(child = m_fork())) {
+ execlp(DIFF, "diff", "-u", src, path, NULL);
+ ohshite(_("failed to exec 'diff -u %s %s'"),
+ src, path);
+ }
+
+ result = waitsubproc(child, "diff", PROCNOERR | PROCSAVESTATUS);
+
+ free(src);
+ return result;
+}
diff --git a/src/conffiledb.h b/src/conffiledb.h
new file mode 100644
index 0000000..5c8185c
--- /dev/null
+++ b/src/conffiledb.h
@@ -0,0 +1,182 @@
+/**
+ * @file conffiledb.h
+ *
+ * Functions for tracking referenceable versions of packages' conffiles.
+ *
+ * This library allows for tracking the pristine versions (and in some specific
+ * cases a few other versions) of packages' conffiles.  This allows for a
+ * number of desirable features, such as conffile merging and more
+ * featureful reporting of the differences between conffiles on package
+ * upgrade even when the local admin may have modified the installed
+ * version of the file.
+ *
+ * The internal database of conffiles follows the convention:
+ *
+ * #CONFFILEDBDIRROOT/\<base\>/\<pkg\>/\<md5\>
+ *
+ * where:
+ * - @b \<base\> is a short string describing which version of the conffile
+ *   this is. See enum #conffiledb_base for the enumerated values and
+ *   #conffiledb_base_dirs for the corresponding strings.
+ * - @b \<pkg\> is the package name.
+ * - @b \<md5\> is the md5 hash of the location of a package's conffile.  A
+ *   hash is used to keep a flat and balanced directory structure, and has
+ *   the added benefit of a simpler implementation.
+ *
+ * Some further terminology to keep things clear later on:
+ *
+ * - @b "conffiledb file" or @b "conffiledb entry" refers to a file whose
+ *   contents correspond to a particular conffiledb_base/package/conffile
+ *   triplet.
+ * - @anchor conffiledb @b "conffile database" or @b "conffiledb" refers
+ *   to a directory specific to a conffiledb_base + package tuple.  Such a
+ *   directory will contain copies of the conffiles named after the hash
+ *   of their respective installed locations.
+ * - @b "conffiledb basedir" refers to the subdirectory directly
+ *   underneath the conffiledb root directory, which divides the the
+ *   different conffiledbs by different "reference types" (i.e. the
+ *   original conffile provided by the currently installed package vs.
+ *   a newly unpacked version). The available basedirs are defined in
+ *   conffiledb_base_dirs, which is indexed on the respective enumerated
+ *   values from conffiledb_base.
+ * - @b "conffiledb root" refers to the top-most directory containing
+ *   all conffile databases, and is defined by #CONFFILEDBDIRROOT.
+ */
+#ifndef CONFFILES_H
+#define CONFFILES_H
+
+#include <sys/types.h>
+
+/* Defined in dpkg/dpkg-db.h. */
+struct pkginfo;
+
+/** The different base reference types underneath the conffile db. */
+typedef enum {
+ conffiledb_base_current, /**< The currently installed version */
+ conffiledb_base_new, /**< A newly unpacked version */
+ conffiledb_base_resolved, /**< The most recently resolved conflict */
+ conffiledb_base_tmp, /**< A temporary version for internal use */
+ conffiledb_base_COUNT /**< The count of possible values for this type */
+} conffiledb_base;
+
+/**
+ * The directory names of the conffiledb base dirs, indexed by the respective
+ * conffiledb_base value.
+ */
+extern const char *conffiledb_base_dirs[conffiledb_base_COUNT];
+
+/** Length of the MD5 string used in conffile path hashing (NULL not counted) */
+#define CONFFILEDB_MD5SUM_LEN 32
+
+/**
+ * Determine the canonical path for a conffiledb entry.
+ *
+ * This is a helper function to resolve the full path to a specific location
+ * within the conffile database. The primary use is to find the location
+ * of a conffiledb file, but it can also be used to find the location
+ * of the @ref conffiledb for a particular conffiledb_base + package,
+ * as well as the parent conffiledb basedir corresponding to a particular
+ * conffiledb_base.
+ *
+ * @param pkg The package name. If NULL, the returned value will be
+ *        the conffiledb basedir corresponding to base.
+ * @param path The absolute path to the conffile. If NULL, the returned
+ *        value will be the conffiledb directory which should contain all
+ *        conffiledb files for the combination of pkg and base.
+ * @param base Which conffiledb base reference type is requested.
+ *
+ * @return An allocated string with the path (your job to free it).
+ */
+char *conffiledb_path(const struct pkginfo *pkg, const char *path,
+ conffiledb_base base);
+
+/**
+ * Add a new conffiledb entry for a package.
+ *
+ * For use when unpacking a package.  Create a conffiledb entry in the
+ * conffiledb dir for the #conffiledb_base_new version of a particular
+ * package. The conffile will be later processed in the package configuration
+ * stage, after which there should be a call to conffiledb_commit() before
+ * configuration can be considered successful.
+ *
+ * The use of a file descriptor allows the new entry to be written very early
+ * in the unpack phase, before it exists on disk.  This interface is designed
+ * to hook cleanly with what's available, which is filedescriptor representing
+ * output from tar extraction.
+ *
+ * @param pkg The package owning the conffile.
+ * @param path The installed location of the conffile.
+ * @param fd A file descriptor holding the contents of the conffile.
+ * @param sz Size of the conffile in bytes.
+ *           (XXX why is this not off_t, -1 allowed?)
+ */
+void conffiledb_register_new_conffile(const struct pkginfo *pkg,
+ const char *path, int fd, size_t sz);
+
+/**
+ * Open a conffiledb entry for reading.
+ *
+ * @param pkg The package owning the conffile.
+ * @param path The installed location of the conffile.
+ * @param base Which conffiledb basedir to use.
+ *
+ * @return A read-only file descriptor for the registered conffile
+ *         (your job to close it).
+ */
+int conffiledb_get_conffile(const struct pkginfo *pkg,
+ const char *path, conffiledb_base base);
+
+/**
+ * Remove a package’s conffiledb.
+ *
+ * Remove the indicated version of the specified package’s conffiledb.
+ *
+ * This should be useful in two cases:
+ * - purging a package’s configuration files
+ * - replacing one conffiledb with another.
+ *
+ * @param pkg The package owning the conffile db to be removed.
+ * @param base Which conffiledb basedir to use with respect to pkg.
+ */
+void conffiledb_remove(const struct pkginfo *pkg, conffiledb_base base);
+
+/**
+ * Move a conffiledb from one base reference type to another.
+ *
+ * This is a safer way to perform the operation
+ *  
+ * rm -r $CONFFILEDBDIRROOT/$to_base/$pkg
+ * mv $CONFFILEDBDIRROOT/$from_base/$pkg $CONFFILEDBDIRROOT/$to_base/
+ *
+ * The conffiledb corresponding to pkg and the tmp base reference type
+ * is used as a temporary location to hold the entries from to_base until
+ * the entries from from_base are put into place.
+ *
+ * Any entries already in the tmp/$pkg conffiledb will be removed.  The
+ * invoking program is responsible for putting things back in order upon
+ * failure.
+ *
+ * @param pkg The package owning the conffile database
+ * @param from_base The current conffiledb basedir which should be renamed.
+ * @param to_base The conffiledb basedir which should be replaced by the
+ *        version in from_base.
+ */
+void conffiledb_commit(const struct pkginfo *pkg,
+ conffiledb_base from_base, conffiledb_base to_base);
+
+/**
+ * Print a diff between a conffiledb entry and the installed version.
+ *
+ * Print a unified diff between a conffiledb entry and the corresponding
+ * installed conffile to stdout.
+ *
+ * @param pkg The package that owns the conffile.
+ * @param path The pathname to the installed version of the conffile.
+ * @param base Which conffiledb basedir to use.
+ *
+ * @return The exit status from diff(1).
+ */
+int conffiledb_diff(const struct pkginfo *pkg, const char *path,
+ conffiledb_base base);
+
+#endif /* CONFFILES_H */
--
1.6.5.2


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Jonathan Nieder]

Jonathan Nieder wrote:

> For reference when looking at other patches, I made a bunch of
> cosmetic changes to make this patch a little easier to read.  This
> does not address Guillem’s comments, nor even all of my own.  The
> changes have not been considered carefully.  But maybe it can be of
> some use nevertheless.
>
> Changes from Sean’s original:
[...]

Here is the promised interdiff.

 src/conffiledb.c   |  253 +++++++++++++++++++++++++++++------------------------
 src/conffiledb.h   |  141 +++++++++++++++++------------
 lib/dpkg/subproc.c |   34 +++----
 lib/dpkg/subproc.h |    1
 4 files changed, 243 insertions(+), 186 deletions(-)

diff -u b/src/conffiledb.c b/src/conffiledb.c
--- b/src/conffiledb.c
+++ b/src/conffiledb.c
@@ -12,95 +12,94 @@
 
 #include <dpkg/dpkg.h>
 #include <dpkg/i18n.h>
+#include <dpkg/varbuf.h>
 #include <dpkg/buffer.h>
 #include <dpkg/path.h>
+#include <dpkg/subproc.h>
 #include <dpkg/dpkg-db.h>
 #include "main.h"
 
 const char *conffiledb_base_dirs[] = { "current", "new", "resolved", "tmp" };
 
-char*
-conffiledb_path(const char *pkg, const char *path, conffiledb_base base)
+/* Find a conffiledb entry, conffiledb, or conffiledb base dir. */
+char *
+conffiledb_path(const struct pkginfo *pkg, const char *path,
+ conffiledb_base base)
 {
- char *full_path = NULL, *hash = NULL;
- const char *basedir = NULL;
- size_t basedir_sz = 0, conffiledb_sz = 0, full_path_sz = 0;
+ struct varbuf full_path = VARBUF_INIT;
+ const char *basedir;
 
- /* sanity check for a valid base dir */
+ /* sanity check */
  if (base >= conffiledb_base_COUNT)
- ohshit("conffiledb_path called with unsupported base %x", base);
+ ohshit(_("conffiledb_path called with unsupported base %x"),
+ base);
 
  basedir = conffiledb_base_dirs[base];
+ varbufprintf(&full_path, "%s/%s%s/",
+ admindir, CONFFILEDBDIRROOT, basedir);
 
- /* the root path is <admindir>/<#CONFFILEDBDIRROOT/><basedir>/
- * (note that the '/' after #CONFFILEDBDIRROOT is implicitly included
- * in the #define'd string). The null byte is also accounted for
- * at this point. */
- basedir_sz = strlen(admindir) + 1 + strlen(CONFFILEDBDIRROOT) +
-             strlen(basedir) + 2;
- full_path_sz = conffiledb_sz = basedir_sz;
-
- /* we will add "<pkg>/" to the string later if <pkg> is not null */
- if (pkg != NULL)
- full_path_sz = conffiledb_sz += strlen(pkg) + 1;
-
- /* and if a conffile is being requested (not just the db root)... */
- if (path != NULL)
- full_path_sz += CONFFILEDB_MD5SUM_LEN;
-
- /* this is the path to the conffile db for the given base/pkg */
- full_path = m_malloc(full_path_sz);
- snprintf(full_path, basedir_sz, "%s/%s%s/", admindir,
-         CONFFILEDBDIRROOT, basedir);
-
- /* append "<pkg>/" if needed */
- if (pkg != NULL)
- sprintf(full_path + basedir_sz - 1, "%s/", pkg);
-
- /* append the pathname's hash if relevant */
- if (path != NULL) {
- hash = full_path + conffiledb_sz - 1;
- buffer_md5(path, hash, strlen(path));
- }
-
- debug(dbg_conffdetail, "confffiledb_path(%s, %s, %x) = %s\n", pkg,
-      path, base, full_path);
-
- return full_path;
+ if (pkg == NULL)
+ goto done;
+ varbufprintf(&full_path, "%s/", pkg->name);
+
+ if (path == NULL)
+ goto done;
+ varbuf_grow(&full_path, CONFFILEDB_MD5SUM_LEN + 1);
+ buffer_md5(path, full_path.buf + full_path.used,
+ strlen(path));
+ full_path.used += CONFFILEDB_MD5SUM_LEN;
+
+done:
+ debug(dbg_conffdetail, "confffiledb_path(%s, %s, %x) = %s\n",
+ pkg ? pkg->name : "(null)", path ? path : "(null)", base,
+ full_path.buf);
+ return full_path.buf;
 }
 
-/** Ensure that a particular conffiledb dir exists.
+/**
+ * Ensure that a particular conffiledb dir exists.
+ *
+ * mkdir -p $CONFFILEDBDIRROOT/$base/$pkg
  *
  * @param pkg The package owning the conffiledb dir.
  * @param base Which conffiledb basedir to use.
  */
 static void
-conffiledb_ensure_db(const char *pkg, conffiledb_base base)
+conffiledb_ensure_db(const struct pkginfo *pkg, conffiledb_base base)
 {
- struct stat s;
- short i = 0;
- char *dbdir = NULL;
- char *separators[3];
+ struct stat s;
+ int i;
+ char *dbdir, *end, *separators[3];
 
  dbdir = conffiledb_path(pkg, NULL, base);
- /* temporarily truncate the string to cheaply traverse up to the
+
+ /* sanity check */
+ if (*dbdir != '/')
+ ohshit(_("conffiledb_path returned a relative path"));
+
+ /*
+ * Temporarily truncate the string to cheaply traverse up to the
  * the parent and its parent. store the locations so we can cheaply
- * get back to them later. */
+ * get back to them later.
+ */
+ end = dbdir + strlen(dbdir);
  for (i = 0; i < 3; i++) {
- separators[i] = rindex(dbdir, '/');
- *separators[i] = '\0';
+ while (*end != '/' && end != dbdir)
+ end--;
+ *end = '\0';
+ separators[i] = end;
  }
 
- /* now ensure each directory exists while reconstructing the path */
+ /* Now ensure each directory exists while reconstructing the path. */
  for (i = 2; i >= 0; i--) {
  if (stat(dbdir, &s)) {
- debug(dbg_conffdetail,
+ debug(dbg_conffdetail,
       "conffiledb_ensure_db: creating %s\n", dbdir);
- if (errno != ENOENT)
- ohshite("conffiledb_ensure_db: stat(%s)",
+ if (errno != ENOENT)
+ ohshite("conffiledb_ensure_db: stat(%s)",
         dbdir);
- if (mkdir(dbdir, S_IRWXU))
- ohshite("conffiledb_ensure_db: mkdir(%s)",
+ if (mkdir(dbdir, S_IRWXU))
+ ohshite("conffiledb_ensure_db: mkdir(%s)",
         dbdir);
  }
  *separators[i] = '/';
@@ -109,76 +108,93 @@
  free(dbdir);
 }
 
+/* Add a new entry to $CONFFILEDBDIRROOT/new/$pkg. */
 void
-conffiledb_register_new_conffile(const char *pkg, const char *path, int fd,
-                                 size_t sz)
+conffiledb_register_new_conffile(const struct pkginfo *pkg,
+ const char *path, int fd, size_t sz)
 {
  int cfgfd;
- /* get the path to where the registered file goes */
- char *p = conffiledb_path(pkg, path, conffiledb_base_new);
- char fnamebuf[256];
+ char *dest;
+ char msg_dest[256];
+
+ /* sanity check */
+ if (path == NULL || pkg == NULL)
+ ohshit(_("conffiledb_register_new_conffile called "
+ "with NULL arguments"));
+
+ dest = conffiledb_path(pkg, path, conffiledb_base_new);
+ path_quote_filename(msg_dest, dest, sizeof(msg_dest));
+ debug(dbg_conff, "conffile_register_new_conffile: %s, %s, %s\n",
+ pkg->name, path, msg_dest);
 
  conffiledb_ensure_db(pkg, conffiledb_base_new);
- debug(dbg_conff, "conffile_reg_fd: %s, %s, %s\n", pkg, path, p);
- /* make a mode 600 copy of file to p */
- cfgfd = open(p, (O_CREAT|O_EXCL|O_WRONLY), (S_IRUSR|S_IWUSR));
- if (cfgfd < 0)
- ohshite(_("unable to create `%.255s'"),p);
- fd_fd_copy(fd, cfgfd, sz, _("backend dpkg-deb during `%.255s'"),
-           path_quote_filename(fnamebuf, p, 256));
- if (close(cfgfd))
- ohshite("can't close %s", p);
 
- free(p);
+ cfgfd = open(dest, O_CREAT|O_EXCL|O_WRONLY, S_IRUSR|S_IWUSR);
+ if (cfgfd < 0)
+ ohshite(_("unable to create `%.255s'"), msg_dest);
+ fd_fd_copy(fd, cfgfd, sz, _("new conffiledb entry '%.255s'"),
+           msg_dest);
+
+ if (close(cfgfd))
+ ohshite("can't close %.255s", msg_dest);
+ free(dest);
 }
 
+/* Open a conffiledb entry for reading. */
 int
-conffiledb_get_conffile(const char *pkg, const char *path, conffiledb_base base)
+conffiledb_get_conffile(const struct pkginfo *pkg,
+ const char *path, conffiledb_base base)
 {
- int fd = -1;
+ int fd;
  char *p = conffiledb_path(pkg, path, base);
 
  fd = open(p, O_RDONLY);
- if (fd == -1)
- ohshite("error opening conffile registered at %s", p);
+ if (fd == -1) {
+ char msg_p[256];
+ ohshite(_("error opening conffile registered at %.255s"),
+ path_quote_filename(msg_p, p, sizeof(msg_p)));
+ }
 
  free(p);
  return fd;
 }
 
+/*
+ * Update the conffiles "db" dir.  This consists of the following steps:
+ * - nuke the tmp dir if it exists
+ * - rename to_base to the tmp dir
+ * - rename from_base to to_base
+ * - nuke the tmp dir
+ */
 void
-conffiledb_commit(const char *pkg, conffiledb_base from_base,
+conffiledb_commit(const struct pkginfo *pkg, conffiledb_base from_base,
                   conffiledb_base to_base)
 {
- /* update the conffiles "db" dir.  this consists of the following steps:
- * - nuke the tmp dir if it exists
- * - rename to_base to the tmp dir
- * - rename from_base to to_base
- * - nuke the tmp dir
- */
  char *cfdb_to = NULL, *cfdb_from = NULL, *cfdb_tmp = NULL;
- debug(dbg_conff, "conffiledb_commit(%s, %d, %d)", pkg, from_base,
-      to_base);
+
+ /* sanity check */
+ if (pkg == NULL)
+ ohshit(_("conffiledb_commit called for NULL package"));
+
+ debug(dbg_conff, "conffiledb_commit(%s, %d, %d)",
+ pkg->name, from_base, to_base);
 
  cfdb_from = conffiledb_path(pkg, NULL, from_base);
  cfdb_to = conffiledb_path(pkg, NULL, to_base);
  cfdb_tmp = conffiledb_path(pkg, NULL, conffiledb_base_tmp);
 
- /* Make sure all leading components of the tmp conffiledb exist */
  conffiledb_ensure_db(pkg, conffiledb_base_tmp);
- /* Then nuke the tmp conffiledb if it exists */
  conffiledb_remove(pkg, conffiledb_base_tmp);
- /* Make sure all leading components of the target conffiledb exist */
+
  conffiledb_ensure_db(pkg, to_base);
- /* Rename the target conffiledb to the tmp one */
  if (rename(cfdb_to, cfdb_tmp))
- ohshite("conffiledb_commit: rename(%s,%s)", cfdb_to, cfdb_tmp);
- /* Make sure all leading components of the source conffiledb exist */
+ ohshite(_("conffiledb_commit: rename(%s, %s)"),
+ cfdb_to, cfdb_tmp);
  conffiledb_ensure_db(pkg, from_base);
- /* Rename from_base to to_base */
  if (rename(cfdb_from, cfdb_to))
- ohshite("conffiledb_commit: rename(%s,%s)", cfdb_from, cfdb_to);
- /* Nuke the tmp version if it exists */
+ ohshite(_("conffiledb_commit: rename(%s, %s)"),
+ cfdb_from, cfdb_to);
+
  conffiledb_remove(pkg, conffiledb_base_tmp);
 
  free(cfdb_from);
@@ -188,31 +204,44 @@
 
+/* Remove a conffiledb with all of its entries. */
 void
-conffiledb_remove(const char *pkg, conffiledb_base base)
+conffiledb_remove(const struct pkginfo *pkg, conffiledb_base base)
 {
- char *cfdb = conffiledb_path(pkg, NULL, base);
+ char *cfdb;
+
+ /* sanity check */
+ if (pkg == NULL)
+ ohshit(_("conffiledb_remove called for NULL package"));
 
- debug(dbg_conff, "conffiledb_remove(%s): removing %s\n", pkg, cfdb);
+ cfdb = conffiledb_path(pkg, NULL, base);
+ debug(dbg_conff, "conffiledb_remove(%s): removing %s\n",
+ pkg->name, cfdb);
  ensure_pathname_nonexisting(cfdb);
 
  free(cfdb);
 }
 
+/* Spawn 'diff -u <conffiledb entry> <installed conffile>'. */
 int
-conffiledb_diff (const char *pkg, const char *path, conffiledb_base base)
+conffiledb_diff(const struct pkginfo *pkg, const char *path,
+ conffiledb_base base)
 {
- int status;
- char *src = conffiledb_path(pkg, path, base);
- char *cmd = NULL;
- const char *opts = "-u";
- /* DIFF + ' ' + <opts> + ' ' + <srcfile> + ' ' + <dstfile> + '\0' */
- size_t cmd_sz = strlen(DIFF) + 1 + strlen(opts) + 1 + strlen(src) +
-                1 + strlen(path) + 1;
-
- cmd = m_malloc(cmd_sz);
- sprintf(cmd, "%s %s %s %s", DIFF, opts, src, path);
- status = system(cmd);
+ int child, result;
+ char *src;
+
+ /* sanity check */
+ if (pkg == NULL || path == NULL)
+ ohshit(_("conffiledb_diff called for NULL conffile"));
+
+ src = conffiledb_path(pkg, path, base);
+
+ if (!(child = m_fork())) {
+ execlp(DIFF, "diff", "-u", src, path, NULL);
+ ohshite(_("failed to exec 'diff -u %s %s'"),
+ src, path);
+ }
+
+ result = waitsubproc(child, "diff", PROCNOERR | PROCSAVESTATUS);
 
  free(src);
- free(cmd);
- return WEXITSTATUS(status);
+ return result;
 }
diff -u b/src/conffiledb.h b/src/conffiledb.h
--- b/src/conffiledb.h
+++ b/src/conffiledb.h
@@ -1,7 +1,7 @@
 /**
  * @file conffiledb.h
  *
- * Functions for tracking referencable versions of packages' conffiles.
+ * Functions for tracking referenceable versions of packages' conffiles.
  *
  * This library allows for tracking the pristine versions (and in some specific
  * cases a few other versions) of packages' conffiles.  This allows for a
@@ -15,19 +15,23 @@
  * #CONFFILEDBDIRROOT/\<base\>/\<pkg\>/\<md5\>
  *
  * where:
- * - @b \<base\> is a short string which corresponds to the base reference
- *   type in question. See the documentation for conffiledb_base for the
- *   enumerated values and conffiledb_base_dirs for the corresponding
- *   string values.
- * - @b \<pkg\> is the conffile database directory for an individual package.
- * - @b \<md5\> is the md5 hash of the location of a package's conffile.  A
+ * - @b \<base\> is a short string describing which version of the conffile
+ *   this is. See enum #conffiledb_base for the enumerated values and
+ *   #conffiledb_base_dirs for the corresponding strings.
+ * - @b \<pkg\> is the package name.
+ * - @b \<md5\> is the md5 hash of the location of a package's conffile.  A
  *   hash is used to keep a flat and balanced directory structure, and has
  *   the added benefit of a simpler implementation.
  *
  * Some further terminology to keep things clear later on:
  *
- * - @b "conffiledb root" refers to the top-most directory containing
- *   all conffile databases, and is defined by #CONFFILEDBDIRROOT.
+ * - @b "conffiledb file" or @b "conffiledb entry" refers to a file whose
+ *   contents correspond to a particular conffiledb_base/package/conffile
+ *   triplet.
+ * - @anchor conffiledb @b "conffile database" or @b "conffiledb" refers
+ *   to a directory specific to a conffiledb_base + package tuple.  Such a
+ *   directory will contain copies of the conffiles named after the hash
+ *   of their respective installed locations.
  * - @b "conffiledb basedir" refers to the subdirectory directly
  *   underneath the conffiledb root directory, which divides the the
  *   different conffiledbs by different "reference types" (i.e. the
@@ -35,19 +39,17 @@
  *   a newly unpacked version). The available basedirs are defined in
  *   conffiledb_base_dirs, which is indexed on the respective enumerated
  *   values from conffiledb_base.
- * - @anchor conffiledb @b "conffile database" or @b "conffiledb" refers
- *   to a directory specific to a conffiledb_base + package tuple.  Such a
- *   directory will contain copies of the conffiles named after the hash
- *   of their respective installed locations.
- * - @b "conffiledb file" or @b "conffiledb entry" refers to a file whose
- *   contents correspond to a particular conffiledb_base/package/conffile
- *   triplet.
+ * - @b "conffiledb root" refers to the top-most directory containing
+ *   all conffile databases, and is defined by #CONFFILEDBDIRROOT.
  */
 #ifndef CONFFILES_H
 #define CONFFILES_H
 
 #include <sys/types.h>
 
+/* Defined in dpkg/dpkg-db.h. */
+struct pkginfo;
+
 /** The different base reference types underneath the conffile db. */
 typedef enum {
  conffiledb_base_current, /**< The currently installed version */
@@ -57,7 +59,7 @@
  conffiledb_base_COUNT /**< The count of possible values for this type */
 } conffiledb_base;
 
-/**
+/**
  * The directory names of the conffiledb base dirs, indexed by the respective
  * conffiledb_base value.
  */
@@ -66,13 +68,14 @@
 /** Length of the MD5 string used in conffile path hashing (NULL not counted) */
 #define CONFFILEDB_MD5SUM_LEN 32
 
-/** Return the canonical path for a conffiledb entry or other location.
+/**
+ * Determine the canonical path for a conffiledb entry.
  *
- * This is a helper function to resolve the full path to a specific location
+ * This is a helper function to resolve the full path to a specific location
  * within the conffile database. The primary use is to find the location
  * of a conffiledb file, but it can also be used to find the location
  * of the @ref conffiledb for a particular conffiledb_base + package,
- * as well as the parent conffiledb basedir corresponding to a particular
+ * as well as the parent conffiledb basedir corresponding to a particular
  * conffiledb_base.
  *
  * @param pkg The package name. If NULL, the returned value will be
@@ -81,75 +84,99 @@
  *        value will be the conffiledb directory which should contain all
  *        conffiledb files for the combination of pkg and base.
- * @param base Which conffiledb basedir is requested.
+ * @param base Which conffiledb base reference type is requested.
+ *
  * @return An allocated string with the path (your job to free it).
  */
-char* conffiledb_path(const char *pkg, const char *path, conffiledb_base base);
+char *conffiledb_path(const struct pkginfo *pkg, const char *path,
+ conffiledb_base base);
 
-/** Register a new conffiledb file for a package.
+/**
+ * Add a new conffiledb entry for a package.
  *
- * Create a conffiledb entry in the conffiledb dir for the::conffiledb_base_new
- * version of a particular package. The conffile will be later processed in the
- * package configuration stage, after which there should be a call to
- * conffiledb_commit_new before configuration can be considered successful.
- *
- * The use of a filedescriptor is due to the fact that the new conffile is
- * intercepted very early during the unpack phase, before it exists on disk.
- * Therefore this interface is designed to hook cleanly into what's available
- * at that point, which is a filedescriptor generated from a struct TarInfo
- * (see tarobject in archives.c).
+ * For use when unpacking a package.  Create a conffiledb entry in the
+ * conffiledb dir for the #conffiledb_base_new version of a particular
+ * package. The conffile will be later processed in the package configuration
+ * stage, after which there should be a call to conffiledb_commit() before
+ * configuration can be considered successful.
+ *
+ * The use of a file descriptor allows the new entry to be written very early
+ * in the unpack phase, before it exists on disk.  This interface is designed
+ * to hook cleanly with what's available, which is filedescriptor representing
+ * output from tar extraction.
  *
  * @param pkg The package owning the conffile.
  * @param path The installed location of the conffile.
  * @param fd A file descriptor holding the contents of the conffile.
- * @param sz The size of the conffile.
+ * @param sz Size of the conffile in bytes.
+ *           (XXX why is this not off_t, -1 allowed?)
  */
-void conffiledb_register_new_conffile(const char *pkg, const char *path, int fd,
-                                      size_t sz);
+void conffiledb_register_new_conffile(const struct pkginfo *pkg,
+ const char *path, int fd, size_t sz);
 
-/** Get a readable filehandle to the contents of a conffiledb entry.
+/**
+ * Open a conffiledb entry for reading.
  *
  * @param pkg The package owning the conffile.
  * @param path The installed location of the conffile.
  * @param base Which conffiledb basedir to use.
- * @return A read-only file descriptor for the registered conffile.
+ *
+ * @return A read-only file descriptor for the registered conffile
+ *         (your job to close it).
  */
-int conffiledb_get_conffile(const char *pkg, const char *path,
-                            conffiledb_base base);
+int conffiledb_get_conffile(const struct pkginfo *pkg,
+ const char *path, conffiledb_base base);
 
-/** Remove a conffiledb directory for a package.
+/**
+ * Remove a package’s conffiledb.
+ *
+ * Remove the indicated version of the specified package’s conffiledb.
  *
- * This is designed to be used in two use cases:
- * - package removal and cleanup
- * - after one conffiledb directory has been replaced by another.
+ * This should be useful in two cases:
+ * - purging a package’s configuration files
+ * - replacing one conffiledb with another.
  *
  * @param pkg The package owning the conffile db to be removed.
  * @param base Which conffiledb basedir to use with respect to pkg.
  */
-void conffiledb_remove(const char *pkg, conffiledb_base base);
+void conffiledb_remove(const struct pkginfo *pkg, conffiledb_base base);
 
-/** Commit one conffiledb as another, replacing any existing one.
+/**
+ * Move a conffiledb from one base reference type to another.
  *
- * This function takes the conffiledb defined by pkg and from_base and
- * commits it as being defined by pkg and to_base. Any existing conffiledb
- * for pkg / to_base is first renamed as conffiledb_base_tmp and then
- * removed after the successful "commit" (to keep things as atomic as
- * possible).
+ * This is a safer way to perform the operation
+ *  
+ * rm -r $CONFFILEDBDIRROOT/$to_base/$pkg
+ * mv $CONFFILEDBDIRROOT/$from_base/$pkg $CONFFILEDBDIRROOT/$to_base/
+ *
+ * The conffiledb corresponding to pkg and the tmp base reference type
+ * is used as a temporary location to hold the entries from to_base until
+ * the entries from from_base are put into place.
+ *
+ * Any entries already in the tmp/$pkg conffiledb will be removed.  The
+ * invoking program is responsible for putting things back in order upon
+ * failure.
  *
  * @param pkg The package owning the conffile database
  * @param from_base The current conffiledb basedir which should be renamed.
  * @param to_base The conffiledb basedir which should be replaced by the
  *        version in from_base.
  */
-void conffiledb_commit(const char *pkg, conffiledb_base from_base,
-                       conffiledb_base to_base);
+void conffiledb_commit(const struct pkginfo *pkg,
+ conffiledb_base from_base, conffiledb_base to_base);
 
-/** Show the diff between a conffiledb entry and a corresponding conffile.
- *
+/**
+ * Print a diff between a conffiledb entry and the installed version.
+ *
+ * Print a unified diff between a conffiledb entry and the corresponding
+ * installed conffile to stdout.
+ *
  * @param pkg The package that owns the conffile.
  * @param path The pathname to the installed version of the conffile.
  * @param base Which conffiledb basedir to use.
- * @return The exit status of the underlying call to diff(1).
+ *
+ * @return The exit status from diff(1).
  */
-int conffiledb_diff(const char *pkg, const char *path, conffiledb_base base);
+int conffiledb_diff(const struct pkginfo *pkg, const char *path,
+ conffiledb_base base);
 
 #endif /* CONFFILES_H */
only in patch2:
unchanged:
--- a/lib/dpkg/subproc.c
+++ b/lib/dpkg/subproc.c
@@ -73,22 +73,24 @@ cu_subproc_signals(int argc, void **argv)
 int
 checksubprocerr(int status, const char *description, int flags)
 {
- int n;
+ if (flags & PROCWARN)
+ flags |= PROCNOERR;
 
  if (WIFEXITED(status)) {
- n = WEXITSTATUS(status);
+ int n = WEXITSTATUS(status);
+
  if (!n)
  return 0;
- if (flags & PROCNOERR)
- return -1;
  if (flags & PROCWARN)
  warning(_("%s returned error exit status %d"),
         description, n);
- else
- ohshit(_("subprocess %s returned error exit status %d"),
-       description, n);
+ if (flags & PROCNOERR)
+ return flags & PROCSAVESTATUS ? n : -1;
+ ohshit(_("subprocess %s returned error exit status %d"),
+       description, n);
  } else if (WIFSIGNALED(status)) {
- n = WTERMSIG(status);
+ int n = WTERMSIG(status);
+
  if (!n)
  return 0;
  if ((flags & PROCPIPE) && n == SIGPIPE)
@@ -97,16 +99,14 @@ checksubprocerr(int status, const char *description, int flags)
  warning(_("%s killed by signal (%s)%s"),
         description, strsignal(n),
         WCOREDUMP(status) ? _(", core dumped") : "");
- else
- ohshit(_("subprocess %s killed by signal (%s)%s"),
-       description, strsignal(n),
-       WCOREDUMP(status) ? _(", core dumped") : "");
- } else {
- ohshit(_("subprocess %s failed with wait status code %d"),
-       description, status);
+ if (flags & PROCNOERR)
+ return -1;
+ ohshit(_("subprocess %s killed by signal (%s)%s"),
+       description, strsignal(n),
+       WCOREDUMP(status) ? _(", core dumped") : "");
  }
-
- return -1;
+ ohshit(_("subprocess %s failed with wait status code %d"),
+       description, status);
 }
 
 int
only in patch2:
unchanged:
--- a/lib/dpkg/subproc.h
+++ b/lib/dpkg/subproc.h
@@ -34,6 +34,7 @@ void cu_subproc_signals(int argc, void **argv);
 #define PROCPIPE 1
 #define PROCWARN 2
 #define PROCNOERR 4
+#define PROCSAVESTATUS 8
 
 int checksubprocerr(int status, const char *desc, int flags);
 int waitsubproc(pid_t pid, const char *desc, int flags);


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...