[PATCH 0/6] patch series for review: conffile database + merging

[sean finney]

the following series of patches is a renewed attempt at conffile tracking,
where the packaged conffiles are stored in a seperate location on the
system in their pristine forms.  the patches the follow contain more
information on the details.

the first 3 patches are required changes in dpkg, mostly changing the
interface of internal static functions and renaming/exposing other static
functions that are needed for the features in the patches that follow.

Sean Finney (6):
  move quote_filename into lib/dpkg/path.{c,h} as path_quote_filename
  move copyfileperms to non-static file_copyfileperms
  update promptconfaction() to also require package name
  conffile-database handling functions
  hook conffile database into dpkg unpack/configure/remove stages
  implement 'm' option for conffile merging during conflict resolution

 lib/dpkg/Makefile.am |    1 +
 lib/dpkg/dpkg.h      |    3 +
 lib/dpkg/file.c      |   59 +++++++++++++
 lib/dpkg/file.h      |   35 ++++++++
 lib/dpkg/path.c      |   59 +++++++++++++
 lib/dpkg/path.h      |    1 +
 src/Makefile.am      |    1 +
 src/archives.c       |   77 +++-------------
 src/conffiles.c      |  236 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/conffiles.h      |   55 ++++++++++++
 src/configure.c      |   55 +++++-------
 src/processarc.c     |    4 +
 src/remove.c         |    3 +
 13 files changed, 496 insertions(+), 93 deletions(-)
 create mode 100644 lib/dpkg/file.c
 create mode 100644 lib/dpkg/file.h
 create mode 100644 src/conffiles.c
 create mode 100644 src/conffiles.h


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

[sean finney]

From: Sean Finney <seanius@...>

this function will be useful for other parts of dpkg, so the function
has been moved to a more sensible location, the static qualifier removed,
and its name appropriately prefixed.
---
 lib/dpkg/path.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/dpkg/path.h |    1 +
 src/archives.c  |   63 ++----------------------------------------------------
 3 files changed, 63 insertions(+), 60 deletions(-)

diff --git a/lib/dpkg/path.c b/lib/dpkg/path.c
index 8ea891e..ad14a45 100644
--- a/lib/dpkg/path.c
+++ b/lib/dpkg/path.c
@@ -23,6 +23,7 @@
 #include <config.h>
 #include <compat.h>
 
+#include <stdio.h>
 #include <string.h>
 
 #include <dpkg/path.h>
@@ -54,3 +55,61 @@ path_skip_slash_dotslash(const char *path)
  return path;
 }
 
+/* snprintf(3) doesn't work if format contains %.<nnn>s and an argument has
+ * invalid char for locale, then it returns -1.
+ * ohshite() is ok, but fd_fd_copy(), which is used in tarobject() in this
+ * file, is not ok, because
+ * - fd_fd_copy() == buffer_copy_setup() [include/dpkg.h]
+ * - buffer_copy_setup() uses varbufvprintf(&v, desc, al); [lib/mlib.c]
+ * - varbufvprintf() fails and memory exausted, because it call
+ *    fmt = "backend dpkg-deb during `%.255s'
+ *    arg may contain some invalid char, for example,
+ *    /usr/share/doc/console-tools/examples/unicode/\342\231\252\342\231\254
+ *   in console-tools.
+ *   In this case, if user uses some locale which doesn't support \342\231...,
+ *   vsnprintf() always returns -1 and varbufextend() get called again
+ *   and again until memory is exausted and it aborts.
+ *
+ * So, we need to escape invalid char, probably as in
+ * tar-1.13.19/lib/quotearg.c: quotearg_buffer_restyled()
+ * but here I escape all 8bit chars, in order to be simple.
+ * - ukai@...
+ */
+char *
+path_quote_filename(char *buf, int size, char *s)
+{
+ char *r = buf;
+
+ while (size > 0) {
+ switch (*s) {
+ case '\0':
+ *buf = '\0';
+ return r;
+ case '\\':
+ *buf++ = '\\';
+ *buf++ = '\\';
+ size -= 2;
+ break;
+ default:
+ if (((*s) & 0x80) == '\0') {
+ *buf++ = *s++;
+ --size;
+ } else {
+ if (size > 4) {
+ sprintf(buf, "\\%03o",
+        *(unsigned char *)s);
+ size -= 4;
+ buf += 4;
+ s++;
+ } else {
+ /* buffer full */
+ *buf = '\0'; /* XXX */
+ return r;
+ }
+ }
+ }
+ }
+ *buf = '\0'; /* XXX */
+
+ return r;
+}
diff --git a/lib/dpkg/path.h b/lib/dpkg/path.h
index 701eb8e..ae65f06 100644
--- a/lib/dpkg/path.h
+++ b/lib/dpkg/path.h
@@ -32,6 +32,7 @@ DPKG_BEGIN_DECLS
 
 size_t path_rtrim_slash_slashdot(char *path);
 const char *path_skip_slash_dotslash(const char *path);
+char *path_quote_filename(char *buf, int size, char *s);
 
 DPKG_END_DECLS
 
diff --git a/src/archives.c b/src/archives.c
index f9f95a8..d2a6da4 100644
--- a/src/archives.c
+++ b/src/archives.c
@@ -47,6 +47,7 @@
 #include <dpkg/subproc.h>
 #include <dpkg/tarfn.h>
 #include <dpkg/myopt.h>
+#include <dpkg/path.h>
 
 #ifdef WITH_SELINUX
 #include <selinux/selinux.h>
@@ -63,64 +64,6 @@ static security_context_t scontext    = NULL;
 struct pkginfo *conflictor[MAXCONFLICTORS];
 int cflict_index = 0;
 
-/* snprintf(3) doesn't work if format contains %.<nnn>s and an argument has
- * invalid char for locale, then it returns -1.
- * ohshite() is ok, but fd_fd_copy(), which is used in tarobject() in this
- * file, is not ok, because
- * - fd_fd_copy() == buffer_copy_setup() [include/dpkg.h]
- * - buffer_copy_setup() uses varbufvprintf(&v, desc, al); [lib/mlib.c]
- * - varbufvprintf() fails and memory exausted, because it call
- *    fmt = "backend dpkg-deb during `%.255s'
- *    arg may contain some invalid char, for example,
- *    /usr/share/doc/console-tools/examples/unicode/\342\231\252\342\231\254
- *   in console-tools.
- *   In this case, if user uses some locale which doesn't support \342\231...,
- *   vsnprintf() always returns -1 and varbufextend() get called again
- *   and again until memory is exausted and it aborts.
- *
- * So, we need to escape invalid char, probably as in
- * tar-1.13.19/lib/quotearg.c: quotearg_buffer_restyled()
- * but here I escape all 8bit chars, in order to be simple.
- * - ukai@...
- */
-static char *
-quote_filename(char *buf, int size, char *s)
-{
-  char *r = buf;
-
-  while (size > 0) {
-    switch (*s) {
-    case '\0':
-      *buf = '\0';
-      return r;
-    case '\\':
-      *buf++ = '\\';
-      *buf++ = '\\';
-      size -= 2;
-      break;
-    default:
-      if (((*s) & 0x80) == '\0') {
-        *buf++ = *s++;
-        --size;
-      } else {
-        if (size > 4) {
-          sprintf(buf, "\\%03o", *(unsigned char *)s);
-          size -= 4;
-          buf += 4;
-          s++;
-        } else {
-          /* buffer full */
-          *buf = '\0'; /* XXX */
-          return r;
-        }
-      }
-    }
-  }
-  *buf = '\0'; /* XXX */
-
-  return r;
-}
-
 /* special routine to handle partial reads from the tarfile */
 static int safe_read(int fd, void *buf, int len)
 {
@@ -255,7 +198,7 @@ tarfile_skip_one_forward(struct TarInfo *ti,
 
     fd_null_copy(tc->backendpipe, ti->Size,
                  _("skipped unpacking file '%.255s' (replaced or excluded?)"),
-                 quote_filename(fnamebuf, 256, ti->Name));
+                 path_quote_filename(fnamebuf, 256, ti->Name));
     r = ti->Size % TARBLKSZ;
     if (r > 0)
       r = safe_read(tc->backendpipe, databuf, TARBLKSZ - r);
@@ -686,7 +629,7 @@ int tarobject(struct TarInfo *ti) {
     debug(dbg_eachfiledetail,"tarobject NormalFile[01] open size=%lu",
           (unsigned long)ti->Size);
     { char fnamebuf[256];
-    fd_fd_copy(tc->backendpipe, fd, ti->Size, _("backend dpkg-deb during `%.255s'"),quote_filename(fnamebuf,256,ti->Name));
+    fd_fd_copy(tc->backendpipe, fd, ti->Size, _("backend dpkg-deb during `%.255s'"),path_quote_filename(fnamebuf,256,ti->Name));
     }
     r= ti->Size % TARBLKSZ;
     if (r > 0) r= safe_read(tc->backendpipe,databuf,TARBLKSZ - r);
--
1.6.4.3


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

[sean finney]

this functionality is also needed by the conffile handling code to ensure
that the merge output is stored in a file with the same permissions as
the original conffile, preventing the accidental oppurtunity for
unintended information disclosure.

therefore the function is moved into a new library module (file.{c,h}),
and given an appropriate prefix.  note that some of the translatable error
messages have been modified as they would otherwise be misleading.
---
 lib/dpkg/Makefile.am |    1 +
 lib/dpkg/file.c      |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/dpkg/file.h      |   35 +++++++++++++++++++++++++++++
 src/configure.c      |   28 +----------------------
 4 files changed, 97 insertions(+), 26 deletions(-)
 create mode 100644 lib/dpkg/file.c
 create mode 100644 lib/dpkg/file.h

diff --git a/lib/dpkg/Makefile.am b/lib/dpkg/Makefile.am
index d45bcd4..245f2c6 100644
--- a/lib/dpkg/Makefile.am
+++ b/lib/dpkg/Makefile.am
@@ -26,6 +26,7 @@ libdpkg_a_SOURCES = \
  dbmodify.c \
  dump.c \
  ehandle.c \
+ file.c file.h \
  fields.c \
  i18n.h \
  lock.c \
diff --git a/lib/dpkg/file.c b/lib/dpkg/file.c
new file mode 100644
index 0000000..0408844
--- /dev/null
+++ b/lib/dpkg/file.c
@@ -0,0 +1,59 @@
+/*
+ * libdpkg - Debian packaging suite library routines
+ * file.c - file handling functions
+ *
+ * Copyright © 1995 Ian Jackson <ian@...>
+ * Copyright © 2008 Guillem Jover <guillem@...>
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2,
+ * or (at your option) any later version.
+ *
+ * This is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with dpkg; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <config.h>
+#include <compat.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <errno.h>
+
+#include <dpkg/file.h>
+#include <dpkg/dpkg.h>
+#include <dpkg/i18n.h>
+
+/*
+ * Copy file ownership and permissions from one file to another.
+ */
+void
+file_copyfileperm(const char *source, const char *target)
+{
+ struct stat stab;
+
+ if (stat(source, &stab) == -1) {
+ if (errno == ENOENT)
+ return;
+ ohshite(_("unable to stat installed file `%.250s'"), source);
+ }
+
+ if (chown(target, stab.st_uid, stab.st_gid) == -1)
+ ohshite(_("unable to change ownership of target file`%.250s'"),
+        target);
+
+ if (chmod(target, (stab.st_mode & 07777)) == -1)
+ ohshite(_("unable to set mode of target file`%.250s'"), target);
+}
+
+/*
+ * vim: noet ts=8
+ */
diff --git a/lib/dpkg/file.h b/lib/dpkg/file.h
new file mode 100644
index 0000000..cb95647
--- /dev/null
+++ b/lib/dpkg/file.h
@@ -0,0 +1,35 @@
+/*
+ * libdpkg - Debian packaging suite library routines
+ * file.h - file handling routines
+ *
+ * Copyright © 2008 Guillem Jover <guillem@...>
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2,
+ * or (at your option) any later version.
+ *
+ * This is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with dpkg; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef DPKG_FILE_H
+#define DPKG_FILE_H
+
+#include <dpkg/macros.h>
+
+DPKG_BEGIN_DECLS
+
+/* set permissions on target to equal those of source */
+void file_copyfileperm(const char *source, const char *target);
+
+DPKG_END_DECLS
+
+#endif /* DPKG_FILE_H */
+
diff --git a/src/configure.c b/src/configure.c
index f691046..17bf941 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -47,6 +47,7 @@
 #include <dpkg/dpkg.h>
 #include <dpkg/dpkg-db.h>
 #include <dpkg/buffer.h>
+#include <dpkg/file.h>
 
 #include "filesdb.h"
 #include "main.h"
@@ -58,7 +59,6 @@ static int conffoptcells[2][2] = {
 };
 
 static void md5hash(struct pkginfo *pkg, char *hashbuf, const char *fn);
-static void copyfileperm(const char *source, const char *target);
 static void showdiff(const char *old, const char *new);
 static void suspend(void);
 static enum conffopt promptconfaction(const char *cfgfile,
@@ -107,7 +107,7 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
  /* Copy the permissions from the installed version to the new
  * distributed version. */
  if (!stat(cdr.buf, &stab))
- copyfileperm(cdr.buf, cdr2.buf);
+ file_copyfileperm(cdr.buf, cdr2.buf);
  else if (errno != ENOENT)
  ohshite(_("unable to stat current installed conffile `%.250s'"),
         cdr.buf);
@@ -471,30 +471,6 @@ md5hash(struct pkginfo *pkg, char *hashbuf, const char *fn)
 }
 
 /*
- * Copy file ownership and permissions from one file to another.
- */
-static void
-copyfileperm(const char *source, const char *target)
-{
- struct stat stab;
-
- if (stat(source, &stab) == -1) {
- if (errno == ENOENT)
- return;
- ohshite(_("unable to stat current installed conffile `%.250s'"),
-        source);
- }
-
- if (chown(target, stab.st_uid, stab.st_gid) == -1)
- ohshite(_("unable to change ownership of new dist conffile `%.250s'"),
-        target);
-
- if (chmod(target, (stab.st_mode & 07777)) == -1)
- ohshite(_("unable to set mode of new dist conffile `%.250s'"),
-        target);
-}
-
-/*
  * Show a diff between two files.
  */
 static void
--
1.6.4.3


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

[sean finney]

in order to provide an interface into the conffile 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 17bf941..e195362 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);
@@ -151,7 +151,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)) {
@@ -560,8 +560,9 @@ suspend(void)
  * Select what to do with a configuration file.
  */
 static enum conffopt
-promptconfaction(const char *cfgfile, const char *realold, const char *realnew,
-                 int useredited, int distedited, enum conffopt what)
+promptconfaction(const char *pkg, const char *cfgfile, const char *realold,
+                 const char *realnew, int useredited, int distedited,
+                 enum conffopt what)
 {
  const char *s;
  int c, cc;
--
1.6.4.3


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

[sean finney]

conffiles.{c,h} defines 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[-new]/<pkg>/<hash>

where "-new" signifies a temporary copy of an unpacked/unconfigured
package's configuration files.  <pkg> is the package owning the conffiles
in question, and <hash> is an md5sum of the conffile's path.  this ensures
a flat and normalized layout.

it's assumed that this database is not meant for direct user consumption,
and instead that there will be a provided utility (similar in features
and function to ucf(1) utility) and/or exported library routines for
such interaction.
---
 lib/dpkg/dpkg.h |    1 +
 src/Makefile.am |    1 +
 src/conffiles.c |  178 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/conffiles.h |   50 +++++++++++++++
 4 files changed, 230 insertions(+), 0 deletions(-)
 create mode 100644 src/conffiles.c
 create mode 100644 src/conffiles.h

diff --git a/lib/dpkg/dpkg.h b/lib/dpkg/dpkg.h
index afe650f..335a826 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 CONFFILESDIR      "conffiles"
 #define TRIGGERSDIR       "triggers/"
 #define TRIGGERSFILEFILE  "File"
 #define TRIGGERSDEFERREDFILE "Unincorp"
diff --git a/src/Makefile.am b/src/Makefile.am
index 820cf1c..0877c4a 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -16,6 +16,7 @@ bin_PROGRAMS = dpkg dpkg-query dpkg-trigger
 dpkg_SOURCES = \
  archives.c archives.h \
  cleanup.c \
+ conffiles.c conffiles.h \
  configure.c \
  depcon.c \
  enquiry.c \
diff --git a/src/conffiles.c b/src/conffiles.c
new file mode 100644
index 0000000..fe426d7
--- /dev/null
+++ b/src/conffiles.c
@@ -0,0 +1,178 @@
+#include "conffiles.h"
+
+#include <config.h>
+
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.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"
+
+/* MD5 string length, minus null byte.  mostly here for readability below */
+#define MD5SUM_LEN 32
+
+/* return a pointer to a char* which has the full on-disk location of
+ * the file in the "conffile db" registered for the file at path.  pass
+ * flags f to indicate whether you want the "new", "old", or "current" version.
+ *
+ * no checking is done that the file actually exists, this is just where it
+ * ought to be.  your job to free() the result.
+ */
+static
+char* conff_db_path(const char *pkg, const char *path, conff_flag f)
+{
+ char *p = NULL, *hash = NULL;
+ const char *ext = "";
+ size_t root_sz = 0, path_sz = 0;
+
+ /* <admindir>/<conffiles[-ext]>/<pkg> + '\0' */
+ root_sz = strlen(admindir) + 1 + strlen(CONFFILESDIR) + 1 +
+          strlen(pkg) + 1;
+
+ /* in the case of new/old, we add extra space for the [-ext] */
+ switch (f) {
+ case conff_db_new:
+ ext = NEWDBEXT;
+ root_sz+=strlen(NEWDBEXT);
+ break;
+ case conff_db_cur:
+ ext = "";
+ break;
+ default:
+ ohshit("conff_db_path called with unsupported flags %x", f);
+ break;
+ }
+
+ path_sz = root_sz;
+ /* and if a conffile is being requested (not just the db root)... */
+ if (path != NULL) {
+ /* / + hash */
+ path_sz += (1 + MD5SUM_LEN);
+ }
+
+ /* this is the path to the conffile db root for pkg */
+ p = m_malloc(path_sz);
+ snprintf(p, root_sz, "%s/%s%s/%s", admindir, CONFFILESDIR, ext, pkg);
+
+ /* append the pathname's hash if relevant */
+ if (path != NULL) {
+ p[root_sz-1] = '/';
+ hash = p + root_sz;
+ buffer_md5(path, hash, strlen(path));
+ }
+
+ debug(dbg_conffdetail, "conff_db_path(%s, %s, %x) = %s\n", pkg, path,
+      f, p);
+
+ return p;
+}
+
+/* ensure that the <admindir>/<conffilesdb/><pkg> exists */
+void
+conff_ensure_db(const char *pkg, conff_flag f)
+{
+ struct stat s;
+ char *dbdir = NULL;
+ short dirsmade = 0;
+ size_t base_sz = 0; /* offset of <pkg> in <conffilesdb>/<pkg> */
+
+ dbdir = conff_db_path(pkg, NULL, f);
+ /* to avoid an extra malloc() we reuse the same string twice */
+ base_sz = strlen(dbdir) - (strlen(pkg) + 1);
+ /* and use a creative for loop to prevent a need for copy/paste :) */
+ for (dbdir[base_sz] = '\0'; dirsmade != 2; dirsmade++) {
+ if (stat(dbdir, &s)) {
+ debug(dbg_conffdetail, "conff_ensure_db: creating %s\n",
+      dbdir);
+ if (errno != ENOENT)
+ ohshite("conff_ensure_db: stat(%s)", dbdir);
+ if(mkdir(dbdir, S_IRWXU))
+ ohshite("conff_ensure_db: mkdir(%s)", dbdir);
+ }
+ dbdir[base_sz] = '/';
+ }
+
+ free(dbdir);
+}
+
+void
+conff_reg_fd(const char *pkg, const char *path, int fd, size_t sz)
+{
+ int cfgfd;
+ /* get the path to where the registered file goes */
+ char *p = conff_db_path(pkg, path, conff_db_new);
+ char fnamebuf[256];
+
+ conff_ensure_db(pkg, conff_db_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,256,p));
+ if (close(cfgfd))
+ ohshite("can't close %s", p);
+
+ free(p);
+}
+
+int
+conff_get_fd(const char *pkg, const char *path, conff_flag f)
+{
+ int fd = -1;
+ char *p = conff_db_path(pkg, path, f);
+
+ fd = open(p, O_RDONLY);
+ if (fd == -1)
+ ohshite("error opening conffile registered at %s", p);
+
+ free(p);
+ return fd;
+}
+
+void
+conff_commit_new(const char *pkg)
+{
+ /* update the conffiles "db" dir.  this consists of the following steps:
+ * - nuke the existing current db dir
+ * - move the new db into place
+ */
+ char *cfgdb = NULL, *cfgdbnew = NULL;
+ debug(dbg_conff, "conff_commit_new(%s)", pkg);
+
+ cfgdb = conff_db_path(pkg, NULL, conff_db_cur);
+ cfgdbnew = conff_db_path(pkg, NULL, conff_db_new);
+
+ /* make sure <admindir>/<conffdb/>/<pkg> exists, and then nuke <pkg> */
+ conff_ensure_db(pkg, conff_db_cur);
+ ensure_pathname_nonexisting(cfgdb);
+ if (rename(cfgdbnew, cfgdb))
+ ohshite("conff_commit_new: rename(%s,%s)", cfgdbnew, cfgdb);
+
+ free(cfgdb);
+ free(cfgdbnew);
+}
+
+void
+conff_db_remove(const char *pkg, conff_flag f)
+{
+ char *cfgdb = conff_db_path(pkg, NULL, f);
+
+ debug(dbg_conff, "conff_db_remove(%s): removing %s\n", pkg, cfgdb);
+ ensure_pathname_nonexisting(cfgdb);
+
+ free(cfgdb);
+}
+
+/*
+ * vim: noet ts=8 sw=8
+ */
diff --git a/src/conffiles.h b/src/conffiles.h
new file mode 100644
index 0000000..9cf6e5b
--- /dev/null
+++ b/src/conffiles.h
@@ -0,0 +1,50 @@
+/*
+ * conffiles.h
+ *
+ * module for registering, deregistering, and otherwise handling conffiles
+ *
+ */
+#ifndef CONFFILES_H
+#define CONFFILES_H
+
+#include <sys/types.h>
+
+/* conff_flag is used in various conffile db functions to determine whether
+ * to reference the installed package's conffile db or the to-be-installed
+ * package's conffile db
+ */
+typedef enum {
+ conff_db_cur=0x1,
+ conff_db_new=0x2,
+} conff_flag;
+
+/* register a new conffile for package pkg at path... um... path, reading
+ * the contents of the distributed conffile from fd, which is expected to
+ * be sz bytes
+ *
+ * after calling this function it can be expected that the conffile is
+ * "registered" as a new conffile, which will be later processed in the
+ * package configuration stage, where it will be checked for stuff like
+ * new/changed/obsoleted/etc.
+ */
+void conff_reg_fd(const char *pkg, const char *path, int fd, size_t sz);
+
+/* return an fd opened for reading the contents of the "registered" conffile
+ * which is expected to be installed at path... path from package pkg.  the
+ * flag f is used to determine which version of the conffile is requested
+ * (i.e. old/new/current)
+ */
+int conff_get_fd(const char *pkg, const char *path, conff_flag f);
+
+/* remove the on-disk conffile database for package pkg, using the flag f
+ * to determine which version of the database to remove (new/cur/old)
+ */
+void conff_db_remove(const char *pkg, conff_flag f);
+
+/* update the conffiles db so that the latest "new" files are now recorded
+ * as "current" for package pkg.
+ */
+void conff_commit_new(const char *pkg);
+
+#endif /* CONFFILES_H */
+
--
1.6.4.3


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

[sean finney]

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

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

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

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

diff --git a/src/archives.c b/src/archives.c
index d2a6da4..1175994 100644
--- a/src/archives.c
+++ b/src/archives.c
@@ -58,6 +58,7 @@ static security_context_t scontext    = NULL;
 #include "filesdb.h"
 #include "main.h"
 #include "archives.h"
+#include "conffiles.h"
 
 #define MAXCONFLICTORS 20
 
@@ -352,7 +353,7 @@ int tarobject(struct TarInfo *ti) {
 
   struct conffile *conff;
   struct tarcontext *tc= (struct tarcontext*)ti->UserData;
-  int statr, i, existingdirectory, keepexisting;
+  int statr, i, existingdirectory, keepexisting, copyfd;
   ssize_t r;
   struct stat stab, stabtmp;
   char databuf[TARBLKSZ];
@@ -615,10 +616,19 @@ int tarobject(struct TarInfo *ti) {
   }
 #endif /* WITH_SELINUX */
 
-
+  /* in the case of a conffile we may want to extract the file somewhere
+   * else first, thus we use a "middleman" variable to keep the code clean
+   * and generalizable
+   */
+  copyfd = tc->backendpipe;
   /* Extract whatever it is as .dpkg-new ... */
   switch (ti->Type) {
   case NormalFile0: case NormalFile1:
+    /* if the file is a conffile... */
+    if (nifd->namenode->flags & fnnf_new_conff) {
+      conff_reg_fd(tc->pkg->name, fnamevb.buf, copyfd, ti->Size);
+      copyfd = conff_get_fd(tc->pkg->name, fnamevb.buf, conff_db_new);
+    }
     /* We create the file with mode 0 to make sure nobody can do anything with
      * it until we apply the proper mode, which might be a statoverride.
      */
@@ -629,7 +639,7 @@ int tarobject(struct TarInfo *ti) {
     debug(dbg_eachfiledetail,"tarobject NormalFile[01] open size=%lu",
           (unsigned long)ti->Size);
     { char fnamebuf[256];
-    fd_fd_copy(tc->backendpipe, fd, ti->Size, _("backend dpkg-deb during `%.255s'"),path_quote_filename(fnamebuf,256,ti->Name));
+    fd_fd_copy(copyfd, fd, ti->Size, _("backend dpkg-deb during `%.255s'"),path_quote_filename(fnamebuf,256,ti->Name));
     }
     r= ti->Size % TARBLKSZ;
     if (r > 0) r= safe_read(tc->backendpipe,databuf,TARBLKSZ - r);
diff --git a/src/configure.c b/src/configure.c
index e195362..8b7b424 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -50,6 +50,7 @@
 #include <dpkg/file.h>
 
 #include "filesdb.h"
+#include "conffiles.h"
 #include "main.h"
 
 static int conffoptcells[2][2] = {
@@ -329,6 +330,9 @@ deferred_configure(struct pkginfo *pkg)
  for (conff = pkg->installed.conffiles; conff; conff = conff->next)
  deferred_configure_conffile(pkg, conff);
 
+ /* commit the conffiles database for the new version of this package */
+ conff_commit_new(pkg->name);
+
  pkg->status = stat_halfconfigured;
  }
 
diff --git a/src/processarc.c b/src/processarc.c
index 4d2f7fe..837da7c 100644
--- a/src/processarc.c
+++ b/src/processarc.c
@@ -48,6 +48,7 @@
 #include "filesdb.h"
 #include "main.h"
 #include "archives.h"
+#include "conffiles.h"
 
 void process_archive(const char *filename) {
   static const struct TarFunctions tf = {
@@ -312,6 +313,9 @@ void process_archive(const char *filename) {
    * OK, we're going ahead.
    */
 
+  /* make sure the new conffiles dir does not exist at this point */
+  conff_db_remove(pkg->name, conff_db_new);
+
   trig_activate_packageprocessing(pkg);
   strcpy(cidirrest, TRIGGERSCIFILE);
   trig_parse_ci(cidir, NULL, trig_cicb_statuschange_activate, pkg);
diff --git a/src/remove.c b/src/remove.c
index 44ff139..62239d3 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -42,6 +42,7 @@
 #include <dpkg/myopt.h>
 
 #include "filesdb.h"
+#include "conffiles.h"
 #include "main.h"
 
 static void checkforremoval(struct pkginfo *pkgtoremove,
@@ -490,6 +491,8 @@ static void removal_bulk_remove_configfiles(struct pkginfo *pkg) {
       pop_cleanup(ehflag_normaltidy); /* closedir */
     
     }
+    /* remove the current conffile db for this package */
+    conff_db_remove(pkg->name, conff_db_cur);
     
     pkg->installed.conffiles = NULL;
     modstatdb_note(pkg);
--
1.6.4.3


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

[sean finney]

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

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

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

the merge output is stored into a temporary file (<file>.dpkg-merge).
on 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/conffiles.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/conffiles.h |    5 ++++
 src/configure.c |   14 ++++++++++++-
 4 files changed, 78 insertions(+), 1 deletions(-)

diff --git a/lib/dpkg/dpkg.h b/lib/dpkg/dpkg.h
index 335a826..9205b24 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/conffiles.c b/src/conffiles.c
index fe426d7..1a57ae4 100644
--- a/src/conffiles.c
+++ b/src/conffiles.c
@@ -8,11 +8,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"
 
@@ -173,6 +175,62 @@ conff_db_remove(const char *pkg, conff_flag f)
  free(cfgdb);
 }
 
+int conff_automerge(const char *pkg, const char *pathname)
+{
+ int res, merge_fd;
+ size_t cmd_sz = 0, merge_output_fname_sz = 0;
+ char *cmdbuf = NULL, *cnfold = NULL, *cnfnew = NULL;
+ char *merge_output_fname;
+
+ /* i.e. <path> + ".dpkg-merge" + '\0' */
+ merge_output_fname_sz = strlen(pathname) + strlen(DPKGMERGEEXT) + 1;
+ merge_output_fname = m_malloc(merge_output_fname_sz);
+ sprintf(merge_output_fname, "%s%s", pathname, DPKGMERGEEXT);
+
+ /* the 'old' version is actually the current dist version in the db */
+ cnfold = conff_db_path(pkg, pathname, conff_db_cur);
+ cnfnew = conff_db_path(pkg, pathname, conff_db_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("conff_automerge: can not open %s for merge output",
+        merge_output_fname);
+ else if (close(merge_fd))
+ ohshite("conff_automerge: close failed");
+
+ /* copy file permissions from the original file */
+ file_copyfileperm(pathname, merge_output_fname);
+
+ /* DIFF3 + " -m " + new + " " + old + " " + cur + " > " + out + '\0' */
+ cmd_sz = strlen(DIFF3) + 4 + strlen(cnfnew) + 1 + strlen(cnfold) + 1 +
+         strlen(pathname) + 3 + merge_output_fname_sz + 1;
+ cmdbuf = m_malloc(cmd_sz);
+ snprintf(cmdbuf, cmd_sz, "%s -m %s %s %s > %s", DIFF3, cnfnew, cnfold,
+         pathname, 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, pathname) == -1)
+ ohshite("conff_automerge: can not rename %s",
+        merge_output_fname);
+ } else {
+ /* oh noes, merge failed :( */
+ if (unlink(merge_output_fname))
+ ohshite("conff_automerge: can not unlink %s",
+        merge_output_fname);
+ }
+
+ free(merge_output_fname);
+ free(cmdbuf);
+ free(cnfnew);
+ free(cnfold);
+
+ return res;
+}
+
 /*
  * vim: noet ts=8 sw=8
  */
diff --git a/src/conffiles.h b/src/conffiles.h
index 9cf6e5b..1e76a72 100644
--- a/src/conffiles.h
+++ b/src/conffiles.h
@@ -46,5 +46,10 @@ void conff_db_remove(const char *pkg, conff_flag f);
  */
 void conff_commit_new(const char *pkg);
 
+/* attempt to automagically merge the 3-way delta for conffile at pathname
+ * passes the return value of diff3, aka nonzero on merge failure
+ */
+int conff_automerge(const char *pkg, const char *pathname);
+
 #endif /* CONFFILES_H */
 
diff --git a/src/configure.c b/src/configure.c
index 8b7b424..8393e7a 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -636,6 +636,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)
@@ -646,7 +647,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]") :
@@ -680,6 +681,17 @@ 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( conff_automerge(pkg, cfgfile) == 0){
+ fprintf(stderr, _("success.\n"));
+ /* so let's just pretend they said "keep my version" ;) */
+ cc = 'n';
+ } else {
+ fprintf(stderr, _("failed.\n"));
+ }
+ }
+
  if (cc == 'z')
  suspend();
  } while (!strchr("yino", cc));
--
1.6.4.3


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

[Raphaël Hertzog]

Hi,

very small comments:

On Mon, 28 Sep 2009, Sean Finney wrote:
> + ohshite(_("unable to stat installed file `%.250s'"), source);
> + ohshite(_("unable to change ownership of target file`%.250s'"),
> +        target);
> + ohshite(_("unable to set mode of target file`%.250s'"), target);

We move away from the `' quotes when we have to change strings, we use the
usual ones (''). You lack a space before the quotes.

Cheers,
--
Raphaël Hertzog


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

[Raphaël Hertzog]

On Mon, 28 Sep 2009, Sean Finney wrote:

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

Hum, instead of the "the pristine version of the conffile
shipped in the currently installed package", in theory it could also be the
ancestor of the currently installed conffile — that is the pristine version
of the conffile that last got installed (automatically or by a Y answer to
the prompt or by a successful merge).

Shouldn't that file be kept around as well and used in priority for the
merge? (In a conffiles-base directory for example)

P: pristine conffile of currently installed package
N: conffile of the new package to be installed
I: installed conffile
B: base conffile (last version successfully installed)

We could then try:
- diff(P → N) on top of I
- or diff(B → I) on top of N

That way a conffile upgrade skipped due to installations with
--force-confold can be recovered and the changes merged at the next
interactive upgrade.

We also want a --force-confmerge option to try out a merge
non-interactively before falling back to --force-confold/new.

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

I would like to be able to review successful merges as well.

Cheers,
--
Raphaël Hertzog


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

[sean finney]

hey raphael,

On Tue, Sep 29, 2009 at 08:55:56AM +0200, Raphael Hertzog 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).
>
> Hum, instead of the "the pristine version of the conffile
> shipped in the currently installed package", in theory it could also be the
> ancestor of the currently installed conffile — that is the pristine version
> of the conffile that last got installed (automatically or by a Y answer to
> the prompt or by a successful merge).

if i understand you correctly:

- pristine packaged conffile stored during unpack
- successfully installed conffiles (Y or post-merge) stored during or
  after configuration
- during conflict resolution, multiple merge paths are tried (or offered?)

is that correct?
 
> Shouldn't that file be kept around as well and used in priority for the
> merge? (In a conffiles-base directory for example)

i suppose it could be useful for a "show me what's changed since the
last conffile conflict/resolution", and cases where the conffile was
further modified after a successful resolution.

it does complicate things a bit though--not so much in implementation
which is fairly straightforward, but more with the installation process
(see below wrt the merge reviewing)

> P: pristine conffile of currently installed package
> N: conffile of the new package to be installed
> I: installed conffile
> B: base conffile (last version successfully installed)
>
> We could then try:
> - diff(P → N) on top of I
> - or diff(B → I) on top of N

would you suggest that we do the latter first?  should they be seperate
options or something tried in succession as part of a single merge option?

> That way a conffile upgrade skipped due to installations with
> --force-confold can be recovered and the changes merged at the next
> interactive upgrade.

i could be missing something since i'm only passingly familiar with
a few codepaths in dpkg, but i think that we still get the conffiles
into the conffile db even when --force-confold is passed (since the
hook into this is pretty early, during unpack)  i'll see if i can
verify this tonight.

> We also want a --force-confmerge option to try out a merge
> non-interactively before falling back to --force-confold/new.

yeah i think that'd be a nice idea.

> > 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.
>
> I would like to be able to review successful merges as well.

yeah.  the only problem is that it adds an extra step to the installation
process (i.e. an extra prompt) so i figured that this (as well as stuff
like the new suggestions above) should be hammered out in discussion for
exactly how it should work, prompt strings, etc, and also seperate from
the initial implementation.
 
On Tue, Sep 29, 2009 at 08:19:48AM +0200, Raphael Hertzog wrote:
> On Mon, 28 Sep 2009, Sean Finney wrote:
> > + ohshite(_("unable to stat installed file `%.250s'"), source);
> > + ohshite(_("unable to change ownership of target file`%.250s'"),
> > +        target);
> > + ohshite(_("unable to set mode of target file`%.250s'"), target);
>
> We move away from the `' quotes when we have to change strings, we use the
> usual ones (''). You lack a space before the quotes.

okay.  i think i copied those verbatim, to avoid fuzzying strings, but
then ended up having to change them anyway since they didn't make sense
in a general context.  i'll fix them up in the next version.

btw, this patch series is sitting at the top of the following location:

        ssh://git.debian.org/git/users/seanius/dpkg.git
        http://git.debian.org/git/users/seanius/dpkg.git

        branch: seanius-conffiledb-20090928

and any future revisions i'll put in a similarly timestamped (and rebased)
branch to keep things clean for further review.


        sean

--

[Raphaël Hertzog]

On Tue, 29 Sep 2009, sean finney wrote:
> if i understand you correctly:
>
> - pristine packaged conffile stored during unpack
> - successfully installed conffiles (Y or post-merge) stored during or
>   after configuration
> - during conflict resolution, multiple merge paths are tried (or offered?)
>
> is that correct?

Yes. Not sure what the proper UI is. Either the choice of the merge path is
automatic based on what works and what not, or we can let the user select
it by offering two types of merges.

> > Shouldn't that file be kept around as well and used in priority for the
> > merge? (In a conffiles-base directory for example)
>
> i suppose it could be useful for a "show me what's changed since the
> last conffile conflict/resolution", and cases where the conffile was
> further modified after a successful resolution.
>
> it does complicate things a bit though--not so much in implementation
> which is fairly straightforward, but more with the installation process
> (see below wrt the merge reviewing)

Indeed.

>
> > P: pristine conffile of currently installed package
> > N: conffile of the new package to be installed
> > I: installed conffile
> > B: base conffile (last version successfully installed)
> >
> > We could then try:
> > - diff(P → N) on top of I
> > - or diff(B → I) on top of N
>
> would you suggest that we do the latter first?  should they be seperate
> options or something tried in succession as part of a single merge option?

I don't have a definitive answer. Both are useful depending on the
scenario imagined.

I would suggest to do the latter first, and rewrite the "No" choices in
two variants:
- No, but keep changes for later merge
- No, skip the changes introduced by this version

The second one would update the base file while the first one would not.

> > That way a conffile upgrade skipped due to installations with
> > --force-confold can be recovered and the changes merged at the next
> > interactive upgrade.
>
> i could be missing something since i'm only passingly familiar with
> a few codepaths in dpkg, but i think that we still get the conffiles
> into the conffile db even when --force-confold is passed (since the
> hook into this is pretty early, during unpack)  i'll see if i can
> verify this tonight.

You get the new conffile in the DB yes, but if the installed conffile has
not been updated (due to --force-conffold), then you have no way to
reintroduce the changes that were not applied if you haven't kept around
the base file.

Imagine a conffile that is modified two times (CF1 → CF2 → CF3). If you
install v2 with --force-conffold you get CF1 installed. When you install
v3 interactively you have no chance to catch up and include the changes
from CF1 → CF3, you will only be offered to merge the changes CF2 → CF3.

If we keep a base file around, we know that CF1 is the last version
installed/merge and we can include both changes too.

So in fact we have 3 ways to do the merge:
- diff(P → N) on top of I
- diff(B → N) on top of I
- diff(B → I) on top of N

Urgh.

> > We also want a --force-confmerge option to try out a merge
> > non-interactively before falling back to --force-confold/new.
>
> yeah i think that'd be a nice idea.

(Reading myself saying "we want" I realize I'm a bit too strong in my
wording, at least it meant that if I were working on this I would also
implement this for consistency with the existing options)

> yeah.  the only problem is that it adds an extra step to the installation
> process (i.e. an extra prompt) so i figured that this (as well as stuff
> like the new suggestions above) should be hammered out in discussion for
> exactly how it should work, prompt strings, etc, and also seperate from
> the initial implementation.

Sure.

> ssh://git.debian.org/git/users/seanius/dpkg.git
> http://git.debian.org/git/users/seanius/dpkg.git

I prefer this one:
git://git.debian.org/~seanius/dpkg.git

Shorter and using the optimized git server.

Cheers,
--
Raphaël Hertzog


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

[Goswin von Brederlow-2]

Sean Finney <seanius@...> writes:

> the following series of patches is a renewed attempt at conffile tracking,
> where the packaged conffiles are stored in a seperate location on the
> system in their pristine forms.  the patches the follow contain more
> information on the details.

Hi,

in a recent discussion on irc we pondered the possibility of
maintaining /etc in an RCS (git in that case). Two ideas then came up:

1) commit changes per package

Every time a package is installed or removed it would be nice to
commit those changes automatically. Idealy on a per package basis but
we recognised that sometimes dpkg needs to install packages in groups.

2) merging using the RCS capabilities

Using a normal RCS workflow one would keep the original conffiles in
an upstream branch and merge that with the working directory / local
branch. To make this feasable and usefull it would be nice if the dpkg
conffile handling could be made aware of this so it unpacks the
conffiles into an upstream branch and merges instead of the current
way.


Both could be made to work if dpkg had hooks at the right places. It
could even be made possible for users to replace the conffile tracking
in this patch series with git or mercurial or other RCS of their
choice. Maybe there should be an API for conffile tracking and merging
and different RCSes could provide wrappers for the API while dpkg
provides a default.

What do you think?

MfG
        Goswin

PS: ucf would have to plug into theat API too.


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

[sean finney]

hi,

On Mon, Oct 05, 2009 at 01:32:49PM +0200, Goswin von Brederlow wrote:
> in a recent discussion on irc we pondered the possibility of
> maintaining /etc in an RCS (git in that case). Two ideas then came up:

i think there's a few solutions that come close to this already (etcgit
being one off the top of my head).  i think that keeping it closer to
the package manager would be better though, so use of aptitude/synaptic
wouldn't result in missing some commits.

> 1) commit changes per package
>
> Every time a package is installed or removed it would be nice to
> commit those changes automatically. Idealy on a per package basis but
> we recognised that sometimes dpkg needs to install packages in groups.

it wouldn't be too hard to expose some kind of per-package hook for
the first use case, and through use of triggers it would be possible
to catch the second use case.  this would allow for a commit+tag on a
per-package basis and a second tag for batches of changes.

> 2) merging using the RCS capabilities
>
> Using a normal RCS workflow one would keep the original conffiles in
> an upstream branch and merge that with the working directory / local
> branch. To make this feasable and usefull it would be nice if the dpkg
> conffile handling could be made aware of this so it unpacks the
> conffiles into an upstream branch and merges instead of the current
> way.

this sounds a bit scary imo, because it travels pretty far out from the
safe confines of dpkg and into a fairly complex and relatively unstable
system.

> Both could be made to work if dpkg had hooks at the right places. It
> could even be made possible for users to replace the conffile tracking
> in this patch series with git or mercurial or other RCS of their
> choice. Maybe there should be an API for conffile tracking and merging
> and different RCSes could provide wrappers for the API while dpkg
> provides a default.

i'd prefer to leave any such implementation as a follow-up to this initial
effort, as the current code is relatively simple and streamlined and in
no way closes the door for future work.


        sean

--

[Guillem Jover]

Hi!

I've pushed this with few minor modifications. I thought it would be
less work for both of you, and it was just nit picking stuff.
Commmenting anyway for reference.

On Mon, 2009-09-28 at 23:34:28 +0200, Sean Finney wrote:
> From: Sean Finney <seanius@...>
>
> this function will be useful for other parts of dpkg, so the function
> has been moved to a more sensible location, the static qualifier removed,
> and its name appropriately prefixed.

I've added the subsystem to the short description, and capitalized the
sentences.

> ---
>  lib/dpkg/path.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/dpkg/path.h |    1 +
>  src/archives.c  |   63 ++----------------------------------------------------
>  3 files changed, 63 insertions(+), 60 deletions(-)
>
> diff --git a/lib/dpkg/path.c b/lib/dpkg/path.c
> index 8ea891e..ad14a45 100644
> --- a/lib/dpkg/path.c
> +++ b/lib/dpkg/path.c
> @@ -54,3 +55,61 @@ path_skip_slash_dotslash(const char *path)
>   return path;
>  }
>  
> +/* snprintf(3) doesn't work if format contains %.<nnn>s and an argument has
> + * invalid char for locale, then it returns -1.
> + * ohshite() is ok, but fd_fd_copy(), which is used in tarobject() in this
> + * file, is not ok, because
> + * - fd_fd_copy() == buffer_copy_setup() [include/dpkg.h]
> + * - buffer_copy_setup() uses varbufvprintf(&v, desc, al); [lib/mlib.c]
> + * - varbufvprintf() fails and memory exausted, because it call
> + *    fmt = "backend dpkg-deb during `%.255s'
> + *    arg may contain some invalid char, for example,
> + *    /usr/share/doc/console-tools/examples/unicode/\342\231\252\342\231\254
> + *   in console-tools.
> + *   In this case, if user uses some locale which doesn't support \342\231...,
> + *   vsnprintf() always returns -1 and varbufextend() get called again
> + *   and again until memory is exausted and it aborts.
> + *
> + * So, we need to escape invalid char, probably as in
> + * tar-1.13.19/lib/quotearg.c: quotearg_buffer_restyled()
> + * but here I escape all 8bit chars, in order to be simple.
> + * - ukai@...
> + */

Reformatted slightly the comment.

> diff --git a/src/archives.c b/src/archives.c
> index f9f95a8..d2a6da4 100644
> --- a/src/archives.c
> +++ b/src/archives.c
> @@ -47,6 +47,7 @@
>  #include <dpkg/subproc.h>
>  #include <dpkg/tarfn.h>
>  #include <dpkg/myopt.h>
> +#include <dpkg/path.h>

Placed this one just before <buffer.h>, sorted by complexity.

>  #ifdef WITH_SELINUX
>  #include <selinux/selinux.h>

> @@ -255,7 +198,7 @@ tarfile_skip_one_forward(struct TarInfo *ti,
>  
>      fd_null_copy(tc->backendpipe, ti->Size,
>                   _("skipped unpacking file '%.255s' (replaced or excluded?)"),
> -                 quote_filename(fnamebuf, 256, ti->Name));
> +                 path_quote_filename(fnamebuf, 256, ti->Name));
>      r = ti->Size % TARBLKSZ;
>      if (r > 0)
>        r = safe_read(tc->backendpipe, databuf, TARBLKSZ - r);
> @@ -686,7 +629,7 @@ int tarobject(struct TarInfo *ti) {
>      debug(dbg_eachfiledetail,"tarobject NormalFile[01] open size=%lu",
>            (unsigned long)ti->Size);
>      { char fnamebuf[256];
> -    fd_fd_copy(tc->backendpipe, fd, ti->Size, _("backend dpkg-deb during `%.255s'"),quote_filename(fnamebuf,256,ti->Name));
> +    fd_fd_copy(tc->backendpipe, fd, ti->Size, _("backend dpkg-deb during `%.255s'"),path_quote_filename(fnamebuf,256,ti->Name));

And wrapped this one and added the needed spaces after comma.

>      }
>      r= ti->Size % TARBLKSZ;
>      if (r > 0) r= safe_read(tc->backendpipe,databuf,TARBLKSZ - r);

thanks,
guillem


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

[Guillem Jover]

Hi!

Same for this one, comments on the minor modifications follow.

On Mon, 2009-09-28 at 23:34:29 +0200, Sean Finney wrote:
> this functionality is also needed by the conffile handling code to ensure
> that the merge output is stored in a file with the same permissions as
> the original conffile, preventing the accidental oppurtunity for
> unintended information disclosure.
>
> therefore the function is moved into a new library module (file.{c,h}),
> and given an appropriate prefix.  note that some of the translatable error
> messages have been modified as they would otherwise be misleading.

Capitalizaion. One space after dot.

> ---
>  lib/dpkg/Makefile.am |    1 +
>  lib/dpkg/file.c      |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/dpkg/file.h      |   35 +++++++++++++++++++++++++++++
>  src/configure.c      |   28 +----------------------
>  4 files changed, 97 insertions(+), 26 deletions(-)
>  create mode 100644 lib/dpkg/file.c
>  create mode 100644 lib/dpkg/file.h

> diff --git a/lib/dpkg/file.c b/lib/dpkg/file.c
> new file mode 100644
> index 0000000..0408844
> --- /dev/null
> +++ b/lib/dpkg/file.c
> @@ -0,0 +1,59 @@
[...]

> +#include <config.h>
> +#include <compat.h>
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +#include <errno.h>
> +
> +#include <dpkg/file.h>
> +#include <dpkg/dpkg.h>
> +#include <dpkg/i18n.h>

Sorted a bit the includes.

> +/*
> + * Copy file ownership and permissions from one file to another.
> + */

Moved this comment to the header file. At some point we should start
properly documenting the functions and variables (at least for the
stuff exposed by libdpkg), with something like doxygen. I've an old
Doxyfile around for dpkg that might need to tunning before committing.

> +void
> +file_copyfileperm(const char *source, const char *target)
> +{
> + struct stat stab;
> +
> + if (stat(source, &stab) == -1) {
> + if (errno == ENOENT)
> + return;
> + ohshite(_("unable to stat installed file `%.250s'"), source);
> + }
> +
> + if (chown(target, stab.st_uid, stab.st_gid) == -1)
> + ohshite(_("unable to change ownership of target file`%.250s'"),
> +        target);
> +
> + if (chmod(target, (stab.st_mode & 07777)) == -1)
> + ohshite(_("unable to set mode of target file`%.250s'"), target);
> +}

Rename this to file_copyfileperm file_copy_perms, the former seems too
redundant. Also fixed the missing spaces, the old quotes and the strings a
bit.

> +/*
> + * vim: noet ts=8
> + */

Removed this one.

thanks,
guillem


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

[Goswin von Brederlow-2]

Sean Finney <seanius@...> writes:

> hi,
>
> On Mon, Oct 05, 2009 at 01:32:49PM +0200, Goswin von Brederlow wrote:
>> in a recent discussion on irc we pondered the possibility of
>> maintaining /etc in an RCS (git in that case). Two ideas then came up:
>
> i think there's a few solutions that come close to this already (etcgit
> being one off the top of my head).  i think that keeping it closer to
> the package manager would be better though, so use of aptitude/synaptic
> wouldn't result in missing some commits.

Yes. But the best they can do is do a commit per apt invokation,
which is usualy for a ton of packages at a time. Even if apt where
more fine grained it would be per dpkg invokation, still a lot of
packages.

One could probably try to detect which file belongs to which package
and commit them first, then commit anything not belonging to anyone.

Not optimal.

>> 1) commit changes per package
>>
>> Every time a package is installed or removed it would be nice to
>> commit those changes automatically. Idealy on a per package basis but
>> we recognised that sometimes dpkg needs to install packages in groups.
>
> it wouldn't be too hard to expose some kind of per-package hook for
> the first use case, and through use of triggers it would be possible
> to catch the second use case.  this would allow for a commit+tag on a
> per-package basis and a second tag for batches of changes.

The hook could be triggered for a set of packages and as frequently as
possible. The packages and old/new version could be given as command
line arg or stdin. Maybe even a file list on stdin in the form "<pkg>
<file>\000"?

If 2 packages must be installed together then commiting the changes
together isn't the end of the world. It should just be more frequently
than a "apt-get dist-upgrade" call.

>> 2) merging using the RCS capabilities
>>
>> Using a normal RCS workflow one would keep the original conffiles in
>> an upstream branch and merge that with the working directory / local
>> branch. To make this feasable and usefull it would be nice if the dpkg
>> conffile handling could be made aware of this so it unpacks the
>> conffiles into an upstream branch and merges instead of the current
>> way.
>
> this sounds a bit scary imo, because it travels pretty far out from the
> safe confines of dpkg and into a fairly complex and relatively unstable
> system.

The simplest implementation could just add another option to the dpkg
menu to call an external hook. The hook would get the current, new
orig, old orig files as argument and do its magic. Alternatively just
the current filename and the dpkg-conffile interface to retrieve
new/old orig itself. It could be moving the new orig into an upstream
branch and merge or it could call vimdiff or whatever the user chooses
to use as hook.

The important part would be to define a stable API, give access to the
new/old orig conffile and current file and define return codes for all
ok (0), back to the menu (100) and error (any other) or something.

Doesn't need to be a complicated implementation. And use at your own
risk. :)

>> Both could be made to work if dpkg had hooks at the right places. It
>> could even be made possible for users to replace the conffile tracking
>> in this patch series with git or mercurial or other RCS of their
>> choice. Maybe there should be an API for conffile tracking and merging
>> and different RCSes could provide wrappers for the API while dpkg
>> provides a default.
>
> i'd prefer to leave any such implementation as a follow-up to this initial
> effort, as the current code is relatively simple and streamlined and in
> no way closes the door for future work.
>
>
> sean

Fair enough. Just keep it in mind you don't design something that
later blocks it.

MfG
        Goswin

PS: thanks for the effort, this is wanted a lot


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

[Guillem Jover]

Hi!

On Tue, 2009-09-29 at 12:17:02 +0200, Raphael Hertzog wrote:
> On Tue, 29 Sep 2009, sean finney wrote:
> > > P: pristine conffile of currently installed package
> > > N: conffile of the new package to be installed
> > > I: installed conffile
> > > B: base conffile (last version successfully installed)

I'll call P “D: dist”, to try to match more closely the current
nomenclature. And I think B should be instead:

  B: base conffile (last dist conffile after skipping an upgrade)

> > > We could then try:
> > > - diff(P → N) on top of I
> > > - or diff(B → I) on top of N
> >
> > would you suggest that we do the latter first?  should they be seperate
> > options or something tried in succession as part of a single merge option?
>
> I don't have a definitive answer. Both are useful depending on the
> scenario imagined.
>
> I would suggest to do the latter first, and rewrite the "No" choices in
> two variants:
> - No, but keep changes for later merge
> - No, skip the changes introduced by this version
>
> The second one would update the base file while the first one would not.

The base conffile only needs to be present when the installed conffile
has been modifed and it has been kept for more than one new conffile
upgrade.

So if it's present it would be used for the merge, something like:

  if (B exists)
    diff3 I B N
  else
    diff3 I D N

And cannot see why we'd want to fallback to use dist if the merge with
base does not work? dist is not going to be closer to inst if base is
present.

Once the changes relative to base have been merged into new, there's
no point in keeping it any longer, as the new base is now new, which
will become dist.

So I think this could be hooked in deferred_configure_conffile(), if
there's no base conffile, store one when “cfo_keep | cfof_backup”, as
this is the case were the new and inst conffiles have been changed,
and the user has been “prompted” and it's been decided to keep it.

And remove it when “cfo_install | cfof_backup”, as this is also the
case were the new and inst conffile have changed, and the user has
been “prompted”, and it's been decided to install the new version,
which implies either ignoring or taking the merged changes.

Something to consider is that currently on this later case, if the
user selected to ignore the local changes, a backup of the previous
modified conffile gets stotered as foo.dpkg-old, so we could decide to
store the base and the old inst conffiles in case the user wants to
recover those in the future. And not doing so in case of merge, either
automatic, or one done through the shell (which could be detected when
the inst candidate gets modified).

Does this sound right?

regards,
guillem


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