Re: [PATCH 3/6] Conffile database handling functions

[Guillem Jover <guillem@debian.org>]

Hi!

On Sun, 2009-10-25 at 16:46:04 -0500, Jonathan Nieder wrote:
> Sean Finney wrote:
> > --- /dev/null
> > +++ b/src/conffiledb.c
> > @@ -0,0 +1,218 @@
> [...]
> > +char* 
> 
> I think the asterisk and space should be swapped here.

Right. There's other trialing spaces around.

> > +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;

Yeah, better like this, now that we have a saner varbufprintf, there's
no reason to not use it, at least on code paths that are not critical.

> [...]
> > +	if (cfgfd < 0) 
> > +		ohshite(_("unable to create `%.255s'"),p);
> 
> Straight quotes are preferred (see doc/coding-style.txt).

Only as long as this does not introduce a gratuituous new string. In
this case this exact same string appears elsewhere so it gets reused.

> [...]
> > +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.

Yeah, open better than get.

> > +void
> > +conffiledb_commit(const char *pkg, conffiledb_base from_base, 
> > +                  conffiledb_base to_base)
> > +{
> [...]
> > +	/* 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.

Right.

> 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?

Yeah, if as suggested it gets switched from dir based to conffile base
it should be possible to guarantee atomicity, and rollback

> [...]
> > +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.

Yeah.

> A simple example of use of this API would be really nice.  Maybe even
> a test (see <dpkg/test.h> for some helpers).

That'd be nice, although not a requirement for merging.

thanks,
guillem