Hi Karen,
thank you very much for code review.
Let me address your comments related to section F -
please see my response in line.
Jan
On 12/30/10 12:17 AM, Karen Tung wrote:
Hi Jan and Keith,
Here are my comments for files in sections E and F.
General comment:
-------------------
3 comments on the following line in all the files for the text installer:
self.install_profile = doc.get_descendants(name="install_profile")[0]
- Instead of hard coding the string "install_profile" here, I think
you should
use the label string defined in the install_profile object.
- I think it would be useful to check and make sure the
value returned from doc.get_descendants is not None instead of just
assuming
that it is valid.
- Would it be useful to define a helper function for getting the
install profile obj so we don't have the exact same code all over the
place?
I understand that it is just 2 lines, but it would still be useful
to just make 1 change instead of 10+ changes, if something needs to
change.
Specific comments:
---------------------------
> usr/src/cmd/text-install/osol_install/text_install/__init__.py
- line 35: why are you doing "del gettext" here? It would be useful
to put a comment here explaining the reason.
> usr/src/cmd/text-install/osol_install/text_install/disk_selection.py
- line 99: instead of hard coding the full path to the text installer
help file
here, I think it would be better for the future if the path to the text
installer help file is defined in a common place, and then, just append
the specific help text file for the screen here.
> usr/src/cmd/text-install/osol_install/text_install/fdisk_partitions.py
- lines 70-77: same comment about the help file paths.
> usr/src/cmd/text-install/osol_install/text_install/install_status.py
- line 81: "install_succeeded" is not defined in the install_profile
object.
I understand that you can just add variables to an object in Python.
But I think it would be useful for debugging to initialize it
to False in the __init__ of the object, and also to print the
value in the __str__ method.
>
usr/src/cmd/text-install/osol_install/text_install/partition_edit_screen.py
- lines 82-90: same comment regarding hard coding the full help file
path.
> usr/src/cmd/text-install/osol_install/text_install/summary.py
- lines 55-56: same comment regarding hard coding the full help file
path.
- lines 151-152: I think it would be better to define somewhere, perhaps
somewhere in the sysconfig module,
constants for indexes in the array for the root and primary user, so,
we don't
need to remember that 0 means root and 1 means primary user.
> usr/src/cmd/text-install/osol_install/text_install/text_install.py
- line 149: why is the loglevel hard coded to 0, instead of the
value in options.log_level?
- lines 155-156: "no_install_mode" and "is_x86" are not defined in the
install_profile object. Should they be. As with the previous comment,
it would be useful to define them and to print them out in __str__ for
debugging.
- line 160: I know there's no harm to register the checkpoint here.
But would it make the code easier to read to register it before you need
to run it?
- line 254: The way you are getting a reference to the install profile
here
is different than all the other places in the text installer, where you
are searching for name="install_profile". Please be consistent.
> usr/src/cmd/text-install/osol_install/text_install/ti_install.py
- lines 69-73: these lines are no longer needed.
Yep. I will remove them.
- 386-402: I believe all these lines are no longer needed since the
ti_install.py file isn't involve in calling any functions directly
to create the users. Please also fix the function definition and the
calling of
the run_ICTs() function, since most of those arguments are no longer
used.
You are right. I will fix that part.
- line 491: since this is temporary, perhaps move the registration
of the checkpoint to execute here? That way, we have all the
checkpoint execution
stuff that need to be "relocated" in one place.
That sounds reasonable. I will change that.
By the way, is the name
"apply-sc-profile" defined somewhere in the sys
config code? If so, it will be better to use that constant than hard
coding
the string here.
Currently, it is not defined as a constant. Since it is used at several
places,
I agree it makes sense to do so, perhaps in system-config/__init__.py
- line 580-581: since the previous C ICT "ict_set_host_node_name" is
split
into 2 separate ICTs. You are just calling the ict_set_hosts here,
Do you also need to call the ict_set_nodename ICT?
No, since system hostname is now configured in System Configuration manifest
as config/nodename property of svc:/system/identity:node smf service.
This is actually reason why ict_set_host_node_name was split, since we still
need to take care of populating hosts(4) file (until CR6996436 is fixed).
Once that happens, we can remove ict_set_hosts() ICT task as well.
> usr/src/cmd/text-install/osol_install/text_install/welcome.py
- line 56: same comment about full path for help file.
> usr/src/lib/libict/ict_test.c
- line 114, line 125: I know you are just following the convention in
this file.
Is there a reason why a hard coded number is used as the 3rd argument to
the strncmp() function instead of doing strlen(SET_NODENAME) and
strlen(SET_HOSTS)? Using strlen() seems more maintainable.
Yep. Following existing convention was the only reason for using hard
wired numbers instead
of strlen(). I actually considered going with sizeof(), as those are
arrays with constant length,
but at the end I preferred consistency over maintainability, since that
code is going to disappear
anyway as a result of new ICT effort Ginnie is involved in.
However, if you would prefer to have it changed, I can certainly do
that. Please let me know.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
