Hi William,
thank you for making those changes,
please see my response in-line.
Jan
William Schumann wrote:
> Jan,
> If you don't see a comment, that means I've implemented your suggestion.
>
> jan damborsky wrote:
>> 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 ?
> Fixed test driver "disktest" so that the proper path will be invoked
> for GUI vs. AI testing.
Thanks.
> Changed Makefile so that test driver "dummy_install" will build.
This is more than I asked for - looking at dummy_install,
it is out of date, as it seems to test pfinstall installation.
As far as AI is concerned, I think that this one is to be replaced
by introducing support for dry-run in AI engine.
>
> I added functionality to disktest originally for testing the
> Orchestrator enhancements to maintain slices and partitions, but found
> it unnecessary when AI manifest processing was implemented. The test
> driver code reads a text file and performs operations according to
> commands and parameters. I have not tried to use it since initial
> development. For now, I am not going to unit test it, since I am not
> certain that it will be used.
I think that QE team is interested in this test driver for testing
the orchestrator. If you think this is not the appropriate tool
to take advantage of, should we probably offer another way, how
those changes might 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:
>>
>>
> ...
>>
>>
>> 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
> There's no need to enter line numbers when a text editor can just
> substitute.
Good point :-)
>>
>>
>> 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 ?
> Bug 5653 implementation will read "max_size" from partition_size and
> call om_create_partition() with a size of zero. Use of "max_size" is
> the documented method.
ok.
>>
>>
>> 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 ?
> sorted_parts is a small table of partitions sorted by starting offset
> that is reinitialized and reused each time the free space table is
> built. I will clarify this in the comments. Code was cleaner having
> this as a static. It should be cleared with each usage.
Thanks for clarifying this.
>>
>>
>> 1735 - it is mentioned that "sometimes sectors field is blank"
>> Could you please put more specific comments about cases when it
>> happens ?
> I have not been able to identify cases when this occurs. I will
> comment that the same target information in different units is stored
> in the same struct, and this technique is prone to bugs. Fixing this
> in TD could require some time.
ok - could you please file bug for tracking this problem ?
>>
>>
>> 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 ?
> Perhaps, but it operates differently than the free space managers in
> disk_parts.c and disk_slices.c, which I would like to keep uniform,
> and it would require recoding in disk_parts.c, so I don't think it is
> worthwhile.
ok.
>>
>>
>> 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 ?
> It isn't, since if all partitions are in use, the free space table
> code will not be called, since it will be screened by a check in
> create_partition().
ok.
> William
>>
>>
>>
>> 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
>>>>
>>>
>>>
>>
