Evan Layton wrote: > Jack Schwartz wrote: >> Hi Evan. >> >> Here are my code review comments: >> >> libbe.h: OK >> >> libbe_priv.h: OK >> >> be_activate.c: OK >> >> be_create.c: OK >> >> be_list.c: OK >> >> be_rename.c: OK >> >> be_utils.c: >> >> 292+ : Missing be_orig_root_ds in header comment. > > fixed. > >> 529: missing explanation > > fixed > >> 944: You may want to clarify here that a bootfs entry with no name is >> still valid. Such cases are found on line 1005 and 1018-1021. > > I'm not sure I understand what you're referring to here. A bootfs entry > with no name is not a valid entry and is in effect meaningless. This > function doesn't check the validity of the bootfs entries of the grub > menu but looks for the default entry. If the default entry has a bootfs > line with no name then we'll end up returning NULL. This indicates that > we didn't find a valid bootfs line corresponding to the default grub > entry so we can't use that to determine the "active on reboot" BE. > >> 969: def_bootfs not returning NULL for this error condition, contrary to >> the header comment. > > If this is run on a unsupported architecture def_boot_fs is left alone. > (see Ethan's earlier comment on this). > >> 1456: Do you mean the boot or grub menu, or the boot/grub menu? If the >> former, then the comment can be misleading. > > SPARC doesn't have a grub menu but does have a menu.lst file, he contents > of which can be displayed at boot time with the boot -L option. This > indicates that the fucntion can deal with both types of boot menu file. > >> Nit: 1505-1506: can be condensed to >> *entry = ent_num - 1; > > done. > >> 2653: Is it misleding to say "external error" here? Though not likely, >> is it possible that a programming error internal to this module caused >> the fault? > > The cause of this error will always be external to libbe. > >> 3024, 3051 and 3074: Since these are private functions, please declare >> them static. > > actually these should have been marked semi-private and moved up in the > file with the rest of the semi-private functions. I'll move them... > >> 3054: Can be optimized as: >> return (! (strcmp((char *)be_get_default_isa(), name) == 0)); > > This amounts to basically the same thing and doesn't seem like much of an > optimization. I believe that the compiler will do much the same thing for > either one of these. I think the way it is is a bit more readable.
> >> 3081: This statement will always be true. > > not necessarily since it's declared as static. > > > Thanks for looking this over! > > -evan > >> Thanks, >> Jack >> >> On 12/04/08 12:01, 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 > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
