[PATCH 0/7] conffiledb tracking, redux
Return of the dpkg conffiledb tracking will follow. I assume those on
the list are familiar enough with it that i won't repeat the description
of the proposal, though feel free to read the embedded comments in patch
number 0002 as a refresher if necessary.
Notable changes since the last patchset:
* use a slightly different layout scheme, which doesn't require moving
registered conffiles around once they are put in place.
* This whole system should now work more sanely in half-configured and
other corner-case states.
* conffiledb_commit has been removed entirely, as it's no longer necessary to
move directories around or otherwise exchange files between directories.
* conffiledb_remove, however, still exists, and is just a loose wrapper
around ensure_pathname_nonexisting. It would not be very hard to extend
this function to remove individual files, but ultimately the same call
to ensure_pathname_nonexisting would end up in the package purge (to
make sure the hopefully-empty directory was removed), so I'm not sure
I see the gain in also doing it for individual files. If this is a
blocker I can implement it though, by adding an extra path parameter
to the existing function, which could then behave similarly to the
conffiledb_path function wrt NULL parameters.
* Across the board, struct varbuf is now used in almost all
string-assembling code. There are a few places where instead of calling
varbuffree() the contained .buf element is returned (where the user is
expected to later free the resulting char*). I confirmed with Guillem
via irc that this was okay. Otherwise the functions could be modified
to return struct varbuf pointers instead.
* In conffiledb_ensure_db, the slightly sketchy tokenizing stuff with
memrchr/rindex has been removed in favor of calculating each directory
seperately. It means extra malloc()/free() calls, but the resulting
code is decidedly less sketchy looking and much more readable.
* Upon a successful merge the "common ancestor" (the pristine
version of the conffile from the previous version of the package)
is also recorded, which may be useful in future merge resolutions.
this is recorded as a "special version", which is stored alongside
the version-specific directories, but named such that it will never
conflict with policy-compliant package versions. If it's desired, this
seperation could be guaranteed with an extra level of subdirectories
to seperate them, but I'm not convinced it's necessary. In this
particular case i decided to use the arbitrary string "merge:parent".
Other such special versions could be stored in a similar manner, such as
dynamically registered ones, other members of merge history/hierarchy,
etc, but i don't want to over complicate the initial patches.
* conffiledb_get_conffile was renamed to conffiledb_open_conffile
as suggested. Really, with the current code the function isn't entirely
necessary (the hook in archive.c could just seek back after siphoning
out the conffile), but I suspect that the need for this function would
reappear with the cmdline program so I'm leaving it in.
* various .dpkg-tmp and .dpkg-merge output files are now opened with
O_TRUNC instead of O_EXCL, to catch any case where we may be picking up
after a previously failed invocation of dpkg.
* fork()/exec()/subproc_wait() are now used instead of calling system()
for the call to diff3.
* The internal implementation of conffiledb_diff was removed in favor of
the already existing showdiff() function (didn't know it existed!).
* Test cases galore! Pretty much all of the new functionality is covered
by test cases in t-conffiledb.c. Since most of the code to be tested
is outside of lib, the required changes to Makefile.am aren't incredibly
pretty (but nothing too objectionable I hope).
Other, perhaps more aesthetic/stylistic changes:
* Trailing whitespace has been removed from any and all patches.
* Various changes to comply with the coding conventions ocassionally referenced
on the mailing list and irc (but still not in the repo? hm... :)
* Generally speaking doxygen comments have been moved as close as possible
to the definitions of the code, with regards to Guillem's comments
in an earlier thread. Other doxygen styles mentioned in the thread
were also adopted (first line formatting, blank link between params
and return value).
* Generally speaking non-doxygen comments are used much more sparingly
and judiciously.
* License and copyright headers are now included.
Items left for further discussion:
The following are things I have not yet addressed or would like to discuss
further. I personally don't feel that any of them are blocking issues,
though naturally feel free to state disagreement if you do.
* Is path_quote_filename really necessary for anything, and if so, is it
being used where it should be used? From the description it seems that
it's only necessary when printing strings with "%.<nnn>s" style format
strings, as found in the existing uses of fd_fd_copy. So... why not
just use "%s" instead? In the meantime iIm following the existing
convention and continuing to do so, but I thought I'd raise the question.
* Error strings are left open to discussion. I've kept them all
consistant and generic (and thus reusable, like _("open '%s' failed")),
per irc discussions with Guillem. However, I would prefer if debates
of their content are seperate from the rest of this discussion.
* There's hypothetical error paths that could currently lead to cruft
being left behind in the conffiledb dir. Harmless junk, but junk.
For example, unpacking one version of a package, which fails, and is
followed by unpacking a newer version of the package, might result in
the failed version's files never being purged from the conffiledb. This
could be solved by being more stringent after successful configuration.
Currently in deferred_configure after a successful configuration the
previously configured version's files are removed--instead of this
we could remove all files/directories *but* the ones we know we want
(the newly configured version and any special versions such as the
merge parents). If any cruft builds up in the meantime it would be
fixed whenever we decided to impliment the more stringent cleaning,
so i don't think it's incredibly critical.
* Dynamic registration isn't implemented yet (I figure that will come with
the cmdline program). Most likely this'll get solved in the same way
as the "merge parent version".
* Alternate diff/merge options using the merge:parent or dynamic:whatever
versions isn't implemented. This is left for future discussion and
improvements.
* There's no inspection step for the merge results. It might be nice to
have the ability to do so, but doing so would be a very noticable change
to dpkg's UI behavior so I felt it should be left for further discussion
on how to proceed there.
Sean Finney (7):
Split some useful functions from help.c to (new) util.c
Conffile database handling functions
Include the conffiledb directory in the dpkg deb package
Hook conffile database into dpkg unpack/configure/remove stages
Implement 'p' option for diffing dist conffiles during conflict resolution
Implement 'm' option for conffile merging during conflict resolution
Track common ancestors of a successful conffiledb merge
debian/dpkg.dirs | 1 +
lib/dpkg/dpkg.h | 2 +
lib/dpkg/test/Makefile.am | 4 +-
lib/dpkg/test/t-conffiledb.c | 396 ++++++++++++++++++++++++++++++++++++++
src/Makefile.am | 4 +-
src/archives.c | 17 ++-
src/conffiledb.c | 429 ++++++++++++++++++++++++++++++++++++++++++
src/conffiledb.h | 84 ++++++++
src/configure.c | 41 ++++-
src/help.c | 72 -------
src/remove.c | 4 +
src/util.c | 154 +++++++++++++++
12 files changed, 1129 insertions(+), 79 deletions(-)
create mode 100644 debian/dpkg.dirs
create mode 100644 lib/dpkg/test/t-conffiledb.c
create mode 100644 src/conffiledb.c
create mode 100644 src/conffiledb.h
create mode 100644 src/util.c
Reply to: