Re: Review of file exclusion branch requested

[Guillem Jover <guillem@debian.org>]

On Sun, 2008-01-20 at 23:37:25 +0100, Tollef Fog Heen wrote:
> * Guillem Jover 
> | On Sat, 2008-01-05 at 11:18:13 +0100, Tollef Fog Heen wrote:

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

Ah, right! was scanning the patch from top to bottom, and missed the
fact that those checks were done in loadfilters. This is fine then.

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

Just did that, I was waiting until after the release, introduced
m_strdup as well, you can use it now. And you can remove the checks
for NULL after m_ functions (those are not supposed to ever return
that).

> | +    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 filters has been initialized before, filtertail will not be the
second time, and it will be dereferencing an undefined pointer.

What I was suggesting was, outside the functions something like:

    static struct filterlist *filters = NULL;
    static struct filterlist **filtertail = &filters;

and in loadfilter() just:

    *filtertail = filter;
    filtertail = &filter->next;

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

Sure, and yeah, I didn't test it either, but I think we should be able
to merge it for .17 once both of us are happy with it. :)

thanks,
guillem