Re: [PATCH 3/6] Conffile database handling functions
Hi!
Some of the comments from Jonathan's review apply, I'll reply to his
mail instead.
On Wed, 2009-10-14 at 21:23:06 +0200, Sean Finney wrote:
> 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).
> * <pkg> is the conffile database directory for a package.
> * <hash> is the md5 hash of the installed location of a package's conffile.
> A hash is used to keep the directory structure flat and balanced.
Hmmm, I think it'd be better to have the <base> part as an extension
to the conffile, similar to how the conffiles are handled right now on
the file system. It would also guarantee that the different
directories are on the same file system and rename(2) will always
succeed, even though putting them in different file systems would be
silly.
So something like “<admindir>/conffiles/<pkg>/<hash>[.<base>]”, where
<base> could be one of the already existing extensions “.dpkg-<ext>”.
> diff --git a/lib/dpkg/dpkg.h b/lib/dpkg/dpkg.h
> index afe650f..9b06c73 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 CONFFILEDBDIRROOT "conffiles/" /**< @ref conffilesdb root location */
> #define TRIGGERSDIR "triggers/"
> #define TRIGGERSFILEFILE "File"
> #define TRIGGERSDEFERREDFILE "Unincorp"
Place this in the conffiledb.c src/, the only place where it's going
to be used.
> diff --git a/src/conffiledb.h b/src/conffiledb.h
> new file mode 100644
> index 0000000..9278cf1
> --- /dev/null
> +++ b/src/conffiledb.h
> @@ -0,0 +1,155 @@
Missing license and copyright comment header.
Comments about the doxygen stuff:
<http://lists.debian.org/debian-dpkg/2009/10/msg00146.html>
> +/**
> + * @file conffiledb.h
> + *
[...]
> + */
Missing blank line
> +#ifndef CONFFILES_H
> +#define CONFFILES_H
Namespace those with DPKG_ for stuff under src/, and use the header
name, so DPKG_CONFFILEDB_H.
> +#include <sys/types.h>
> +
> +/** The different base reference types underneath the conffile db. */
> +typedef enum {
> + conffiledb_base_current, /**< The currently installed version */
> + conffiledb_base_new, /**< A newly unpacked version */
> + conffiledb_base_resolved, /**< The most recently resolved conflict */
> + conffiledb_base_tmp, /**< A temporary version for internal use */
> + conffiledb_base_COUNT /**< The count of possible values for this type */
> +} conffiledb_base;
No typedefs for enums, structs, unions.
> +/**
> + * The directory names of the conffiledb base dirs, indexed by the respective
> + * conffiledb_base value.
> + */
> +extern const char *conffiledb_base_dirs[conffiledb_base_COUNT];
This does not seem to be needed as extern.
> +/** Length of the MD5 string used in conffile path hashing (NULL not counted) */
> +#define CONFFILEDB_MD5SUM_LEN 32
There's already MD5HASHLEN in <dpkg/dpkg.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.
> +void conffiledb_register_new_conffile(const char *pkg, const char *path, int fd,
> + size_t sz);
sz should be off_t. off_t is for files, size_t for memory.
> +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.
> diff --git a/src/conffiledb.c b/src/conffiledb.c
> new file mode 100644
> index 0000000..46cbfaf
> --- /dev/null
> +++ b/src/conffiledb.c
> @@ -0,0 +1,218 @@
Missing license and copyright comment header.
> +#include "conffiledb.h"
Place this the last with the local includes.
> +#include <config.h>
Missing <compat.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.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>
Missing blank line.
> +#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?
> +char*
> +conffiledb_path(const char *pkg, const char *path, conffiledb_base base)
> +{
> + char *full_path = NULL, *hash = NULL;
> + const char *basedir = NULL;
> + size_t basedir_sz = 0, conffiledb_sz = 0, full_path_sz = 0;
> +
> + /* sanity check for a valid base dir */
> + if (base >= conffiledb_base_COUNT)
> + ohshit("conffiledb_path called with unsupported base %x", base);
Use internerr here, as this would be a programming error, and we
should act like in an assert.
> +}
> +/** 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;
Just use int, as the preferred native integer type. Also no need to
initialize as that's already done in the loop.
> + 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';
> + }
> +
> + /* now ensure each directory exists while reconstructing the path */
> + for (i = 2; i >= 0; i--) {
> + if (stat(dbdir, &s)) {
> + debug(dbg_conffdetail,
> + "conffiledb_ensure_db: creating %s\n", dbdir);
> + if (errno != ENOENT)
> + ohshite("conffiledb_ensure_db: stat(%s)",
> + dbdir);
> + if (mkdir(dbdir, S_IRWXU))
> + ohshite("conffiledb_ensure_db: mkdir(%s)",
> + dbdir);
> + }
> + *separators[i] = '/';
> + }
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.
> + free(dbdir);
> +}
> +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.
> + 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'"),
This string seems out of place (probably copy-pasted from
src/archive.c :).
> + path_quote_filename(fnamebuf, p, 256));
Use fsync before the close.
> + 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.
> + char *p = conffiledb_path(pkg, path, base);
> +
> + fd = open(p, O_RDONLY);
> + if (fd == -1)
> + ohshite("error opening conffile registered at %s", p);
> +
> + free(p);
> + return fd;
> +}
> +int
> +conffiledb_diff (const char *pkg, const char *path, conffiledb_base base)
> +{
> + int status;
> + char *src = conffiledb_path(pkg, path, base);
> + char *cmd = NULL;
> + const char *opts = "-u";
> + /* DIFF + ' ' + <opts> + ' ' + <srcfile> + ' ' + <dstfile> + '\0' */
> + size_t cmd_sz = strlen(DIFF) + 1 + strlen(opts) + 1 + strlen(src) +
> + 1 + strlen(path) + 1;
> +
> + cmd = m_malloc(cmd_sz);
> + sprintf(cmd, "%s %s %s %s", DIFF, opts, src, path);
> + status = system(cmd);
> +
> + free(src);
> + free(cmd);
> + return WEXITSTATUS(status);
> +}
This is a reimplementation of showdiff, but it does not use a pager
for example.
thanks,
guillem
Reply to: