Ethan Quach wrote:
>
>
> Keith Mitchell wrote:
>>> be_utils.c
>
>>> At lines 3502-3503, we're simply returning an errno, and
>>> for the success case errno == 0, so saying we're returning
>>> BE_SUCCESS for success at 3429 means we're making the
>>> assumption BE_SUCCESS == 0.
>>>
>>> I think we should revert lines 3429 and 3439; and then just
>>> change 3477-3478 to:
>>>
>>> if (_be_list(NULL, &be_nodes) == BE_SUCCESS) {
>>>
>>>
>>> The setting of err there gets masked later on anyway so
>>> it doesn't seem like we need to set it here.
>> The purpose of bug 3837 is to have consistent return codes throughout
>> libbe.While the *_callback functions need to return 0 for success as
>> previously discussed, the remaining functions should return
>> BE_SUCCESS for successful cases. It's not the most elegant option,
>> but explicitly returning BE_SUCCESS when err == 0 seems more
>> appropriate:
>>
>> *menu_fp = fopen(menu_file, mode);
>> err = errno;
>> if (err != 0)
>> return (err);
>>
>> return (BE_SUCCESS);
>>
>> What do you think?
>
> Okay, this is fine for replacing 3502-3503, but can you
> change the third line to be "if (*menu_fp == NULL)"
> instead?
>
> Also can you move 3468 to be right after 3465?
Done and done. I feel like a broken record here... but I should have
test results "soon"
>
>
> After inspecting the other usages of err in this function,
> I see a couple other things wrong. If you feel these are
> out of scope of this fix, that is fine, I won't hold up this
> review any more, but please a file a new bug for these
> two additional issues:
>
> 1) 3457-3459, 3497-3499 - these chunks of code don't
> return an errno. They capture the return from the system()
> call, which if not 0 means that its returning a -1 or the
> return of whatever the shell command returned, neither
> of which are errnos.
>
> 2) 3478 - if _be_list() doesn't return BE_SUCCESS, it doesn't
> seem like we should be continuing here. There won't be
> any entries added to the new menu file so continuing would
> seem to produce unpredictable results.
As discussed, filed 10589 to capture these issues.
>
>
> thanks,
> -ethan
>
