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

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

Split up the rename(<conffile>.dpkg-new, <conffile>) into

link(<conffile>.dpkg-new, <conffile>);
unlink(<conffile>.dpkg-new);

in preparation for an operation in between. Since link() does
not actually atomically replace its target, we must simulate it
by linking to a temporary file and then renaming into place.

Signed-off-by: Jonathan Nieder <jrnieder.RemoveThis@gmail.com>
---
lib/dpkg/dpkg.h | 1 +
src/configure.c | 24 +++++++++++++++++++-----
src/remove.c | 7 ++++++-
3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/lib/dpkg/dpkg.h b/lib/dpkg/dpkg.h
index 78a2fe6..97b1e0b 100644
--- a/lib/dpkg/dpkg.h
+++ b/lib/dpkg/dpkg.h
@@ -57,6 +57,7 @@ DPKG_BEGIN_DECLS
#define DPKGNEWEXT ".dpkg-new"
#define DPKGOLDEXT ".dpkg-old"
#define DPKGDISTEXT ".dpkg-dist"
+#define DPKGLINKEXT ".dpkg-inst"

#define CONTROLFILE "control"
#define CONFFILESFILE "conffiles"
diff --git a/src/configure.c b/src/configure.c
index 2764729..29878e4 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -74,10 +74,11 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
enum conffopt what;
struct stat stab;
struct varbuf cdr = VARBUF_INIT, cdr2;
- const int max_extension_sz = max(
- max(sizeof(DPKGNEWEXT),
- sizeof(DPKGOLDEXT)),
- sizeof(DPKGDISTEXT));
+ const int max_extension_sz = max(max(max(
+ sizeof(DPKGNEWEXT),
+ sizeof(DPKGOLDEXT)),
+ sizeof(DPKGDISTEXT)),
+ sizeof(DPKGLINKEXT));
char *cdrrest, *cdr2rest;
int r;

@@ -205,11 +206,24 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
printf(_("Installing new version of config file %s ...\n"),
usenode->name);
case cfo_newconff:
- strcpy(cdr2rest, DPKGNEWEXT);
+ strcpy(cdrrest, DPKGNEWEXT);
+ strcpy(cdr2rest, DPKGLINKEXT);
+ if (unlink(cdr2.buf) && errno != ENOENT)
+ warning(_("%s: failed to remove '%.250s' "
+ "(before overwrite): %s"),
+ pkg->name, cdr2.buf, strerror(errno));
+ if (link(cdr.buf, cdr2.buf))
+ ohshite(_("failed to link '%.250s' to '%.250s'"),
+ cdr.buf, cdr2.buf);
trig_file_activate(usenode, pkg);
+ strcpy(cdrrest, "");
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);
diff --git a/src/remove.c b/src/remove.c
index 5e44a3b..59f4128 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -219,9 +219,14 @@ static void removal_bulk_remove_files(
varbufaddstr(&fnvb,DPKGTEMPEXT);
varbufaddc(&fnvb,0);
debug(dbg_eachfiledetail, "removal_bulk cleaning temp `%s'", fnvb.buf);
-
ensure_pathname_nonexisting(fnvb.buf);

+ fnvb.used = before;
+ varbufaddstr(&fnvb, DPKGLINKEXT);
+ varbufaddc(&fnvb, 0);
+ debug(dbg_eachfiledetail, "remove_bulk cleaning temp `%s'", fnvb.buf);
+ ensure_pathname_nonexisting(fnvb.buf);
+
fnvb.used= before;
varbufaddstr(&fnvb,DPKGNEWEXT);
varbufaddc(&fnvb,0);
--
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

When backing up the .dpkg-new file, make a hardlink backup and
then fall through to the "deleting .dpkg-new because it is not to
be used" case. This is messier than a true rename, but it
creates some flexibility by allowing us to perform some
operations between the two steps in the future.

Signed-off-by: Jonathan Nieder <jrnieder.TakeThisOut@gmail.com>
---
src/configure.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/configure.c b/src/configure.c
index 6c9b7da..2764729 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -170,12 +170,17 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
warning(_("%s: failed to remove old backup '%.250s': %s"),
pkg->name, cdr2.buf, strerror(errno));
strcpy(cdrrest, DPKGDISTEXT);
+ if (unlink(cdr.buf) && errno != ENOENT)
+ warning(_("%s: failed to remove '%.250s' "
+ "(before overwrite): %s"),
+ pkg->name, cdr.buf, strerror(errno));
trig_file_activate(usenode, pkg);
strcpy(cdr2rest, DPKGNEWEXT);
- if (rename(cdr2.buf, cdr.buf))
- warning(_("%s: failed to rename '%.250s' to '%.250s': %s"),
+ if (link(cdr2.buf, cdr.buf))
+ warning(_("%s: failed to link '%.250s' "
+ "to '%.250s': %s"),
pkg->name, cdr2.buf, cdr.buf, strerror(errno));
- break;
+ /* Fall through. */
case cfo_keep:
strcpy(cdr2rest, DPKGNEWEXT);
if (unlink(cdr2.buf))
--
1.6.5.2

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

Free filename buffers when returning early from
deferred_configure_conffile().

While we're at it, speed up the initialization of the buffer
cdr2. The current code scans cdr for its end unnecessarily,
twice, and then uses that length to calculate a buffer size about
40 bytes too large.

Also let the filename cdr acquire extensions in the same way as
cdr2. This should make it a little easier to change the
extension of cdr more often.

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

diff --git a/src/configure.c b/src/configure.c
index 3ae745b..6c9b7da 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -73,8 +73,12 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
int useredited, distedited;
enum conffopt what;
struct stat stab;
- struct varbuf cdr = VARBUF_INIT, cdr2 = VARBUF_INIT;
- char *cdr2rest;
+ struct varbuf cdr = VARBUF_INIT, cdr2;
+ const int max_extension_sz = max(
+ max(sizeof(DPKGNEWEXT),
+ sizeof(DPKGOLDEXT)),
+ sizeof(DPKGDISTEXT));
+ char *cdrrest, *cdr2rest;
int r;

usenode = namenodetouse(findnamenode(conff->name, fnn_nocopy), pkg);
@@ -82,23 +86,29 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
r = conffderef(pkg, &cdr, usenode->name);
if (r == -1) {
conff->hash = EMPTY_HASH;
+ varbuffree(&cdr);
return;
}
md5hash(pkg, currenthash, cdr.buf);

- varbufreset(&cdr2);
- varbufaddstr(&cdr2, cdr.buf);
- varbufaddc(&cdr2, 0);
- /* XXX: Make sure there's enough room for extensions. */
- varbuf_grow(&cdr2, 50);
- cdr2rest = cdr2.buf + strlen(cdr.buf);
+ /* Make sure there's enough room for extensions. */
+ varbuf_grow(&cdr, max_extension_sz - 1);
+ varbufinit(&cdr2, cdr.used);
+ cdr.used -= max_extension_sz - 1;
+
+ varbufaddbuf(&cdr2, cdr.buf, cdr.used);
+ cdrrest = cdr.buf + cdr.used - 1;
+ cdr2rest = cdr2.buf + cdr2.used - 1;
/* From now on we can just strcpy(cdr2rest, extension); */

- strcpy(cdr2rest, DPKGNEWEXT);
/* If the .dpkg-new file is no longer there, ignore this one. */
+ strcpy(cdr2rest, DPKGNEWEXT);
if (lstat(cdr2.buf, &stab)) {
- if (errno == ENOENT)
+ if (errno == ENOENT) {
+ varbuffree(&cdr2);
+ varbuffree(&cdr);
return;
+ }
ohshite(_("unable to stat new dist conffile `%.250s'"), cdr2.buf);
}
md5hash(pkg, newdisthash, cdr2.buf);
@@ -159,11 +169,9 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
if (unlink(cdr2.buf) && errno != ENOENT)
warning(_("%s: failed to remove old backup '%.250s': %s"),
pkg->name, cdr2.buf, strerror(errno));
- cdr.used--;
- varbufaddstr(&cdr, DPKGDISTEXT);
- varbufaddc(&cdr, 0);
- strcpy(cdr2rest, DPKGNEWEXT);
+ strcpy(cdrrest, DPKGDISTEXT);
trig_file_activate(usenode, pkg);
+ strcpy(cdr2rest, DPKGNEWEXT);
if (rename(cdr2.buf, cdr.buf))
warning(_("%s: failed to rename '%.250s' to '%.250s': %s"),
pkg->name, cdr2.buf, cdr.buf, strerror(errno));
--
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

There's a typo in this one:

Jonathan Nieder wrote:

> --- a/src/configure.c
> +++ b/src/configure.c
> @@ -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);
> }

The 'break;' should not have been removed.

In other words, the following paper-bag would be needed on top. Sorry
for the bogus code.

Nothing else like this seems to stand out among the remaining changes.
I did find what looks like a minor pre-existing bug. Patch to come.

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

diff --git a/src/configure.c b/src/configure.c
index e4fffc9..79f973b 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -216,6 +216,7 @@ 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);
+ break;
default:
internerr("unknown conffopt '%d'", what);
}
--
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

Currently, dpkg lets interested packages know when saving a
backup (.dpkg-dist file) for an updated conffile that is not
going to used.

This is, strictly speaking, harmless, since file triggers were
never a guarantee that a file was actually modified. But on one
hand, the triggered action can be an expensive no-op (such as
restarting a server process), and on the other hand, any package
that relies on triggers being activated in this case will break
once some user decides not to keep a .dpkg-dist file. So it
seems safer to avoid the unnecessary and misleading trigger.

Signed-off-by: Jonathan Nieder <jrnieder RemoveThis @gmail.com>
---
Jonathan Nieder wrote:

> Nothing else like this seems to stand out among the remaining changes.
> I did find what looks like a minor pre-existing bug. Patch to come.

On reflection, 'bug' is a little too strong a word. Smile

But this is an
improvement, imho.

Patch applies to master.

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

diff --git a/src/configure.c b/src/configure.c
index f5254bc..63b2e87 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -164,7 +164,6 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
varbufaddstr(&cdr, DPKGDISTEXT);
varbufaddc(&cdr, 0);
strcpy(cdr2rest, DPKGNEWEXT);
- trig_file_activate(usenode, pkg);
if (rename(cdr2.buf, cdr.buf))
warning(_("%s: failed to rename '%.250s' to '%.250s': %s"),
pkg->name, cdr2.buf, cdr.buf, strerror(errno));
--
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

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

Hi!

On Sat, 2009-11-07 at 17:06:09 -0600, Jonathan Nieder wrote:
> Currently, dpkg lets interested packages know when saving a
> backup (.dpkg-dist file) for an updated conffile that is not
> going to used.
>
> This is, strictly speaking, harmless, since file triggers were
> never a guarantee that a file was actually modified. But on one
> hand, the triggered action can be an expensive no-op (such as
> restarting a server process), and on the other hand, any package
> that relies on triggers being activated in this case will break
> once some user decides not to keep a .dpkg-dist file. So it
> seems safer to avoid the unnecessary and misleading trigger.

On that specific case "cfo_keep | cfof_backup", the user has been
prompted, dpkg stopped, allowing to modify the installed conffile,
through dpkg spawning a shell or backgrounding itself, or through
another external means. So dpkg does not currently have a way to track
if the installed conffile got modified, and that's why the trigger.

For example some times depending on the amount of changes, I either
forward port them from inst → new, or the other way around. In the
latter case you have to keep your current file, and it has been
modified.

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,

Guillem Jover wrote:

> On that specific case "cfo_keep | cfof_backup", the user has been
> prompted, dpkg stopped, allowing to modify the installed conffile,

Thanks for the explanation. If no one else beats me to it, I'll
write a comment to that effect tomorrow.

> through dpkg spawning a shell or backgrounding itself, or through
> another external means. So dpkg does not currently have a way to track
> if the installed conffile got modified, and that's why the trigger.

Hmm. I'm imagining a fictional initramfs-tools package interested in
files installed or modified in /etc/initramfs-tools/hooks.

The initramfs has to be rebuilt to take any configuration change into
account. If the system administrator makes a configuration change
herself, she can be relied upon to run update-initramfs; on the other
hand, if the change occurs from a dpkg run, dpkg is responsible for
running update-initramfs.

What happens if the sysadmin makes a change but forgets to run
update-initramfs? dpkg may or may not run update-initramfs; we can
only _expect_ it to if it changes the conffile itself.

The relevant question, then, is whether the conffile changes during
("because of") the dpkg run. Maybe it would be worth saving the
file's hash or stat information before passing control to the user?

I was hoping this would convince me we need to remember the old
installed (resolved) conffile, but no such luck. If the conffile has
changed since the last upgrade, that might only indicate that the
sysadmin modified it between upgrades.

Good night,
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

A file trigger is guaranteed to be activated before the file in
question is modified by dpkg, ensuring that interested packages
are notified even if dpkg is interrupted immediately after
modifying the file. But for conffiles modified by the user on
dpkg's behalf, triggers are being activated _after_ control
returns from the user, and it is possible for the trigger to be
forgotten.

For example, in the following sequence of events:

configuring package
dpkg asks user to guide it in merging changes to the
distributed conffile
user picks (Z) suspend; dpkg sends itself SIGSTOP
user modifies conffile to match new version and forgets
about dpkg
*clean system shutdown*
configuring package again
file matches new version, so nothing to do
...

although the conffile is updated under dpkg's watch, no trigger
gets activated.

If we trigger before transferring control to the user, that
problem goes away. This also makes the code a little more
obvious.

It would be tempting to trigger only on (Z) suspend, but the user
is just as likely to modify the conffile some other way (in
another process, hitting, ^Z, etc) after the prompt is presented.
It is safer to trigger before presenting the prompt, regardless
of the user's response.

Signed-off-by: Jonathan Nieder <jrnieder.DeleteThis@gmail.com>
---
Guillem Jover wrote:
> On that specific case "cfo_keep | cfof_backup", the user has been
> prompted, dpkg stopped, allowing to modify the installed conffile,

Hmm, maybe activating the trigger closer to the point when the file
is changed would make that more obvious? As a side effect, this
fixes a small theoretical race.

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

diff --git a/src/configure.c b/src/configure.c
index 030bde3..b5595ce 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -59,7 +59,8 @@ 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(struct pkginfo *pkg, const char *cfgfile,
+static enum conffopt promptconfaction(struct pkginfo *pkg,
+ struct filenamenode *cfgfile,
const char *realold, const char *realnew,
int useredited, int distedited,
enum conffopt what);
@@ -150,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(pkg, usenode->name, cdr.buf, cdr2.buf,
+ what = promptconfaction(pkg, usenode, cdr.buf, cdr2.buf,
useredited, distedited, what);

switch (what & ~(cfof_isnew | cfof_userrmd)) {
@@ -163,7 +164,6 @@ deferred_configure_conffile(struct pkginfo *pkg, struct conffile *conff)
varbufaddstr(&cdr, DPKGDISTEXT);
varbufaddc(&cdr, 0);
strcpy(cdr2rest, DPKGNEWEXT);
- trig_file_activate(usenode, pkg);
if (rename(cdr2.buf, cdr.buf))
warning(_("%s: failed to rename '%.250s' to '%.250s': %s"),
pkg->name, cdr2.buf, cdr.buf, strerror(errno));
@@ -579,7 +579,7 @@ suspend(void)
* default actions as configured by cmdline/configuration options.
*/
static enum conffopt
-promptconfaction(struct pkginfo *pkg, const char *cfgfile,
+promptconfaction(struct pkginfo *pkg, struct filenamenode *cfgfile,
const char *realold, const char *realnew,
int useredited, int distedited, enum conffopt what)
{
@@ -590,15 +590,28 @@ promptconfaction(struct pkginfo *pkg, const char *cfgfile,
return what;

statusfd_send("status: %s : %s : '%s' '%s' %i %i ",
- cfgfile, "conffile-prompt",
+ cfgfile->name, "conffile-prompt",
realold, realnew, useredited, distedited);

+ /*
+ * The user is about to be prompted and dpkg stopped, allowing to
+ * modify the installed conffile, through dpkg spawning a shell or
+ * backgrounding itself, or through another external means. Since
+ * dpkg does not currently have a way to track if the installed
+ * conffile gets modified, just unconditionally trigger.
+ *
+ * To avoid missing the trigger, we have to activate the trigger
+ * before the file is modified, anyway. But maybe some day dpkg will
+ * learn to cancel the trigger if it escapes this loop with the file
+ * unchanged.
+ */
+ trig_file_activate(cfgfile, pkg);
do {
/* Flush the terminal's input in case the user involuntarily
* typed some characters. */
tcflush(STDIN_FILENO, TCIFLUSH);
- fprintf(stderr, _("\nConfiguration file `%s'"), cfgfile);
- if (strcmp(cfgfile, realold))
+ fprintf(stderr, _("\nConfiguration file `%s'"), cfgfile->name);
+ if (strcmp(cfgfile->name, realold))
fprintf(stderr, _(" (actually `%s')"), realold);

if (what & cfof_isnew) {
@@ -658,9 +671,9 @@ promptconfaction(struct pkginfo *pkg, const char *cfgfile,
else if (what & cfof_install)
fprintf(stderr, _(" The default action is to install the new version.\n"));

- s = strrchr(cfgfile, '/');
+ s = strrchr(cfgfile->name, '/');
if (!s || !*++s)
- s = cfgfile;
+ s = cfgfile->name;
fprintf(stderr, "*** %s (Y/I/N/O/D/Z) %s ? ",
s,
(what & cfof_keep) ? _("[default=N]") :
@@ -699,7 +712,7 @@ promptconfaction(struct pkginfo *pkg, const char *cfgfile,
suspend();
} while (!strchr("yino", cc));

- log_message("conffile %s %s", cfgfile,
+ log_message("conffile %s %s", cfgfile->name,
(cc == 'i' || cc == 'y') ? "install" : "keep");

what &= cfof_userrmd;
--
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

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

sean finney wrote:

> discussion:
>
> namely, while working on a conffile-by-conffile basis ensures atomicity
> at the file level, there's a bigger window where things can go wrong
> compared to renaming the directories.

I am not sure I understand this: isn't configuring a new version
already an irreversible operation, since the conffiles themselves are
being overwritten in /etc? It seems relatively safe to me to move
each entry of the conffile db in deferred_configure_conffile() as soon
as it is no longer needed to help with that.

In other words, if the order of operations is:

check for .dpkg-new file
calculate .dpkg-new hash

if (new db entry absent)
save hash in status
delete .dpkg-new file
done

do 3-way merge (or whatever)

move new db entry to current
save hash in status
delete .dpkg-new file

then dpkg can be interrupted at any point without being put in an
inconsistent state, and the next run will recover appropriately from
that.

Complications:

- I am not sure how nice this would be for dealing with renamed
conffiles. Probably it is not worth worrying about that yet.

- Maybe some merge backends would want to work with all conffiles at
once. git, at least, works like this (though git itself probably
wouldn't be using the conffile db). This seems too theoretical
to think about supporting yet, and it should not be totally
impossible to change the file layout later to support something
like this if needed.

But maybe multiarch changes everything. I dunno.

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,

sean finney wrote:
> On Sun, Oct 25, 2009 at 03:50:44PM -0500, Jonathan Nieder wrote:

>> Will documentation processing tools choke on the string /*** used
>> elsewhere? (Just curious.)
>
> in my experience doxygen is pretty robust in not treating documentation
> and/or formatting errors as fatal errors

Good.

>>> + * @param what Hints on what action should be taken by defualt.
>>
>> These contain not just hints but required information, right?
>
> you can document a function without defining the params/return, but
> doxygen will complain that the function documentation is incomplete. so,
> not required but generally a good practice.

Oh, sorry to be unclear. I meant the bits of 'what'. Looking more
closely now, it looks like 'what' contains the default action, plus
whether the conffile is new or has was deleted by the user. So maybe:

@param what Bitfield with the current state of the conffile and
action to be taken by default.

I was most interested in what value could be passed if the caller
doesn't want a default, so:

For example, cfof_prompt with no other bits set means
"existing conffile changed, no default action".

But as I hinted before, I personally think it would be fine to leave
addressing this sort of nitpick to later (i.e., a separate patch, once
the code has settled down). Your documentation here is already far
better than just about everywhere else in dpkg.

> gotta start somewhere right?

Yes, much appreciated.

Many thanks,
Jonathan

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

Some of this was addressed by comments from Jonathan, specially the
db atomicity issues, but commenting anyway.

On Mon, 2009-11-09 at 23:26:27 +0100, sean finney wrote:
> On Fri, Nov 06, 2009 at 08:48:43AM +0100, Guillem Jover wrote:
> > > +/**
> > > + * @file conffiledb.h
> > > + *
> > [...]
> > > + */
> >
> > Missing blank line
>
> clarification: after what?

Between the comment and the preprocessor directive.

> > > +#ifndef CONFFILES_H
> > > +#define CONFFILES_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.
>
> it's currently needed as extern for the unpublished cmdline prog, which
> you couldn't have known since it's... unpublished. but i've changed it
> to pass a struct pkginfo instead. fixed (as long as you don't object
> to it being exposed in the meantime)

If it's going to be needed, then no problem, in case the using code
gets changed afterwards we can always make it static.

> > > +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.
>
> discussion:
>
> namely, while working on a conffile-by-conffile basis ensures atomicity
> at the file level, there's a bigger window where things can go wrong
> compared to renaming the directories.

How so?

> *however*, one idea did come
> up in the discussion with jonathan, that might be a nicer alternative.
> combining it with what you've suggested in this thread, how about
> the layout:
>
> <admindir>/conffiles/<pkg>/<pkgversion>/<hash>.<basetype>
>
> since we're passing struct pkginfo's around instead of package names, we
> get the version information for free.

What would be the advantage?

> i don't know if it would also be useful for multiarch (are to multiarch
> packages allowed to have conflicting conffiles?), but we could throw the
> arch in there as well.

Multiarch packages should not have conflicting conffiles, no.
Something that we might allow is for the different packages to ship
the exact same conffile, and not consider that a conflict.

The only thing that we'll need for multiarch is to place the arch
along the package name, so something like <pkg:arch> instead of just
<pkg> but I don't think we need much more.

> anyway, with
> a layout like above, in the <pkg> directory we could have symlinks which could
> be updated to point at the respective <pkgversion> subdirectory instead
> of actually moving files/directories around. what do you think?

I think this just will complicate things a bit more, as you'd have to
take care of updating the symlinks, ensuring they don't ever become
dangling, and removing old files from the db, at which point you could
have just handled the files themselves and be done with it.

> otherwise i can do as you suggest, but the error handling will be
> quite a bit more involved.

Well, just a bit (certainly more than with directory move Smile

, but I
don't expect it to be that much.

> > > +#include <dpkg/dpkg.h>
> > > +#include <dpkg/i18n.h>
> > > +#include <dpkg/buffer.h>
> > > +#include <dpkg/path.h>
> > > +#include <dpkg/dpkg-db.h>
> >
> > Missing blank line.
>
> clarification: not sure what you mean here: between dpkg-db.h and main.h?

Right.

> > > +#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?
>
> it was a suggestion from buxy or mrvn, at this point my memory is a bit
> fuzzy. if you don't think it's useful then it can be removed.

I read those mails, but I'm not entirely sure that's what was suggested.
Anyway if someone can provide an example of when that'd be useful we
could reconsider, but right now I don't see the need. For example with
"git rerere" you can store the resolution of conflicted merges, but
that's mostly useful when you are going to find the same conflict over
and over again, and I'm not sure that usually applies to conffiles.

> > > + /* now ensure each directory exists while reconstructing the path */
> > > + for (i = 2; i >= 0; i--) {
> ...
> > > + if (mkdir(dbdir, S_IRWXU))
> > > + ohshite("conffiledb_ensure_db: mkdir(%s)",
> > > + dbdir);
>
> > 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.
>
> discussion:
>
> this is dependant on the outcome of the above discussion item wrt layout,
> so i'm holding off on changing this until we work that out.

Ok.

> > > +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.
>
> clarification: instead of (S_IRUSR|S_IWUSR)? seems a bit odd to avoid
> use of flags for their intended purpose doesn't it? i can remove the
> comment regardless if you think it's burdensome to the code ;p.

Well yeah, I agree that in general using descriptive names for
constants is better than just using the literals there, but in this
case I think it's more clear and compact to use the octal value, as
it's something we use pretty often when dealing with file system
perms on the command line on Unix systems, for example.

Also regarding comments, too many, or too obvious can also be pretty
distracting. Usually if the code is self descriptive (small functions,
good named variables, etc) there should not be much need for many
inline comments. Of course there's always tricky sections or specific
pieces that might deserve explanations why it's done that way or some
overall logic, etc, but certainly not detailing what each line is
doing.

> > > + 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.
>
> this is just a general habit of mine... even when the variable
> is initialized a couple lines later. it doesn't have any cost
> since the compiler removes it and as a general practice prevents
> various refactorings/etc from removing what seems like an invariant.
> but fixed in this case, anyway.

Personally I just find it distracting, as you have to work out if it
was intended or if there's any discrepancy which one is the right one.

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

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

Guillem Jover wrote:

> Multiarch packages should not have conflicting conffiles, no.
> Something that we might allow is for the different packages to ship
> the exact same conffile, and not consider that a conflict.
>
> The only thing that we'll need for multiarch is to place the arch
> along the package name, so something like <pkg:arch> instead of just
> <pkg> but I don't think we need much more.

Thanks for the explanation. (I had been wondering what the general
strategy is for situations like this.)

--
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 again,

Is your current code in a git repo somewhere? I would like to try it
out, even if the filesystem layout might change later, to see what it
is like to use and if I run into any issues.

(I would be happy to see this move forward so that we can get some
experience using it before squeeze.)

sean finney wrote:
> On Thu, Nov 19, 2009 at 07:44:51AM +0100, Guillem Jover wrote:
>> On Mon, 2009-11-09 at 23:26:27 +0100, sean finney wrote:

>>> namely, while working on a conffile-by-conffile basis ensures atomicity
>>> at the file level, there's a bigger window where things can go wrong
>>> compared to renaming the directories.
>>
>> How so?
>
> there's more operations where disk/inodes could run out and time during which
> the user/system could kill the process, etc. and supporting being able to
> clean up from such a state seems a bit messy. so i was thinking on a
> less granular level wrt overall conffile db state.

I fear I am still missing something. Current code:

enter unpacked state
for each conffile:
update it in an idempotent way (deferred_configure_conffile)
enter halfconfigured state
run postinst
update Config-Version

Updating the db on a conffile-by-conffile basis adds to the
deferred_configure_conffile() step, without destroying the idempotency
or safety. During the loop, the conffile db is always in a valid
state, so there is nothing to clean up. Some "current" entries belong
to the old version of the package and some to the new version.
Committing a db entry could not cause the system to run of
disk/inodes, since it is a rename(), which reuses the inode.

Maybe there is some reason to avoid letting the "current" db mix
entries from multiple versions of the package. In this case, dpkg
_would_ need to commit the entire db at once.

enter unpacked state
for each conffile:
update it in an idempotent way
commit new current conffiledb in an idempotent way
enter halfconfigured state
run postinst
update Config-Version

Commiting the conffiledb could mean something like this:

rm -r current
mv new current

But that is not idempotent (repeating it does just the wrong thing).
As you mentioned, it is easier and safer to maintain some notion of
the current version number (though a symlink, or another field in
status like Config-Version) and update that idempotently.

Would it make sense to make the commit happen after the postinst runs?
This is what it would mean to use Config-Version for this purpose.
Intuitively, it makes more sense to me to make the commit happen
earlier, once the old conffiles are not needed any more for a possible
three-way merge.

Does that make sense?

Jonathan

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