Re: [PATCH 5/6] libtraceeval histogram: Fix the return value of traceeval_query()

[Ross Zwisler <zwisler@xxxxxxxxxx>]

On Tue, Aug 08, 2023 at 11:13:12PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
> 
> The comments state that traceeval_query() returns 1 if found, 0 if not,
> and -1 on error, but in reality it returns 0 if found and 1 if not found.

The comment currently says:

> /*
>  * Find the entry that @keys corresponds to within @teval.
>  *
>  * Returns 0 on success, 1 if no match found, -1 on error.
>  */

I think this needs to be updated to match the new logic (1=found, 0=not
found, -1=error)

> It makes more sense to have it return 1 if found and zero if not found as
> the comment states.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> ---
>  src/histograms.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/histograms.c b/src/histograms.c
> index 3cf5c5389700..226c2792995c 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -551,7 +551,7 @@ static int get_entry(struct traceeval *teval, const union traceeval_data *keys,
>  		/* return entry if keys match */
>  		if (!check) {
>  			*result = entry;
> -			return 0;
> +			return 1;
>  		} else if (check == 1) {
>  			continue;
>  		} else {
> @@ -559,7 +559,7 @@ static int get_entry(struct traceeval *teval, const union traceeval_data *keys,
>  		}
>  	}
>  
> -	return 1;
> +	return 0;
>  }
>  
>  /*
> @@ -660,7 +660,7 @@ int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
>  		return -1;
>  
>  	/* find key and copy its corresponding value pair */
> -	if ((check = get_entry(teval, keys, &entry)))
> +	if ((check = get_entry(teval, keys, &entry)) < 0)

should this be 
	if ((check = get_entry(teval, keys, &entry)) <= 0)

so we return 0 (not found) in that case, and avoid dropping into
copy_traceeval_data_set() when we have nothing to copy?

>  		return check;
>  	return copy_traceeval_data_set(teval->nr_val_types, teval->val_types,
>  			entry->vals, results);
> -- 
> 2.40.1
> 

I think you also need to update the logic in traceeval_insert(), which right
now uses a return of 1 to mean "not found" and will do a create_entry() in
response.

For clarity maybe in all these consumers 'check' should be renamed 'found'?

Then the logic would read:

 found = get_entry(..);

 if (found == -1)
  error;

 if (found)
  update_entry();
 else /* ! found */
  create_entry();