Dong-Hai Han wrote: > > It's 3 am in BEIJING now... Ouch... ;-(
> See the text below. Thanks! :-) > Roland Mainz Wrote: > > Cathy Zhou wrote: > > > >>John Beck wrote: > >> > >>>I reviewed the changes for NWAM, SMF, install/bfu/upgrade and other. > >>>The work looks to be very well done, as I only found one issue where > >>>a change in the net-physical start method appears to be needed in > >>>the net-nwam method as well, plus some nits in bfu.sh: > > > > [snip] > > > > Erm... it seems I never got the initial email with the webrev URL... > > where can I find it ? > > We'd like each of you to participate in the code review of the Clearview UV > component. The changes are major, and we've divided them into different > portions and identified each of you as potential candidate to review > specific portion. If any of you is unable to review the code, please let us > know as soon as possible, and please also let me know who you think would be > a good alternative reviewer. > > The timeout is set as two weeks. [snip] > External webrev: > > http://cr.grommit.com/~yun/webrev_uv_09_28 > > Internal webrev: > > http://jurassic.sfbay/net/forwar.prc/builds/yz147064/clearview-uv-review/webrev_review/ > > Cscope: > > /net/forwar.prc/builds/yz147064/clearview-uv-review/ [snip] I only did a quick look (e.g. 5min race through the code) over all the stuff and found some minor nits: - It would be nice to compile a new library and all related code with "-xstrconst" (it puts string literals into read-only sections, which allows them to be shared between processes and multiple copies folded into one (e.g. it should save both memory and disk space)) - "usr/src/cmd/dlmgmtd/svc-dlmgmtd" contains something weired: -- snip -- +# The real daemon is not started in a non-global zone. But we need to +# create a dummy background process to preserve contract lifetime. + +if smf_is_nonglobalzone; then + (while true ; do sleep 3600 ; done) & + exit $SMF_EXIT_OK +fi -- snip -- Erm... questions: 1. why is this done.. 2. ... and how should this work ? I assume the shell child process will "meet the truck" at the first SIGHUP... or am I wrong in this case ? 3. At which runlevel is this script used, before or after /usr has been mounted ? - "usr/src/pkgdefs/SUNWcnetr/postinstall": -- snip -- -# Move existing /etc/aggregation.conf entries to /etc/dladm/aggregation.conf -ORIG=$BASEDIR/etc/aggregation.conf +AGGR_CONF=/etc/aggregation.conf +ORIG=$BASEDIR$AGGR_CONF +if [ ! -f $ORIG ]; then + # Try the alternate location. + AGGR_CONF=/etc/dladm/aggregation.conf + ORIG=$BASEDIR$AGGR_CONF +fi -- snip -- Please use quotes around ${ORIG}, e.g. -- snip -- if [ ! -f "${ORIG}" ]; then -- snip -- (the rest of the script needs similar work to avoid that it trips over for unexpected input (see http://www.opensolaris.org/os/project/shell/shellstyle/ for further suggestions)) - "bfu.sh" 1. Please use "if [[ ... ]]" instead of "if [ ... ]" (and see comment for quotes above). 2. Erm, wouldn't it be better to filter the comment lines directly after the "cat", e.g. turn... -- snip -- + if [ -f $ORIG ]; then + cat $ORIG | while read line; do + # Strip off comments, then each remaining line lists + # properties the administrator configured for a + # particular interface. Each line includes several + # properties, but we can only set one property per + # dladm invocation. + echo $line | grep '^[^#]' | while read link rest + do -- snip -- ... into ... -- snip -- + if [[ -f "${ORIG}" ]]; then + cat "${ORIG}" | grep '^[^#]' | while read line; do + # Strip off comments, then each remaining line lists + # properties the administrator configured for a + # particular interface. Each line includes several + # properties, but we can only set one property per + # dladm invocation. + echo $line | while read link rest + do -- snip -- ... AFAIK this would avoid a |fork()|/|exec()| of "grep" for each line (I ignore the extra "cat" in this case :-) ) ... 3. AFAIK it may be better to shift the subshell one TAB right to make it clearer what the code does, e.g. turn... -- snip -- +smf_cleanup_dlmgmtd() { +( + smf_delete_manifest "var/svc/manifest/network/dlmgmt.xml" + cd $root; + rm -f lib/svc/method/svc-dlmgmtd + rm -f etc/.dlmgmt_door + rm -f sbin/dlmgmtd +) +} -- snip -- ... into .. -- snip -- +smf_cleanup_dlmgmtd() +{ + smf_delete_manifest "var/svc/manifest/network/dlmgmt.xml" + + ( + cd "${root}" + rm -f lib/svc/method/svc-dlmgmtd + rm -f etc/.dlmgmt_door + rm -f sbin/dlmgmtd + ) +} -- snip -- ... or ... -- snip -- +smf_cleanup_dlmgmtd() +{ + smf_delete_manifest "var/svc/manifest/network/dlmgmt.xml" + rm -f \ + "${root}/lib/svc/method/svc-dlmgmtd" \ + "${root}/etc/.dlmgmt_door" \ + "${root}/sbin/dlmgmtd" +} -- snip -- AFAIK thath's all for now... ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;)
