Re: [PATCH] squid: Update-suggestion for initscript

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 4885 bytes --]

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.
> 
> Changed loop detecting if 'squid' is still running.
> 
> 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 script.

> 
> Changed flush option to '/bin/rm -f /var/log/cache/swap.state'.
> 
> Signed-off-by: Matthias Fischer <matthias.fischer(a)ipfire.org>
> ---
>  src/initscripts/init.d/squid | 47 +++++++++++++++++++++++++++--------------
> ---
>  1 file changed, 29 insertions(+), 18 deletions(-)
> 
> 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() {
>  			CONN_TYPE=`echo "$LINE" | awk -F, '{ print $5 }'`
>  			if [ "$CONN_TYPE" != "net" ]; then
>  				continue
> -			fi  
> -  		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
>  		done < $FILE
> -		
> +
>  		if [ "$RED_TYPE" == "STATIC" ]; then
>  			iptables -t nat -A SQUID -i $1 -p tcp -d
> $RED_NETADDRESS/$RED_NETMASK --dport 80 -j RETURN
>  		fi
> -		
> +
>  		iptables -t nat -A SQUID -i $1 -p tcp -d $LOCALIP --dport 80
> -j RETURN
> -		
> +
>  		iptables -t nat -A SQUID -i $1 -p tcp --dport 80 -j REDIRECT
> --to-port "${TRANSPARENT_PORT}"
>  }

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.

>  case "$1" in
>  	start)
> +
>  		getpids "squid"
>  
>  		if [ -n "${pidlist}" ]; then

Same here. When ever re-formatting code is needed, do that in a different patch.

These empty lines there should not be there though.

> @@ -93,14 +94,16 @@ case "$1" in
>  		;;
>  
>  	stop)
> +
>  			iptables -t nat -F SQUID
>  		if [ -e /var/run/squid.pid ]; then
>  			boot_mesg "Stopping Squid Proxy Server..."
> -			squid -k shutdown >/dev/null 2>&1
> +			/usr/sbin/squid -k shutdown >/dev/null 2>&1
>  			evaluate_retval

Why is the full path name needed? It shouldn't.

>  
>  			# Stop squidGuard, updxlrator, squidclamav
> -			# and redirect_wrappers.			
> +			# and redirect_wrappers.
> +
>  			killproc /usr/bin/squidGuard >/dev/null &
>  			killproc /usr/sbin/updxlrator >/dev/null &
>  			killproc /usr/bin/squidclamav >/dev/null &
> @@ -109,22 +112,30 @@ case "$1" in
>  			# Wait until all redirectors have been stopped.
>  			wait
>  
> -			# If squid is still running, wait up to 30 seconds
> +			# If squid is still running, wait up to 120 seconds
>  			# before we go on to kill it.
> -			counter=30
>  
> -			while [ ${counter} -gt 0 ]; do
> -				statusproc /usr/sbin/squid >/dev/null &&
> break;
> +			n=0
> +			while /usr/sbin/squid -k check && [ $n -lt 120 ]; do
>  				sleep 1
> -				counter=$(( ${counter} - 1))
> +				echo -n .
> +				n=`/usr/bin/expr $n + 1`
> +
>  			done

You can use the same syntax that was used previously to increment n:
  n=$(( ${n} + 1 ))

That avoids starting a new process and compute the result and lets the shell do
it.

>  
> -			# Kill squid service, if still running.
> +# Kill squid service, if still running.
>  			killproc /usr/sbin/squid >/dev/null

Comments should be indented as well.

>  
> -			# Trash remain pid file from squid.
> -			rm -rf /var/run/squid.pid	
> +# 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
>  		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.

> +
>  		;;
>  
>  	restart)
> @@ -141,14 +152,14 @@ case "$1" in
>  		statusproc /usr/sbin/squid
>  		statusproc /usr/lib/squid/unlinkd
>  		;;
> -		
> +
>  	flush)
>  		$0 stop
> -		echo > /var/log/cache/swap.state
> -		chown squid.squid /var/log/cache/swap.state
> +		/bin/rm -f /var/log/cache/swap.state
>  		sleep 1
>  		$0 start
>  		;;
> +
>  	setperms)
>  		chown -R nobody.squid /var/updatecache/
>  		;;

Best,
-Michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]