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?
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.
thanks,
-ethan
