Re: Review of file exclusion branch requested
* Guillem Jover
| 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.
Will do.
| > 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:
Sure.
| 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.
Moved to dpkg's main function.
| 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.
Fixed.
| + 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.
No, this is loadfilter, not loadfilters. loadfilters does:
char *dirname = CONFIGDIR "/filters.d";
DIR *dir = opendir(dirname);
if (!dir) {
if (errno == ENOENT) {
return NULL;
} else {
ohshite(_("Error opening filters.d"));
}
}
while ((dent = readdir(dir)) != NULL) {
[...]
if (S_ISREG(statbuf.st_mode)) {
loadfilter(file);
}
So the only case you'd see the «failed to open filter file» error is
if somebody (presumably with root access) removed a file between
readdir returning the file name and loadfilter getting around to
opening it. If you think this is a problem, I can make loadfilter
take in a FILE * or file descriptor instead and have loadfilters be
responsible for opening the file.
| + 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.
Fixed. I notice there's quite a lot of normal malloc calls around the
code. Should we fix those to be m_malloc as well?
| + 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).
Fixed.
| + 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.
I'm probably dense, but what do you mean by this?
| + }
| +
| + 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.
Both fixed.
| +void loadfilters() {
|
| Argument should be 'void'. And this function is missing a declaration
| in the header.
Fixed.
| + 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.
Ok, fixed.
| + 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.
Fixed.
| + 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.
Done.
| 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.
Moved.
[...]
| Empty line.
[...]
| Tab on empty line.
[...]
| m_malloc.
[...]
| Tab on empty line.
[...]
| Please wrap at 80 chars.
[...]
| Trailing space.
All done.
[...]
| 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.
Done.
[...]
| And as this has been copied from another part of the code, it could be
| refactored into a new function (in a different patch).
Done.
I haven't squashed the history yet (how do I do that?), but I've at
least pushed what I have, so you can take a look (or point and laugh
:-), at your leisure.
Note that the current code in my git branch is untested, so I won't
formally ask for a merge until I have something I'm happier with and
have tested.
--
Tollef Fog Heen
UNIX is user friendly, it's just picky about who its friends are
Reply to: