Re: [PATCH 2/2] Reimplement dpkg-divert in C
Hi!
For recurring issues I've commented on the first occurence only. And
I've stopped reviewing in detail at some point as major parts of the
code need to be rewritten anyway.
On Wed, 2009-10-14 at 02:36:45 +0700, Mikhail Gusarov wrote:
> Also remove dpkg-divert.pl and switch testsuite to
> test C implementation of dpkg-divert.
> ---
> scripts/.gitignore | 1 -
> scripts/Makefile.am | 2 -
> scripts/dpkg-divert.pl | 353 ---------------------
> scripts/t/950_dpkg_divert.t | 4 +-
> src/.gitignore | 1 +
> src/Makefile.am | 11 +-
> src/divert.c | 734 +++++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 747 insertions(+), 359 deletions(-)
> delete mode 100755 scripts/dpkg-divert.pl
> create mode 100644 src/divert.c
> diff --git a/scripts/t/950_dpkg_divert.t b/scripts/t/950_dpkg_divert.t
> index df96849..2b82766 100644
> --- a/scripts/t/950_dpkg_divert.t
> +++ b/scripts/t/950_dpkg_divert.t
> @@ -11,8 +11,8 @@ my $srcdir = $ENV{srcdir} || '.';
> my $admindir = File::Spec->rel2abs('t.tmp/dpkg-divert/admindir');
> my $testdir = File::Spec->rel2abs('t.tmp/dpkg-divert/testdir');
>
> -my @dd = ("perl", "$srcdir/dpkg-divert.pl");
> -#my @dd = ("$srcdir/../src/dpkg-divert");
> +#my @dd = ("perl", "$srcdir/dpkg-divert.pl");
> +my @dd = ("$srcdir/../src/dpkg-divert");
>
> plan tests => 248;
>
This does not seem to support out of tree builds, which should fail
when building the packages.
> diff --git a/src/.gitignore b/src/.gitignore
> index 10f60e9..8a6411e 100644
> --- a/src/.gitignore
> +++ b/src/.gitignore
> @@ -2,3 +2,4 @@ dpkg
> dpkg-query
> dpkg-statoverride
> dpkg-trigger
> +dpkg-divert
Insert it in alphabetical order.
> diff --git a/src/Makefile.am b/src/Makefile.am
> index a29b629..0971482 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -15,7 +15,8 @@ bin_PROGRAMS = \
> dpkg \
> dpkg-query \
> dpkg-statoverride \
> - dpkg-trigger
> + dpkg-trigger \
> + dpkg-divert
Ditto.
> diff --git a/src/divert.c b/src/divert.c
> new file mode 100644
> index 0000000..8695142
> --- /dev/null
> +++ b/src/divert.c
Name it divertcmd.c instead, to conform with the other commands.
> @@ -0,0 +1,734 @@
> +/*
> + * dpkg - main program for package management
> + * divert.c - implementation of dpkg-divert(8)
Just:
* dpkg-divert - override a package's version of a file
> +#include <getopt.h>
> +#include <fnmatch.h>
> +#include <stdlib.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <fcntl.h>
Sort from more specific to more generic.
> +#include <dpkg/dpkg.h>
> +#include <dpkg/i18n.h>
> +#include <dpkg/dpkg-db.h>
You need to include <locale.h> for the LC_* macros. And then you should
include <dpkg/i18n.h> before <locale.h>, otherwise it breaks on some
systems. That's why on other commands <dpkg/i18n.h> gets included just
after <compat.h> (this is an inconsistency I've to fix, thought, maybe
I'll push a fix later on).
> +const char thisname[]= "dpkg-divert";
Missing space.
> +static int quiet;
> +static int testmode;
> +static int dorename_;
No underscores at the beggining or end of names. And prefix them with
f_ to conform to the name of the flags on other programs, and to not
pollute the namespace.
> +static const char *divertto;
> +static const char *package;
> +static const char *admindir;
> +
> +typedef struct _diversion {
> + const char *contest;
> + const char *altname;
> + const char *package;
> + struct _diversion *next;
> +} diversion;
This is a reimplementation of the the current diversion handling in
filesdb.c and divertdb.c, use those instead, in a similar way as how
the statcmd.c is doing. Also no typedefs for structs.
> +static void
> +usage()
> +{
> + printf(_(
> +"Usage: %s [<option> ...] <command>\n"
> +"\n"
> +"Commands:\n"
> +" [--add] <file> add a diversion.\n"
> +" --remove <file> remove the diversion.\n"
> +" --list [<glob-pattern>] show file diversions.\n"
> +" --listpackage <file> show what package diverts the file.\n"
> +" --truename <file> return the diverted file.\n"
> +"\n"
> +"Options:\n"
> +" --package <package> name of the package whose copy of <file> will not\n"
> +" be diverted.\n"
> +" --local all packages' versions are diverted.\n"
> +" --divert <divert-to> the name used by other packages' versions.\n"
> +" --rename actually move the file aside (or back).\n"
> +" --admindir <directory> set the directory with the diversions file.\n"
> +" --test don't do anything, just demonstrate.\n"
> +" --quiet quiet operation, minimal output.\n"
> +" --help show this help message.\n"
> +" --version show the version.\n"
> +"\n"
> +"When adding, default is --local and --divert <original>.distrib.\n"
> +"When removing, --package or --local and --divert must match if specified.\n"
> +"Package preinst/postrm scripts should always specify --package and --divert.\n"
> + ), thisname);
> +}
As the string got modified, use the opportunity to split it in several
pieces. One for each of usage, commands, options, and the ending
paragraphs.
Also call m_output as the text is precious.
> +static void
> +badusage(const char *fmt, ...)
> +{
> + if (fmt) {
> + fprintf(stderr, "%s: ", thisname);
> + va_list arg;
> + va_start(arg, fmt);
> + vfprintf(stderr, fmt, arg);
> + va_end(arg);
> + fprintf(stderr, "\n\n");
> + }
> + usage();
> + exit(2);
> +}
This is already present in myopt.c.
> +static void
> +infol(const char *file, const char *divertto, const char *package,
> + struct varbuf *ret)
Use a more descriptive name. I know this is how this is named in the
perl version but no need to carry over this legacy.
> +{
> + if (package) {
> + if (strcmp(package, ":") == 0)
> + varbufaddstr(ret, "local ");
> + }
> + else
Put the else on the same line as the brace.
> + varbufaddstr(ret, "any ");
> +
> + varbufaddstr(ret, "diversion of ");
> + varbufaddstr(ret, file);
> +
> + if (divertto) {
> + varbufaddstr(ret, " to ");
> + varbufaddstr(ret, divertto);
> + }
> +
> + if (package && strcmp(package, ":") != 0) {
> + varbufaddstr(ret, " by ");
> + varbufaddstr(ret, package);
> + }
Hmm, this makes translators work pretty hard. Also carried over from
the perl code, but as this is getting rewritten let's get it right.
> + varbufaddc(ret, 0);
Use nul character '\0' notation.
> +}
> +static void
> +checkrename(const char *rsrc, const char *rdest)
Just name the arguments src and dst.
> +{
> + struct stat ssrc;
> + struct stat sdest;
And those two stat_src and stat_dst.
> + int has_src;
> + int has_dest;
Make these two bool.
> + static struct varbuf tmpfilename;
No static, it's not going to be used that much so we don't gain
anything from it.
> + int tmpfile;
> +
> + if (!dorename_) return;
Place the return on the next line. And do not use globals inside this
function, do the checks on the caller. Make it return int to get back
the case were disable renaming.
> + has_src = !lstat(rsrc, &ssrc);
> + if (!has_src && errno != ENOENT)
> + ohshite(_("cannot stat old name '%s'"), rsrc);
> + has_dest = !lstat(rdest, &sdest);
> + if (!has_dest && errno != ENOENT)
> + ohshite(_("cannot state new name '%s'"), rdest);
state → stat
> +
> + /* Unfortunately we have to check for write access in both places, just
> + * having +w is not enough, since people do mount things RO, and we need
> + * to fail before we start mucking around with things. So we open a file
> + * with the same name as the diversions but with an extension that
> + * (hopefully) wont overwrite anything. If it succeeds, we assume a
> + * writable filesystem.
> + */
> +
> + varbufreset(&tmpfilename);
> + varbufaddstr(&tmpfilename, rsrc);
> + varbufaddstr(&tmpfilename, ".dpkg-devert.tmp");
Use a macro for the extension name. Oh and you can use the opportunity
to fix the cute typo (devert → divert) that has been passing over since
ad8957a3. :)
> + varbufaddc(&tmpfilename, 0);
> +
> + tmpfile = open(tmpfilename.buf, O_WRONLY | O_CREAT);
> + if (tmpfile != -1) {
Use positive form instead.
> + unlink(tmpfilename.buf);
> + close(tmpfile);
> + }
> + else if (errno == ENOENT) {
> + dorename_ = 0;
> + /* If the source file is not present and we are not going to do
> + the rename anyway there's no point in checking the target. */
Place the comment before the code being described.
> + return;
> + }
> + else
> + ohshite(_("error checking '%s'"), rsrc);
> +
> + varbufreset(&tmpfilename);
> + varbufaddstr(&tmpfilename, rdest);
> + varbufaddstr(&tmpfilename, ".dpkg-devert.tmp");
> + varbufaddc(&tmpfilename, 0);
> +
> + tmpfile = open(tmpfilename.buf, O_WRONLY | O_CREAT);
> + if (tmpfile != -1) {
> + unlink(tmpfilename.buf);
> + close(tmpfile);
> + }
> + else
> + ohshite(_("error checking '%s'"), rdest);
> +
> + if (has_src && has_dest &&
> + !(ssrc.st_dev == sdest.st_dev && ssrc.st_ino == sdest.st_ino)) {
> + ohshite(_("rename involves overwriting '%s' with \n"
> + " different file '%s', not allowed"), rdest, rsrc);
This should be ohshit, as there's no errno from any previous system
call. There's also a tab too much in the second ohshite line.
> + }
> +}
> +static int
> +rename_mv(const char *src, const char *dest)
> +{
> + pid_t pid;
> +
> + if (rename(src, dest) == 0)
> + return 0;
> +
> + pid = fork();
> + if(pid == -1)
> + return -1;
> +
> + if (!pid) {
> + /* child */
> + execlp("mv", "mv", src, dest, NULL);
> + exit(EXIT_FAILURE);
> + }
> +
> + /* parent */
> + int status;
> + if(waitpid(pid, &status, 0) == -1)
> + return -1;
> +
> + return !(WIFEXITED(status) && WEXITSTATUS(status) == 0);
> +}
Use the stuff from subproc.h. Or actually just copy it yourself, use
fd_fd_copy and file_copy_perms.
> +static void
> +dorename(const char *rsrc, const char *rdest)
> +{
> + if (!dorename_) return;
> + if (testmode) return;
Do not use globals here, check those on the caller.
> + struct stat ssrc;
> + struct stat sdest;
No C99 style mixed code and declarations.
> + int has_src = !lstat(rsrc, &ssrc);
> + int has_dest = !lstat(rdest, &sdest);
No need for the variables, just check the return code in the if
itself.
> + if (!has_src) return;
> +
> + if (has_dest) {
> + if (-1 == unlink(rsrc))
Place the function call on the left side of the comparison, it's more
clear.
> + ohshite(_("rename: remove duplicate old link '%s'"),
> + rsrc);
> + } else {
> + if (-1 == rename_mv(rsrc, rdest))
> + ohshite(_("rename: rename '%s' to '%s'"), rsrc, rdest);
As you've modified the string anyway, remove the redundant rename,
something like “cannot rename '%s' ...”.
> + }
> +}
> +
> +static void
> +save(diversion *diversions)
> +{
> + diversion *d;
> + static struct varbuf filename;
> + static struct varbuf new_filename;
> + static struct varbuf old_filename;
Do not make this static either.
> + FILE *new_file;
Name those dbname_* and dbfile.
> + if (testmode) return;
> +
> + varbufreset(&new_filename);
> + varbufaddstr(&new_filename, admindir);
> + varbufaddstr(&new_filename, "/" DIVERSIONSFILE "-new");
Use NEWDBEXT for the extension, and don't concatanate the string
literal so that we don't get a new almost identical string on the
binary.
> + varbufaddc(&new_filename, 0);
> + new_file = fopen(new_filename.buf, "w");
> + if (!new_file)
> + ohshite(_("create diversions-new"));
> + chmod(new_filename.buf, 0644);
> +
> + for (d = diversions; d; d = d->next)
> + {
The brace on the same line as the for.
> + if (fprintf(new_file, "%s\n%s\n%s\n",
> + d->contest,
> + d->altname,
> + d->package) < 0)
> + ohshite(_("write diversions-new"));
> + }
> +
> + if (fclose(new_file) == EOF)
> + ohshite(_("close diversions-new"));
> +
> + varbufreset(&old_filename);
> + varbufaddstr(&old_filename, admindir);
> + varbufaddstr(&old_filename, "/" DIVERSIONSFILE "-old");
Also use OLDDBEXT.
> + varbufaddc(&old_filename, 0);
> +
> + if (unlink(old_filename.buf) != 0 && errno != ENOENT)
> + ohshite(_("remove old diversions-old"));
> +
> + varbufreset(&filename);
> + varbufaddstr(&filename, admindir);
> + varbufaddstr(&filename, "/" DIVERSIONSFILE);
> + varbufaddc(&filename, 0);
> +
> + if (link(filename.buf, old_filename.buf) != 0 && errno != ENOENT)
> + ohshite(_("create new diversions-old"));
> +
> + if (rename(new_filename.buf, filename.buf) != 0)
> + ohshite(_("install new diversions"));
> +}
> +static void
> +chomp(char *s)
> +{
> + for (; *s; s++)
> + if (*s == '\n') {
> + *s = 0;
> + return;
> + }
> +}
This function is not needed. fgets_checked already removes trailing
new lines.
> +static diversion *
> +invert_diversions_list(diversion *diversions)
> +{
> + diversion *n = NULL;
> + while (diversions)
> + {
> + diversion *newhead = diversions;
> + diversions = newhead->next;
> + newhead->next = n;
> + n = newhead;
> + }
> + return n;
> +}
Instead of inverting the list after the fact, just add to its tail
instead. OTOH I don't think this should be needed at all.
> +static diversion *
> +read_diversions()
> +{
> + diversion *diversions = NULL;
> + char linebuf[MAXDIVERTFILENAME];
> +
> + static struct varbuf vb;
> + varbufreset(&vb);
> + varbufaddstr(&vb, admindir);
> + varbufaddstr(&vb, "/" DIVERSIONSFILE);
> + varbufaddc(&vb, 0);
> +
> + FILE *file = fopen(vb.buf, "r");
> + if (!file)
> + ohshite(_("failed to open diversions file"));
> +
> + for (;;)
> + {
> + diversion *next = nfmalloc(sizeof(diversion));
> +
> + if (fgets_checked(linebuf, sizeof(linebuf), file, vb.buf) < 0)
> + break;
> + chomp(linebuf);
> + next->contest = strdup(linebuf);
> + fgets_must(linebuf, sizeof(linebuf), file, vb.buf);
> + chomp(linebuf);
> + next->altname = strdup(linebuf);
> + fgets_must(linebuf, sizeof(linebuf), file, vb.buf);
> + chomp(linebuf);
> + next->package = strdup(linebuf);
> +
> + next->next = diversions;
> + diversions = next;
> + }
> +
> + fclose(file);
> +
> + return invert_diversions_list(diversions);
> +}
This is a reimplementation of what's in divertdb.c, use that instead.
> +static diversion *diversions_remove(diversion *diversions, diversion *d)
> +{
> + diversion *c;
> +
> + if (d == diversions)
> + return diversions->next;
> +
> + c = diversions;
> + while (c && c->next != d)
> + c = c->next;
> +
> + if (!c)
> + ohshit(_("Internal error: trying to remove non-existent diversion"));
On these cases use internerr() instead.
> + c->next = d->next;
> + d->next = NULL;
> + return diversions;
> +}
> +
> +static int
> +match_diversion(diversion *diversion, char *const *patterns)
> +{
> + for (; *patterns; patterns++) {
> + if(!fnmatch(*patterns, diversion->contest, FNM_NOESCAPE))
> + return 0;
> + if(!fnmatch(*patterns, diversion->altname, FNM_NOESCAPE))
> + return 0;
> + if(!fnmatch(*patterns, diversion->package, FNM_NOESCAPE))
> + return 0;
Why the FNM_NOESCAPE?
> + }
> + return -1;
> +}
Make this return bool instead.
> +static void
> +op_add(diversion *diversions, const char *file)
Name the functions that access the db divertdb_foo, and the ones
interacting with the command line diversion_foo.
> +static void
> +version()
> +{
> + printf(_("Debian %s version %s.\n"), thisname, DPKG_VERSION_ARCH);
> + puts(_("\n"
> + "Copyright (C) 1995 Ian Jackson.\n"
> + "Copyright (C) 2000,2001 Wichert Akkerman.\n"
> + "Copyright (C) 2009 Mikhail Gusarov."));
> + puts(_("This is free software; see the GNU General Public Licence "
> + "version 2 or\n"
> + "later for copying conditions. There is NO warranty."));
Use printf instead, or the missing '\n' makes the string not get
merged with the ones from other commands.
> +}
> +static struct option long_options[] = {
> + { "add", no_argument, NULL, OPT_ADD },
> + { "remove", no_argument, NULL, OPT_REMOVE },
> + { "list", no_argument, NULL, OPT_LIST },
> + { "listpackage", no_argument, NULL, OPT_LISTPACKAGE },
> + { "truename", no_argument, NULL, OPT_TRUENAME },
> +
> + { "package", required_argument, NULL, OPT_PACKAGE },
> + { "local", no_argument, NULL, OPT_LOCAL },
> + { "divert", required_argument, NULL, OPT_DIVERT },
> + { "rename", no_argument, NULL, OPT_RENAME },
> + { "admindir", required_argument, NULL, OPT_ADMINDIR },
> + { "test", no_argument, NULL, OPT_TEST },
> + { "quiet", no_argument, NULL, OPT_QUIET },
> + { "help", no_argument, NULL, OPT_HELP },
> + { "version", no_argument, NULL, OPT_VERSION },
> + { NULL, 0, NULL, 0 },
> +};
Use struct cmdinfo from myopt.h instead.
> +static void
> +checkmanymodes(const char *curmode, const char *newmode)
> +{
> + if (!curmode)
> + return;
> + badusage(_("two commands specified: --%s and --%s"), newmode, curmode);
> +}
Use setaction similar to the other commands.
> +int
> +main(int argc, char *const *argv)
> +{
> + jmp_buf ejbuf;
> +
> + standard_startup(&ejbuf);
> +
> + const char *mode = NULL;
> + admindir = ADMINDIR;
Initialize admindir when declaring it.
> +/*
> + * Local Variables:
> + * mode: c
> + * c-file-style: "linux"
> + * c-basic-offset: 8
> + * tab-width: 8
> + * indent-tabs-mode: t
> + * End:
> + */
No editor markers.
thanks,
guillem
Reply to: