| Message ID | 20211004190954.2116002-1-morganamilo@archlinux.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | alpm: return -1 for error in find_dl_candidates | expand |
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.
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
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.
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.
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
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 >
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));
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(-)