Re: [PATCH 3/6] Conffile database handling functions
Hi!
Some of this was addressed by comments from Jonathan, specially the
db atomicity issues, but commenting anyway.
On Mon, 2009-11-09 at 23:26:27 +0100, sean finney wrote:
> On Fri, Nov 06, 2009 at 08:48:43AM +0100, Guillem Jover wrote:
> > > +/**
> > > + * @file conffiledb.h
> > > + *
> > [...]
> > > + */
> >
> > Missing blank line
>
> clarification: after what?
Between the comment and the preprocessor directive.
> > > +#ifndef CONFFILES_H
> > > +#define CONFFILES_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.
>
> 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)
If it's going to be needed, then no problem, in case the using code
gets changed afterwards we can always make it static.
> > > +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.
How so?
> *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.
What would be the advantage?
> 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.
Multiarch packages should not have conflicting conffiles, no.
Something that we might allow is for the different packages to ship
the exact same conffile, and not consider that a conflict.
The only thing that we'll need for multiarch is to place the arch
along the package name, so something like <pkg:arch> instead of just
<pkg> but I don't think we need much more.
> 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?
I think this just will complicate things a bit more, as you'd have to
take care of updating the symlinks, ensuring they don't ever become
dangling, and removing old files from the db, at which point you could
have just handled the files themselves and be done with it.
> otherwise i can do as you suggest, but the error handling will be
> quite a bit more involved.
Well, just a bit (certainly more than with directory move :), but I
don't expect it to be that much.
> > > +#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?
Right.
> > > +#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.
I read those mails, but I'm not entirely sure that's what was suggested.
Anyway if someone can provide an example of when that'd be useful we
could reconsider, but right now I don't see the need. For example with
“git rerere” you can store the resolution of conflicted merges, but
that's mostly useful when you are going to find the same conflict over
and over again, and I'm not sure that usually applies to conffiles.
> > > + /* 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.
Ok.
> > > +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.
Well yeah, I agree that in general using descriptive names for
constants is better than just using the literals there, but in this
case I think it's more clear and compact to use the octal value, as
it's something we use pretty often when dealing with file system
perms on the command line on Unix systems, for example.
Also regarding comments, too many, or too obvious can also be pretty
distracting. Usually if the code is self descriptive (small functions,
good named variables, etc) there should not be much need for many
inline comments. Of course there's always tricky sections or specific
pieces that might deserve explanations why it's done that way or some
overall logic, etc, but certainly not detailing what each line is
doing.
> > > + 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.
Personally I just find it distracting, as you have to work out if it
was intended or if there's any discrepancy which one is the right one.
regards,
guillem
Reply to: