Hi Roland, Thank you for very useful feedback. Please see my comments inline:
Roland Mainz wrote: >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" > > Done >2. AFAIK you want "-xipo=2". > > Unfortunately we do not have enough time to do the necessary testing to verify this option for this release. However, we would like to try this out in for future releases. >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 ? > > Again, we have used this in our testing and would like to keep it for now. >[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 -- > > > Done >>--- /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 -- > > We might consider ksh/ksh93 for future releases. > > >>+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) > > Done > > >>+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 added quotes here. >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). > > > noted >># 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)) ... > > > Done >>[ -n "$val" ] && echo $val >> >> > >Please add quotes around "$val" ... > > > Done >>} >> >>MCBIN=/usr/lib/ >>MCOPTIONS=`getproparg memcached/options` >> >> > >Please add quotes around "`cmd`" (and/or use POSIX shell syntax, e.g. >"$(cmd)" ) ... > > > Done >>echo $MCOPTIONS >> >> > >Is this a debug option ? > > > Removed >>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... > > > I used your suggestion from the attached file: case "$1" in 'start') "${MCBIN}/memcached" $MCOPTIONS & # detach deamon from shell to avoid that it gets a 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)). > > > Removed $ >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 > > > >------------------------------------------------------------------------ > >#!/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 > > >------------------------------------------------------------------------ > >#!/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 > > >------------------------------------------------------------------------ > >_______________________________________________ >webstack-discuss mailing list >webstack-discuss at opensolaris.org >http://mail.opensolaris.org/mailman/listinfo/webstack-discuss > >
