Evan,
be_create.c - 1005 - left over dev debug message?
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
