Roland Mainz wrote:
> Victor Kirkebo wrote:
>
>> 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
>>
>
> Thanks! :-)
>
>
>>> 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.
>>
>
> Ok...
> ... BTW: Did you check the logs that "xipo" is used at _link_ time, too
> (a common problem with the GNU stuff is that they sometimes don't pass
> CFLAGS to the link stage (which would render "xipo" useless since the
> all-in-one optimisation is done when "cc" is called for the final link
> pass)) ?
>
>
It looks good.
Thanks
>>> 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.
>>
>
> Ok...
>
> BTW: Check whether -xspace makes any difference - sometimes smaller code
> is better...
>
> [snip]
>
>>> AFAIK you can turn this into one "rm" call, e.g.
>>> -- snip --
>>> -rm -rf \
>>> $(VERMEMCACHED) \
>>> $(VERLIBMEMCACHE)
>>> -- snip --
>>>
>> Done
>>
>
> Thanks! :-)
>
>
>>>> --- /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.
>>
>
> Ok...
> ... but there is a giant catch with the SMF script below (see issue
> 'SIGHUP vs. "disown"' below...) ...
>
> [snip]
>
>>> 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
>>
>
> Thanks! :-)
>
>
>>>> +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.
>>
>
> Ok...
>
> [snip]
>
>>>> 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
>> ;;
>>
>
> Erm... where is the "disown" call ? The point was to call "disown" top
> make sure the background process is completely detached and won't be
> hit&&run-over&&squished by the "SIGHUP" bus.
> If you can't use ksh93's "disown"-builtin please add a small line of
> code to "memcached" which causes SIGHUP to be ignored in the deamon
> itself.
>
Yes, this was sloppy...
Our conclusion, however, is that since memcached will be started via SMF
and not a terminal SIGHUP will not be an issue here.
> [snip]
>
>>> 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 $
>>
>
> Thanks! :-)
>
> ----
>
> Bye,
> Roland
>
>
