Re: [PATCH] btrfs-progs: check: Sanitize the return value for qgroup error

[Nikolay Borisov <nborisov@suse.com>]

On 30.03.20 г. 10:01 ч., Qu Wenruo wrote:
> [BUG]
> btrfs check can return strange return value for shell:
>  [Inferior 1 (process 48641) exited with code 0213]
> 					      ^^^^
> 
> [CAUSE]
> It's caused by the incorrect handling of qgroup error.
> 
> qgroup_report_ret can be -117 (-EUCLEAN), using that value with exit()
> can cause overflow, causing return value not properly recognized.
> 
> [FIX]
> Fix it by sanitize the return value to 0 or 1.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  check/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/check/main.c b/check/main.c
> index 37c5b35a36bd..8e29337c5c23 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -10458,7 +10458,7 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
>  			goto out;
>  		}
>  		if (qgroup_report_ret && (!qgroups_repaired || ret))
> -			err |= qgroup_report_ret;
> +			err |= !!qgroup_report_ret;

The  original code was just "wow". qgroup_report_ret was an ordinary
engative error code and it was "ORed" with err ... Just wow...

While this is correct absed on what the function is doing it's awful.
WHy can't simply err = ret in every error branch? Err is really used as
a boolean and frankly instead of doing err = blah or err = true we have
err |= !!blah. Seriously? Was this code being primed for submission to
https://www.ioccc.org/ ?

Sheesh... in any case:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

>  		ret = 0;
>  	} else {
>  		fprintf(stderr,
>