Hi Ed, In addition to Thorsten's comments: - usr/src/cmd/scinstall/lib/ips_package_processing.ksh * line 106 .$$.$$ is too generic a name. Replace the first .$$ with something more descriptive * line 123 wrong indentation * line 124 fi not needed. Presumably you get syntax error currently
- usr/src/ipsdefs/Makefile.com * line 94 onwards. This file is for common variable definitions. The target rules should go in usr/src/ipsdefs/Makefile.targ. This also means you can get rid of the $(MP_SIGNATURE_FILE): targets in the individual Makefiles. Thanks Jonathan Thorsten Frueauf wrote: > Hi Ed et al, > > here are my comments: > > generic: > * should we try to follow shell scripting coding styles like outlined at > http://www.opensolaris.org/os/project/shell/shellstyle/ > http://www.opensolaris.org/os/community/on/shellstyle/ > for complete new scripts - and thus try to use ksk93? > I may have some comments because of those recommendations, feel free to > ignore if we don't want to introduce this now. It won't be complete > anyway. > > > - usr/src/cmd/scinstall/lib/ips_package_common.ksh > * line 61, typo: postrmove -> postremove > > * line 115/116 should use the variables $SVCADM and $SVCCFG instead, > > * line 115/116 is the redirect > to /dev/null missing? > > * line 115/116 what if the operations fail? No logging of that fact? > > * line 142, I know you set $PATH for the script, but sometimes you call > commands with their full path (which I prefer), like line 150, here > you don't for grep. Recommend to stay consistent. > > * line 195, recommend to use $(...) instead of `...` > > * line 197, is the space within ' auth_att' on purpose? > > * line 226 and 230, missing -f testflag > > * line 226-236, you might want to consider to turn those into > three one-liners, like > [ -f "${TMPFILE}" ] && rm -f ${TMPFILE} > > * line 252, you might want to redirect stdout to /dev/null > > > - usr/src/cmd/scinstall/lib/ips_package_processing.ksh > * line 49, recommend to use $(date) > > * line 61, are the two spaces between "%s: Unable" on purpose? > > * line 61, shouldn't ${PROG} be ${PROGNAME} like in line 62? > Don't see where PROG gets defined. > > * line 68, typo postermove -> postremove > Note that line 76-86 allows only modes with ips_ prefixed. > You might then want to refer to those on the comments as well > > * line 114, do you need the ; ? > > > - usr/src/cmd/scinstall/lib/ips_package_uninstall.ksh > * line 61, are the two spaces between "%s: Unable" on purpose? > > * line 61, shouldn't ${PROG} be ${PROGNAME} like in line 62? > Don't see where PROG gets defined. > > * line 68, typo IPS_FAIL -> $IPS_FAIL > > * line 73, typo IPS_FAIL -> $IPS_FAIL > > * line 81, recommend to use $(...) instead of `...` > > * line 83, missing closing " after ${mp} > > * line 90, recommend to use $(...) instead of `...` > > * line 92, missing closing " after ${p} > > * line 100, missing closing " after list > > * line 112 and 114, recommend to use $(...) instead of `...` > > > - usr/src/cmd/scinstall/lib/Makefile > * is it ok to list the scripts without the .ksh extension? > If so, consider that you set PROGNAME and IPS_LIB to contain the .ksh > extension. > > > - usr/src/cmd/scinstall/lib/sc_common.ksh > * line 49/50, you say "must have one or the other", but note that > ha-cluster-framework-full depends on ha-cluster-framework-minimal. > Therefor you will either need minimal, or have both. > Currently you can't have ha-cluster-framework-full without > ha-cluster-framework-minimal. > > * line 1987-2004 seems to do the same as 739-761. > Recommend to delete 739-761 - and check where it gets used currently. > I recall Lucia introducing it, so her code may already use it. > We should have only one such function and using it consistently. > > Note that sc_is_ips() has the oposite logic than is_ips(). > I would prefer return code 0 for "IPS packaging used", and > 1 for "IPS packaging _not_ used". > > I realize this might impose quite some changes to your existing usage > of is_ips() though. > > It is also confusing the SC_TRUE is actually 1 - but that is > pre-existing > logic I guess. > > * line 1999-2001, turn it into a one-liner, like: > [[ -x ${IPS_FLAG_FILE} ]] && return 1 > > Of course use return 0 if you acceppt my comment above. > > Btw. why checking with -x (being executable), is -f not enough? > The > > * line 2017-2020 and 2022-2025 can get changed to: > > if /usr/bin/pkg list ${IPS_PKG_MINIMAL} < /dev/null 2>&1 > then > return 0 > fi > > if /usr/bin/pkg list ${IPS_PKG_FULL} < /dev/null 2>&1 > then > return 0 > fi > > In any case, the "then" statement is missing after line 2018 and 2023. > > * line 2027, are the two spaces within "%s: Neither" on purpose? > > * line 2027, where is $PROG comming from? > > * line 2029, the closing } for test_ips_install() is missing. > > > - usr/src/ipsdefs/ha-cluster-agent-builder/Makefile > * line 29, you need to add $(IPSMANIFEST_COM) > > * line 36-37, you could move that into usr/src/ipsdefs/Makefile.targ, > since it gets used in other Makefiles as well > > > - usr/src/ipsdefs/ha-cluster-agent-builder/manifest_com > * line 35, I don't understand why the entry for the "1" directory? > should that be "usr/cluster/lib/scadmin/ips/sig-mp" perhaps? > > > - usr/src/ipsdefs/ha-cluster-framework-cmass/Makefile > * line 29, you need to add $(IPSMANIFEST_COM) > > * line 36-37, you could move that into usr/src/ipsdefs/Makefile.targ, > since it gets used in other Makefiles as well > > > - usr/src/ipsdefs/ha-cluster-framework-cmass/manifest_com > * line 35, I don't understand why the entry for the "1" directory? > should that be "usr/cluster/lib/scadmin/ips/sig-mp" perhaps? > > > - usr/src/ipsdefs/ha-cluster-framework-dtk/Makefile > * line 29, you need to add $(IPSMANIFEST_COM) > > * line 36-37, you could move that into usr/src/ipsdefs/Makefile.targ, > since it gets used in other Makefiles as well > > > - usr/src/ipsdefs/ha-cluster-framework-dtk/manifest_com > * line 35, I don't understand why the entry for the "1" directory? > should that be "usr/cluster/lib/scadmin/ips/sig-mp" perhaps? > > > - usr/src/ipsdefs/ha-cluster-framework-full/Makefile > * line 29, you need to add $(IPSMANIFEST_COM) > > * line 36-37, you could move that into usr/src/ipsdefs/Makefile.targ, > since it gets used in other Makefiles as well > > > - Where is the change to > usr/src/ipsdefs/ha-cluster-framework-full/manifest_com? > > > - usr/src/ipsdefs/ha-cluster-framework-minimal/Makefile > * line 38-39, you could move that into usr/src/ipsdefs/Makefile.targ, > since it gets used in other Makefiles as well > > > - Where is the change to > usr/src/ipsdefs/ha-cluster-framework-minimal/manifest_com? > > > - usr/src/ipsdefs/ha-cluster-framework-scm/Makefile > * line 29, you need to add $(IPSMANIFEST_COM) > > * line 36-37, you could move that into usr/src/ipsdefs/Makefile.targ, > since it gets used in other Makefiles as well > > > - usr/src/ipsdefs/ha-cluster-framework-scm/manifest_com > * line 35, I don't understand why the entry for the "1" directory? > should that be "usr/cluster/lib/scadmin/ips/sig-mp" perhaps? > > > - usr/src/ipsdefs/ha-cluster-framework-svm/Makefile > * line 29, you need to add $(IPSMANIFEST_COM) > > * line 36-37, you could move that into usr/src/ipsdefs/Makefile.targ, > since it gets used in other Makefiles as well > > > - usr/src/ipsdefs/ha-cluster-framework-svm/manifest_com > * line 35, I don't understand why the entry for the "1" directory? > should that be "usr/cluster/lib/scadmin/ips/sig-mp" perhaps? > > > - usr/src/ipsdefs/ha-cluster-quorum-server/Makefile > * line 29, you need to add $(IPSMANIFEST_COM) > > * line 36-37, you could move that into usr/src/ipsdefs/Makefile.targ, > since it gets used in other Makefiles as well > > > - Where is the change to > usr/src/ipsdefs/ha-cluster-quorum-server/manifest_com? > > > - usr/src/ipsdefs/Makefile > * line 188/189 are IMHO not really needed. > One would perform a "nbmake IPSREPO_URL=<url> install" from > usr/src/ipsdefs > > - usr/src/ipsdefs/SUNWmdmr/SUNWmdmr.ips-processing.ksh > * line 94, recommend to use $(...) instead of `...` > > > - Where is the change to usr/src/ipsdefs/SUNWscmasar/Makefile? > > > - usr/src/ipsdefs/SUNWscmasar/manifest_com > * shouldn't there be entries to include > usr/src/ipsdefs/SUNWscmasar/SUNWscmasar.ips-processing.ksh as well? > > > - usr/src/ipsdefs/SUNWscqsr/SUNWscqsr.ips-processing.ksh > * line 68, 69, 71 and 99, recommend to use $(...) instead of `...` > > > - usr/src/ipsdefs/SUNWscr/SUNWscr.ips-processing.ksh > * line 33 and 34, do you really want `...`? Not perhaps "..."? > If the former, recommend to use $(...) instead of `...` > > * line 57-60, can get turned into a one-liner: > [ ! -s ${TMPFILE1} ] && return 1 > > * line 97 and 496, where is ${SC_MAXNODEID} getting defined? > > * line 103, 107, 108, 110 and 116, recommend to use $(...) instead of > `...` > > * line 107 and 108, missing variable for `expr ${chgerr} + 1`? > > * line 107 and 108 can get merged into > /usr/bin/chown root:sys ${dir} || mkdirerrs="$(expr ${chgerr} + 1)" > > * line 135-137 can get turned into a one-liner: > [ ! -f "${update_file}" ] && return 0 > > * line 175, recommend to call full path for tr. > Note that /usr/bin/tr is not utf-8 safe, you may want to invoke > /usr/xpg4/bin/tr > > * line 175, recommend to use $(...) instead of `...` > > * line 176, recommend to call full path for bc > > * line 197, 209 and 212, recommend to call full path for sed > > * line 197, 198, 202, 209, 210, 212, 213, 215 and 233, recommend to > use $(...) instead of `...` > > * line 202, recommend to call full path for cat > > * line 208, 211, recommend to call full path for grep > > * line 298-300 can get turned into a one-liner: > [ -f "${rcfile}" ] && rm -f ${rcfile} > > * line 519-521 can get turned into a one-liner: > [ -f ${STCLIENT} ] && ${STCLIENT} -d -i ${URN} >/dev/null 2>&1 > > * line 552-565, recommend to use $(...) instead of `...` > > * line 614-626, recommend to use $(...) instead of `...` > > > - usr/src/ipsdefs/SUNWscu/Makefile > * line 30, might want to add a comment that dot-ips turns into .ips > within manifest_com? Otherwise it gets hard to understand. > Just curious, why not directly create the .ips file? > > > - usr/src/ipsdefs/SUNWsczr/SUNWsczr.ips-processing.ksh > * line 33 and 34, do you really want `...`? Not perhaps "..."? > If the former, recommend to use $(...) instead of `...` > > * line 51-53, recommend to use $(...) instead of `...` > > * line 78, recommend to use $(...) instead of `...` > > * line 105-107 can get turned into a one-liner: > [ ! -f ${upgrade_file} ] && /usr/bin/touch ${upgrade_file} > > > * line 128-130 and 134, recommend to use $(...) instead of `...` > > * line 166-169 can get turned into a one-liner: > [[ -f ${NSS_FILE} ]] && /usr/bin/rm -rf ${NSS_FILE} > > * line 171, recommend to use $(...) instead of `...` > > * line 201-238 is commented out - line 203 looks like a debug comment? > Add some XXX to mark work in progress, guess this changes later? > > > Greets > Thorsten > > > Ed McKnight wrote: > >> Folks, >> >> Near the top of >> http://cr.opensolaris.org/~emk/scinstall-ips-integration-1/ is >> a README link. Please read it in order to get a handle on the changes >> and to decide what order to look at the changes in. >> >> thx, --emk > >
