Hi Karen.
Thanks for your patience. Many changes from your comments, and then some...
Highlights:
- I combined all classes of support_info.py into one. (I explain why I
initially hesitated to do this below.)
- I added code for handling hanging calls to Oracle. ocmadm and asradm
scripts are now called in a subprocess with a timeout.
- Clear passwords are not stored in memory for any longer than necessary
now. When possible, encrypted passwords are used instead.
Webrev V3:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7104766_3
Delta V3 vs V2:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7104766_3_2
Please review ASAP. Many lines counted as changed were merely rearranged.
More comments below...
On 03/05/12 16:40, Karen Tung wrote:
Hi Jack,
Please see my response below. I removed the items that I have no further
comments on. I also reviewed the updated webrev, and if I still have
comments
about the changes, the line numbers are referring to the updated webrev.
On 03/02/12 23:43, Jack Schwartz wrote:
/usr/src/cmd/text-install/summary.py:
- lines 283-292: Since this is the text installer's summary screen,
I don't
think these lines are lines appropriate? Only lines 279-282 are
appropriate
for text installer summary screen.
OK.
I think the updated webrev is still not quiet right. Line 295 makes
the unnecessary
check to see whether we are in the live_env_boot. The text installer
is only run from
live environments. I do not think there's any need to check.
Fixed.
usr/src/cmd/system-config/summary.py:
- lines 247-250: These few lines are not needed, because if you are
at the sys config tool summary screen, you are not running from
a live env.
Yup, removed. Also removed some extraneous comments.
The updated webrev is still not quiet right. lines 269 and 272
mentions about
"validation will be attempted on installed systems(s)". If we are
running
the sys config tool, we must be running on an installed system, right?
Also, since the sys config tool can only be run from installed systems,
is the message from 267-269 necessary?
Revised this message to be more general, to account for the AI case
where created profile is used on a different system, as well as to
account for the current system being the target.
usr/src/cmd/system-config/profile/support_info.py:
- Gneral question. Why _is_live_env(), _current_net_exists() and
_write_password_file() functions start with "_" for the name?
They are used only in this file.
OK. Is that a Python convention? I don't remember seeing it in any
other code..
From:
http://docs.python.org/tutorial/classes.html#tut-private
"...a name prefixed with an underscore (e.g. _spam) should be treated as
a non-public part of the API (whether it is a function, a method or a
data member). It should be considered an implementation detail and
subject to change without notice."
- General comment about this file:
There are 4 classes defined in this file, OCM_Bridge, ASR_Bridge
SupportCreds and SupportInfo. I do not see the need for these
4 separate classes. I belive it's better for their functionalities
to be combined into 1 single class.
I've combined them, and re-whacked most of the module accordingly.
Hence the large re-review.
From what I can see, SupportCreds and SupportInfo class contains
the same information. The only thing is the SupportInfo class
is a DOC object. Because they contain the same information, you
need to define SupportCreds.save() and SupportCreds.get() functions
to "transfer" the information between them. Also, in the support.py
UI screens, you need to keep track of whether the information
got saved or not. Why do we need the separate SupportCreds class?
I think having the separate classes are very error prone, because
people might forget to "save".
Other modules, such as users.py, take their input into local
variables, call validate() on the local variable values, and then
copy them to the DOC object in on_change_screen(). I assume this was
done for a reason.
I am not quiet sure which example are you trying to follow there. I
looked at users.py.
They are always manipulating DOC objects. The _show() function in
UserScreen() class
either retrieves the user and role value from the DOC. If they don't
exist, they are created
in the DOC. The on_change_screen() function assigns values directly
to the objects in the DOC.
We have talked about this in person, and I have changed my code. It
conforms better to the upcoming sysconfig groupings project changes.
Still, there were several reasons why I did what I did:
A) The header comment in terminalui/base_screen.py validate_loop()
suggested saving state in on_change_screen.
B) Consider users.py, validate(). It reads login name and real_name
from the UI widgets, and those fields are used in calculations, checks,
etc. Plus there are other local variables and the UI widgets themselves
which are used in validate() as well. They are not stored in the DOC
until on_change_screen() which is called later. This is the protocol I
was talking about. I did the same thing in my code. I stored to the
DOC only after I am done manipulating all of the fields.
C) Isn't the point of on_change_screen() to store variables to the DOC?
If not, why not just store everything in validate()?
Snipping other related comments, which I don't need to elaborate on as
I've changed the code anyway...
Thanks,
Jack
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
