auto_install.c
423: Please don't do complex assignments in a function args. I see
that this is not specifically your change but could you assign
slice_number and the pass it in.
disk_slices.c:
om_set_vtoc_attrs
775: Please change B_FALSE to 0 since the caller checks for 0 and
not B_FALSE. It also matches the comments at the top of function.
Can you elaborate why you are returning 0 and not marking
this as an error and jumping to line 905 since the caller in each case
does mark that as an error.
perform_slim_install_install.c
Please revert line 2922 to the original form. Though both are
correct. The original line is
explicit and easier to understand.
On 04/15/09 07:09, William Schumann wrote:
> Alok,
> Responses below.
> Alok, Sanjay,
> Generated new webrev:
> http://cr.opensolaris.org/~wmsch/bug-7758/
> Previous webrev for reference:
> http://cr.opensolaris.org/~wmsch/bug-7758-prior/
>
> Alok Aggarwal wrote:
>>
>> On Tue, 14 Apr 2009, William Schumann wrote:
>>
>>> Alok,
>>>
>>> Alok Aggarwal wrote:
>>>> Hi William,
>>>>
>>>> On Thu, 9 Apr 2009, William Schumann wrote:
>>>>
>>>>> There are two problems addressed by this bug.
>>>>>
>>>>> 1) After initial AI installation, the swap slice is regarded as
>>>>> user data in subsequent AI installations instead of scratch
>>>>> 2) The swap slice space allocation in TI is not done with any
>>>>> awareness of AI customization
>>>>>
>>>>> The fix involving making AI aware of 1 and for AI to perform 2
>>>>> instead of leaving everything to TI, which allocates the swap
>>>>> space for GUI.
>>>>
>>>> So, from a high level, is it fair to say that the policy with
>>>> respect to preserving slices by default isn't being
>>>> changed?
>>> Yes, the policy for preserving slices by default does not change.
>>>> And, the change you're making here is to create
>>>> a swap slice if either s1 doesn't exist or s1 exists with
>>>> a V_SWAP tag. Correct?
>>> The decision of whether a swap slice is created depends upon the
>>> memory size of the machine.
>>>>
>>>> Also now that 7718 has been reversed in the source code
>>>> only machines with less than 700MB memory will see this
>>>> problem on every other AI install after the *first* install.
>>> I think it would fail *all* AI installs after the first one (that
>>> didn't explicitly delete slice 1 per the workaround); otherwise,
>>> what you said is true. Also, without the fix, problem 2) above
>>> still exists - if a swap slice is called for, any slice
>>> customization or any existing slices which should be preserved will
>>> be destroyed instead.
>>
>> Okay, thanks for clarifying. Here are my comments -
>>
>> auto_install.c: Why is OM_ROOT being set for installs on
>> slices other than slice zero?
> OM_ROOT indicates that the VTOC partition tag for the install slice
> will be set to "root" - slices 0,1,3,4,5,6, or 7 could be the install
> slice.
>>
>> perform_slim_install.c: line 754: Nit - Sentence seems incomplete.
>> " .. if swap slice needs to be created"
> changed
>>
>> perform_slim_install.c: line 2919: This function no longer
>> returns a boolean_t
> It returns the boolean_t values (0 or 1), which are evaluated in
> logical expressions as either zero or non-zero, so I don't see a
> problem. In principle, the definition of boolean_t could conceivably
> be changed, but much code would break. Also, I think of the boolean_t
> type as helpful in simplifying logical expressions making them more
> readable.
>>
>> ti_dm.c: lines 1503 - 1515: It seems that this comment should
>> be made clearer to indicate under which conditions a swap slice
>> would be created - a) If it is explicitly specified or, b) if
>> the code figured out if the installation was being done on a low
>> memory machine
> added comments here
>>
>> disk_slices.c: line 50: Why this change?
> I tried to explain this in the code review request - I was just
> changing the name from "install", which is obviously not a unique
> token, to "s_install", which would be more easily found in a text
> editor - that's the only reason. Vim has a nice feature - if you type
> '*' on a token, it grabs the token as the search string and jumps to
> the next occurrence.
>>
>> disk_slices.c: line 365: How about check_slices_in_use() as
>> the name instead?
> I thought that "if (are_any_slices_in_table())" would be simple and
> readable for a function that asks the question "are there any slices
> already defined in the table?". Was this unclear? Too casual?
>>
>> disk_slices.c: line 376: What is an exempt slice?
> A slice that is reserved for a special purpose and cannot be used for
> user data. Changed "exempt" to "reserved".
>>
>> disk_slices.c: line 398: What tag will be returned if none
>> was set in the first place? Have you verified it is V_UNASSIGNED?
> Yes, "unassigned" is partition tag == V_UNASSIGNED (zero).
>>
>> disk_slices.c: line 406: Nit - I don't think the comment needs to
>> include the type of this variable
> removed
>>
>> disk_slices.c: lines 430-432: Could you please expound on this?
>> There seems to be implicit ordering here that the callers must
>> maintain.
> Actually, the purpose of the code here is to avoid implicit ordering
> by the caller. If a swap slice is needed, it is easier to create it
> before creating any other slices - otherwise you have to decide which
> slice to take the space from. This create_swap_slice_if_necessary()
> call insures that the swap slice will be allocated before any other
> new slices are created. Also, create_swap_slice_if_necessary() is
> called during finalization for TI.
>
> Perhaps I should mention here that the creation of a swap slice cannot
> be specified in a manifest - it is automatic.
>>
>> In other words, this seems to be true only for AI installs and if
>> auto-install were to be changed to reorder how it executes
>> manifest actions, that could lead to problems here.
> I can't think of any slice action ordering that will break this code.
> Do you have an example in mind?
>>
>> disk_slices.c: line 607: Again this comment is not clear.
>> Suggest rewording as "If default slice action is specified (i.e. no
>> customizations), create a swap slice if deemed neccesary for the
>> install to proceed (typically true only
>> for low memory systems)"
> Took suggested wording.
>>
>> disk_slices.c: line 658: The thought of possible recursion
>> in this function makes me queasy. This seems like a perfect
>> candidate for some code refactoring in the future.
> Perhaps you understand this already, but I'll add the following just
> in case. There is no explicit initialization call in the Orchestrator
> for AI where the swap slice might be allocated. So, the first call
> from AI concerning slice customization could be om_create_slice().
> Within om_create_slice() is a check to see if the swap slice is
> necessary. If it is, om_create_slice will be called recursively once,
> before it does any other work, so make sure that the swap slice is
> allocated. Furthermore, there is a guard so that it is not run more
> than once for the same purpose. Does this help?
>>
>> disk_slices.c: The thing I'm still not clear about is whether
>> AI installs done prior to this bug fix will have V_SWAP set
>> for the swap slice. Will they?
> Yes, I checked this - the partition tag was (and is) being set to
> V_SWAP in /a/etc/vfstab.
> William
