Re: [PATCH 3/6] Conffile database handling functions
Sean Finney wrote:
> The files conffiledb.{c,h} define a set of routines for adding and removing
> stored versions of the pristine conffiles as shipped in their respective
> packages.
>
> Such an implementation allows for some neat features in the future,
> such as:
>
> * 3-way merging of conffile changes
> * showing the old->new delta for the "dist" conffiles
> * dynamic registering of conffiles (à la ucf)
> * a general dpkg/conffile cmdline interface for all of this
I am really looking forward to this.
> The layout pattern for the conffile database is:
>
> <admindir>/conffiles/<base>/<pkg>/<hash>
>
> Where:
>
> * <admindir> is the standard dpkg administrative dir (i.e. /var/lib/dpkg).
> * <base> is a short string corresponding to which type of base reference
> is stored underneath (see source code comments for more info).
This is current, new, resolved, or tmp, right? It seems to almost
correspond to the three stages of the git index and the working tree,
but I haven’t worked this out all the way. Which contains the merge
base in a three-way merge?
> * <pkg> is the conffile database directory for a package.
This is just the package name, right?
> --- /dev/null
> +++ b/src/conffiledb.c
> @@ -0,0 +1,218 @@
[...]
> +char*
I think the asterisk and space should be swapped here.
> +conffiledb_path(const char *pkg, const char *path, conffiledb_base base)
> +{
[...]
> + /* the root path is <admindir>/<#CONFFILEDBDIRROOT/><basedir>/
> + * (note that the '/' after #CONFFILEDBDIRROOT is implicitly included
> + * in the #define'd string). The null byte is also accounted for
> + * at this point. */
> + basedir_sz = strlen(admindir) + 1 + strlen(CONFFILEDBDIRROOT) +
> + strlen(basedir) + 2;
> + full_path_sz = conffiledb_sz = basedir_sz;
This string size accounting makes me nervous about future
modifications to the code. Couldn’t you use struct varbuf?
Example:
struct varbuf full_path = VARBUF_INIT;
varbufprintf(&full_path, "%s/%s%s/",
admindir, CONFFILEDBDIRROOT, basedir);
if (pkg != NULL)
varbufprintf(&full_path, "%s/", pkg);
if (path != NULL) {
varbuf_grow(&full_path, CONFFILEDB_MD5SUM_LEN + 1);
buffer_md5(path, full_path.buf + full_path.used,
strlen(path));
full_path.used += CONFFILEDB_MD5SUM_LEN;
}
return full_path.buf;
[...]
> +/** Ensure that a particular conffiledb dir exists.
> + *
> + * @param pkg The package owning the conffiledb dir.
> + * @param base Which conffiledb basedir to use.
> + */
> +static void
> +conffiledb_ensure_db(const char *pkg, conffiledb_base base)
> +{
> + struct stat s;
> + short i = 0;
> + char *dbdir = NULL;
> + char *separators[3];
> +
> + dbdir = conffiledb_path(pkg, NULL, base);
> + /* temporarily truncate the string to cheaply traverse up to the
> + * the parent and its parent. store the locations so we can cheaply
> + * get back to them later. */
> + for (i = 0; i < 3; i++) {
> + separators[i] = rindex(dbdir, '/');
> + *separators[i] = '\0';
> + }
I was going to suggest a silly micro-optimization:
char *end;
end = dbdir + strlen(dbdir);
for (i = 0; i < 3; i++) {
end = separators[i] = memrchr(dbdir, '/', end - dbdir);
assert(end != NULL);
*end = '\0';
}
But memrchr is not portable, so this is almost certainly not worth it
(unless there is already a ready-made implementation to put in
libcompat).
It might be worth documenting that conffiledb_path needs to return a
path with at least 4 components.
[...]
> +void
> +conffiledb_register_new_conffile(const char *pkg, const char *path, int fd,
> + size_t sz)
> +{
This function just copies a configuration file into the “new” area of
the conffiledb, as preparation before storing it in /etc, right? It’s
hard to see that at a glance. Maybe it would be good to add a comment
to that effect, even if it would be redundant next to the more
longer comment in conffiledb.h.
This function is dominated by error handling, so I am tempted to say
there should be separate xopen() and xclose() functions that take care
of their own error handling (quitting with a message prefixed by a
caller-specified string when appropriate). I dunno.
[...]
> + if (cfgfd < 0)
> + ohshite(_("unable to create `%.255s'"),p);
Straight quotes are preferred (see doc/coding-style.txt).
[...]
> +int
> +conffiledb_get_conffile(const char *pkg, const char *path, conffiledb_base base)
[...]
Maybe calling it conffiledb_open_conffile would make it more obvious
the result is a file descriptor. Not sure.
> +void
> +conffiledb_commit(const char *pkg, conffiledb_base from_base,
> + conffiledb_base to_base)
> +{
> + /* update the conffiles "db" dir. this consists of the following steps:
> + * - nuke the tmp dir if it exists
> + * - rename to_base to the tmp dir
> + * - rename from_base to to_base
> + * - nuke the tmp dir
> + */
Better to describe a function outside its body. Even after reading
the doxygen comment in conffiledb.h, I am not sure what this function
is for. Does it just copy from the “from” dir to the “to” dir with an
opportunity to roll back? When does this need arise, on ENOSPC?
[...]
> + /* Make sure all leading components of the tmp conffiledb exist */
> + conffiledb_ensure_db(pkg, conffiledb_base_tmp);
> + /* Then nuke the tmp conffiledb if it exists */
> + conffiledb_remove(pkg, conffiledb_base_tmp);
> + /* Make sure all leading components of the target conffiledb exist */
> + conffiledb_ensure_db(pkg, to_base);
> + /* Rename the target conffiledb to the tmp one */
> + if (rename(cfdb_to, cfdb_tmp))
> + ohshite("conffiledb_commit: rename(%s,%s)", cfdb_to, cfdb_tmp);
> + /* Make sure all leading components of the source conffiledb exist */
> + conffiledb_ensure_db(pkg, from_base);
> + /* Rename from_base to to_base */
> + if (rename(cfdb_from, cfdb_to))
> + ohshite("conffiledb_commit: rename(%s,%s)", cfdb_from, cfdb_to);
> + /* Nuke the tmp version if it exists */
> + conffiledb_remove(pkg, conffiledb_base_tmp);
The heavy comments are distracting. Much more interesting than _what_
the code does is why it does it. In particular, the atomiticity here
is hard to understand, at least for me. Atomicity usually provides a
guarantee of the form, “if I interrupt the code at any point, ____
will always hold.” What is the invariant being maintained here?
[...]
> +int
> +conffiledb_diff (const char *pkg, const char *path, conffiledb_base base)
> +{
[...]
> + /* DIFF + ' ' + <opts> + ' ' + <srcfile> + ' ' + <dstfile> + '\0' */
> + size_t cmd_sz = strlen(DIFF) + 1 + strlen(opts) + 1 + strlen(src) +
> + 1 + strlen(path) + 1;
struct varbuf seems like a good fit here.
> --- /dev/null
> +++ b/src/conffiledb.h
> @@ -0,0 +1,155 @@
[...]
> +/** Register a new conffiledb file for a package.
> + *
> + * Create a conffiledb entry in the conffiledb dir for the::conffiledb_base_new
> + * version of a particular package. The conffile will be later processed in the
> + * package configuration stage, after which there should be a call to
> + * conffiledb_commit_new before configuration can be considered successful.
^^^
conffiledb_commit?
A simple example of use of this API would be really nice. Maybe even
a test (see <dpkg/test.h> for some helpers).
I hope to read your remaining patches later this week, but sadly there
is not enough time now. I hope my comments are of some use anyway.
As it is now, each local modification to a configuration file is a
burden to be painfully maintained, so I am glad you seem to be moving
towards a fix.
Regards,
Jonathan
Reply to: