Hi Keith. On 12/29/10 09:28 AM, Keith Mitchell wrote:
The issue is in the setter, not the getter. If __init__() sets self.system to None (which it does by default), the users setter will crash since it will try to set self.system.users, but self.system is None.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).
I verified this with the following:
class A(object):
def __init__(self):
self.system = None
@property
def users(self):
return getattr(self.system, "users", None)
@users.setter
def users(self, user_infos):
if user_infos:
self.system.users = user_infos
a = A()
a.users = "hello"
print a.users
This crashes with:
strongheart:/tmp> python a.py
Traceback (most recent call last):
File "a.py", line 16, in <module>
a.users = "hello"
File "a.py", line 13, in users
self.system.users = user_infos
AttributeError: 'NoneType' object has no attribute 'users'
96, 108: Just checking: Is a sanity check needed for sysinfo or netinfo?The DataObjectBase class will perform the minimal sanity checking required here.
OK.
OK, so a single users object holds multiple users, and a new instance of a users object replaces an older one in line 117 of profile/__init__.py.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.
OK.
Maybe I wasn't clear... self.address is initialized on 47 but is not used anywhere. Yet self._address is used throughout the IPAddress class. Likewise for self.netmask. Should 44-45 become: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.
self._address = address
self._netmask = netmask
and lines 47-48 go away?
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.
Thanks.
Cool python wizardry on line 101 and 112!Thanks!
Too cool!
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.
OK.
Since this is a private function, raising an exception may not be needed. I suggest putting something in the header that this function is designed not to fail, and why.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 followingPSARC 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 thatwe could perhaps change it to something likeDefines 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_fileI 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 wouldgo 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 sameas 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 awareof that on Summary screen.[b] terminal type setting would reflect the one configured on running system[c] terminal type would not be configured at allI slightly prefer [a], since it seems desirable approach in case SCI tool is usedto 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 determineif 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 thatsomething 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.
Yes, I was wondering about this too... You'll certainly add more flexibility and adaptability to future uses by doing this. To make things easier, you could default some of the standard things, like /export/home/<user> (or the equivalent for autohome) for the homedir.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).
Also, does it make sense for an account to have a type="role" and set a role for itself as "root"(128)? This logic can be cleaned up at the same time if it doesn't make sense as is.
Thanks,
Jack
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 versionof 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 thatdecision - we consider following to be the most important ones:* There is a significant amount of external dependences. Instead of waitingfor all of them to be satisfied, we believe it makes sense to deliverin steps taking into account time frame in which particular dependences get satisfied.* There are bunch of ongoing 'CUD efforts' which are assumed to share followingpieces 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 transitionWe assume that making those common pieces available as soon as possible willhelp related CUD project with making smoother progress.That first phase intends to deliver SCI tool and modified text installerwith 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 profileThings which will be addressed in next steps (e.g. when related dependencesare 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.pdfSystem Configuration SMF Service design spec:http://hub.opensolaris.org/bin/download/Project+caiman/System+Configuration+Project/scsmfdesignlatest.pdfSince couple of thousand of lines in slim_source were affected, we divided changesinto 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 therest 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 inspectedGroups ------ [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.pyusr/src/cmd/text-install/osol_install/text_install/partition_edit_screen.pyusr/src/cmd/text-install/osol_install/text_install/summary.pyusr/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.pyusr/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
