Ethan Quach wrote:
>
>
> 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.
That's a valid point but I still see this as mostly a style issue and
a nit. However I'm not all that tied to what I have here currently
so I'll change it to match what you've suggested.
>
> 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.
Changed and webrev updated...
>
>>
>>>
>>>
>>> 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.
We still end up having to check all the same things. The main thing
that this change would do here is possibly change the order of those
checks.
Changed this to pass the error back and the default bootfs string as
a reference pointer.
>
>
> thanks,
> -ethan
>
