Victor Kirkebo wrote: > Kindly find the webrev for memcached integration at : > > http://cr.opensolaris.org/~vk136562/memcached/ > > Code reviews are solicited from this team and review/feedback is > requested by 11/20.
A quick 5min race over http://cr.opensolaris.org/~vk136562/memcached/memcached.patch (patch code is quoted as "> "): > --- /dev/null Mon Nov 19 03:47:27 2007 > +++ new/usr/src/lib/memcached/Makefile.sfw Mon Nov 19 03:47:27 2007 [snip] > +# Memcached 1.2.2 > +# Libmemcache 1.4.0 > + > +VERMEMCACHED=memcached-1.2.2 > +VERLIBMEMCACHE=libmemcache-1.4.0.rc2 > + > +include ../Makefile.lib > + > +CFLAGS += -mt -xO5 -xipo -xtarget=generic 1. Please add "-xstrconst" 2. AFAIK you want "-xipo=2". 3. Are you sure you want -xO5 ? This optimisation level enables lots of magic and should only be used with care, e.g. does "memcached" have a testsuite or something like that which can gurantee that the code still works ? [snip] > + > +clean: > + -rm -rf $(VERMEMCACHED) > + -rm -rf $(VERLIBMEMCACHE) AFAIK you can turn this into one "rm" call, e.g. -- snip -- -rm -rf \ $(VERMEMCACHED) \ $(VERLIBMEMCACHE) -- snip -- > > --- /dev/null Mon Nov 19 03:47:27 2007 > +++ new/usr/src/lib/memcached/install-sfw Mon Nov 19 03:47:27 2007 > @@ -0,0 +1,68 @@ > +#!/bin/sh [snip] > +# Memcached 1.2.2 is dependent on libevent > +# Libmemcached 1.4.0 is a C API for memcached Standard template for this kind of script: -- snip -- AFAIK it may be nice to use /usr/bin/ksh or /usr/bin/ksh93 (or "bash" if it supports the "errexit" flag) for this script and then do a $ set -o errexit # at the beginning that the script - the "errexit" flag causes the shell to immediately abort the script when it hits a command which returns a failure instead of continuing operation (and maybe running amok (and ordering 500 new ${product.expensive} via your ${bankaccount} etc. =:-) )) -- snip -- > +VERMEMCACHED=memcached-1.2.2 > +VERLIBMEMCACHE=libmemcache-1.4.0.rc2 > +PREFIX=${ROOT}/usr > +BINDIR=${PREFIX}/bin > +LIBDIR=${PREFIX}/lib > +INCDIR=${PREFIX}/include > +INCMEMCACHEDDIR=${INCDIR}/memcached > +SHAREDIR=${PREFIX}/share > +MAN1MDIR=${SHAREDIR}/man/man1m > +MAN3LIBDIR=${SHAREDIR}/man/man3lib > +DOCDIR=${SHAREDIR}/doc > +DOCMEMCACHEDDIR=${DOCDIR}/memcached > +MANIFESTDIR=${ROOT}/var/svc/manifest/application/database > +METHODDIR=${ROOT}/lib/svc/method Quotes around variable expansions would be nice, e.g. ${x}/foo ----> "${x}/foo" - this should limit the damage generated by errors, e.g. when a variable contains garbage (e.g. usage message in stdout) > +MANSCRIPT=sunman-stability > +. ${SRC}/tools/install.subr If you use ksh93 or bash as suggested above replace the "." with "source" ... > +_install N memcached.xml ${MANIFESTDIR}/memcached.xml 444 > +_install N memcached ${METHODDIR}/memcached 555 > +_install N libmemcache.html ${DOCMEMCACHEDDIR}/libmemcache.html 444 > +_install M memcached.1m ${MAN1MDIR}/memcached.1m 444 > +_install M libmemcache.3lib ${MAN3LIBDIR}/libmemcache.3lib 444 > + > +cd ${VERMEMCACHED} > + > +_install E ./memcached ${LIBDIR}/memcached 555 > + > +cd ../${VERLIBMEMCACHE} > + > +_install N ./src/.libs/libmemcache.a ${LIBDIR}/libmemcache.a 444 > +_install D ./src/.libs/libmemcache.so.0.4.0 ${LIBDIR}/libmemcache.so.0.4.0 > 555 > +_install N ./include/memcache.h ${INCMEMCACHEDDIR}/memcache.h 444 > +_install L libmemcache.so.0.4.0 ${LIBDIR}/libmemcache.so > +_install L libmemcache.so.0.4.0 ${LIBDIR}/libmemcache.so.0 > + > +exit 0 I've attached an updated version of the script as "install-sfw.new20071119.txt" ... > #!/sbin/sh [snip] > # ident "@(#)memcached 1.1 %E SMI" > > . /lib/svc/share/smf_include.sh Please change this to "source /lib/svc/share/smf_include.sh" if you use use ksh93 or bash (see below). > # SMF_FMRI is the name of the target service. This allows multiple instances > # to use the same script. > > > getproparg() { > val=`svcprop -p $1 memcached` Please use quotes around "$1" (and/or use POSIX shell syntax, e.g. val="$(svcprop -p $1 memcached)" (if you use ksh, ksh93 or bash)) ... > [ -n "$val" ] && echo $val Please add quotes around "$val" ... > } > > MCBIN=/usr/lib/ > MCOPTIONS=`getproparg memcached/options` Please add quotes around "`cmd`" (and/or use POSIX shell syntax, e.g. "$(cmd)" ) ... > echo $MCOPTIONS Is this a debug option ? > case "$1" in > 'start') > $MCBIN/memcached $MCOPTIONS & Erm... AFAIK if I recall it correctly this will not work. If the terminal goes away the "memcached" will get SIGHUP (and likely die). AFAIK the only fixes are: a) Add some glue to "memcached" to ignore SIGHUP or b) switch to ksh93 which has the "disown" builtin command which will prevent the SIGHUP... > 'stop') > smf_kill_contract $2 TERM 1 > ;; > > *) > echo $"Usage: $0 {start|stop}" Uhm... the $"mymsg" string literals only work in bash or ksh93 (and I'm not sure whether the SMF scripts should be localised or not... (IMO you can keep it (and a l10n catalog file for the "C" locale would be nice that the l10n people at Sun get a template...) but please sync with smf-discuss at opensolaris.org)). I've attached an updated version of the script as "memcached.new20071119.txt" which handles the issues listed above (except the "echo $MCOPTIONS" thing since I'm not sure why it's there...). ... the remaining stuff looks good except the issues listed by Stefan Teleman ... ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;) -------------- next part -------------- #!/usr/bin/ksh93 # # CDDL HEADER START # # The contents of this file are subject to the terms of the # Common Development and Distribution License (the "License"). # You may not use this file except in compliance with the License. # # You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE # or http://www.opensolaris.org/os/licensing. # See the License for the specific language governing permissions # and limitations under the License. # # When distributing Covered Code, include this CDDL HEADER in each # file and include the License file at usr/src/OPENSOLARIS.LICENSE. # If applicable, add the following below this CDDL HEADER, with the # fields enclosed by brackets "[]" replaced with your own identifying # information: Portions Copyright [yyyy] [name of copyright owner] # # CDDL HEADER END # # # Copyright 2007 Sun Microsystems, Inc. All rights reserved. # Use is subject to license terms. # #ident "@(#)install-sfw 1.1 07/10/05 SMI" # # Stop script if we hit an error set -o errexit # Memcached 1.2.2 is dependent on libevent # Libmemcached 1.4.0 is a C API for memcached typeset VERMEMCACHED="memcached-1.2.2" typeset VERLIBMEMCACHE="libmemcache-1.4.0.rc2" typeset PREFIX="${ROOT}/usr" typeset BINDIR="${PREFIX}/bin" typeset LIBDIR="${PREFIX}/lib" typeset INCDIR="${PREFIX}/include" typeset INCMEMCACHEDDIR=${INCDIR}/memcached typeset SHAREDIR="${PREFIX}/share" typeset MAN1MDIR="${SHAREDIR}/man/man1m" typeset MAN3LIBDIR="${SHAREDIR}/man/man3lib" typeset DOCDIR="${SHAREDIR}/doc" typeset DOCMEMCACHEDDIR="${DOCDIR}/memcached" typeset MANIFESTDIR="${ROOT}/var/svc/manifest/application/database" typeset METHODDIR="${ROOT}/lib/svc/method" typeset MANSCRIPT="sunman-stability" source "${SRC}/tools/install.subr" _install N memcached.xml "${MANIFESTDIR}/memcached.xml" 444 _install N memcached "${METHODDIR}/memcached" 555 _install N libmemcache.html "${DOCMEMCACHEDDIR}/libmemcache.html" 444 _install M memcached.1m "${MAN1MDIR}/memcached.1m" 444 _install M libmemcache.3lib "${MAN3LIBDIR}/libmemcache.3lib" 444 cd "${VERMEMCACHED}" _install E ./memcached "${LIBDIR}/memcached" 555 cd "../${VERLIBMEMCACHE}" _install N ./src/.libs/libmemcache.a "${LIBDIR}/libmemcache.a" 444 _install D ./src/.libs/libmemcache.so.0.4.0 "${LIBDIR}/libmemcache.so.0.4.0" 555 _install N ./include/memcache.h "${INCMEMCACHEDDIR}/memcache.h" 444 _install L libmemcache.so.0.4.0 "${LIBDIR}/libmemcache.so" _install L libmemcache.so.0.4.0 "${LIBDIR}/libmemcache.so.0" exit 0 -------------- next part -------------- #!/sbin/sh # # CDDL HEADER START # # The contents of this file are subject to the terms of the # Common Development and Distribution License (the "License"). # You may not use this file except in compliance with the License. # # You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE # or http://www.opensolaris.org/os/licensing. # See the License for the specific language governing permissions # and limitations under the License. # # When distributing Covered Code, include this CDDL HEADER in each # file and include the License file at usr/src/OPENSOLARIS.LICENSE. # If applicable, add the following below this CDDL HEADER, with the # fields enclosed by brackets "[]" replaced with your own identifying # information: Portions Copyright [yyyy] [name of copyright owner] # # CDDL HEADER END # # Copyright 2007 Sun Microsystems, Inc. All rights reserved. # Use is subject to license terms. # # ident "@(#)memcached 1.1 %E SMI" source /lib/svc/share/smf_include.sh # SMF_FMRI is the name of the target service. This allows multiple instances # to use the same script. function getproparg { typeset val="$(svcprop -p "$1" memcached)" [[ -n "$val" ]] && print -r -- "$val" } # main typeset MCBIN="/usr/lib/" typeset MCOPTIONS="$(getproparg memcached/options)" print -r -- "$MCOPTIONS" case "$1" in 'start') "${MCBIN}/memcached" $MCOPTIONS & # detach deamon from shell to avoid that it gets a SIGHUP disown $? ;; 'stop') smf_kill_contract "$2" TERM 1 ;; *) print $"Usage: $0 {start|stop}" exit 1 ;; esac # not reached
