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;)
