Help!

[PATCH 0/2] Rewrite of dpkg-divert to C patch series v2

 
  

Post new topic   General Reply to Topic (not reply to a specific post)    Forums Home -> PacKaGe RSS
Next:  [PATCH] Fix va_copy test for cross-compilation ca..  
Author Message
Mikhail Gusarov
External


Since: Oct 09, 2005
Posts: 116



PostPosted: Thu Oct 15, 2009 8:10 pm    Post subject: [PATCH 0/2] Rewrite of dpkg-divert to C patch series v2
Archived from groups: linux>debian>maint>dpkg (more info?)

Whoops, previous patchset contained a last-minute typo which prevented
compilation of divert.c


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org
Back to top
Mikhail Gusarov
External


Since: Oct 09, 2005
Posts: 116



PostPosted: Thu Oct 15, 2009 8:10 pm    Post subject: [PATCH 2/2] Reimplement dpkg-divert in C [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)

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/.gitignore b/scripts/.gitignore
index 3fc292e..7d08324 100644
--- a/scripts/.gitignore
+++ b/scripts/.gitignore
@@ -2,7 +2,6 @@ dpkg-architecture
dpkg-buildpackage
dpkg-checkbuilddeps
dpkg-distaddfile
-dpkg-divert
dpkg-genchanges
dpkg-gencontrol
dpkg-gensymbols
diff --git a/scripts/Makefile.am b/scripts/Makefile.am
index 9b287a0..3eadfb5 100644
--- a/scripts/Makefile.am
+++ b/scripts/Makefile.am
@@ -7,7 +7,6 @@ bin_SCRIPTS = \
dpkg-buildpackage \
dpkg-checkbuilddeps \
dpkg-distaddfile \
- dpkg-divert \
dpkg-genchanges \
dpkg-gencontrol \
dpkg-gensymbols \
@@ -39,7 +38,6 @@ EXTRA_DIST = \
dpkg-scansources.pl \
dpkg-shlibdeps.pl \
dpkg-source.pl \
- dpkg-divert.pl \
dpkg-vendor.pl \
update-alternatives.pl \
changelog/debian.pl \
diff --git a/scripts/dpkg-divert.pl b/scripts/dpkg-divert.pl
deleted file mode 100755
index 0da77de..0000000
--- a/scripts/dpkg-divert.pl
+++ /dev/null
@@ -1,353 +0,0 @@
-#!/usr/bin/perl --
-
-BEGIN { # Work-around for bug #479711 in perl
- $ENV{PERL_DL_NONLAZY} = 1;
-}
-
-use strict;
-use warnings;
-
-use POSIX qw(:errno_h);
-use Dpkg;
-use Dpkg::Gettext;
-
-textdomain("dpkg");
-
-sub version {
- printf _g("Debian %s version %s.\n"), $progname, $version;
-
- printf _g("
-Copyright (C) 1995 Ian Jackson.
-Copyright (C) 2000,2001 Wichert Akkerman.");
-
- printf _g("
-This is free software; see the GNU General Public Licence version 2 or
-later for copying conditions. There is NO warranty.
-");
-}
-
-sub usage {
- printf(_g(
-"Usage: %s [<option> ...] <command>
-
-Commands:
- [--add] <file> add a diversion.
- --remove <file> remove the diversion.
- --list [<glob-pattern>] show file diversions.
- --listpackage <file> show what package diverts the file.
- --truename <file> return the diverted file.
-
-Options:
- --package <package> name of the package whose copy of <file> will not
- be diverted.
- --local all packages' versions are diverted.
- --divert <divert-to> the name used by other packages' versions.
- --rename actually move the file aside (or back).
- --admindir <directory> set the directory with the diversions file.
- --test don't do anything, just demonstrate.
- --quiet quiet operation, minimal output.
- --help show this help message.
- --version show the version.
-
-When adding, default is --local and --divert <original>.distrib.
-When removing, --package or --local and --divert must match if specified.
-Package preinst/postrm scripts should always specify --package and --divert.
-"), $progname);
-}
-
-my $testmode = 0;
-my $dorename = 0;
-my $verbose = 1;
-my $mode = '';
-my $package = undef;
-my $divertto = undef;
-my @contest;
-my @altname;
-my @package;
-my $file;
-$|=1;
-
-
-# FIXME: those should be local.
-my ($rsrc, $rdest);
-my (@ssrc, @sdest);
-
-sub checkmanymodes {
- return unless $mode;
- badusage(sprintf(_g("two commands specified: %s and --%s"), $_, $mode));
-}
-
-while (@ARGV) {
- $_= shift(@ARGV);
- last if m/^--$/;
- if (!m/^-/) {
- unshift(@ARGV,$_); last;
- } elsif (m/^--help$/) {
- usage();
- exit(0);
- } elsif (m/^--version$/) {
- version();
- exit(0);
- } elsif (m/^--test$/) {
- $testmode= 1;
- } elsif (m/^--rename$/) {
- $dorename= 1;
- } elsif (m/^--quiet$/) {
- $verbose= 0;
- } elsif (m/^--local$/) {
- $package= ':';
- } elsif (m/^--add$/) {
- checkmanymodes();
- $mode= 'add';
- } elsif (m/^--remove$/) {
- checkmanymodes();
- $mode= 'remove';
- } elsif (m/^--list$/) {
- checkmanymodes();
- $mode= 'list';
- } elsif (m/^--listpackage$/) {
- checkmanymodes();
- $mode= 'listpackage';
- } elsif (m/^--truename$/) {
- checkmanymodes();
- $mode= 'truename';
- } elsif (m/^--divert$/) {
- @ARGV || badusage(sprintf(_g("--%s needs a divert-to argument"), "divert"));
- $divertto= shift(@ARGV);
- $divertto =~ m/\n/ && badusage(_g("divert-to may not contain newlines"));
- } elsif (m/^--package$/) {
- @ARGV || badusage(sprintf(_g("--%s needs a <package> argument"), "package"));
- $package= shift(@ARGV);
- $package =~ m/\n/ && badusage(_g("package may not contain newlines"));
- } elsif (m/^--admindir$/) {
- @ARGV || badusage(sprintf(_g("--%s needs a <directory> argument"), "admindir"));
- $admindir= shift(@ARGV);
- } else {
- badusage(sprintf(_g("unknown option \`%s'"), $_));
- }
-}
-
-$mode='add' unless $mode;
-
-open(O, "$admindir/diversions") || quit(sprintf(_g("cannot open diversions: %s"), $!));
-while(<O>) {
- s/\n$//; push(@contest,$_);
- $_ = <O>;
- s/\n$// || badfmt(_g("missing altname"));
- push(@altname,$_);
- $_ = <O>;
- s/\n$// || badfmt(_g("missing package"));
- push(@package,$_);
-}
-close(O);
-
-if ($mode eq 'add') {
- @ARGV == 1 || badusage(sprintf(_g("--%s needs a single argument"), "add"));
- $file= $ARGV[0];
- $file =~ m#^/# || badusage(sprintf(_g("filename "%s" is not absolute"), $file));
- $file =~ m/\n/ && badusage(_g("file may not contain newlines"));
- -d $file && badusage(_g("Cannot divert directories"));
- $divertto= "$file.distrib" unless defined($divertto);
- $divertto =~ m#^/# || badusage(sprintf(_g("filename "%s" is not absolute"), $divertto));
- $package= ':' unless defined($package);
- for (my $i = 0; $i <= $#contest; $i++) {
- if ($contest[$i] eq $file || $altname[$i] eq $file ||
- $contest[$i] eq $divertto || $altname[$i] eq $divertto) {
- if ($contest[$i] eq $file && $altname[$i] eq $divertto &&
- $package[$i] eq $package) {
- printf(_g("Leaving \`%s'")."\n", infon($i)) if $verbose > 0;
- exit(0);
- }
- quit(sprintf(_g("\`%s' clashes with \`%s'"), infoa(), infon($i)));
- }
- }
- push(@contest,$file);
- push(@altname,$divertto);
- push(@package,$package);
- printf(_g("Adding \`%s'")."\n", infon($#contest)) if $verbose > 0;
- checkrename($file, $divertto);
- save();
- dorename($file, $divertto);
- exit(0);
-} elsif ($mode eq 'remove') {
- @ARGV == 1 || badusage(sprintf(_g("--%s needs a single argument"), "remove"));
- $file= $ARGV[0];
- for (my $i = 0; $i <= $#contest; $i++) {
- next unless $file eq $contest[$i];
- quit(sprintf(_g("mismatch on divert-to\n when removing \`%s'\n found \`%s'"), infoa(), infon($i)))
- if defined($divertto) && $altname[$i] ne $divertto;
- quit(sprintf(_g("mismatch on package\n when removing \`%s'\n found \`%s'"), infoa(), infon($i)))
- if defined($package) && $package[$i] ne $package;
- printf(_g("Removing \`%s'")."\n", infon($i)) if $verbose > 0;
- my $orgfile = $contest[$i];
- my $orgdivertto = $altname[$i];
- @contest= (($i > 0 ? @contest[0..$i-1] : ()),
- ($i < $#contest ? @contest[$i+1..$#contest] : ()));
- @altname= (($i > 0 ? @altname[0..$i-1] : ()),
- ($i < $#altname ? @altname[$i+1..$#altname] : ()));
- @package= (($i > 0 ? @package[0..$i-1] : ()),
- ($i < $#package ? @package[$i+1..$#package] : ()));
- checkrename($orgdivertto, $orgfile);
- dorename($orgdivertto, $orgfile);
- save();
- exit(0);
- }
- printf(_g("No diversion \`%s', none removed")."\n", infoa())
- if $verbose > 0;
- exit(0);
-} elsif ($mode eq 'list') {
- my @list;
- my @ilist = @ARGV ? @ARGV : ('*');
- while (defined($_=shift(@ilist))) {
- s/\W/\\$&/g;
- s/\\\?/./g;
- s/\\\*/.*/g;
- push(@list,"^$_\$");
- }
- my $pat = join('|', @list);
- for (my $i = 0; $i <= $#contest; $i++) {
- next unless ($contest[$i] =~ m/$pat/o ||
- $altname[$i] =~ m/$pat/o ||
- $package[$i] =~ m/$pat/o);
- print infon($i), "\n";
- }
- exit(0);
-} elsif ($mode eq 'truename') {
- @ARGV == 1 || badusage(sprintf(_g("--%s needs a single argument"), "truename"));
- $file= $ARGV[0];
- for (my $i = 0; $i <= $#contest; $i++) {
- next unless $file eq $contest[$i];
- print $altname[$i], "\n";
- exit(0);
- }
- print $file, "\n";
- exit(0);
-} elsif ($mode eq 'listpackage') {
- @ARGV == 1 || badusage(sprintf(_g("--%s needs a single argument"), $mode));
- $file= $ARGV[0];
- for (my $i = 0; $i <= $#contest; $i++) {
- next unless $file eq $contest[$i];
- if ($package[$i] eq ':') {
- # indicate package is local using something not in package namespace
- print "LOCAL\n";
- } else {
- print $package[$i], "\n";
- }
- exit(0);
- }
- # print nothing if file is not diverted
- exit(0);
-} else {
- quit(sprintf(_g("internal error - bad mode \`%s'"), $mode));
-}
-
-sub infol {
- return ((defined($_[2]) ? ($_[2] eq ':' ? "local " : "") : "any ").
- "diversion of $_[0]".
- (defined($_[1]) ? " to $_[1]" : "").
- (defined($_[2]) && $_[2] ne ':' ? " by $_[2]" : ""));
-}
-
-sub checkrename {
- return unless $dorename;
- ($rsrc,$rdest) = @_;
- (@ssrc = lstat($rsrc)) || $! == ENOENT ||
- quit(sprintf(_g("cannot stat old name \`%s': %s"), $rsrc, $!));
- (@sdest = lstat($rdest)) || $! == ENOENT ||
- quit(sprintf(_g("cannot stat new name \`%s': %s"), $rdest, $!));
- # 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.
- if (open (TMP, ">>", "${rsrc}.dpkg-devert.tmp")) {
- close TMP;
- unlink ("${rsrc}.dpkg-devert.tmp");
- } elsif ($! == ENOENT) {
- $dorename = !$dorename;
- # 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.
- return;
- } else {
- quit(sprintf(_g("error checking \`%s': %s"), $rsrc, $!));
- }
-
- if (open (TMP, ">>", "${rdest}.dpkg-devert.tmp")) {
- close TMP;
- unlink ("${rdest}.dpkg-devert.tmp");
- } else {
- quit(sprintf(_g("error checking \`%s': %s"), $rdest, $!));
- }
- if (@ssrc && @sdest &&
- !($ssrc[0] == $sdest[0] && $ssrc[1] == $sdest[1])) {
- quit(sprintf(_g("rename involves overwriting \`%s' with\n".
- " different file \`%s', not allowed"), $rdest, $rsrc));
- }
-}
-
-sub rename_mv($$)
-{
- return (rename($_[0], $_[1]) || (system(("mv", $_[0], $_[1])) == 0));
-}
-
-sub dorename {
- return unless $dorename;
- return if $testmode;
- if (@ssrc) {
- if (@sdest) {
- unlink($rsrc) || quit(sprintf(_g("rename: remove duplicate old link \`%s': %s"), $rsrc, $!));
- } else {
- rename_mv($rsrc, $rdest) ||
- quit(sprintf(_g("rename: rename \`%s' to \`%s': %s"), $rsrc, $rdest, $!));
- }
- }
-}
-
-sub save {
- return if $testmode;
- open(N, "> $admindir/diversions-new") || quit(sprintf(_g("create diversions-new: %s"), $!));
- chmod 0644, "$admindir/diversions-new";
- for (my $i = 0; $i <= $#contest; $i++) {
- print(N "$contest[$i]\n$altname[$i]\n$package[$i]\n")
- || quit(sprintf(_g("write diversions-new: %s"), $!));
- }
- close(N) || quit(sprintf(_g("close diversions-new: %s"), $!));
- unlink("$admindir/diversions-old") ||
- $! == ENOENT || quit(sprintf(_g("remove old diversions-old: %s"), $!));
- link("$admindir/diversions","$admindir/diversions-old") ||
- $! == ENOENT || quit(sprintf(_g("create new diversions-old: %s"), $!));
- rename("$admindir/diversions-new","$admindir/diversions")
- || quit(sprintf(_g("install new diversions: %s"), $!));
-}
-
-sub infoa
-{
- infol($file, $divertto, $package);
-}
-
-sub infon
-{
- my $i = shift;
- infol($contest[$i], $altname[$i], $package[$i]);
-}
-
-sub quit
-{
- printf STDERR "%s: %s\n", $progname, "@_";
- exit(2);
-}
-
-sub badusage
-{
- printf STDERR "%s: %s\n\n", $progname, "@_";
- usage();
- exit(2);
-}
-
-sub badfmt
-{
- quit(sprintf(_g("internal error: %s corrupt: %s"), "$admindir/diversions", $_[0]));
-}
-
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;

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

dpkg_SOURCES = \
archives.c archives.h \
@@ -74,6 +75,14 @@ dpkg_trigger_LDADD = \
../lib/compat/libcompat.a \
$(LIBINTL)

+dpkg_divert_SOURCES = \
+ divert.c
+
+dpkg_divert_LDADD = \
+ ../lib/dpkg/libdpkg.a \
+ ../lib/compat/libcompat.a \
+ $(LIBINTL)
+
install-data-local:
$(mkdir_p) $(DESTDIR)$(pkgconfdir)/dpkg.cfg.d
$(mkdir_p) $(DESTDIR)$(admindir)/alternatives
diff --git a/src/divert.c b/src/divert.c
new file mode 100644
index 0000000..8695142
--- /dev/null
+++ b/src/divert.c
@@ -0,0 +1,734 @@
+/*
+ * dpkg - main program for package management
+ * divert.c - implementation of dpkg-divert(Cool
+ *
+ * Copyright © 2009 Mikhail Gusarov
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2,
+ * or (at your option) any later version.
+ *
+ * This is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with dpkg; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <config.h>
+#include <compat.h>
+
+#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>
+
+#include <dpkg/dpkg.h>
+#include <dpkg/i18n.h>
+#include <dpkg/dpkg-db.h>
+
+const char thisname[]= "dpkg-divert";
+
+static int quiet;
+static int testmode;
+static int dorename_;
+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;
+
+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);
+}
+
+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);
+}
+
+static void
+infol(const char *file, const char *divertto, const char *package,
+ struct varbuf *ret)
+{
+ if (package) {
+ if (strcmp(package, ":") == 0)
+ varbufaddstr(ret, "local ");
+ }
+ else
+ 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);
+ }
+
+ varbufaddc(ret, 0);
+}
+
+
+/* Returned value will be overwritten by next call to infoa() */
+static const char *
+infoa(const char *file)
+{
+ static struct varbuf ret;
+ varbufreset(&ret);
+ infol(file, divertto, package, &ret);
+ return ret.buf;
+}
+
+/* Returned value will be overwritten by next call to infon() */
+static const char *
+infon(const diversion *d)
+{
+ static struct varbuf ret;
+ varbufreset(&ret);
+ infol(d->contest, d->altname, d->package, &ret);
+ return ret.buf;
+}
+
+static void
+checkrename(const char *rsrc, const char *rdest)
+{
+ struct stat ssrc;
+ struct stat sdest;
+ int has_src;
+ int has_dest;
+ static struct varbuf tmpfilename;
+ int tmpfile;
+
+ if (!dorename_) return;
+
+ 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);
+
+ /* 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");
+ varbufaddc(&tmpfilename, 0);
+
+ tmpfile = open(tmpfilename.buf, O_WRONLY | O_CREAT);
+ if (tmpfile != -1) {
+ 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. */
+ 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);
+ }
+}
+
+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);
+}
+
+static void
+dorename(const char *rsrc, const char *rdest)
+{
+ if (!dorename_) return;
+ if (testmode) return;
+
+ struct stat ssrc;
+ struct stat sdest;
+
+ int has_src = !lstat(rsrc, &ssrc);
+ int has_dest = !lstat(rdest, &sdest);
+
+ if (!has_src) return;
+
+ if (has_dest) {
+ if (-1 == unlink(rsrc))
+ ohshite(_("rename: remove duplicate old link '%s'"),
+ rsrc);
+ } else {
+ if (-1 == rename_mv(rsrc, rdest))
+ ohshite(_("rename: rename '%s' to '%s'"), rsrc, rdest);
+ }
+}
+
+static void
+save(diversion *diversions)
+{
+ diversion *d;
+ static struct varbuf filename;
+ static struct varbuf new_filename;
+ static struct varbuf old_filename;
+ FILE *new_file;
+
+ if (testmode) return;
+
+ varbufreset(&new_filename);
+ varbufaddstr(&new_filename, admindir);
+ varbufaddstr(&new_filename, "/" DIVERSIONSFILE "-new");
+ 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)
+ {
+ 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");
+ 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;
+ }
+}
+
+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;
+}
+
+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 (;Wink
+ {
+ 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);
+}
+
+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"));
+
+ 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;
+ }
+ return -1;
+}
+
+static void
+op_add(diversion *diversions, const char *file)
+{
+ diversion *d;
+
+ if (file[0] != '/')
+ badusage(_("filename "%s" is not absolute"), file);
+ if (strchr(file, '\n'))
+ badusage(_("filename may not contain newlines"));
+ struct stat file_stat;
+ if (stat(file, &file_stat) == 0 && S_ISDIR(file_stat.st_mode))
+ badusage(_("Cannot divert directories"));
+
+ if (!divertto) {
+ static struct varbuf vb;
+ varbufreset(&vb);
+ varbufaddstr(&vb, file);
+ varbufaddstr(&vb, ".distrib");
+ divertto = vb.buf;
+ }
+
+ if (divertto[0] != '/')
+ badusage(_("filename "%s" is not absolute"), divertto);
+
+ if (!package)
+ package = ":";
+
+ diversions = invert_diversions_list(diversions);
+
+ for (d = diversions; d; d = d->next) {
+ if (strcmp(d->contest, file) == 0 ||
+ strcmp(d->altname, file) == 0 ||
+ strcmp(d->contest, divertto) == 0 ||
+ strcmp(d->altname, divertto) == 0) {
+ if (strcmp(d->contest, file) == 0 &&
+ strcmp(d->altname, divertto) == 0 &&
+ strcmp(d->package, package) == 0) {
+ if (!quiet)
+ printf(_("Leaving '%s'\n"), infon(d));
+ exit(0);
+ }
+ else
+ ohshit(_("'%s' clashes with '%s'"), infoa(file),
+ infon(d));
+ }
+ }
+
+ d = nfmalloc(sizeof(diversion));
+ d->contest = file;
+ d->altname = divertto;
+ d->package = package;
+ d->next = diversions;
+ diversions = d;
+
+ diversions = invert_diversions_list(diversions);
+
+ if (!quiet)
+ printf(_("Adding '%s'"), infon(d));
+
+ checkrename(file, divertto);
+ save(diversions);
+ dorename(file, divertto);
+ exit(0);
+}
+
+static void
+op_remove(diversion *diversions, const char *file)
+{
+ diversion *d;
+ for (d = diversions; d; d = d->next) {
+ if (strcmp(d->contest, file) == 0) {
+ if (divertto && strcmp(d->altname, divertto) != 0)
+ ohshit(_("mismatch on divert-to\n"
+ "when removing '%s'\n"
+ "found '%s"), infoa(file), infon(d));
+ if (package && strcmp(d->package, package) != 0)
+ ohshit(_("mismatch on package\n"
+ "when removing '%s'\n"
+ "found '%s'"), infoa(file), infon(d));
+ if (!quiet)
+ printf(_("Removing '%s'\n"), infon(d));
+
+ checkrename(d->altname, d->contest);
+ dorename(d->altname, d->contest);
+
+ diversions = diversions_remove(diversions, d);
+ save(diversions);
+ exit(0);
+ }
+ }
+
+ if (!quiet) {
+ printf(_("No diversion '%s', none removed"), infoa(file));
+ putchar('\n');
+ }
+ exit(0);
+}
+
+static void
+op_list(diversion *diversions, char *const *patterns)
+{
+ diversion *d;
+ for (d = diversions; d; d = d->next)
+ if (match_diversion(d, patterns) == 0)
+ printf("%s\n", infon(d));
+
+ exit(0);
+}
+
+static void
+op_truename(diversion *diversions, const char *file)
+{
+ diversion *d;
+ for (d = diversions; d; d = d->next)
+ if (strcmp(d->contest, file) == 0) {
+ printf("%s\n", d->altname);
+ exit(0);
+ }
+
+ printf("%s\n", file);
+ exit(0);
+}
+
+static void
+op_listpackage(diversion *diversions, const char *file)
+{
+ diversion *d;
+ for (d = diversions; d; d = d->next) {
+ if (strcmp(d->contest, file) == 0) {
+ if (strcmp(d->package, ":") == 0) {
+ /* indicate package is local using something not
+ * in package namespace */
+ printf("LOCAL\n");
+ }
+ else
+ printf("%s\n", d->package);
+ exit(0);
+ }
+ }
+ /* print nothing if file is not diverted */
+ exit(0);
+}
+
+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."));
+}
+
+typedef enum {
+ OPT_ADD = 1,
+ OPT_REMOVE,
+ OPT_LIST,
+ OPT_LISTPACKAGE,
+ OPT_TRUENAME,
+ OPT_PACKAGE,
+ OPT_LOCAL,
+ OPT_DIVERT,
+ OPT_RENAME,
+ OPT_ADMINDIR,
+ OPT_TEST,
+ OPT_QUIET,
+ OPT_HELP,
+ OPT_VERSION,
+} options;
+
+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 },
+};
+
+static void
+checkmanymodes(const char *curmode, const char *newmode)
+{
+ if (!curmode)
+ return;
+ badusage(_("two commands specified: --%s and --%s"), newmode, curmode);
+}
+
+int
+main(int argc, char *const *argv)
+{
+ jmp_buf ejbuf;
+
+ standard_startup(&ejbuf);
+
+ const char *mode = NULL;
+ admindir = ADMINDIR;
+
+ int opt;
+ while ((opt = getopt_long(argc, argv, ":", long_options, NULL)) != -1) {
+ switch (opt) {
+ case OPT_HELP:
+ usage();
+ return 0;
+ case OPT_VERSION:
+ version();
+ return 0;
+ case OPT_TEST:
+ testmode = 1;
+ break;
+ case OPT_RENAME:
+ dorename_ = 1;
+ break;
+ case OPT_QUIET:
+ quiet = 1;
+ break;
+ case OPT_LOCAL:
+ package = ":";
+ break;
+ case OPT_ADD:
+ checkmanymodes(mode, "add");
+ mode = "add";
+ break;
+ case OPT_REMOVE:
+ checkmanymodes(mode, "remove");
+ mode = "remove";
+ break;
+ case OPT_LIST:
+ checkmanymodes(mode, "list");
+ mode = "list";
+ break;
+ case OPT_LISTPACKAGE:
+ checkmanymodes(mode, "listpackage");
+ mode = "listpackage";
+ break;
+ case OPT_TRUENAME:
+ checkmanymodes(mode, "truename");
+ mode = "truename";
+ break;
+ case OPT_DIVERT:
+ if(strchr(optarg, '\n'))
+ badusage(_("divert-to may not contain newlines"));
+ divertto = optarg;
+ break;
+ case OPT_PACKAGE:
+ if(strchr(optarg, '\n'))
+ badusage(_("package may not contain newlines"));
+ package = optarg;
+ break;
+ case OPT_ADMINDIR:
+ admindir = optarg;
+ break;
+ case ':':
+ if(optopt == OPT_DIVERT)
+ badusage(_("--divert needs a divert-to argument"));
+ if(optopt == OPT_PACKAGE)
+ badusage(_("--package needs a package argument"));
+ if(optopt == OPT_ADMINDIR)
+ badusage(_("--admindir needs an admindir argument"));
+ ohshit(_("internal error: unknown option %d"), optopt);
+ default:
+ badusage(_("unknown option '%s'"), argv[optind]);
+ }
+ }
+
+ diversion* diversions = read_diversions();
+
+ if (!mode || !strcmp(mode, "add")) {
+ if (optind != argc - 1)
+ badusage(_("--%s needs a single argument"), "add");
+ op_add(diversions, argv[optind]);
+ }
+ if (!strcmp(mode, "remove")) {
+ if (optind != argc - 1)
+ badusage(_("--%s needs a single argument"), "remove");
+ op_remove(diversions, argv[optind]);
+ }
+ if (!strcmp(mode, "list")) {
+ if (optind >= argc) {
+ char *const null_pattern[] = { "*", NULL };
+ op_list(diversions, null_pattern);
+ } else {
+ op_list(diversions, argv + optind);
+ }
+ }
+
+ if (!strcmp(mode, "truename")) {
+ if (optind != argc - 1)
+ badusage(_("--%s needs a single argument"), mode);
+ op_truename(diversions, argv[optind]);
+ }
+
+ if (!strcmp(mode, "listpackage")) {
+ if (optind != argc - 1)
+ badusage(_("--%s needs a single argument"), mode);
+ op_listpackage(diversions, argv[optind]);
+ }
+
+ ohshit(_("internal error - bad mode '%s'"), mode);
+}
+
+/*
+ * Local Variables:
+ * mode: c
+ * c-file-style: "linux"
+ * c-basic-offset: 8
+ * tab-width: 8
+ * indent-tabs-mode: t
+ * End:
+ */
--
1.6.3.3


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org
Back to top
Guillem Jover
External


Since: Nov 13, 2004
Posts: 478



PostPosted: Fri Oct 16, 2009 5:10 pm    Post subject: Re: [PATCH 0/2] Rewrite of dpkg-divert to C patch series v2 [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)

On Sat, 2009-10-17 at 01:40:02 +0700, Mikhail Gusarov wrote:
> Twas brillig at 02:36:43 14.10.2009 UTC+07 when dottedmag DeleteThis @dottedmag.net did gyre and gimble:
>
> *ping*
>
> Any comments, ACKs, NACKs?

Sure, started reviewing the other day but postponed as it was taking
too long. It's coming.

regards,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org
Back to top
Guillem Jover
External


Since: Nov 13, 2004
Posts: 478



PostPosted: Sat Oct 17, 2009 1:10 am    Post subject: Re: [PATCH 2/2] Reimplement dpkg-divert in C [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)

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(Cool

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. Smile

> + 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 (;Wink
> + {
> + 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


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org
Back to top
Guillem Jover
External


Since: Nov 13, 2004
Posts: 478



PostPosted: Sat Oct 17, 2009 9:10 am    Post subject: Re: [PATCH 2/2] Reimplement dpkg-divert in C [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)

On Sat, 2009-10-17 at 14:58:55 +0700, Mikhail Gusarov wrote:
> Twas brillig at 02:05:47 17.10.2009 UTC+02 when guillem.RemoveThis@debian.org did gyre and gimble:
> GJ> Instead of inverting the list after the fact, just add to its tail
> GJ> instead. OTOH I don't think this should be needed at all.
>
> I have tried to replicate dpkg-divert.pl behaviour exactly, including
> sorting of diversions file. If it is not necessary, I could make a
> testsuite a bit smarter instead.

I mean that it should not be needed at all once the code uses the
divertdb infrastructure. But it seems the db parser is prepending the
diversions to the head of the list, so it will need to be changed to
append to the tail instead.

The behaviour should be preserved, yes, but the code layout does not
need to, just what fits best with the rest of the code base. In this
case duplicating part of the current infrastructure is not good. And
we might need to expose the functionality to write the db and add new
diversion from dpkg-divert to libdpkg at some point in the future.

> >> +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;
>
> GJ> Why the FNM_NOESCAPE?
>
> To match Perl code - '\' is escaped there. Not that it really matters.

The --list option is supposed to be taking a glob as argument, the perl
code was trying to convert it into a regex. And there, all non word
characters get escaped to protect the regex. My take is that the coder
probably just didn't bother handling the escape case, which seems
useful to me now that we get it for free.

regards,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org
Back to top
Goswin von Brederlow
External


Since: Feb 09, 2009
Posts: 90



PostPosted: Thu Oct 22, 2009 6:10 am    Post subject: Re: [PATCH 2/2] Reimplement dpkg-divert in C [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)

Mikhail Gusarov <dottedmag.TakeThisOut@dottedmag.net> writes:

> Twas brillig at 02:05:47 17.10.2009 UTC+02 when guillem.TakeThisOut@debian.org did gyre and gimble:
>
> >> + 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);
> >> + }
>
> GJ> Hmm, this makes translators work pretty hard. Also carried over from
> GJ> the perl code, but as this is getting rewritten let's get it right.
>
> Is --list supposed to be only human-readable? Then we can change output
> format a bit to make it more localizable (12 similar strings like "local
> diversion of %s to %s by %s", "diversion of %s", "any diversion of %s by
> %s"... are localizable, but insane).

I think some maintainer scripts use --list. But I think they only care
wether there is a diversion. Maybe a --test for testing if something
is already diverted by a previous version and --list for users would
be better.

MfG
Goswin


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST.TakeThisOut@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.TakeThisOut@lists.debian.org
Back to top
Guillem Jover
External


Since: Nov 13, 2004
Posts: 478



PostPosted: Thu Nov 19, 2009 3:10 am    Post subject: Re: [PATCH 2/2] Reimplement dpkg-divert in C [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)

Hi!

On Tue, 2009-10-20 at 15:00:31 +0700, Mikhail Gusarov wrote:
> Twas brillig at 02:05:47 17.10.2009 UTC+02 when guillem.RemoveThis@debian.org did gyre and gimble:
>
> >> + 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);
> >> + }
>
> GJ> Hmm, this makes translators work pretty hard. Also carried over from
> GJ> the perl code, but as this is getting rewritten let's get it right.
>
> Is --list supposed to be only human-readable?

I'd expect so, also if anyone is actually parsing it, they should be
making sure to pass LC_ALL=C to guarantee a constant locale. And
they'd have to be prepared to parse all variants anyway.

> Then we can change output format a bit to make it more localizable (12
> similar strings like "local diversion of %s to %s by %s",
> "diversion of %s", "any diversion of %s by %s"... are localizable,
> but insane).

Yes please, also there should not be that many possibilities, check
src/query.c:searchoutput(). The package name should either be a
pointer or NULL (in case of local diversions), so the "any " does
not apply, and cannot see where it did in the perl code either.

regards,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org
Back to top
Guillem Jover
External


Since: Nov 13, 2004
Posts: 478



PostPosted: Thu Nov 19, 2009 3:10 am    Post subject: Re: [PATCH 2/2] Reimplement dpkg-divert in C [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)

Hi!

On Thu, 2009-10-22 at 11:29:04 +0200, Goswin von Brederlow wrote:
> Mikhail Gusarov <dottedmag RemoveThis @dottedmag.net> writes:
> > Is --list supposed to be only human-readable? Then we can change output
> > format a bit to make it more localizable (12 similar strings like "local
> > diversion of %s to %s by %s", "diversion of %s", "any diversion of %s by
> > %s"... are localizable, but insane).
>
> I think some maintainer scripts use --list. But I think they only care
> wether there is a diversion. Maybe a --test for testing if something
> is already diverted by a previous version and --list for users would
> be better.

dpkg-divert already has a --test option which is a dry-run. Also
there's already --listpackage and --truename to be used for such
automated checks.

regards,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org
Back to top
Display posts from previous:   
Post new topic   General Reply to Topic (not reply to a specific post)    Forums Home -> PacKaGe All times are: Eastern Time (US & Canada) (change)
Page 1 of 1

 
You can post new topics in this forum
You can reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum