hi guillem,
okay, here's the follow-up from my end. items that i mention as fixed
are already locally commited (modulo some rebasing/squashing/compile fixes).
items mentioned as "will fix" are next up and will be included before the
next patch submission. items preceded with "discussion:" warrant further...
discussion, and i've also asked for clarification in one or two places.
On Fri, Nov 06, 2009 at 08:48:43AM +0100, Guillem Jover wrote:
> 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>”.
this makes sense... will fix.
> > 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.
fixed.
> > 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.
will fix.
> Comments about the doxygen stuff:
>
> <http://lists.debian.org/debian-dpkg/2009/10/msg00146.html>
will fix.
> > +/**
> > + * @file conffiledb.h
> > + *
> [...]
> > + */
>
> Missing blank line
clarification: after what?
> > +#ifndef CONFFILES_H
> > +#define CONFFILES_H
>
> Namespace those with DPKG_ for stuff under src/, and use the header
> name, so DPKG_CONFFILEDB_H.
fixed.
> > +#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.
fixed.
> > +/**
> > + * 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.
perhaps not... i think in an older version i was referencing this
in other files, but no longer. fixed.
> > +/** 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>.
removed and replaced... fixed.
> > +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.
it's currently needed as extern for the unpublished cmdline prog, which
you couldn't have known since it's... unpublished. but i've changed it
to pass a struct pkginfo instead. fixed (as long as you don't object
to it being exposed in the meantime)
> > +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.
fixed.
> > +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.
discussion:
namely, while working on a conffile-by-conffile basis ensures atomicity
at the file level, there's a bigger window where things can go wrong
compared to renaming the directories. *however*, one idea did come
up in the discussion with jonathan, that might be a nicer alternative.
combining it with what you've suggested in this thread, how about
the layout:
<admindir>/conffiles/<pkg>/<pkgversion>/<hash>.<basetype>
since we're passing struct pkginfo's around instead of package names, we
get the version information for free. i don't know if it would also be
useful for multiarch (are to multiarch packages allowed to have conflicting
conffiles?), but we could throw the arch in there as well. anyway, with
a layout like above, in the <pkg> directory we could have symlinks which could
be updated to point at the respective <pkgversion> subdirectory instead
of actually moving files/directories around. what do you think?
otherwise i can do as you suggest, but the error handling will be
quite a bit more involved.
> > 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.
will fix.
> > +#include "conffiledb.h"
>
> Place this the last with the local includes.
fixed.
> > +#include <config.h>
>
> Missing <compat.h>
fixed.
> > +#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.
clarification: not sure what you mean here: between dpkg-db.h and main.h?
> > +#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?
it was a suggestion from buxy or mrvn, at this point my memory is a bit
fuzzy. if you don't think it's useful then it can be removed.
> > +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.
will fix.
> > +/** 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.
fixed.
> > + /* now ensure each directory exists while reconstructing the path */
> > + for (i = 2; i >= 0; i--) {
...
> > + if (mkdir(dbdir, S_IRWXU))
> > + ohshite("conffiledb_ensure_db: mkdir(%s)",
> > + dbdir);
> 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.
discussion:
this is dependant on the outcome of the above discussion item wrt layout,
so i'm holding off on changing this until we work that out.
> > +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.
clarification: instead of (S_IRUSR|S_IWUSR)? seems a bit odd to avoid
use of flags for their intended purpose doesn't it? i can remove the
comment regardless if you think it's burdensome to the code ;p.
> > + 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 :).
yup... will fix (including proper quoting).
> > + path_quote_filename(fnamebuf, p, 256));
>
> Use fsync before the close.
fixed.
> > + 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.
this is just a general habit of mine... even when the variable
is initialized a couple lines later. it doesn't have any cost
since the compiler removes it and as a general practice prevents
various refactorings/etc from removing what seems like an invariant.
but fixed in this case, anyway.
> > +int
> > +conffiledb_diff (const char *pkg, const char *path, conffiledb_base base)
>
> This is a reimplementation of showdiff, but it does not use a pager
> for example.
i wasn't aware of showdiff... will fix.
there you go!
sean
Attachment:
signature.asc
Description: Digital signature