[webstack-discuss] [sfwnv-discuss] Squid 2.6 integration into SXDE review request

[Roland Mainz]

rahul wrote:
> I have prepared changes for including Squid 2.6 into SXDE - through
> WebStack. Here is a webrev http://cr.opensolaris.org/~vrthra/squid/ Can
> I get a reviewer for this from sfwnv community?
> 
> (We'd like someone outside the project team to take a look.)

Looking at
http://cr.opensolaris.org/~vrthra/squid/LOCAL-SQUID-GATE.patch ...

1. Why is "perl" needed here ?
-- snip --
+	   /usr/perl5/bin/perl -pi -e 's/^KERBLIBS =.*/KERBLIBS =-R
\/usr\/lib\/gss -L\/usr\/lib\/gss -lgss \/usr\/lib\/gss\/mech_krb5.so
-lkrb5 -lsocket /g' \
+	   helpers/negotiate_auth/squid_kerb_auth/Makefile.*; \
-- snip --
AFAIK both "sed" and "ksh93" can do that job, too (e.g. smaller
filters). For example this should work (untested):
-- snip --
for i in helpers/negotiate_auth/squid_kerb_auth/Makefile.* ; do
  c="$(< "$i")"
  print -r -- "${c//~(E)KERBLIBS =.*$'\n'/KERBLIBS =-R /usr/lib/gss
-L/usr/lib/gss -lgss /usr/lib/gss/mech_krb5.so -lkrb5 -lsocket$'\n'}"
>"$i"
done
-- snip --


2. Please think about using C99 by default, e.g. "-xc99=%all
-D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1". The advantage is that libc&co.
Behave closer to the standard (and similar like glibc) which may be
usefull (one precedent is the ksh93-integration work where these flags
had a huge (positive) impact on performance and footprint) ... and as a
small side-effect things like |system()|, |popen()| etc. use
/usr/xpg4/bin/sh instead of /sbin/sh as system shell which matches the
expectations of the GNU people for a "normal" system shell.

3. Nit: Use "/usr/bin/ksh" (or "/usr/bin/ksh93"), not "/bin/ksh":
-- snip --
+	   MAKE=/usr/ccs/bin/make \
+	   /bin/ksh ./configure \
-- snip --
BTW: What about using /usr/bin/ksh93 in this case ?


4. Script nits in "new/usr/src/cmd/squid/Solaris/http-squid":
-- snip --
> +. /lib/svc/share/smf_include.sh
> +
> +SQUID_HOME=/usr/squid
> +CONF_FILE=/etc/squid/squid.conf
> +PIDFILE=/var/squid/logs/squid.pid
> +SQUID=${SQUID_HOME}/sbin/squid
> +
> +[ ! -f ${CONF_FILE} ] &&  exit $SMF_EXIT_ERR_CONFIG

Quotes missing, e.g. please change this to
[ ! -f "${CONF_FILE}" ] &&  exit $SMF_EXIT_ERR_CONFIG

> +
> +
> +case "$1" in
> +start)
> +	/bin/rm -f ${PIDFILE}

Quotes missing and use /usr/bin, not /bin, e.g.
/usr/bin/rm -f "${PIDFILE}"

> +    if [ ! -f /var/squid/cache/00 ]; then

Quotes missing.

> +		out=`${SQUID} -z`
> +	fi
> +
> +	exec ${SQUID} 2>&1

Quotes...

> +	;;
> +stop)
> +	exec ${SQUID} -k shutdown 2>&1

Quotes...

> +	;;
> +*)
> +	echo "Usage: $0 {start|stop}"
> +	exit 1
> +	;;
> +esac
-- snip --


5. Script nits in "new/usr/src/cmd/squid/install-squid":
General:
- Would it be usefull to use $ set -o errexit # to stop the script when
an error is hit, e.g. to avoid that the script continues after an error
and then runs amok ?
- Who or what defines "_install" (I'm not happy about the leading "_" in
front of the same... usually it indicates something (bery) "private" and
I'm not sure whether it applies in this case...) ?

-- snip --
> +
> +ins_file() {
> +    iprog=$1
> +    idir=$2
> +    imode=$3

Missing quotes around $1, $2, $3

> +    _install N ${iprog} ${idir}/${iprog} ${imode}
> +}
> +
> +ins_file_as() {
> +    iprog=$1
> +    iprogas=$2
> +    idir=$3
> +    imode=$4

quotes...

> +    _install N ${iprog} ${idir}/${iprogas} ${imode}

quotes...

> +}
> +
> +install_smf() {
> +    cd ${TOP}/Solaris
> +    ins_file http-squid.xml ${ROOT}/var/svc/manifest/network 444
> +    ins_file http-squid ${ROOT}/lib/svc/method 555
> +    cd ${TOP}/${PKGVERS}

quotes...

> +}
> +
> +install_config() {
> +    cd src
> +    ins_file_as squid.conf.default squid.conf ${CONFDIR} 644
> +    ins_file_as mime.conf.default mime.conf ${CONFDIR} 644
> +    cd ..
> +
> +    cd helpers/basic_auth/MSNT
> +    ins_file_as msntauth.conf.default msntauth.conf ${CONFDIR} 644
> +    cd ../../..
> +
> +    cd tools
> +    ins_file cachemgr.conf ${CONFDIR} 644
> +    cd ..

Alternatively you could use
  (
      cd tools
      ins_file cachemgr.conf ${CONFDIR} 644
  )
e.g. at the end of the subshell the CWD gets restored to the original
value.

> +}
> +
> +install_shared() {
> +    cd src
> +    ins_file mib.txt ${SHAREDIR} 644

quotes...

> +    cd ..
> +    cd icons
> +    for i in *.gif
> +    do
> +        ins_file $i ${SHAREDIR}/icons 644

quotes...

> +    done
> +    cd ..
> +}
> +
> +install_errtxt() {
> +    cd errors
> +    for i in `ls -p | grep '\/$'|sed 's/\/$//'`

Somehow I wish you would use /usr/bin/ksh93 for this (e.g. neither "ls"
or "sed" would be neccesary in this case) ... ;-(

> +    do 
> +        cd $i

Quotes...

> +        for j in * 
> +        do
> +            ins_file $j ${SHAREDIR}/errors/$i 644

Quotes missing, e.g.
ins_file "$j" "${SHAREDIR}/errors/$i" 644

> +        done
> +        cd ..
> +    done
> +    cd ..
> +}
> +
> +install_man() {
> +    cd doc
> +    for i in *.8
> +    do
> +        ins_file $i ${MAN8DIR} 644

Quotes..

> +    done
> +    cd ..
> +
> +    cd helpers/basic_auth/DB/
> +    ins_file squid_db_auth.8 ${MAN8DIR} 644
> +    cd ../../../

What about using the subshell "trick" listed above ?

> +    cd helpers/basic_auth/NCSA/
> +    ins_file ncsa_auth.8 ${MAN8DIR} 644
> +    cd ../../../

Quotes...

[snip]
> +install_cachemgmt() {
> +    cd scripts
> +    ins_file RunCache ${BINDIR} 555

Quotes...
[snip (50000 more "quotes..." skipped...)]

> +install_standalone() {
> +    cd src
> +    ins_file squid ${SBINDIR} 555
> +    cd ..
> +}
> +
> +# START HERE
> +PKGVERS=`sed -ne '/VER=/s/.*=//p' Makefile.sfw`
> +PREFIX=${ROOT}/usr/squid # Going with the apache style.
> +BINDIR=${PREFIX}/bin
> +SBINDIR=${PREFIX}/sbin
> +LIBEXECDIR=${PREFIX}/libexec
> +SHAREDIR=${PREFIX}/share
> +CONFDIR=${ROOT}/etc/squid
> +MAN8DIR=${PREFIX}/man/man8

Erm... are these local variables which are not exported to the
environment ? If "yes" then these variable names should be lowercase...

> +. ${SRC}/tools/install.subr
> +
> +TOP=`pwd`
> +
> +cd ${PKGVERS}

Quotes...

> +
> +install_config
> +install_shared
> +install_errtxt
> +install_man
> +install_cachemgmt
> +install_internal
> +install_standalone
> +install_smf
-- snip --

AFAIK that's all what I could find in a 5min race through the code...

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 7950090
 (;O/ \/ \O;)