Re: [PATCH] alpm: return -1 for error in find_dl_candidates
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
participants (2)
-
Andrew Gregory
-
Morgan Adamiec