Hi Jack,
Responding to the rest of the review (and adding a few additional
comments to Jan's responses). See below.
- Keith
On 12/29/10 08:25 AM, Jan Damborsky wrote:
Hi Jack,
thank you very much for review.
Let me address some of your comments - please see my response in line.
I will let Keith respond to the rest of them.
Keith, feel free to let me know if I skipped something which
I should address.
Thank you,
Jan
On 12/28/10 07:21 PM, Jack Schwartz wrote:
Hi Jan and Keith.
I reviewed section (B) sysconfig module - mapping data in DOC & smf
profile generator. Here are my comments:
usr/src/cmd/system-config/profile/Makefile
------------------------------------------
LGTM.
usr/src/cmd/system-config/profile/__init__.py
---------------------------------------------
71: I want to verify: the reason why from_xml() is a no-op here is
because the to_xml() is used to generate the XML which will be
translated into an SMF profile later, not to be restored via from_xml.
Yep. In this interactive configuration scenario, Data Object Cache
is populated with system configuration information from user's input,
thus from_xml() methods are implemented as no-op.
117: For users property, is a check needed that system property
exists before try to install or extract users property from it? (Note
that the constructor sets it to None by default on line 79)
I don't believe so. The users property is simply used as a "shortcut" to
system.users; there are checks in the users getter (use of getattr() to
return a default) to ensure proper handling; and the SystemInfo object
should simply not generate XML for users if there are no users
specified. (It's up to a given consumer to determine if users are
actually required; for example, it's theoretically possible to not
install users initially, then rely on the SCI service to come up on
first boot to then prompt for user data).
96, 108: Just checking: Is a sanity check needed for sysinfo or netinfo?
The DataObjectBase class will perform the minimal sanity checking
required here.
106: I assume there is only one sysinfo (and so old ones get removed
in its setter) and there can be many users (so old users don't get
removed when a new one is added).
The old users actually do get removed - see the @users.setter property
of SystemInfo. (Seems there's a lack of docstrings in the ConfigProfile
object - that should get corrected and hopefully will clarify things)
If my assumptions are correct, then how come NIC setter removes old
children, since a system can have many NICs? Or am I incorrect here?
Currently, we only support setting static IP properties for a single NIC
at install time. Should that change, the single "nic" attribute could be
changed to a list of NICs.
usr/src/cmd/system-config/profile/ip_address.py
-------------------------------------------------
I think 44-45 go away and 47-48 refer to _address and _netmask since
self.address and self.netmask are not used anywhere, and since one
will get unexpected results when doing an str on a newly-created
IPAddress object, where __init__ gets passed explicit address and
netmask args.
Pylint 'requires' that all attributes be laid out in the __init__
function (I'm not sure of anyway around this...). Lines 47-48 are used
to ensure that the 'address' and 'netmask' values passed into __init__
are sent through the setter functions, ensuring validity.
Address property is set with property(), while netmask property is
set using @property and @netmask.setter decorators. This is not
consistent.
Good catch. The address property should be updated for consistency.
Cool python wizardry on line 101 and 112!
Thanks!
usr/src/cmd/system-config/profile/network_info.py
-------------------------------------------------
221: The implicit assumption is made that if there is an error
running dhcpinfo that there will be no output. Callers to
_run_dhcpinfo will ignore errors if it is possible for dhcpinfo to
give both stderr and stdout output.
Correct. It was a conscious decision (during development of the Text
Installer) to ensure that this function never failed; its purpose was to
make educated guesses at good defaults for things like the netmask and
DNS domain if there was a responsive DHCP server; the code is intended
to leave things blank/None if dhcpinfo can't gather that information.
It may be reasonable to raise an exception instead of ignoring (to cover
future use cases) and then to update the find_* functions accordingly;
as _run_dhcpinfo() is a non-public function, that wasn't a priority when
finishing off the Text Installer, but it could change now if that would
be valuable.
259-260: Is it correct to say both stateless and stateful equal "yes"?
Yep. That is correct. Those two options represent two IPv6
auto-configuration
mechanisms which can work simultaneously - this is actually default
configuration
when IPv6 NIC is auto-configured by means of ipadm(1m) command.
ipadm(1m) is used
by network/install smf service to apply networking configuration in
case of 'Manual'
scenario. If you are interested in more details, ipadm(1m) man page
and following
PSARC would provide you with additional information:
http://arc.opensolaris.org/caselog/PSARC/2010/164/
usr/src/cmd/system-config/profile/system_info.py
-------------------------------------------------
26: Does "systemwide" mean global zone? If yes, is it possible to
set up non-global zones with different locales?
Actually, most of those sysconfig parameters have per-zone scope
(keyboard layout is the exception - it is inherited from global zone),
thus that comment is misleading and should be changed. I am thinking that
we could perhaps change it to something like
Defines SystemInfo class which serves as a container for following
system information:
- hostname
- time zone
- locale
- keyboard layout
- terminal type
Do you think it might better capture what that file contains ?
Nit: code seems less maintainable mentioning
/usr/share/lib/keytables/type_6/kbd_layouts
in two places in the file (87 and 175). Not sure what is better:
- moving the comment at 174 to where self.kbd_layout_file gets set
up on line 87
- hardwiring the filename where it is used and getting rid of
self.kbd_layout_file
I tend to prefer the former suggestion. I will change the code
accordingly.
Just curious: 313: How come terminal type is hardwired to vt100? Text
installer sets terminals differently: From text-mode-menu.ksh:
# If connected to SPARC via keyboard/monitor, set TERM to "sun"
# If connected to X86 via keyboard/monitor, set TERM to "sun-color"
# If running on serial console, set TERM to "xterm"
This is a good point. The initial idea behind hardcoding terminal type
to vt100
was that since SCI tool itself does not provide for configuring it, we
would
go with hardwired value which would serve most of the scenarios.
But as you pointed out, we do allow to configure it in text installer.
Based on that, I am wondering if changing the behavior in following way
might better serve needs in that area:
* In case of text installer, terminal type on installed system would
be the same
as the one used during installation.
* When running SCI tool as a standalone application, I can think about
following possibilities:
[a] terminal type would be configured to 'vt100'. User would be made
aware
of that on Summary screen.
[b] terminal type setting would reflect the one configured on running
system
[c] terminal type would not be configured at all
I slightly prefer [a], since it seems desirable approach in case SCI
tool is used
to configure non-global zone.
usr/src/cmd/system-config/profile/user_info.py
----------------------------------------------
64: Usernames have restrictions. From passwd(4):
"string of no more than eight bytes consisting of characters from
the set of alphabetic characters, numeric characters, period (.),
underscore (_), and hyphen (-). The first character should be
alphabetic and the field should contain at least one lower case
alphabetic character.
user name is actually validated sooner - when user configures it on
User screen:
http://cr.opensolaris.org/~kemitche/sci.1/usr/src/cmd/system-config/users.py.html
(see function validate())
is_username_valid() in user_info.py is consumed by to_xml() to determine
if portion of SC manifest configuring user account should be generated
or not
(when user account was not created on User screen).
We should perhaps use different name for that function - do you think
that
something like is_username_configured() might be better ?
To add to this, it wouldn't be a bad idea to consider moving the core of
the above mentioned "validate()" function into UserInfo; the Text
Installer does tend to do more validation in the UI than it really should.
127: I would prefer to see the shell passed in from somewhere else
(maybe a place where there are other default definitions?) as opposed
to it being hardwired. Suppose it gets changed in the future?
That is a good suggestion. I will initialize it in __init__().
Along those same lines, perhaps the sudoers permissions, homedir, and
"roles" should be set during __init__ as well (and perhaps roles and
sudoers should NOT default to being permissive; but rather, be set by
the application).
usr/src/cmd/system-config/xslt/Makefile
---------------------------------------
I'm not completely sure that this:
37 $(ROOTUSRSHARESCXSLT):
38 $(INS.dir)
is needed or correctly placed, since proto-area directory
installation is usually done in Makefile.targ (and Makefile.targ is
included at the bottom of this Makefile). I looked in slim_source
and there are 3 other application-specific Makefiles
which do this, which is why I'm not sure whether it is correct or not...
I agree this does not belong here.
Does the Make work without this?
Yep. I will remove that code.
usr/src/cmd/system-config/xslt/doc2sc_profile.xslt
--------------------------------------------------
LGTM.
Thanks,
Jack
On 12/14/10 07:00, Jan Damborsky wrote:
Hello Caimaniacs,
we (me & Keith) would like to ask you for a feedback for the first
version
of System Configuration Interactive tool.
Please see below for details.
Thank you very much,
Jan
Overview
--------
We decided to deliver SCI tool in steps. There are couple of reasons
for that
decision - we consider following to be the most important ones:
* There is a significant amount of external dependences. Instead of
waiting
for all of them to be satisfied, we believe it makes sense to deliver
in steps taking into account time frame in which particular
dependences get satisfied.
* There are bunch of ongoing 'CUD efforts' which are assumed to
share following
pieces delivered by SCI tool project:
* DOC data structures holding system configuration information
* mechanism for generating SC profile
* changes in ICT world
* initial changes for text installer->CUD transition
We assume that making those common pieces available as soon as
possible will
help related CUD project with making smoother progress.
That first phase intends to deliver SCI tool and modified text
installer
with following characteristics:
[1] SCI tool
* system configuration information stored in DOC
* SCI tool compliant with CUD
* limited set of screens available - in particular those
which are currently available in text installer plus
support IPv4 manual network configuration & DNS
[2] text installer
* SCI tool embedded in text installer
* initial steps taken to transition text installer into CUD
* generated SC profile used for configuration of installed system
* ICT tasks removed/modified as a result of text installer configuring
most of the things in generated SC profile
Things which will be addressed in next steps (e.g. when related
dependences
are met and screens are designed):
* configuration of naming services
* configuration of locales
* support for IPv6 network configuration
* zone awareness
Code review information
-----------------------
webrev:
http://cr.opensolaris.org/~kemitche/sci.1/
Unit tests are not part of this review - they currently being developed
by Keith and we assume to include them for 2nd (final) round of review.
design documents:
System Configuration Interactive Tool design spec:
http://hub.opensolaris.org/bin/download/Project+caiman/System+Configuration+Project/sci%2Dtool%2Dlatest.pdf
System Configuration SMF Service design spec:
http://hub.opensolaris.org/bin/download/Project+caiman/System+Configuration+Project/scsmfdesignlatest.pdf
Since couple of thousand of lines in slim_source were affected, we
divided changes
into several groups - please see below.
Please let us know off line if you plan to review any of those
changes, so that we can assure we have at least one reviewer for
each group of changes.
We are aware that most of the people will be unavailable at least
for some
period during Christmas time, so we will be collecting the feedback
for the
rest of this year until 12/31.
That said, if you would like to review those changes and need more
time,
just let us know, we will account for that.
Testing done:
* text install media built with modified bits
* different install scenarios tested using built text install media
* SCI tool run as a standalone application
- different scenarios tested and generated System Configuration
profiles inspected
Groups
------
[a] sysconfig module - UI part
usr/src/cmd/system-config/date_time.py
usr/src/cmd/system-config/helpfiles/Makefile
usr/src/cmd/system-config/helpfiles/date_time.txt
usr/src/cmd/system-config/helpfiles/network.txt
usr/src/cmd/system-config/helpfiles/network_manual.txt
usr/src/cmd/system-config/helpfiles/summary.txt
usr/src/cmd/system-config/helpfiles/timezone.txt
usr/src/cmd/system-config/helpfiles/users.txt
usr/src/cmd/system-config/helpfiles/welcome.txt
usr/src/cmd/system-config/network_nic_configure.py
usr/src/cmd/system-config/network_nic_select.py
usr/src/cmd/system-config/network_type.py
usr/src/cmd/system-config/summary.py
usr/src/cmd/system-config/timezone.py
usr/src/cmd/system-config/users.py
usr/src/cmd/system-config/welcome.py
usr/src/cmd/system-config/sysconfig.py
usr/src/cmd/system-config/test/test_users.py
usr/src/pkg/manifests/system-install-configuration.mf
usr/src/cmd/system-config/__init__.py
usr/src/cmd/system-config/Makefile
[b] sysconfig module - mapping data in DOC & smf profile generator
usr/src/cmd/system-config/profile/Makefile
usr/src/cmd/system-config/profile/__init__.py
usr/src/cmd/system-config/profile/ip_address.py
usr/src/cmd/system-config/profile/network_info.py
usr/src/cmd/system-config/profile/system_info.py
usr/src/cmd/system-config/profile/user_info.py
usr/src/cmd/system-config/xslt/Makefile
usr/src/cmd/system-config/xslt/doc2sc_profile.xslt
[c] svc:/system/install/config smf(5) service -
changes needed in order to support configuration
of user/root account in text installer
usr/src/cmd/system-config/svc/svc-system-config
usr/src/cmd/system-config/svc/system-config.xml
[d] terminalui Python module
usr/src/lib/Makefile.targ
usr/src/lib/terminalui/Makefile
usr/src/lib/terminalui/__init__.py
usr/src/lib/terminalui/action.py
usr/src/lib/terminalui/base_screen.py
usr/src/lib/terminalui/color_theme.py
usr/src/lib/terminalui/edit_field.py
usr/src/lib/terminalui/error_window.py
usr/src/lib/terminalui/help_screen.py
usr/src/lib/terminalui/i18n.py
usr/src/lib/terminalui/inner_window.py
usr/src/lib/terminalui/list_item.py
usr/src/lib/terminalui/main_window.py
usr/src/lib/terminalui/screen_list.py
usr/src/lib/terminalui/scroll_window.py
usr/src/lib/terminalui/test/test_edit_field.py
usr/src/lib/terminalui/test/test_i18n.py
usr/src/lib/terminalui/test/test_scroll_window.py
usr/src/lib/terminalui/test/test_window_area.py
usr/src/lib/terminalui/window_area.py
[e] text installer - UI part
usr/src/cmd/text-install/helpfiles/Makefile
usr/src/cmd/text-install/osol_install/profile/Makefile
usr/src/cmd/text-install/osol_install/profile/install_profile.py
usr/src/cmd/text-install/osol_install/text_install/Makefile
usr/src/cmd/text-install/osol_install/text_install/__init__.py
usr/src/cmd/text-install/osol_install/text_install/disk_selection.py
usr/src/cmd/text-install/osol_install/text_install/disk_window.py
usr/src/cmd/text-install/osol_install/text_install/fdisk_partitions.py
usr/src/cmd/text-install/osol_install/text_install/install_progress.py
usr/src/cmd/text-install/osol_install/text_install/install_status.py
usr/src/cmd/text-install/osol_install/text_install/log_viewer.py
usr/src/cmd/text-install/osol_install/text_install/partition_edit_screen.py
usr/src/cmd/text-install/osol_install/text_install/summary.py
usr/src/cmd/text-install/osol_install/text_install/test/test_disk_select.py
usr/src/cmd/text-install/osol_install/text_install/test/test_disk_window.py
usr/src/cmd/text-install/osol_install/text_install/text_install.py
usr/src/cmd/text-install/osol_install/text_install/ti_install.py
usr/src/cmd/text-install/osol_install/text_install/welcome.py
usr/src/pkg/manifests/system-install-text-install.mf
[f] text installer - ICT
usr/src/lib/libict/ict.c
usr/src/lib/libict/ict_api.h
usr/src/lib/libict/ict_test.c
usr/src/lib/libict_pymod/ict.py
usr/src/lib/libict_pymod/sc_template.xml
usr/src/lib/liborchestrator/perform_slim_install.c
usr/src/cmd/slim-install/finish/install-finish
[g] rest
.hgignore
usr/src/Makefile.master
usr/src/Targetdirs
usr/src/cmd/Makefile.targ
usr/src/lib/install_logging_pymod/logger.py
usr/src/lib/install_manifest/writer/writer.py
usr/src/pkg/manifests/system-install.mf
usr/src/tools/tests/config.nose
usr/src/tools/tests/tests.nose
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
