On Tue, 7 Sep 2021 11:53:48 -0700 Ian Rogers <irogers@xxxxxxxxxx> wrote: > Thanks Steve, I think the error handling in this function could use > some TLC. For example, the code: 100% agree. That's one reason I haven't tagged an official release with it yet ;-) > > 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'm sure it is. But I still hate it when a library aborts my own work, so I'm biased ;-) > > 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? Looks as good as any. If you want to send patches that makes the return handling a bit more complete and robust, that would be more than welcomed. -- Steve
|