Re: Review of file exclusion branch requested
Hi,
On Sat, 2008-01-05 at 11:18:13 +0100, Tollef Fog Heen wrote:
> I've finally gotten around to fixing up my support for excluding bits
> of packages as they are unpacked. It can be gotten from
> git://git.err.no/dpkg in the master branch (sorry about that, it
> should probably have gone in a separate branch).
Thanks for working on this. For next time please use git-format-patch,
it makes it easier to review, also it allows one to fix the patches
and resend.
> It's still missing in the documentation department, but it seems to
> work fine for me in my somewhat light testing.
It be really nice if you could write them before I merge this. So few
comments in addition to what Frank wrote:
diff --git a/lib/dpkg.h b/lib/dpkg.h
index ff3ac9a..f1e0c41 100644
--- a/lib/dpkg.h
+++ b/lib/dpkg.h
@@ -181,6 +181,7 @@ extern const char printforhelp[];
umask(022); /* Make sure all our status databases are readable. */\
if (loadcfg)\
loadcfgfile(prog, cmdinfos);\
+ loadfilters();\
myopt(argv,cmdinfos);\
} while (0)
This would get called on all programs, but only dpkg uses the filters,
just call it from dpkg main.
diff --git a/lib/myopt.c b/lib/myopt.c
index 5aed2f3..6defe60 100644
--- a/lib/myopt.c
+++ b/lib/myopt.c
@@ -164,3 +169,91 @@ void myopt(const char *const **argvp, const struct cmdinfo *cmdinfos) {
}
}
}
+
+struct filterlist *filters = NULL;
+
+void loadfilter(char *fn) {
+ FILE* file;
Asterisk near the variable not the type.
+ char linebuf[1024];
+ struct filterlist *filtertail;
+
+ file = fopen(fn, "r");
+ if (!file) {
+ warningf(_("failed to open filter file `%.255s' for reading"), fn);
+ return;
+ }
This will give a warning on all systems that do not have that file,
either this should check for ENOENT or we ship an empty one, the
former being better I'd say.
+ while (fgets(linebuf, sizeof(linebuf), file)) {
+ struct filterlist *filter;
+
+ filter = malloc(sizeof(struct filterlist));
+ if (!filter) {
+ ohshite(_("Error allocating memory for filter entry"));
+ }
Use m_malloc instead.
+ filter->filterstring = malloc(strlen(linebuf));
+ if (!filter->filterstring) {
+ ohshite(_("Error allocating memory for filter entry"));
+ }
+ strcpy(filter->filterstring, &linebuf[1]);
You can use strdup here (will be replaced later on with m_strdup).
+ if (! filters) {
+ filters = filter;
+ filtertail = filter;
+ } else {
+ filtertail->next = filter;
+ filtertail = filtertail->next;
+ }
You could do the same w/o special casing the first element by making
filtertail **. Also you'll have to move filtertail outside the
function, otherwise it will not support being called more than once.
+ }
+
+ if (ferror(file)) ohshite(_("read error in configuration file `%.255s'"), fn);
+ if (fclose(file)) ohshite(_("error closing configuration file `%.255s'"), fn);
Please wrap after the if conditional.
+
+}
Empty line.
+void loadfilters() {
Argument should be 'void'. And this function is missing a declaration
in the header.
+ struct dirent *dent;
+ char *dirname = CONFIGDIR "/filters.d";
+ DIR *dir = opendir(dirname);
+ if (!dir) {
+ if (errno == ENOENT) {
+ return NULL;
+ } else {
+ ohshite(_("Error opening filters.d"));
+ }
+ }
In general, no need for braces when it's non-ambiguous and there's only
one statement line.
+ while ((dent = readdir(dir)) != NULL) {
+ struct stat statbuf;
+ char *file = malloc(strlen(dirname) + 1 + strlen(dent->d_name) + 1);
+ if (!file)
+ ohshite(_("Error allocating memory for file"));
m_malloc.
+ sprintf(file, "%s/%s", dirname, dent->d_name);
+ if (stat(file, &statbuf) != 0) {
+ ohshite(_("Error stating file"));
+ }
+ if (S_ISREG(statbuf.st_mode)) {
+ loadfilter(file);
+ }
+ free(file);
+ }
+ closedir(dir);
+}
Only dpkg will be using this so I think it's better to split it into
src/filters.c so that when statically linking the code is not uselessly
duped on all other programs, and we put related code into the same place.
diff --git a/lib/myopt.h b/lib/myopt.h
index cb59f82..ffa27e3 100644
--- a/lib/myopt.h
+++ b/lib/myopt.h
@@ -39,4 +39,12 @@ struct cmdinfo {
void myfileopt(const char* fn, const struct cmdinfo* cmdinfos);
void myopt(const char *const **argvp, const struct cmdinfo *cmdinfos);
void loadcfgfile(const char *prog, const struct cmdinfo *cmdinfos);
+void loadfilter(char *fn);
+
+struct filterlist {
+ int positive;
+ char *filterstring;
+ struct filterlist *next;
+};
+
This does not feel like it has much to do with option parsing.
Probably best moved to src/filters.h as well.
diff --git a/src/archives.c b/src/archives.c
index df21d27..a123940 100644
--- a/src/archives.c
+++ b/src/archives.c
@@ -33,6 +33,7 @@
#include <fcntl.h>
#include <sys/stat.h>
#include <sys/types.h>
+#include <fnmatch.h>
#include <obstack.h>
#define obstack_chunk_alloc m_malloc
@@ -367,6 +368,8 @@ static int linktosameexistingdir(const struct TarInfo *ti,
return 1;
}
+extern struct filterlist *filters;
+
int tarobject(struct TarInfo *ti) {
static struct varbuf conffderefn, hardlinkfn, symlinkfn;
const char *usename;
@@ -403,6 +406,63 @@ int tarobject(struct TarInfo *ti) {
nifd->namenode->divert && nifd->namenode->divert->useinstead
? nifd->namenode->divert->useinstead->name : "<none>");
+ if (filters) {
+ int remove = 0;
+ struct filterlist *f;
+
+ /* Last match wins */
+ for (f = filters; f != NULL; f = f->next) {
+ debug(dbg_eachfile, "tarobject comparing '%s' and '%s'", &ti->Name[1], f->filterstring);
+ if (fnmatch(f->filterstring, &ti->Name[1], 0) == 0) {
+ if (f->positive == 0) {
+ remove = 1;
+ debug(dbg_eachfile, "tarobject removing %s", ti->Name);
+ } else {
+ remove = 0;
+ debug(dbg_eachfile, "tarobject including %s", ti->Name);
+
Empty line.
+ }
+ }
+ }
+
+ if (remove) {
+ for (f = filters; f != NULL; f = f->next) {
+ char *pattern;
+
Tab on empty line.
+ pattern = malloc(strlen(ti->Name) + 1);
m_malloc.
+ strcpy(pattern, &ti->Name[1]);
+ strcat(pattern, "*");
+
Tab on empty line.
+ debug(dbg_eachfile, "tarobject seeing if '%s' needs to be reincluded", &ti->Name[1]);
Please wrap at 80 chars.
+ if ((f->positive == 1) &&
Trailing space.
+ (ti->Type == Directory) &&
+ (fnmatch(pattern, f->filterstring, 0) == 0)) {
+ remove = 0;
+ debug(dbg_eachfile, "tarobject reincluding %s", ti->Name);
+ }
+ }
+ }
This part of the filtering logic should probably be moved into a
different file and function just returning a boolean with the action
to take.
+ if (remove) {
+ struct filenamenode *fnn = findnamenode(ti->Name, 0);
+ fnn->flags &= ~fnnf_new_inarchive;
+ obstack_free(&tar_obs, nifd);
+ tc->newfilesp= oldnifd;
+ *oldnifd = 0;
+
+ /* We need to advance the tar file to the next object, so read the
+ * file data and set it to oblivion.
+ */
+ if ((ti->Type == NormalFile0) || (ti->Type == NormalFile1)) {
+ char fnamebuf[256];
+ fd_null_copy(tc->backendpipe, ti->Size, _("gobble replaced file `%.255s'"),quote_filename(fnamebuf,256,ti->Name));
+ r= ti->Size % TARBLKSZ;
+ if (r > 0) r= safe_read(tc->backendpipe,databuf,TARBLKSZ - r);
+ }
+ return 0;
+ }
+ }
And as this has been copied from another part of the code, it could be
refactored into a new function (in a different patch).
regards,
guillem
Reply to: