On 06/ 8/10 04:37 AM, Jan Damborsky wrote:
Hi,
could I please ask for reviewing changes enhancing
Automated Installer with support for new SMF based System
Configuration framework [3c] ?
Following bugs cover those changes:
15723 Teach AI to use new SMF based System Configuration framework for
configuring user and root accounts
15410 The installer delivered SMF manifests should be relocated to
/lib/svc/manifest
13737 Automated Installer needs support for setting terminal type from
AI manifest
Webrev:
http://cr.opensolaris.org/~dambi/bug-15723/
Please see below for more details as well as testing accomplished.
Joe offered to review the changes (thanks Joe !) and if possible,
I would like Ethan to take a look at AI portion in order to be
sure AI expectations are met.
As usual, anybody else is of course welcome to take a look as well.
Please provide your comments before COB Monday 6/14.
Thank you very much,
Jan
[1] Summary of changes
* support for new SMF based SC framework in AI (bug 15723)
AI has been modified to handle SC manifest as SMF profile. During ICT
phase,
the profile is syntactically validated and copied to the appropriate
directory
on the target (/a/etc/svc/profile/). It is then processed during
first boot of installed system as part of Early Manifest Import process
(aka EMI) - see [3a] for more details.
The legacy SC parser in AI is still in place and it processes the
existing SC parameters which are not yet transfered to SMF properties
(hostname, timezone). The legacy parser will be removed once those
parameters can be configured via SMF properties.
In order to minimize number of incompatible changes affecting users,
AI can still process SC manifest in legacy format.
The big bang (breaking backward compatibility) is planned to be synced
with the integration of AI/DC manifest rework.
If legacy format is detected, SC manifest is converted to the new format
during the installation. User is informed in log file that legacy SC
manifest
was provided and that the support for this might be removed at any time
without previous notice.
For purposes of the transition, conversion script is delivered into
the AI image. That conversion script can be also used on AI server
side to convert legacy SC manifests to the new format. The script
can handle both standalone SC manifests as well as SC manifest embedded
into AI manifest.
It addresses the common use case when admins have bunch of SC manifests
which are being re-used with every new build coming. Conversion tool
allows to automate the process of transitioning to the new format of
SC manifest.
* new System Configuration SMF service is introduced (bug 15723)
This SMF service takes care of configuring user and root accounts.
See design spec [3b] for more details.
* SMF services residing in install gate now take advantage of EMI (bug
15410)
This changes was needed for correct work of new System Configuration
SMF service. In general, it is required that SMF properties are applied
before 1st SMF service is launched. While there, all SMF services
in slim-source gate were modified.
* NWAM is now configured in SC manifest
As a preparation for static networking configuration in AI, AI was
modified
to enable/disable NWAM via SC manifest - related code was removed from
ICT.
[2] Testing accomplished
* Following types of images were built (based on build 140):
- x86: AI, text installer, GUI
- Sparc: AI, text installer
Installations with built images were tested to guarantee no regressions
were introduced.
* Several scenarios for configuring root&user account were tested -
see following document for list of test cases:
http://cr.opensolaris.org/~dambi/tc_user_root/tc_user_root.txt
* It was verified that old AI image (legacy SC parser) fails
appropriately
when provided with new SC manifests - it fails with following messages
in /tmp/install_log:
...
<AI Jun 8 09:25:19> Parsing system configuration manifest
<AI_E Jun 8 09:25:19> Could not parse property from SC manifest
<AI Jun 8 09:25:19> Automated Installation failed in parser module
<AI Jun 8 09:25:19> Invalid System Configuration manifest provided
* It was verified that new AI image can handle both new as well as
legacy SC manifests.
* It was verified that conversion tool can convert both standalone
as well as embedded legacy SC manifest into new format
* It was verified that installadm(1M) can process new SC manifest -
in particular that 'installadm add' has not been broken by the changes.
[3] References
[3a]
http://hub.opensolaris.org/bin/download/Community+Group+smf/smf_design_docs/emidesignmar09.html
[3b]
http://hub.opensolaris.org/bin/download/Project+caiman/System+Configuration+Project/scsmfdesignv0.1.pdf
[3c]
http://hub.opensolaris.org/bin/view/Project+caiman/System+Configuration+Project
Jan,
This is great, well written code! I had to struggle to find issues! ;)
Review feedback attached. I hope this helps.
Joe
http://cr.opensolaris.org/~dambi/bug-15723/
General:
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
------
Not all the copyrights are the same?
usr/src/Makefile.buildnum 1 line changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/Targetdirs 15 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/Makefile 2 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/Makefile.cmd 1 line changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
------
Perhaps ROOTMANIFEST should include "LIB" as all the other variables
that point to something under /lib do. That is it could be changed
to ROOTLIBMANIFEST. What do you think?
usr/src/cmd/auto-install/auto_install.c 90 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/auto-install/auto_install.h 10 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/auto-install/auto_parse.c 161 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Total Nit:
----------
Line: 43 * ai_exec_cmd()
Listing funciton name is not consistent with other function
block headings. Could be removed.
Issues:
-------
Line: 61 char buf[2048];
line: 1273 char cmd[2048];
Is 2048 a good choice for buf size? Elsewhere in the code
MAXPATHLEN is uses, which also might not be a great choice. ;)
Perhaps a new constant for SHELLCMDLEN should be defined. ??
English suggestions:
-------
Line: 71 "Could not execute following command: %s\n", cmd);
suggestions, add "the"
e.g.:
From:
"Could not execute following command: %s\n", cmd);
To:
"Could not execute the following command: %s\n", cmd);
Question:
---------
Line: 1205 -> 1207 if (strstr(token, AUTO_PROPERTY_ROOTPASS) != NULL) {
Is it correct for ROOTPASS to still be handled by this code?
If ROOTPASS is still handled here then shouldn't the code to validate
it's length be re-added @ lines 1248 -> 1249.
I just realized what you are doing with ROOTPASS. If found then
attempt to convert the old style manifest.
However I will still leave this code review response until we
can talk about this. It is not clear to me if lines 1205 -> 1207
are needed.
English suggestions:
------
General:
Add a "." to the end of the error message sentences"." ;)
From:
1354 "Legacy System Configuration manifest provided, attempt"
TO:
1354 "Legacy System Configuration manifest provided, an attempt"
From:
1357 "Please be aware that support for legacy format can be "
To:
1357 "Please be aware that support for the legacy format can be "
This one is a nit. I just thing prior sounds better than
previous. Clearly only an opinion.
From:
1358 "removed at any time without previous notice\n"));
To:
1358 "removed at any time without prior notice\n"));
From:
1360 "Thus it is strongly recommended to use the latest format "
To:
1360 "Thus it is strongly recommended that the latest format"
From:
1361 "of System Configuration manifest\n"));
To:
1361 "of the System Configuration manifest be used.\n"));
From:
1372 "Could not create copy of legacy System"
To:
1372 "Could not a create copy of the legacy System"
From:
1387 "Could not convert legacy System Configuration"
To:
1387 "Could not convert the legacy System Configuration"
From:
1388 " manifest to new format, err=%d\n", ret);
To:
1388 " manifest to the new format, err=%d\n", ret);
From:
1394 "Detected latest format of System Configuration"
To:
1394 "Detected the latest format of System Configuration"
usr/src/cmd/auto-install/default.xml 43 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Question/Issue:
---------------
Why do the left shift the indentation starting at line 77?
Question:
---------
Line: 90 <propval name="profiles" type="astring" value=""/>
If the value for this is empty by default is it valuable to have
it in this manifest?
usr/src/cmd/slim-install/finish/install-finish 17 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
New usr/src/cmd/system-config/Makefile 47 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks ON
New usr/src/cmd/system-config/svc/Makefile 46 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks ON
New usr/src/cmd/system-config/svc/svc-system-config 778 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Suggestion:
-----------
For shell examples see the scripts at:
slim_source/usr/src/cmd/distro_const/vmc
Issue:
------
Looks as if you already ran:
shcomp -n svc-system-config /dev/null
If not remember to do so.
Issue/Style thing:
------------------
Lines:
42 BASENAME=/usr/bin/basename
43 DIRNAME=/usr/bin/dirname
44 NAWK=/usr/bin/nawk
45 PASSWD=/usr/bin/passwd
46 SED=/usr/bin/sed
47 SVCCFG=/usr/sbin/svccfg
48 SVCPROP=/usr/bin/svcprop
49 USERADD=/usr/sbin/useradd
50 USERMOD=/usr/sbin/usermod
51 ZFS=/usr/sbin/zfs
The guidelines say to:
1- Use PATH to ensure use of the correct commangs.
e.g.:
export PATH=/usr/bin:/usr/sbin
2- Use the full path to the correct command
What you have will do #2 but I think using PATH makes the code
cleaner.
Issue:
------
Defined but not used:
42 BASENAME=/usr/bin/basename
43 DIRNAME=/usr/bin/dirname
Suggestion:
-----------
Use print -u2 and print -u1 for stderr & stdout where print is used:
(This is only a partial list. I suggest grepping for print.)
173 print "Warning: Property $prop_name not available which is" \
176 print '""'
185 print "Failed when trying to remove '\' from $prop_name" \
188 print '""'
...
e.g.:
Change From:
391 print "Failed to copy $INITIAL_DOT_PROFILE to" \
Change To:
391 print -u2 "Failed to copy $INITIAL_DOT_PROFILE to" \
Suggestion:
-----------
Use typeset -i when defining integer variable. typeset can be used
when defining all variables.
It is best to define all variables with typeset before using them.
Issue:
------
I don't believe I am suggesting this as I usually like to break
code up into functions but Lines 105 -> 145 are used to encase
a single line of code into 2 different functions.
Why are the functions necessary?
Why not change from:
342 ! $(astring_prop_configured "$expire") && return
To:
if [[ "${expire}" != "${SMF_DEF_ASTRING}" ]]; then return; fi
This would eliminate 40 lines of code from this file.
Suggestion:
-----------
I've not seen the use of command substitution, $(), for invoking
a local function. I've only seen it used to invoke a shell command.
Which makes me wonder if using $() might spawn a subshell. I can't
seem to confirm that so maybe it does not.
But instead of doing:
305 type=$(get_astring_prop $prop_name)
You might want to consider doing:
get_astring_prop $prop_name; type = $?
pkg_repo_path=${PKG_REPO//file:\/\//}
Issue/Suggestion:
-----------------
ksh93 has built in variable/parameter expansion formats.
For example:
182 prop_value=$(print $prop_value | $SED -e 's/\\\(.\)/\1/g')
Could be done with:
prop_value=${prop_value//\//}
An example:
boomvang > prop_value="hi/what/now/"
boomvang > print $prop_value
hi/what/now/
boomvang > prop_value=${prop_value//\//}
boomvang > print $prop_value
hiwhatnow
For more information see section: "Parameter Expansion" in ksh93(1)
Issue/Suggestion:
-----------------
Using Parameter Expansion would reduce the difference between
functions
161 get_astring_prop()
and
207 get_count_prop()
Which could allow them to be more easily combined into a single
fuction as the logic in both is very similar.
Issue:
------
554 $ZFS set mountpoint="$home_mntpoint" "$home_zfs_fs"
Is it possible the ZFS data set is already mounted?
zfs list -H -o mounted $home_zfs_fs"
e.g.:
$ mounted=$(zfs list -H -o mounted rpool/vmc/media)
$ if [[ "${mounted}" == ~(E)yes ]]; then print -u2 "is mounted"; fi
is mounted
Ah I just noticed that if it is already mounted zfs still
returns success.
Issue:
------
666 rmdir -ps $home_mntpoint
Suggest:
Since this command could fail pipe output to /dev/null
rmdir -p $home_mntpoint > /dev/null 1>&1
What's the -s flag do? I do see it in RMDIR(1)
New usr/src/cmd/system-config/svc/system-config.xml 128 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
New usr/src/cmd/system-config/tools/Makefile 40 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
New usr/src/cmd/system-config/tools/sc_conv.ksh 276 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Suggestion:
-----------
Change:
43 print "$@" >&2
To:
print -u2 "$@"
See print(1)
Suggestion:
-----------
print_err is always followed by "exit 1". Why not change the name to
error_exit and include the "exit 1" in the function.
Typo:
-----
165 # * Append SC teplate compliant with new format
teplate -> template
Issue:
------
Count the NAWK code on lines 207 -> 224 be place in a function?
Then it coule be invoked with, for example:
get_value(username); username=$?
get_value(userpass); userpass=$?
get_value(description); description=$?
get_value(rootpass); rootpass=$?
get_value(timezone); timezone=$?
get_value(nodename); nodename=$?
If it is difficult to pass an argument to the NAWK command for the
string to match perhaps this wouldn't be a good idea, but if it
could be done the code would be more readable.
usr/src/lib/libict/ict.c 65 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libict/ict_test.c 3 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libict_pymod/ict.py 198 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/liborchestrator/orchestrator_private.h 4 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/liborchestrator/perform_slim_install.c 350 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Question:
---------
If uname is NULL at line 387 doesn't need to be set to EMPTY_STR
as it was done in the original version on line 403?
usr/src/pkg/manifests/SUNWslim-utils.mf 22 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkg/manifests/install-installadm.mf 8 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkg/manifests/system-install-auto-install-auto-install-common.mf 2
lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkg/manifests/system-install-auto-install.mf 8 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkg/manifests/system-install-text-install.mf 7 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkg/manifests/system-install.mf 8 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkgdefs/SUNWauto-install-common/prototype_com 8 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkgdefs/SUNWauto-install/prototype_com 12 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkgdefs/SUNWinstall/prototype_com 12 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkgdefs/SUNWinstalladm-tools/prototype_com 11 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkgdefs/SUNWslim-utils/prototype_com 31 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkgdefs/SUNWtext-install/prototype_com 11 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
