[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

[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
* 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: