On Tue, Sep 7, 2021 at 7:57 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Thu, 2 Sep 2021 15:48:40 -0700
> Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> > The warning with clang looks like:
> >
> > src/tracefs-sqlhist.c:1107:2: error: variable 'cmp' is used uninitialized whenever switch default is taken [-Werror,-Wsometimes-uninitialized]
> > default:
> > ^~~~~~~
> > third_party/libtracefs/src/tracefs-sqlhist.c:1112:35: note: uninitialized use occurs here
> > filter->lval->field.field, cmp, val);
> > ^~~
> > third_party/libtracefs/src/tracefs-sqlhist.c:1033:2: note: variable 'cmp' is declared here
> > enum tracefs_compare cmp;
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> > src/tracefs-sqlhist.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/tracefs-sqlhist.c b/src/tracefs-sqlhist.c
> > index 6224677..f4dc004 100644
> > --- a/src/tracefs-sqlhist.c
> > +++ b/src/tracefs-sqlhist.c
> > @@ -1105,7 +1105,7 @@ static int build_filter(struct tep_handle *tep, struct sqlhist_bison *sb,
> > case FILTER_BIN_AND: cmp = TRACEFS_COMPARE_AND; break;
> > case FILTER_STR_CMP: cmp = TRACEFS_COMPARE_RE; break;
> > default:
> > - break;
> > + abort();
>
> Hmm, I wonder if we should do something different than call abort().
>
> Although this should never happen, I think it's rather rude of a library
> function to call abort() when it fails.
>
> I'd like to see this simply return an error. At most, a warning can be
> printed.
>
> Thanks!
Thanks Steve, I think the error handling in this function could use
some TLC. For example, the code:
ret = append_filter(synth, and_or, NULL, 0, NULL);
ret = append_filter(synth, TRACEFS_FILTER_OPEN_PAREN,
NULL, 0, NULL);
doesn't check the value of ret. Assuming the return values are checked
then plumbing these up makes sense. Fwiw, there is evidence that going
in the direction of asserts/aborts is useful:
https://www.microsoft.com/en-us/research/publication/assessing-the-relationship-between-software-assertions-and-code-qualityan-empirical-investigation/
I can resend the patch with something like:
fprintf(stderr, "Error invalid filter type '%d'", filter->type);
return ERANGE;
Does that fit the expected convention?
Thanks!
Ian
> -- Steve
>
>
> > }
> >
> > ret = append_filter(synth, TRACEFS_FILTER_COMPARE,
>
|