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 >>
