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

Sean Finney · External Since: Dec 07, 2004 Posts: 87

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.TakeThisOut@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.TakeThisOut@lists.debian.org

Sean Finney · External Since: Dec 07, 2004 Posts: 87

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 Sad

*/
+ 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" Wink

*/
+ 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.DeleteThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.DeleteThis@lists.debian.org

Sean Finney · External Since: Dec 07, 2004 Posts: 87

(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.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org

Sean Finney · External Since: Dec 07, 2004 Posts: 87

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.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org

Sean Finney · External Since: Dec 07, 2004 Posts: 87

---
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 DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

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.DeleteThis@gmail.com>

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

-- %< --
From: Sean Finney <seanius.DeleteThis@debian.org>
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.DeleteThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.DeleteThis@lists.debian.org

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

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. Smile

Thanks for the interesting read.

Regards,
Jonathan

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

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

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.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org

Guillem Jover · External Since: Nov 13, 2004 Posts: 478

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.DeleteThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.DeleteThis@lists.debian.org

Guillem Jover · External Since: Nov 13, 2004 Posts: 478

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 RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

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.TakeThisOut@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.TakeThisOut@lists.debian.org

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

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.TakeThisOut@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.TakeThisOut@lists.debian.org

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

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. Smile

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. Smile

- 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 RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org

Guillem Jover · External Since: Nov 13, 2004 Posts: 478

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 Smile

.

> + 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 DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Guillem Jover · External Since: Nov 13, 2004 Posts: 478

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.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

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 RemoveThis @debian.org>
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 RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

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.TakeThisOut@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.TakeThisOut@lists.debian.org

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

Hi,

A few quick thoughts and questions about conffiledb_automerge:

Sean Finney wrote:

> 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).
[...]

> +int conffiledb_automerge(const char *pkg, const char *path)
> +{

This merges a user's and package's changes to update a conffile, also
saving the result to the resolved conffile db.

> + /* 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);

Why save here rather than a tmp conffile db? Or, put another way:
why do we write to the conffile itself before the conffile db?

> + /* 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);

If dpkg fails part-way through, the .dpkg-merge will be left lying
around. Would it be safe to delete it and try again in the next run?

> + 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");

This could be made very similar to the conffile unpacking code if dpkg
writes to the tmp db first. If diff3 succeeds, dpkg could copy to the
..dpkg-merge file, then rename both.

I see you save the resolved version in the db for future reference,
so we can tell later if the user decides to modify the result. Most
version control systems do something like this to cache the result and
avoid reading the working tree before commiting it to the repository.
Is dpkg also preparing to use the file later?

If the resolved version will be the "common ancestor" in future
merges, what files will be "mine" and "yours"? I would not imagine
future distributed conffiles could play either role, since the the
resolved version contains both good (user-written) and bad
(distributor-corrected) changes relative to them.

Jonathan

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

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

Jonathan Nieder wrote:

> 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. Smile

> - 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?

That is, how about something like this change?

It is split up into mini-patches for easier review. Patches 1-3 are
preparatory patches intended not to change behavior significantly, and
then patch 4 addresses the (theoretical) bug. If these look good, it
might make sense to apply them as one squashed patch (or it might not;
I never was very good at sculpting histories).

Unfortunately, this small change ends up adding many more lines than
it removes.

Jonathan Nieder (4):
dpkg --configure: plug a small memory leak
dpkg: (K)eep conffile: split rename into link + unlink
dpkg: (I)nstall conffile: split rename into link + unlink
Fix theoretical race condition in interrupted dpkg --configure

lib/dpkg/dpkg.h | 1 +
src/configure.c | 69 ++++++++++++++++++++++++++++++++++++------------------
src/remove.c | 7 ++++-
4 files changed, 56 insertions(+), 24 deletions(-)

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

Jonathan Nieder · External Since: Feb 26, 2009 Posts: 41

If deferred_configure_conffile() is interrupted after the
..dpkg-new file is removed but before its hash is recorded as the
distributed conffile hash, that hash is lost forever, and if it
is different from the previous one then the conffile will appear
to have been modified by the user the next time an upgrade
modifies that conffile.

Fixing this should avoid unnecessary prompting in some cases.

Signed-off-by: Jonathan Nieder <jrnieder.DeleteThis@gmail.com>
---
src/configure.c | 14 +++++---------
1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/src/configure.c b/src/configure.c
index 29878e4..e4fffc9 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -183,10 +183,6 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
pkg->name, cdr2.buf, cdr.buf, strerror(errno));
/* Fall through. */
case cfo_keep:
- strcpy(cdr2rest, DPKGNEWEXT);
- if (unlink(cdr2.buf))
- warning(_("%s: failed to remove '%.250s': %s"),
- pkg->name, cdr2.buf, strerror(errno));
break;
case cfo_install | cfof_backup:
strcpy(cdr2rest, DPKGDISTEXT);
@@ -220,11 +216,6 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
if (rename(cdr2.buf, cdr.buf))
ohshite(_("unable to install `%.250s' as `%.250s'"),
cdr2.buf, cdr.buf);
- strcpy(cdr2rest, DPKGNEWEXT);
- if (unlink(cdr2.buf))
- warning(_("%s: failed to remove '%.250s': %s"),
- pkg->name, cdr2.buf, strerror(errno));
- break;
default:
internerr("unknown conffopt '%d'", what);
}
@@ -232,6 +223,11 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
conff->hash = nfstrsave(newdisthash);
modstatdb_note(pkg);

+ strcpy(cdr2rest, DPKGNEWEXT);
+ if (unlink(cdr2.buf))
+ warning(_("%s: failed to remove '%.250s': %s"),
+ pkg->name, cdr2.buf, strerror(errno));
+
varbuffree(&cdr);
varbuffree(&cdr2);
}
--
1.6.5.2

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