[Bug 165689] Review Request: SquidGuard: filter, redirector and access controller plugin for squid

bugzilla at redhat.com bugzilla at redhat.com
Mon Sep 5 08:24:02 UTC 2005

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: SquidGuard: filter, redirector and access controller plugin for squid


------- Additional Comments From oliver at linux-kernel.at  2005-09-05 04:24 EST -------
Thanks for your review!

(In reply to comment #1)
> * the logrotate file contains "olddir /var/log/old/squid". This is not standard,
> please remove it.


> * this file should also be tagged as %config(noreplace) in case the user makes
> changes.


> * in the package's description, there are "**)" symbols. What does that mean ?
> Maybe a bad copy/paste ?

Of course - removed.

> * Source3 is guard-distrib.tar.gz: where does it come from ? It is copied to
> samples/, but not included in the rpm (not in the %files section). If it is not
> useful, please remove it.


> * SMP flags are not used. If it does not build with it, please write a small
> comment above the make command

Added smpmflags

> * the checks "RPM_BUILD_ROOT != /" are useless in %install and %clean, please
> remove them.


> * use install -p to preserve timestamps

OK - Done.

> * the "mkdir -p $RPM_BUILD_ROOT%{_dbhomedir}" line in %install is useless, the
> dir was created with install above


> * "%dir %{_dbhomedir}" and "%{_dbhomedir}/*" can be summed up in simply
> "%{_dbhomedir}/"


> * don't create $RPM_BUILD_ROOT%{_datadir}/%{name} in %install, since there are
> no files in it.


> * Source 2 is not available anymore. In the same directory, there is a
> blacklists.tar.gz file which seems to be the same as the versionned one. Maybe
> you could use that.

Changed. There should be always a timestamped one and one without timestamp,
which is the same. I removed the _bldate macro and use the non-timestamped one now.

> * BuildRoot: should be
> %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Yes, it should... 

> * The package should contain the text of the license. Please include the COPYING
> file.

Done. Shall I also include the GPL file from the tarball?

> * The default squidGuard.conf file does not work since your blacklists are in
> %dbhomedir/blacklists and the file refers to %dbhomedir/dest. It would be nice
> to make it work by default, and a simple "s,dest/adult/,blacklists/porn/,g" on
> the file should fix it.

Fixed with the provided regex. I didn't fix it, because I thought, that the
admin who installs it, will have a look at the config...

> There is still the problem of the invalid redirection,
> but I don't think we can do much about that, can we ?

I would have to install the guard.cgi and guard.conf into some place and create
an Apache config. But we'll never know the hostname/FQDN of the host running
squidGuard. It could also be possible, that the admin has no webserver on the
proxy-machine and want's to install the cgi on another server. So I think we
_can_ do something about that, but it doesn't make much sense... :-(

> * a possible improvement would be to add a script to update the blacklists. Many
> are available on the web, you could for example try this one:
> http://cuda.port-aransas.k12.tx.us/squid-getlist.html or base yours on it. This
> is not a blocker, but could be a nice improvement if you are interested in
> squidGuard.

Added this and slightly modified it (paths). I also added a var called ENABLED,
which is 0 (therfor disabled) by default...

Build 8. Please have a look.

Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

More information about the fedora-extras-list mailing list