From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH] squid: Update-suggestion for initscript Date: Wed, 11 May 2016 23:01:38 +0100 Message-ID: <1463004098.18591.237.camel@ipfire.org> In-Reply-To: <1462992406-1395-1-git-send-email-matthias.fischer@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6290394198804188968==" List-Id: --===============6290394198804188968== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, great work so far getting to the bottom of all this! Just a few comments and questions: On Wed, 2016-05-11 at 20:46 +0200, Matthias Fischer wrote: > Path fix for squid. >=20 > Changed loop detecting if 'squid' is still running. >=20 > Added '/bin/rm -f /var/log/cache/swap.state' after killing 'squid'. Please include why you added this. I can see that you added the command but I= am not sure why this is needed. You should include this as a comment in the scri= pt. >=20 > Changed flush option to '/bin/rm -f /var/log/cache/swap.state'. >=20 > Signed-off-by: Matthias Fischer > --- > =C2=A0src/initscripts/init.d/squid | 47 +++++++++++++++++++++++++++--------= ------ > --- > =C2=A01 file changed, 29 insertions(+), 18 deletions(-) >=20 > diff --git a/src/initscripts/init.d/squid b/src/initscripts/init.d/squid > index c641c7d..eecad12 100644 > --- a/src/initscripts/init.d/squid > +++ b/src/initscripts/init.d/squid > @@ -33,21 +33,22 @@ transparent() { > =C2=A0 CONN_TYPE=3D`echo "$LINE" | awk -F, '{ print $5 }'` > =C2=A0 if [ "$CONN_TYPE" !=3D "net" ]; then > =C2=A0 continue > - fi=C2=A0=C2=A0 > -=C2=A0=C2=A0 iptables -t nat -A SQUID -i $1 -p tcp -d `echo "$LINE" | > awk -F, '{ print $13 }'` --dport 80 -j RETURN > + fi > + iptables -t nat -A SQUID -i $1 -p tcp -d `echo "$LINE" | awk > -F, '{ print $13 }'` --dport 80 -j RETURN > =C2=A0 done < $FILE > - =09 > + > =C2=A0 if [ "$RED_TYPE" =3D=3D "STATIC" ]; then > =C2=A0 iptables -t nat -A SQUID -i $1 -p tcp -d > $RED_NETADDRESS/$RED_NETMASK --dport 80 -j RETURN > =C2=A0 fi > - =09 > + > =C2=A0 iptables -t nat -A SQUID -i $1 -p tcp -d $LOCALIP --dport 80 > -j RETURN > - =09 > + > =C2=A0 iptables -t nat -A SQUID -i $1 -p tcp --dport 80 -j REDIRECT > --to-port "${TRANSPARENT_PORT}" > =C2=A0} Not sure what happened here. The lines do not seem to have changed. If your editor changed that, please reconfigure it to not do this any more. If this was intentional, please send a second patch for any changes that have= no relevance to what is fixed in this commit. > =C2=A0case "$1" in > =C2=A0 start) > + > =C2=A0 getpids "squid" > =C2=A0 > =C2=A0 if [ -n "${pidlist}" ]; then Same here. When ever re-formatting code is needed, do that in a different pat= ch. These empty lines there should not be there though. > @@ -93,14 +94,16 @@ case "$1" in > =C2=A0 ;; > =C2=A0 > =C2=A0 stop) > + > =C2=A0 iptables -t nat -F SQUID > =C2=A0 if [ -e /var/run/squid.pid ]; then > =C2=A0 boot_mesg "Stopping Squid Proxy Server..." > - squid -k shutdown >/dev/null 2>&1 > + /usr/sbin/squid -k shutdown >/dev/null 2>&1 > =C2=A0 evaluate_retval Why is the full path name needed? It shouldn't. > =C2=A0 > =C2=A0 # Stop squidGuard, updxlrator, squidclamav > - # and redirect_wrappers. =09 > + # and redirect_wrappers. > + > =C2=A0 killproc /usr/bin/squidGuard >/dev/null & > =C2=A0 killproc /usr/sbin/updxlrator >/dev/null & > =C2=A0 killproc /usr/bin/squidclamav >/dev/null & > @@ -109,22 +112,30 @@ case "$1" in > =C2=A0 # Wait until all redirectors have been stopped. > =C2=A0 wait > =C2=A0 > - # If squid is still running, wait up to 30 seconds > + # If squid is still running, wait up to 120 seconds > =C2=A0 # before we go on to kill it. > - counter=3D30 > =C2=A0 > - while [ ${counter} -gt 0 ]; do > - statusproc /usr/sbin/squid >/dev/null && > break; > + n=3D0 > + while /usr/sbin/squid -k check && [ $n -lt 120 ]; do > =C2=A0 sleep 1 > - counter=3D$(( ${counter} - 1)) > + echo -n . > + n=3D`/usr/bin/expr $n + 1` > + > =C2=A0 done You can use the same syntax that was used previously to increment n: =C2=A0 n=3D$(( ${n} + 1 )) That avoids starting a new process and compute the result and lets the shell = do it. > =C2=A0 > - # Kill squid service, if still running. > +# Kill squid service, if still running. > =C2=A0 killproc /usr/sbin/squid >/dev/null Comments should be indented as well. > =C2=A0 > - # Trash remain pid file from squid. > - rm -rf /var/run/squid.pid=09 > +# Delete - damaged - /var/log/cache/swap.state > + /bin/rm -f /var/log/cache/swap.state > + > + fi See comment above. > + > + # Delete remaining pid file from squid if it STILL exists. > + if [ -e '/var/run/squid.pid' ]; then > + rm -rf /var/run/squid.pid > =C2=A0 fi If you run rm with the -f switch, it will delete the file if it is there and = do nothing if it does not exit. So checking before is not needed. > + > =C2=A0 ;; > =C2=A0 > =C2=A0 restart) > @@ -141,14 +152,14 @@ case "$1" in > =C2=A0 statusproc /usr/sbin/squid > =C2=A0 statusproc /usr/lib/squid/unlinkd > =C2=A0 ;; > - =09 > + > =C2=A0 flush) > =C2=A0 $0 stop > - echo > /var/log/cache/swap.state > - chown squid.squid /var/log/cache/swap.state > + /bin/rm -f /var/log/cache/swap.state > =C2=A0 sleep 1 > =C2=A0 $0 start > =C2=A0 ;; > + > =C2=A0 setperms) > =C2=A0 chown -R nobody.squid /var/updatecache/ > =C2=A0 ;; Best, -Michael --===============6290394198804188968== Content-Type: application/pgp-signature Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="signature.asc" MIME-Version: 1.0 LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KVmVyc2lvbjogR251UEcgdjIKCmlRSWNCQUFC Q2dBR0JRSlhNNnZEQUFvSkVJQjU4UDl2a0FrSDd6QVAvMlltbXBxVkxNK3M3RGt2K1U1Qk9nREEK N28xbXdSaUczdSs5eXlQeGRWZWsxbC9KRVU3bkI0M2RJTzgvQXA3OHJvT0hEYkM2VVR1RzROUzBS dmczZG5sVApNbCtQTGJ6WWVCMzBUc1A0VThYNGdKQUFYTmE2THJSdWVoNC8zdTE3Umlzay8vQlFK RUQwVEIreFgzVWJLUXR3CnNwYThXVFhSdUtrUUU2K1R4K2VjKzFMWGEvQ0tRS3NlRVg5emphTnVl STZVMEYxWmlZVTN4WDNXbUlLTlBKc2wKM3BrYUU0SW9uYWhSdHBaVjhtYW85QVptcUczMVE5THdN KzFzOVE0UFU0QlQwSDlidXByYStSYUwzZEVxMHlibAp2N24rUXNSZXB1b0ViL3ZvNDVvZVprdjBY UkxyT1hHR296TGtDMW4vbFNRUEU0S2FTdm5QTldXTTNCeXNJTDdHClRMUHY1NHZyWTJJMy9vTmJJ NzNTd1QyMmd3em9odXIwcy9mM2JMSm9ZSGlFQS95aUp3QUpYN1hmNjNpcHBzaUwKN01XaWZGWWds QkloWGVsMG5aVUFrNkd1VFhxUTZuUVNwN1BQLzA2RStBeFhWWFZvRVcwYjdFK3NrZCttTWNjVwp4 ZXFkZzJuWHNUVUg2WkJoVnZKUk1GQnpQc3drYy9IVEJvT2hxdlhVU0hhclBHZ1kxVUpsZ3RQK2Ja YkJiWWZWClVBSGxZcW9OVnZTeU90ZnQwSjVjczBPVGlyL1hLbUEzUlFhTGxGeVYwUGZmTTF2ZzdI R1BnNWhNMVJZdDk3bE4KZzluSDZ6VGVEVXpnQTAwZmM5U1NiOUsxZWkxenE3QkpyWDd2UlJCNUl2 WmN4RVdrNmkvT1RyRUFTdFdpWm5UcQpFU2JoKy9KUGxTaUZZNE5XcWc0Swo9cXFpbAotLS0tLUVO RCBQR1AgU0lHTkFUVVJFLS0tLS0K --===============6290394198804188968==--