Ethan Quach wrote:
> Evan,
>
> Just some nits ...
>
> be_utils.c
> ----------
> 949,951,952 - these comments need to be updated.
OOPS!
>
> 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.
good point, fixed...
I also got some comments from Greg Onufer on the use of be_is_isa everywhere
for
determining if we support grub of not. Right now this is ok but in the future
grub may be supported on SPARC so what he suggested was that where we're using
be_is_isa we replace that with a be_has_grub call so that in the future SPARC
does support grub we only have to make changes in one place. To do this I added
be_has_grub() which simply calls be_is_isa() right now but I also added a TODO
comment to keep track of this in the event that grub support for SPARC is added
in the future.
I've updated the webrev for all of this.
Thanks,
-evan
>
>
> 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
>>>
>>
