Evan,
Just some nits ...
be_utils.c
----------
949,951,952 - these comments need to be updated.
968,979,1005,1016,1028 - nit - why not just initialize the variable
to NULL somewhere at the beginning of the function and remove all
these lines. If you do decide to do this, I suggest doing it
after line 970 so that for Sparc we'd just return the error and
leave the passed in reference pointer untouched.
thanks,
-ethan
Evan Layton wrote:
> 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
>>
>
