Hi Karen.
Thanks for taking another look.
Replies inline...
On 03/15/12 12:08 AM, Karen Tung wrote:
Hi Jack,
I reviewed everything but the GUI changes again. I have the following
comments:
usr/src/cmd/system-config/helpfiles/Makefile:
- small nit: should lines 43-44 be after summary.txt to maintain
alphabetical order?
Fixed.
usr/src/cmd/system-config/profile/support_info.py:
- The _fork_proc() function:
My understanding of this function is that you want to be able
to kill the process after the specified timeout. If that's the case,
why is it necessary to have all the logic to do non-blocking read
of the stdout and stderr? You are not processing any of the information
you read from stdout and stderr anyway
While it is true that I am not processing info while I am looping, the
stdout "pipe" can fill up depending on the output of ocmadm and asradm.
(Some of the properties are large, and together could overrun the pipe
depending on the pipe size.) By reading, I continuously empty the pipe
so that it won't fill up. Deadlock will occur if the pipe fills up and
more the subprocess tries to write more.
Would it be simpler to just check
whether the command completes or you reached the time out. If command
succeeds, you can then just read the stdout or stderr like we normally do.
- line 218: should this comment be removed?
Yes. I'll also add a comment in to_xml() which says that "None" values
are converted to empty strings in the profile.
- if line 353-354 is moved to line 338, I think you can skip the "if"
statement
in line 356. line 358 can be moved under the "if
self.asr_mos_password" check
in line 342.
Thanks, fixed.
- lines 367-368: can this be:
proxy_hostname += ":" + self.proxy_port
I thought using + for strings was discouraged for performance reasons,
and join was to be used instead.
-parse_ocmadm_output() and parse_asradm_output(). Why do these 2
functions
need to have the @classmethod declorator? From what I can see, these
functions
are only called within the class. So, why not just let them be instance
functions?
Strictly speaking, they don't need to be because they don't use any
instance variables. But since they are very specific to the (instance)
methods which are calling them, I'll make this change.
Thanks again for reviewing,
Jack
Thanks,
--Karen
On 03/14/12 01:27 AM, Jack Schwartz wrote:
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
