On 05/ 6/11 03:25 PM, Karen Tung wrote:
Hi Keith,
Thank you for your code review. Please see my responses inline.
On 05/02/11 13:12, Keith Mitchell wrote:
Hi Karen,
My comments for the UI portion or below. Mostly nits, other than the
Threading points I brought up in person.
Thanks,
Keith
General:
The directory structure used to be:
usr/src/cmd/text-install/osol_install/{text_install,profile}/<file>
Originally, this was so that the Text Installer UI could be run
without building the gate; I think that stopped working or became
rather complicated at some point since the Text Installer went back -
in any case, I think it's easier to run from the proto area these days.
Given that, instead of collapsing to text-install/solaris_install
(which initially confused me, looking at
text-install/solaris_install/__init__.py), can you just collapse it to:
usr/src/cmd/text-install
and
usr/src/cmd/text-install/profile
That makes sense, and it will help to reduce my typing. :-)
We won't need "usr/src/cmd/text-install/profile" since we
are just using DOC objects now. After your suggested update above, we
will
have the following:
usr/src/cmd/text-install (all the python files will be here)
usr/src/cmd/text-install/helpfiles (for all the help files)
usr/src/cmd/svc (this is the SMF service)
Excellent.
Logging:
In a few cases, I'll point out that it's better to use:
logger.debug("my message: %s", my_arg)
rather than
logger.debug("my message: " + my_arg)
The reason is that for the former, if the log level is set such that
debug statements are ignored, then the relatively expensive string
formatting operation will be skipped; in the latter, the
concatenation is done regardless of log level, which isn't desirable.
For both the cases I mention and the ones I don't, please update usage.
OK, I wasn't aware that there's a difference, I will pay attention and
fix all of them.
If you have code that is run frequently (say a function call that is
called in a loop, lots and lots of times), string formatting operations
are expensive and could slow down that code significantly. If the
formatting is just being done for logging purposes, it may be for
nothing, too (for example, if the logging level is set to WARN but the
statement is logging.debug()). The syntax logger.debug("my message: %s",
my_arg) defers the string formatting to the logging module - if the
statement would get ignored, the formatting is skipped, and much time is
saved.
It's a minor difference, *most* of the time irrelevant to what we do,
but a good habit to be in.
import *:
It's best to avoid 'import *' - it becomes difficult, when coming
back to the code later, to figure out where a given function was
imported from. I'd suggest replacing with 'import x as y; y.func();
y.variable" etc.
Yes, Drew pointed this out as well. I have cleaned up all of them now.
Thanks!
if <list> is None or len(<list>) == 0:
These sorts of checks can universally be replaced with a simple: "if
<list>:". Both empty lists and None return False in a boolean context.
This is also fixed now.
=========
usr/src/cmd/text-install/solaris_install/__init__.py
41, 90: Nit: Spaces around '='
Fixed.
58/253: StderrCalledProcessError is a subclass of CalledProcessError;
based on your usage, you only need CalledProcessError
Fixed.
97: Maybe just me, but it would seem this is bending things a bit
more than needed - rather than create all these values here, can we
not just invoke eng.register_checkpoint() with desired arguments as
part of prepare_engine()? I don't see much gain by separating the
data as such.
The reason I have it done this way is I want to have the checkpoint
names defined, because some of them
are used elsewhere in the text installer app. Since I have the
checkpoint names defined, I thought I will
have the checkpoint information defined in the same place too.
I played around with moving the checkpoint information to having it
just inside register_checkpoint. It seems
to look better. However, I will still need to have the checkpoint
names defined separately.
That'll work.
257, 345-348: Nit: Spacing
Fixed.
402: Would it be possible to log the install_data object here? Or
does that not provide the same value as before?
Printing the install_data object here is not too useful, since the
install_data object only have trivial information
about location of the log file...etc.. I think it will be useful to
dump the whole DOC here, since all
install related information is now stored in the DOC.
Since we want to provide as much information as possible when there's
an install failure,
I will dump both the DOC and the install_data object here.
Sounds like a good idea.
usr/src/cmd/text-install/solaris_install/disk_selection.py
113: Since you never read from the global LOGGER, you don't need to
set it as a global - the instances can all be replaced (in this file
and elsewhere) with logger (or LOGGER) =
logging.getLogger(INSTALL_LOGGER_NAME)
As we discussed, I have fixed the places where I have to get an
reference to the logger
before the global one is set in the file. For the ones that's needed
globally in the file,
I just left them as is.
Sounds good.
128: This looks like it's no longer used and can be deleted.
max_disk_size is used immediately in the next line to calculate the
too_big_warn variable.
I must've blinked when looking at that line, sorry!
247: There's an extraneous set of parentheses here.
You mean this line:
246 LOGGER.debug("size: %s, min: %s",
247 (disk.disk_prop.dev_size,
self.minimum_size))
The parentheses is actually needed. However, there's a bug
that the last character for line 246 should be '%', instead of
','. I fixed the bug, but I have to have the parentheses.
Per my previous logger comment, it should be:
246 LOGGER.debug("size: %s, min: %s",
247 disk.disk_prop.dev_size, self.minimum_size)
;-)
257: This should reference NO_TARGETS still. Also, I think 229 should
probably use the NO_DISKS message.
Yes, fixed as suggested.
319, 393: Can you clarify why the handling of GPT labeled disks is
being changed?
At this time, GPT labeled disks are not support in the target code.
So, I just removed
the code for now. When GPT disk support are added to the target code,
we can add
the support back in.
They weren't supported initially either. The code there was set to
inform the user that the disk was GPT labeled; the user would then be
informed that the label would be overwritten with an fdisk label when
attempting to go to the next screen.
331: Nit: on_active() has a reference to target controller via
"disk_select.tc"; a separate parameter isn't strictly required
Good point. Remove the extra parameter passed.
355: Nit: Change to:
LOGGER.debug("disk_selection.on_change_screen, saved_index: %s",
self.selected_disk_index)
Fixed.
367-389: This function should be collapsed into 359:start_discovery.
It shouldn't be necessary to run in a separate thread explicitly (see
362), since you can invoke execute_checkpoints() without blocking.
wait_for_disks() on 170 needs to be integrated with some form of
callback.
Fixed.
426: Change to "if not parts:"
fixed.
usr/src/cmd/text-install/solaris_install/disk_window.py
250: Change to:
LOGGER.debug("have_partition: %s", self.has_partition_data)
fixed
337: Logging formatting
fixed
345/348: Since the statements are the same now, this can get pulled
outside the innermost 'if' block
fixed
458/460: If this doesn't need any special formatting, change %i to %s
and remove the cast to int()
fixed
545-546: I'm concerned to see this code removed. Admittedly, it was a
workaround for an issue I was having with the "size" field for
deleted logical partitions still appearing. Can you point me to the
equivalent replacement?
As you indicated below, every time we make any change, the change is
done in the DOC,
and the UI is completely redraw to display the state of the UI. I
think I removed this block
of code because it is comparing something to the "original" data which
we no longer use.
Furthermore, I haven't done much testing in regards to logical
partitions.
I plan to do that after the initial putback. If I found problems and
find the need to
re-use some of all of this code, I will restore it.
Ok.
717-719: Nit: Replace with:
if in_use["used_by"]:
fixed.
723, 852: I don't think it's correct to call activate_solaris_data.
The user will want the currently selected field to remain the same.
Although after seeing what was done here, I can see perhaps how
545-546 worked; it's re-drawing the entire screen.
Yes, it is not correct to call activate_solaris_data() at these 2
locations. I have changed it so that
I will get the index of the selected item before changes to the DOC
and re-drawing the entire
screen is done. After the screen is re-drawn, I activate the object
at the index I saved.
Great, thanks.
764: This function looks like it no longer does any work - please
remove it.
yep, removed.
usr/src/cmd/text-install/solaris_install/fdisk_partitions.py
162: Nit: "if not all_slices:"
fixed.
209-212: Could be:
found_parts = bool(all_parts)
fixed
246: This seems to be a behavioral change. The 'if' block there was
intended to ensure that if the user selected a given option, moved
forward/backward screens, then came back to this screen, the
selection would be unchanged.
The functional behavior that needs to be preserved is: after getting
to the summary screen, I should be able to hit F2/F3 to bounce
between any set of screens without having my selections get modified
from what I first chose.
ah, ok. I guess I miss understood what the original code was checking.
Since there is no flag in the DOC for indicating whether whole
disk/whole partition
is to be used, to preserve the original behavior, I added a flag in the
FDiskPart class. This flag will have initial value of true so
use_whole_segment is selected.
In the on_continue() function, I set it to indicate whether the user
selected to
use whole segment or not.
Sounds good.
usr/src/cmd/text-install/solaris_install/install_progress.py
As with TD, the engine should handle the threading now.
Fixed.
usr/src/cmd/text-install/solaris_install/install_status.py
69/80: Should the reference to this object not get pulled from the
engine's DOC?
No, because I no longer store this object in the DOC. "install_data"
is now
an object that is initialized when the text installer starts. It
just stores a few "house keeping" information for the text installer.
It just gets passed into the other classes that needs it.
No other DOC components needs to read/write it's value.
I figure that there's no point to store it in the DOC.
Ok, that clarifies.
usr/src/cmd/text-install/solaris_install/log_viewer.py
Same question
same answer. :-)
usr/src/cmd/text-install/solaris_install/partition_edit_screen.py
115, 120, 125: I believe these need to be reverted.
fixed.
177-202: Not the ideal place for this logic, but that's partly my
fault. Still, with the "old" stuff we had a function called
"create_default_layout()" to cover similar scenarios; perhaps
TargetController needs something like
create_default_{disk,partition,slice}_layout() functions?
Actually, this logic will be removed once we can boot GPT labeled
disks/partitions.
I will put a comment here to indicate that. We do have functionality
similar to create_default_disk_layout() in the target controller, to
take care
of the case where you have an empty disk. For partition,
there's no need for a "default" layout. If there's no previous
partition,
you are required to select the whole partition.
I will put a comment here about the fact that the code should be removed
when GPT labeled disk/partition is supported.
Ok.
Also, what happens with perform_final_validation() here - I doubt it
would fail, but what if it does?
perform_final_validation() shouldn't fail. If it does, the whole
application will quit.
The perform_final_validation() function raise "ValueError" if there's
a validation problem.
The user can see the log file to see why.
Ok. I find that strange, but I guess that's a question for target, not
text install.
270/282: Why is perform_final_validation() only called for
x86_slice_mode here?
I was trying to prevent perform_final_validation() to be called when
we are modifying the
partition layout, since we don't want to validate the final layout
until the slices are also modified.
I forgot that will also block out the case for SPARC. So, I need to
change line 270 to
only return only when it is x86 and x86_slice_mode.
usr/src/cmd/text-install/solaris_install/progress.py
No comments
usr/src/cmd/text-install/solaris_install/summary.py
48: Nit: Spaces around '='
fixed
203: Lingering commented line
fixed.
usr/src/cmd/text-install/solaris_install/text-install
No comments
usr/src/cmd/text-install/solaris_install/ti_target_utils.py
51, 52, 53, 55: These variables seem like they should be 'owned' by
something in target
I found PART_TYPE_EXTENDED, BOOT_SLICE and BACKUP_SLICE defined in
target. So, those will
be used. I didn't find 191 defined in target yet. I am going to keep
it here for now, and file
a bug against target to define it.
Ok.
75/78: The must_have_target_disk parameter seems at odds with the
usage. For example, if there is not target disk, the list returned on
78 by the get_descendants call would be length 0, correct? So there
would be an immediate IndexError from trying to get the [0] item from
that list, or from trying to call get_children on line 81.
This is one of the first function I wrote when I was working on this,
and I think I was trying to
be flexible. :-) I will remove the must_have_target_disk parameter
from all the functions.
I can not think of a case where I use these functions without a
desired target. So, the desired
target tree must exist.
Ok.
83-84: Seems odd to raise a RuntimeError here when the DOC has its
own mechanism for raising errors when an expected object isn't found.
The get_first_child() function can not accept the not_found_is_err
argument. So, I was checking.
Now, I have changed it to use get_descendants() call which does
support not_found_is_err.
Ah, ok.
Some of the other functions in this file suffer from similar concerns.
537, 553: "Wrong class" errors should be TypeError
Fixed.
571, 580: Nit: Might just be me, but I think these should be
ValueErrors (or AttributeErrors?)
Actually, if the objects are not in use, we should never be calling
those functions.
So, the fact that those function are called is a big problem. That's
why I think
they are RuntimeError.
Ok. I'm fine with how it is.
But just to clarify why I brought it up at all, I think it's arguable in
either direction; the end result is an uncaught exception, either way.
As a general rule I like to see "specific" exceptions (ValueError,
AttributeError, TypeError) over "general" ones like RuntimeError, when
possible.
566-569: Nit: With how frequently this if/else block is repeated, it
might be convenient to have an "obj()" method. Then these could be
replaced with:
return self.obj().name
Where obj() returns self.doc_obj or self.empty_space_obj as
appropriate (or raises Value/Attribute/RuntimeError if ui_type isn't
properly set)
obj() could also be a property, if desired
I remembered started in this route, eventually, I abandoned it. This
is because if I were to
go this route, I would need to define functions in the EmptySpace
object parallel to
all the functions defined for the Partition object. I also want to
use the same EmptySpace
object for the Slices. Slice objects have some different
functions/properties defined
for it than Partition objects. After I rewrote this a few times
trying to make it work,
I decided to just go with this not elegant route. When I do the
re-factoring
work you mentioned below to combine UIDisk/UIPartition/UISlice, I can
also tackle this
and make it better.
That makes sense. Using obj() would end up replacing all the 'if/else'
blocks with 'if isinstance(...)', so there's no real gain unless
EmptySpace is fleshed out to be "duck-typed" similar to a Partition object.
738-741: Nit: could be:
return (discovered_size_byte - size_byte > precision)
Fixed
UIDisk/UIPartition/UISlice: There's room to condense this file with a
proper abstract parent class. There's quite a bit of very similar
code in functions of the same name. Admittedly an existing problem,
of sorts, and not immediately critical; though filing a bug to
follow-up and address this later would be good, if there's not
time/inclination to do so now.
Since these classes are so critical to the UI layout, and took me a
long time to get them right,
I don't want to take the risk of doing major restructuring now.
I will file a bug to re-examine and improve them later.
Completely agree with that.
1116: I'm concerned by the hardcoding of 'rpool' here (and
elsewhere). Is this going to limit us, should we, say, decide to
allow the user to enter an arbitrary name for the ROOT POOL in the
future?
This is just a temporary limitation. It's my plan to restore the
existing text installer's
feature of using the next available root pool name. Another bug to
fix after my initial putback. :-)
Ok.
usr/src/cmd/text-install/solaris_install/welcome.py
No comments
Thank you so much again for doing the code review.
You're welcome!
- Keith
--Karen
On 04/26/11 01:38 PM, Karen Tung wrote:
Hi everyone,
I am looking for reviewers for changes that move
the Text Installer to the CUD architecture.
I divided up the changes into the following 4 groups.
I would like to get at least 1 person looking at each group.
Please email me privately and let me know which group
you like to review.
I don't think a code walk through is needed, but if you think
having one will be useful, please let me know and I can set one
up on Thurs. AM.
The webrev is at:
http://cr.opensolaris.org/~ktung/text-to-cud/
Please provide me your comments as soon as possible.
I am more than happy to get multiple sets of comments as
you go through the code.
I would like to get most of your comments by COB next Monday, 5/2/2011.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
