Good with me, as we discussed. One further nit - I think that comment on 687 is unnecessary and could just be removed. /jb
On Aug 17, 2011, at 12:33 PM, Drew Fisher wrote: > Sue, > > I can certainly make that change to the output. I'd like to get some > additional comments (if anybody has any) ... > > Thanks for the review! > > -Drew > > On 8/17/11 10:03 AM, Sue Sohn wrote: >> On 08/17/11 08:33 AM, Drew Fisher wrote: >>> Good morning! >>> >>> Could I please get a code review for: >>> >>> 7071109 <http://monaco.us.oracle.com/detail.jsf?cr=7071109> AI installer >>> confused by FC disk connected via two controllers >>> >>> https://cr.opensolaris.org/action/browse/caiman/drewfish/7071109/webrev/ >>> >>> Here's the sample output as shown by AI: >>> >>> 15:19:18 Install Log: /system/volatile/install_log >>> 15:19:18 Using XML Manifest: ai.xml >>> 15:19:18 Starting installation. >>> 15:19:18 0% Preparing for Installation >>> 15:19:18 100% manifest-parser completed. >>> 15:19:19 0% Preparing for Installation >>> 15:19:19 1% Preparing for Installation >>> 15:19:19 3% Preparing for Installation >>> 15:19:23 c2t200700A0B821AA6Cd0 is unlabeled. Forcing a VTOC label >>> 15:19:30 Unable to label c2t200700A0B821AA6Cd0: Current Disk Type is not >>> set. >>> 15:19:44 20% target-discovery completed. >>> 15:19:44 === Executing Target Selection Checkpoint == >>> 15:19:44 Selected Disk(s) : c3t0d0 >>> 15:19:45 67% target-selection completed. >>> 15:19:45 99% ai-configuration completed. >>> >>> >>> Jesse raised a concern that that error really sticks out and it may >>> cause additional bugs to be filed. I wanted to run that issue past C-D >>> to find out how we want to handle an error with forcing the label to the >>> disk. >>> >>> options: >>> >>> - Move the .info() calls to .debug() to hide them >>> - Add additional comments like: "unless this is your install volume, you >>> can safely ignore this" to try to minimize the issue >>> - other? >>> >>> Thanks! >>> >>> -Drew >> >> I kind of agree with Jesse - that looks like a call generator. Maybe add the >> word "Warning" (unless that's inappropriate) and change it to say; >> Warning, Unable to label xxxxxxx: Current Disk Type is not set. >> This warning can be ignored, unless the disk is your install volume. >> >> Sue > _______________________________________________ > caiman-discuss mailing list > [email protected] > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
