Hi Jan.
On 12/29/10 08:25 AM, Jan Damborsky wrote:
Hi Jack,
thank you very much for review.
You are welcome!
Replies inline.
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.
OK, thanks.
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)
96, 108: Just checking: Is a sanity check needed for sysinfo or netinfo?
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). 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?
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.
Address property is set with property(), while netmask property is
set using @property and @netmask.setter decorators. This is not
consistent.
Cool python wizardry on line 101 and 112!
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.
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/
OK. Thanks for the tip. I'll read up on this.
The names make it seem like they are opposites when they are not. That
is what you have to work with, and isn't anything you can change.
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
language and locale?
- keyboard layout
- terminal type
Do you think it might better capture what that file contains ?
Yes.
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.
OK
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.
Sounds good.
* 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.
OK. Setting the term type to a default value when the SCI tool is used
standalone (after an install) seems like a good idea, since I assume
that each non-global zone will likely operate on its own screen and so
one cannot know how to set it up beforehand.
That said, why not set it to the same text-installer default values
based on Sparc-monitor/X86-monitor/serial-console instead of vt100?
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 ?
Yes, that is much more to-the-point.
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__().
OK.
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.
OK.
Thanks,
Jack
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
