Re: [PATCH 2/7] Conffile database handling functions
Hi Sean!
On Mon, 2009-12-14 at 08:27:32 +0100, Sean Finney wrote:
> The layout pattern for the conffile database is:
>
> <admindir>/conffiles/<pkg>/<version>/<hash>
While the implementation resulting from this layout seems more elegant
than the previous iteration, I still think that using the layout I
described in the last thread
“<admindir>/conffiles/<pkg>/<hash>[.<base>]”
is superior as it allows us to switch to use it as the native storage
of conffiles in the future. Currently the configure step knows which
conffiles have been processed by checking if the conffile.dpkg-new is
there, which the user might have removed, intentionally or not. Keeping
track of that fact in the db itself would be more robust. Also there's
not going to be any possibility for dangling files form other versions.
Currently your implementation seems to lose track of files if for
example you do several consecutive unpacks w/o a configure step
inbetween.
Additionally using <base> to describe what kind of conffile it is, seems
more natural than trying to use a fake version instead.
> diff --git a/lib/dpkg/test/Makefile.am b/lib/dpkg/test/Makefile.am
> index 9400267..46d1bfe 100644
> --- a/lib/dpkg/test/Makefile.am
> +++ b/lib/dpkg/test/Makefile.am
> @@ -15,10 +15,12 @@ check_PROGRAMS = \
> t-path \
> t-varbuf \
> t-version \
> - t-pkginfo
> + t-pkginfo \
> + t-conffiledb
>
> CHECK_LDADD = ../libdpkg.a
>
> +t_conffiledb_LDADD = ../../../src/conffiledb.o ../../../src/util.o $(CHECK_LDADD)
> t_macros_LDADD = $(CHECK_LDADD)
> t_path_LDADD = $(CHECK_LDADD)
> t_pkginfo_LDADD = $(CHECK_LDADD)
This test is for stuff from src/, could you move it under scr/test/
instead?
> diff --git a/lib/dpkg/test/t-conffiledb.c b/lib/dpkg/test/t-conffiledb.c
> new file mode 100644
> index 0000000..561b159
> --- /dev/null
> +++ b/lib/dpkg/test/t-conffiledb.c
> @@ -0,0 +1,224 @@
[...]
> +#include <dpkg/test.h>
> +#include <dpkg/dpkg-db.h>
> +#include <dpkg/varbuf.h>
> +#include <dpkg/buffer.h>
> +#include <../src/conffiledb.h>
> +
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/fcntl.h>
> +#include <unistd.h>
> +
> +/* Forward declaration to avoid needing to include main.h */
> +void ensure_pathname_nonexisting(const char *pathname);
Just include main.h so that we won't accidentally desync from the real
function.
> +/**
> + * Our "main program" admindir, which we set to a temporary
> + * location for our tests.
> + */
> +char *admindir;
> +
> +/* Convenience pointer used for inspection of conffiledb contents */
> +char *conffiledbdir;
Make this static.
> +/* some global fixtures used in both tests */
> +static struct pkginfo pkg_a, pkg_a2;
> +static char *pkg_a_cnff_1;
> +static char *pkg_a_cnff_1_md5;
> +
> +/**
> + * Initialization of various test fixtures, etc.
> + */
> +static void
> +setup(void)
> +{
[...]
> + /* pkg_a is in the unpack phase of being upgraded */
> + pkg_a.name = m_strdup("foopkg");
> + pkg_a.installed.version.epoch = 1;
> + pkg_a.installed.version.version = m_strdup("2.3");
> + pkg_a.installed.version.revision = m_strdup("4deb5");
> + pkg_a.available.version.epoch = 1;
> + pkg_a.available.version.version = m_strdup("2.3");
> + pkg_a.available.version.revision = m_strdup("5deb6");
> + /* pkg_a2 is in the configure phase of being upgraded */
> + pkg_a2.name = m_strdup("foopkg");
> + pkg_a2.configversion.epoch = 1;
> + pkg_a2.configversion.version = m_strdup("2.3");
> + pkg_a2.configversion.revision = m_strdup("5deb6");
> + pkg_a2.installed.version.epoch = 1;
> + pkg_a2.installed.version.version = m_strdup("2.3");
> + pkg_a2.installed.version.revision = m_strdup("6deb7");
> + pkg_a2.available.version.epoch = 1;
> + pkg_a2.available.version.version = m_strdup("2.3");
> + pkg_a2.available.version.revision = m_strdup("6deb7");
Just use string literals, then there's not need to free them
afterwards. I should probably export something ressembling the version
macro from t-version.c (but with a less generic name), which would
allow you to simply do:
pkg_a2.configversion = version(1, "2.3", "5deb6");
> +static void
> +teardown(void)
> +{
> + free((char*)pkg_a.name);
> + free((char*)pkg_a.installed.version.version);
> + free((char*)pkg_a.installed.version.revision);
> + free((char*)pkg_a.available.version.version);
> + free((char*)pkg_a.available.version.revision);
> +
> + free((char*)pkg_a2.name);
> + free((char*)pkg_a2.configversion.version);
> + free((char*)pkg_a2.configversion.revision);
> + free((char*)pkg_a2.installed.version.version);
> + free((char*)pkg_a2.installed.version.revision);
> + free((char*)pkg_a2.available.version.version);
> + free((char*)pkg_a2.available.version.revision);
See above.
> diff --git a/src/conffiledb.c b/src/conffiledb.c
> new file mode 100644
> index 0000000..2dec3b5
> --- /dev/null
> +++ b/src/conffiledb.c
[...]
> +
> +#define CONFFILEDBDIRROOT "conffiles" /**< @ref conffiledb root location */
> +
> +static char *conffiledb_path_cversion(const struct pkginfo *pkg,
> + const char *version, const char *path);
Just move the function definition here.
> +char *
> +conffiledb_path(const struct pkginfo *pkg,
> + const struct versionrevision *version, const char *path)
> +{
> +}
> +char *
> +conffiledb_path_cversion(const struct pkginfo *pkg, const char *version,
> + const char *path)
> +{
> + struct varbuf full_path = VARBUF_INIT;
> + char hash[MD5HASHLEN + 1];
> +
> + varbufprintf(&full_path, "%s/%s", admindir, CONFFILEDBDIRROOT);
> +
> + if (pkg != NULL) {
> + varbufaddc(&full_path, '/');
> + varbufaddstr(&full_path, pkg->name);
> + }
> +
> + if (version != NULL) {
> + varbufaddc(&full_path, '/');
> + varbufaddstr(&full_path, version);
> + }
> +
> + /* if a conffile is being requested (not just the db root)... */
> + if (path != NULL) {
> + varbufaddc(&full_path, '/');
> + buffer_md5(path, hash, strlen(path));
> + varbufaddstr(&full_path, hash);
> + }
> + varbufaddc(&full_path, 0);
This function might produce funny results of one of the latter
arguments is non-null but the previous ones are.
> + debug(dbg_conffdetail, "confffiledb_path(%s, %s) = %s\n",
> + pkg?(pkg->name):"(null)", path, full_path.buf);
> +
> + return full_path.buf;
You can use varbuf_detach() now.
> +}
> +static void
> +conffiledb_ensure_db(const struct pkginfo *pkg,
> + const struct versionrevision *version)
> +{
> + struct stat s;
> + int i;
> + char *dbdirs[2];
> +
> + dbdirs[0] = conffiledb_path(pkg, NULL, NULL);
> + dbdirs[1] = conffiledb_path(pkg, version, NULL);
> +
> + /* ensure each directory exists while reconstructing the path */
> + for (i = 0; i < 2; i++) {
> + if (stat(dbdirs[i], &s) != 0) {
> + debug(dbg_conffdetail,
> + "conffiledb_ensure_db: creating %s\n", dbdirs[i]);
> + if (errno != ENOENT)
> + ohshite(_("stat() '%s' failed"), dbdirs[i]);
> + if (mkdir(dbdirs[i], 0700) == -1)
> + ohshite(_("mkdir() '%s' failed"), dbdirs[i]);
You could just do a mkdir and check for success or EEXIST as a non-error.
> + }
> + free(dbdirs[i]);
> + }
> +}
> +void
> +conffiledb_register_new_conffile(const struct pkginfo *pkg, const char *path,
> + int fd, off_t sz)
> +{
> + int cfgfd;
> + /* get the path to where the registered file goes */
> + char *p = conffiledb_path(pkg, &pkg->available.version, path);
> + char quoted_path[256];
> + struct varbuf tmpfn = VARBUF_INIT;
> +
> + varbufprintf(&tmpfn, "%s%s", p, DPKGTEMPEXT);
> +
> + conffiledb_ensure_db(pkg, &pkg->available.version);
> + debug(dbg_conff, "conffile_reg_fd: %s, %s, %s\n", pkg->name, path, p);
In addition to the previous layout comments, tying the registry to the
available version would seem to me might bite us once dpkg-conffile
grows a way to dynamically register files. Of course this could be
fixed by just passing the version as an additional argument, but
still.
> + /* make a copy of file to the .dpkg-tmp location and then rename
> + * it to the proper destination */
> + cfgfd = open(tmpfn.buf, O_CREAT|O_TRUNC|O_WRONLY, 0600);
This perms would disallow normal users to read those conffiles through
dpkg-conffile for example, which might be ok, although I don't see the
problem with world readable ones, something to consider.
> + if (cfgfd == -1)
> + ohshite(_("open '%s' failed"), tmpfn.buf);
> + fd_fd_copy(fd, cfgfd, sz, _("conffiledb registering '%.255s'"),
> + path_quote_filename(quoted_path, path, 256));
> +
> + if (fsync(cfgfd) == -1)
> + ohshite(_("fsync '%s' failed"), p);
> + if (close(cfgfd) == -1)
> + ohshite(_("close '%s' failed"), p);
> + if (rename(tmpfn.buf, p) == -1)
> + ohshite(_("rename '%s' failed"), p);
> +
> + varbuffree(&tmpfn);
> + free(p);
> +}
regards,
guillem
Reply to: