| Message ID | 20230809031313.1298605-6-rostedt@goodmis.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libtraceeval histogram: Updates | expand |
On Tue, Aug 08, 2023 at 11:13:12PM -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > 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@goodmis.org> > --- > 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();
On Thu, 10 Aug 2023 11:47:50 -0600 Ross Zwisler <zwisler@google.com> wrote: > On Tue, Aug 08, 2023 at 11:13:12PM -0400, Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > > > 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) > Yeah, but I believe Stevie's latest version fixed all this. -- Steve
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) return check; return copy_traceeval_data_set(teval->nr_val_types, teval->val_types, entry->vals, results);