On 04/15/09 11:00, William Schumann wrote: > 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? Yes. It is. If it was the only argument that would be fine but there are quite a few in this case. >> >> >> 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. What I mean was the original code was explicitly returning B_TRUE or B_FALSE. Yes, these map to 1 and 0 which is what occurs in the changed expression.
> 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? True but with processors running in GHz and the eon's of cycles between cpu and memory access this should not really be an issue. -Sanjay > > 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 >>
