Ethan Quach wrote:
> Evan,
>
> be_create.c - 1005 - left over dev debug message?
D'OH. Yes that exactly what it is...
I've removed it. Thanks for catching that!!!
>
>
> Other than that, looks okay.
>
> thanks,
> -ethan
>
>
> Evan Layton wrote:
>> 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
>>>>>
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
