On 07/03/2012 05:23, Karen Tung wrote:
Hi Niall,
I looked through the revised webrev. Based on our long discussion
yesterday,
I agree that this approach is much better for correctness and user
experience.
I have a few comments:
disk_window.py:
- line 801: I think this should be log level debug.
Agreed
- In lines 840-849, you are checking to make sure we don't exceed the
max size.
In lines 870-875, you are checking again for the same thing, but with
byte units.
I think lines 840-849 is no longer necessary, right?
840-849 is to catch input errors by the user, so the level of precision
is limited to the
rounding error we incur via the user interface.
870-875 does a more precise, byte granularity check that would not be
possible via user input.
This is to compensate for rounding errors due to the limitations of
precision of the user input.
Notice also that if an error occurs here then it is due to an internal
error rather than user error,
and hence the reason it raises RunTimeError instead of UIMessage.
They fulfill separate, but both necessary purposes in my opinion.
- Now that that resize_*() code is moved to resize_validate(), I don't
think we need
any of the code in on_exit_edit(). There's no point to set it now
since nobody will see it.
If the user were to come back to the screen via F3 to go back,
everything on the screen
will get re-drawn anyway.
This callback is triggered whenever the edit field loses input focus,
not just when the user advances
to the next screen. Most commonly it is triggered by the user switching
focus to another input field
using the arrow keys. If it didn't clean the numerical formatting in
this way things would start to
look visually sloppy and inconsistent, or worse still blank size fields
if the user deleted all digits in the
edit field.
Thanks!
Niall
Thanks,
--Karen
On 03/06/12 02:19, Niall Power wrote:
Thanks again for the reviews. I've created a new webrev
Karen and I noticed a problem that I traced down to an error with the
current webrev that I have fixed also, in disk_window.py:
902 if isinstance(resized_obj, Partition):
903 # Don't do this for GPTPartition because there is no
guarantee this
904 # will be the installation target partition if there is
more than
905 # 1 Solaris partition
906 resized_obj.in_zpool = ROOT_POOL
907 else:
908 if resized_obj.in_zpool == ROOT_POOL:
909 resized_obj.tag = V_ROOT
If the object is a GPTPartition it will fall through into the else
statement at line 907, apply the V_ROOT tag to the GPTpartition,
which we do not want.
The fix fix is simply to change the else to an elif clause for Slices:
902 if isinstance(resized_obj, Partition):
903 # Don't do this for GPTPartition because there is no
guarantee this
904 # will be the installation target partition if there is
more than
905 # 1 Solaris partition
906 resized_obj.in_zpool = ROOT_POOL
907 elif isinstance(resized_obj, Slice):
908 if resized_obj.in_zpool == ROOT_POOL:
909 resized_obj.tag = V_ROOT
Revised webrev:
http://jurassic.us.oracle.com/~npower/webrevs/webrev-7151031-gptmaxsize-2
Thanks,
Niall
On 06/03/2012 20:26, Niall Power wrote:
Hi Sue,
Thanks for reviewing. Responses below...
On 06/03/2012 03:33, Sue Sohn wrote:
On 03/05/12 07:03, Niall Power wrote:
Hi,
I'd like to ask for a couple of reviews for the following few bugs:
A. 7151031: Text Installer resizes Solaris partition AFTER final
validation
UEFI/GPT development specific (no CRs)
B. Text installer doesn't calculate the maximum available GPT
partition
size correctly
C. ui_object is unitialised in validate() method.
7151031 is described in bugster. The proposed fix is to side step the
timing issue altogether by not resizing the partition in
on_exit_edit()
but instead do it much earlier when validating the numeric input into
the edit field. This results in a slight behavioural difference
but one
that is an improvement IMO. It has the following benefits:
- solves the key problem of on_exit_edit() resizing the partition
after
final validation. All size modifications will be completed prior to
final validation.
- Improves the interaction of the text UI as available size field for
other partitions/slices is adjusted and update instantaneously as
user
edits the partition size. UI is more responsive now.
- Resizing errors are caught earlier.
Changes are to disk_window.py.
B. The problem is in
ti_target_utils.py:UIGPTPartition.get_max_size() at
line 671+
The method tries to calculate free space by examining start and end
sectors of adjacent partitions as well as the end sector of the
enclosing disk. This is wrong because it fails to accound for the
sectors at the beginning and end of the disk occupied by the
primary and
backup GPT tables. These sectors are not available for partitions to
consume and this causes resizing to raise an InsufficientSpaceError
exception.
Besides that, iterating through the UI objects to look for
partitions is
a bad idea. The DOC should be used as the definitive reference when
searching for free space. From my own in use observations, this
method
is unreliable and yields incorrect results for available size. It is
just plain broken.
The solution is to replace the implementation with one based on the
get_gap() methods provided by the target physical GPTPartition
object.
These methods account for the start at end sectors reserved by the
primary and GPT table headers and take these calculations out of the
hands of text install.
I think the get_max_size() methods for the DOS partition and Slice
objects (UIPartition and UISlice) could do with some improvement
too by
the way (I logged 7151025).
Changes are to ti_target_utils.py
C. The problem here is that a block of code was accidentally removed
from a previous commit. ui_object in PartEditScreen:validate()
would be
unitialised before reference on SPARC at line 294.
Changes are to partition_edit_screen.py
Webrev at:
http://jurassic.us.oracle.com/~npower/webrevs/webrev-7151031-gptmaxsize/
Thanks!
Niall
Hi Niall,
disk_window.py
815 No need for backslash
Fixed
821 Should this be "return True" as what it used to do when it fell
through to (old) 840?
Good catch! Fixed.
864 artefacts
Fixed.
866 should this also be a return True as on 859?
Yes. Fixed.
Thanks!
Niall
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
