[PATCH] alpm: return -1 for error in find_dl_candidates

[Morgan Adamiec]

On 06/10/2021 19:02, Andrew Gregory wrote:
> On 10/05/21 at 06:53pm, Morgan Adamiec wrote:
>>
>>
>> On 05/10/2021 18:49, Morgan Adamiec wrote:
>>>
>>>
>>> On 05/10/2021 18:10, Andrew Gregory wrote:
>>>> On 10/05/21 at 12:53pm, Morgan Adamiec wrote:
>>>>>    On 4 Oct 2021 8:28 pm, Andrew Gregory <andrew.gregory.8 at gmail.com> wrote:    
>>>>>                                                                                 
>>>>>      On 10/04/21 at 08:09pm, morganamilo wrote:                                 
>>>>>      > This is the error value generally used and the calling function          
>>>>>      > explicitly checks for -1, later causing the error to be missed           
>>>>>      > and the transaction to continue.                                         
>>>>>                                                                                 
>>>>>      This result is not compared to -1, the result of download_files is.  If    
>>>>>      we want                                                                    
>>>>>      to guarantee that download_files will return -1 on error, that's where     
>>>>>      the                                                                        
>>>>>      return should be normalized, not in find_dl_candidates.  Tying the API     
>>>>>      of one                                                                     
>>>>>      function to another like this is just going to cause confusion and         
>>>>>      breakage                                                                   
>>>>>      when somebody forgets in the future.  Really, the caller of                
>>>>>      download_files                                                             
>>>>>      should just check for a successful return; we return 1 as an error from    
>>>>>      lots of                                                                    
>>>>>      functions.                                                                 
>>>>>                                                                                 
>>>>>    I'll change that too. This should still be accepted though.                  
>>>>
>>>> Why?  If your reasoning is just that -1 is a better error value, we use 1 in
>>>> lots of other places like I said and I don't want to change that one at a time.
>>>>
>>>> $ grep 'return 1;' lib/libalpm/*.c src/*/*.c | wc -l                                                                                                                                                                                [0][1016]
>>>> 132
>>>>
>>>
>>> Everywhere in the function returns -1. Lets at least be consistent for
>>> the same function.
>>>
>>
>> Not to mention download_files returns 1 on everything up to date so 1 is
>> not an error in this case.
> 
> find_dl_candidates, the function you are modifying, only return 0 or 1.

No it returns -1 on error except for that one error where it returns 1
instead. I'm not disagreeing with you, I'm just saying lets me
consistent and also fix this.

The
> problem is in download_files because it does not guarantee its own return
> value: it's not returning 0 on success/-1 on error, it's returning <whatever
> the functions it calls return>.  Somebody modifying those functions in the
> future will have a hard time understanding the significance of their return
> values because they have to check not just the function calling them
> (download_files), but also the function calling download_files.  That's a good
> recipe for confusion and exactly this kind of bug in the future.  However you
> want to fix this, it should be clear from looking at download_files what the
> significance of find_dl_candidates' return value is.
> 
> apg
>