Hi Jack,
On 12/29/10 12:07 PM, Jack Schwartz wrote:
Hi Keith.
On 12/29/10 09:28 AM, Keith Mitchell wrote:
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:
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).
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.
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'
Ah! I misunderstood. Thanks for clarifying - that should definitely be
fixed.
[...]
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)
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.
SystemInfo.users is essentially a tuple of UserInfo objects (i.e., it's
can't be appended to). The entire tuple can be replaced, but not modified.
[...]
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.
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:
self._address = address
self._netmask = netmask
and lines 47-48 go away?
(Skipping this comment per your other reply)
[...]
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.
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.
Sounds good; we'll add a comment to the docstring to document the behavior.
[...]
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).
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.
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.
I'm not sure if it's valid or not; even if it's not, I am hesitant to
over-validate, as we then are responsible for keeping up with changes in
what is actually valid - better to let SMF (or the services themselves)
handle that, if possible. (Additionally, the current installers won't
get into those situations)
- Keith
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
