Evan Layton wrote:
> Ethan Quach wrote:
>> Evan,
>>
>> 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.
You mean they will be run, only that they'll evaluate to not True,
right? That's what I don't like about them, why check a condition
that we know isn't *supposed* to be true; why not just not do it at
all.
What I'm suggesting is to replace 425-427 with:
if (be_is_isa("i386") &&
((grub_default_bootfs = be_default_grub_bootfs(rpool, &err)) != NULL) &&
err == 0) {
To me, this makes it much more understandable to trace for the
Sparc case.
>
>>
>>
>> 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.
That's fine with me, though I thought it would help with my comment
above.
thanks,
-ethan
