Sanjay, Sanjay Nadkarni wrote: > > 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. Do you mean that you don't want to see ternary operators selecting function arguments? Is this a readability issue? > > > 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. You correctly point out that the code should be consistent and jump to the error tag as it does nearby. I have changed this. > > > 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. > I don't understand the logic here. The function definition explicitly indicates a boolean is to be returned, and it is clearly returning a boolean - it seems easier to return a simple boolean expression instead of a ternary operation. Following the same logic, doesn't this mean that each time the returned value is to be checked, it should be evaluated against B_TRUE or B_FALSE, too?
William > > > > 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 >
