Hi William,
please see my comment below.
Thank you,
Jan
test_driver.c
-------------
307 - since you implemented separate paths for checking
AI and GUI partitions, could you please enhance test
driver, so that both cases can be tested ?
Also, could you please add this test driver to the recently
introduced SUNWinstall-test package ? It is intended to
bundle Caiman installer test drivers for purposes of QE team
and currently contains TD & TI test drivers. Thanks.
orchestrator_api.h
------------------
In order to be consistent with rest of the code in this file,
I might recommend:
enum partition_allocation_scheme {
/*
* select algorithm for partition allocation
* in om_validate_and_resize_partitions()
*/
AI_allocation = 666,
GUI_allocation = 777
};
->
typedef enum {
/*
* select algorithm for partition allocation
* in om_validate_and_resize_partitions()
*/
AI_allocation = 666,
GUI_allocation = 777
} partition_allocation_scheme_t;
Just out of curiosity, is there any specific reason for the
numbering scheme selected ;-) ?
auto_parse.c
------------
332 (api + i)->partition_start_sector = 0xFFFFFFFFFFFFFFFFULL;
->
332 (api + i)->partition_start_sector = (uint64_t)(-1LL);
disk_part.c
-----------
644, 734 - In order to be more explicit about the intent here,
I might recommend
if (partition_allocation_scheme != AI_allocation) {
->
if (partition_allocation_scheme == GUI_allocation) {
1022, 1034, 1037, 1108, 1121, 1129, 1132, 1134, 1139 -
in order to be consistent across the code, OM_NUMPART
should be used instead of FD_NUMPART
1048 if ((uint64_t)partition_offset_sec == -1LL) {
->
1048 if (partition_offset_sec == (uint64_t)-1LL) {
1062 - partition_size_sec == 0 indicates that the largest
chunk of available space should be picked up and on 1066,
the same check signals that whole disk should be used.
How are these two cases distinguished in AI manifest ?
1733 - Since sorted_parts is defined as static variable,
this might not be needed. Or is there possibility that
this array is initialized more than once ?
1735 - it is mentioned that "sometimes sectors field is blank"
Could you please put more specific comments about cases when it
happens ?
insertion_sort_partition_info() & sort_used_regions()
functions sort partitions by offset.
There is similar functionality already implemented in
target_discovery.c:sort_partitions_by_offset(). Do you
think it might be possible to leverage this, so that
we could avoid duplication ?
1788 - it is mentioned that number of free fragments
can't be ON_NUMPART+1 - this case might happen, when
all four partitions exist and there is a space available
at the beginning as well as at the end of the disk.
Is this corner case avoided to happen here ?
William Schumann wrote:
> Jan,
> I've completed modifications to disk_parts.c so that the AI starting
> sector is not overwritten in GUI code.
>
> I created a separate code path for AI and GUI in
> om_validate_and_resize_partitions(), since it was becoming overly
> complicated to have a single path.
>
> Ran spell(1) on some source files and corrected spelling.
>
> Added a cast when multiplying 32-bit values to compute a 64-bit value
> to prevent data loss.
>
> Tested AI:
> - tested new Solaris partition with best fit algorithm when best fit
> comes at start, middle, and end w.r.t. existing partitions
> - tested in absence of space requested in manifest
>
> Tested changes against GUI:
> - varied partition configuration before running GUI
> - tested deletion of DOS partitions in GUI
>
> Please review if you are still able.
> William
>
> jan damborsky wrote:
>> Hi William,
>>
>> I have just started to look at those code
>> changes. I would like to get back to you
>> tomorrow.
>>
>> Jan
>>
>>
>> William Schumann wrote:
>>> When a partition is created as specified in the AI engine manifest,
>>> the AI
>>> engine can determine the placement of the partition based on the unused
>>> regions in the partition map. The smallest free region
>>> accommodating the new
>>> partition will be automatically selected. Previously, the starting
>>> sector was required.
>>>
>>> http://cr.opensolaris.org/~wmsch/bug-5656/
>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=5656
>>>
>>> The automatic placement can be indicated simply by not specifying a
>>> starting
>>> sector for the partition in the manifest: element
>>> partition_start_sector
>>>
>>> Changed static functions used by
>>> om_validate_and_resize_disk_partitions() that
>>> were assuming sorted partition tables.
>>>
>>> Added additional parameter to
>>> om_validate_and_resize_disk_partitions() to
>>> specify whether or not to remove gaps between partitions. Gaps will
>>> now be
>>> compressed for the GUI, but not for AI.
>>>
>>> Also, code for http://defect.opensolaris.org/bz/show_bug.cgi?id=5655
>>> appears, but will not be invoked. This enhancement should be
>>> available soon.
>>>
>>> Tested by creating different combinations of FAT32 partitions with
>>> fdisk before
>>> using AI to create Solaris partition:
>>> - leaving free space of varying sizes between them
>>> - leaving free space at end, middle, beginning of disk
>>> - denying free space of requested size
>>> - exercised om_validate_and_resize_disk_partitions() functions
>>> - saved output of fdisk -W for testing identical conditions after
>>> code changes
>>>
>>> Made AI manifest element partition_number optional, since it is
>>> ignored when
>>> creating partitions.
>>> _______________________________________________
>>> caiman-discuss mailing list
>>> caiman-discuss at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>
>
>
