Jack Schwartz wrote: > Hi Evan. > >>> >>> be_utils.c: >>> >>> 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. > How come line 1005 (line 1011 in the updated webrev) returns BE_SUCCESS > if there is nothing beyond "bootfs" in that line?
In this context we successfully found the default grub entry but it had no valid bootfs line so there was no error as far as finding the default line. We return NULL for bootfs and BE_SUCCESS because there was no failure. > > OK... after a good night's sleep I see that the error conditions > returned are not based on the presence/contents of a bootfs line. A > bad/missing bootfs line just returns NULL. correct. >> >>> >>> 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). > OK. >> >>> >>> 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. > Then I would have a nit to change from "boot/grub" to "boot or grub" but > I can't find either in the new webrev, so I guess it's not a problem... :) I changed it to boot menu. >> >>> >>> 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. > I saw your other email on this, and I agree. >> >>> >>> 3081: This statement will always be true. >> >> not necessarily since it's declared as static. > Yup, that's true. Missed that. > > So, LGTM. > > Thanks, > Jack >> >> >> 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 >> >
