I neglected to mention that the webrev has been updated. -evan
Evan Layton wrote: > Ethan Quach wrote: >> Evan, >> >> be_activate.c >> --------------------- >> 519 - "current" -> "this" to be consistent with 617 and 696 > > Fixed > >> 1116, 1131-1142 - Can you make these block function comment >> lines consistent with other block function comments. > > Fixed > >> Seems random to place these two new functions in be_activate.c, >> they don't have any particular relation to activate. I can't think of >> a better place to put them though, maybe be_utils.c instead? > > I had placed these here since that was the first place I used them > however I have no problem with moving them. However at one time we > had talked about trying to move all the functions out of be_utils.c > and into appropriate other functions so we could get rid of be_utils.c. > That's why I had not put them in be_utils.c but I can easily move it > if that's what we want to do. > >> >> be_list.c >> ------------- >> 427, 560, 663 - These lines look awkward; they really shouldn't >> even be executed if we're (!i386) right? Perhaps they should just >> be combined in with the if() clause at 425,558, 662 respectively. > > These lines will not be run if we're (!386) since grub_default_bootfs is > initialized to NULL. If we're (!386) we won't run be_default_grub_bootfs() > and > grub_default_bootfs will be NULL so we will not go into these lines. > >> >> be_utils.c >> --------------- >> 955 - nit - Could we instead pass the err back as the return value >> and pass the default bootfs string via a reference pointer? > > I don't think this really buys us much right now and I'd really rather leave > it > this way for now. I'll make this change as part of the changes I'm making for > 3608. I've updated 3608 to reflect that this change should be made there. > >> 1066 - Can we not use "Sparc" here. I think somewhere else >> you used "this architecture" instead. > > Fixed. > >> 1068 - nit - Add an empty line after this line. > > Fixed > >> 1218 - Should "grub" still be here? > > no it shouldn't, I've changed it to "boot". > >> >> libbe.h >> ----------- >> 102 - This needs to be added to the end of the enum. >> Recall we have a copy of this in beadm/messages.py > > oops you're correct, I've fixed this... > > > Thanks for looking at this! > > -evan >> >> >> thanks, >> -ethan >> >> >> Evan Layton wrote: >>> We need to get some code reviews for the SPARC support >>> for SNAP. >>> >>> The changes were mainly in how the SPARC boot menu is >>> handled compared to how the grub menu is handled. >>> >>> The changes cover the following CR's only: >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4297 >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=5401 >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=5420 >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=5604 >>> >>> The webrev is available from: >>> http://cr.opensolaris.org/~evanl/5420 >>> >>> Unit testing for this has been completed however general >>> testing for this is not yet complete. This testing is in >>> progress and has shown no new issues thus far. >>> >>> Thanks! >>> -evan >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
