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?
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.
>
>>
>> 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... :)
>
>>
>> 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
>
