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
