alpm: return -1 for error in find_dl_candidates

Message ID 20211004190954.2116002-1-morganamilo@archlinux.org
State Changes Requested
Headers show
Series alpm: return -1 for error in find_dl_candidates | expand

Commit Message

Morgan Adamiec Oct. 4, 2021, 7:09 p.m. UTC
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.

> pacman -S xterm
warning: xterm-369-1 is up to date -- reinstalling
resolving dependencies...
looking for conflicting packages...

Package (1)  Old Version  New Version  Net Change  Download Size

extra/xterm  369-1        369-1          0.00 MiB       0.42 MiB

Total Download Size:   0.42 MiB
Total Installed Size:  1.05 MiB
Net Upgrade Size:      0.00 MiB

:: Proceed with installation? [Y/n]
error: no servers configured for repository: extra
(1/1) checking keys in keyring                                                                 [--------------------------------------------------------] 100%
(1/1) checking package integrity                                                               [--------------------------------------------------------] 100%
error: failed to commit transaction (wrong or NULL argument passed)
Errors occurred, no packages were upgraded.
---
 lib/libalpm/sync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Gregory Oct. 4, 2021, 7:28 p.m. UTC | #1
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.
Andrew Gregory Oct. 5, 2021, 5:10 p.m. UTC | #2
On 10/05/21 at 12:53pm, Morgan Adamiec wrote:
>    On 4 Oct 2021 8:28 pm, Andrew Gregory <andrew.gregory.8@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
Morgan Adamiec Oct. 5, 2021, 5:49 p.m. UTC | #3
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@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.
Morgan Adamiec Oct. 5, 2021, 5:53 p.m. UTC | #4
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@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.
Andrew Gregory Oct. 6, 2021, 6:02 p.m. UTC | #5
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@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.  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
Morgan Adamiec Oct. 7, 2021, 7:57 p.m. UTC | #6
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@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
>

Patch

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
index 36ad6242..acca375e 100644
--- a/lib/libalpm/sync.c
+++ b/lib/libalpm/sync.c
@@ -732,7 +732,7 @@  static int find_dl_candidates(alpm_handle_t *handle, alpm_list_t **files)
 				handle->pm_errno = ALPM_ERR_SERVER_NONE;
 				_alpm_log(handle, ALPM_LOG_ERROR, "%s: %s\n",
 						alpm_strerror(handle->pm_errno), repo->treename);
-				return 1;
+				return -1;
 			}
 
 			ASSERT(spkg->filename != NULL, RET_ERR(handle, ALPM_ERR_PKG_INVALID_NAME, -1));