On Fri, 16 Apr 2021 13:14:01 +0300
Tzvetomir Stoyanov <tz.stoyanov@xxxxxxxxx> wrote:
> I can fix "-q" to suppress warnings from all trace libraries, but the
> problem is that "-q" is not set by default. These "Not being able to
> parse" messages could be very annoying in some cases and as I
> understood they are not critical ? The other problem is that not all
> trace-cmd commands have the "-q" option, but almost all of them need a
> tep handler that could cause printing of these messages.
> Yodan found one more issue, the pr_stat() libtraceevent function is
> not affected by tep_vwarning() redefine. It prints statistics on
> KernelShark startup. I think we should add some kind of priorities of
> all those library messages and decide which of them when will be
> printed. It makes sense only for fatal library messages to be printed
> by default, if the "-q" option is not set. All others should be
> visible only if trace-cmd runs in debug mode.
In principle I agree, but I have mixed feelings about this, because these
messages have helped me in the past to fix something. And not that I was
debugging it. I would trace an event and this would warn on it, and then I
would know the event had issues.
But I do find that they are more annoying when it's showing too much,
especially on "-e all" runs. Personally, I would love to only show those
warnings if the record was on a subset of events, and not all of them.
Because when I specify events, and it fails to parse, then I really do care
about that.
I wonder if we should do the following:
On record, if its not "-e all" then add an option that tells report that
not all events are traced. And it will allow for showing of failed to parse
events by default, and quieted by "-q". If that option does not exist, then
only show the failed parsings if "--debug" is set.
That is, if the user specified events, they should care if they parse or
not. But if they just said "-e all" then they probably do not care, because
there's going to be events that do not parse.
Thoughts?
As for pr_stat(), I think we should rename it to tep_info() and tep_vinfo()
that acts just like tep_warning(), except it is for informational output
(stdout instead of stderr). This is similar to what the kernel has.
Since tep_vwarning() takes a name, so can tep_vinfo(), and I was thinking
that we should expose this string to the application.
extern const char *tep_name;
As well for libtracefs:
extern const char *tracefs_name;
Then in tep_warning(), we could do things like:
if (name == tep_name) {
/* do something for libtraceevent errors */
} else if (name == tracefs_name) {
/* do something for libtracefs errors */
}
Instead of doing the error prone:
if (!strcmp(name, "libtraceevent")) {
[..]
} else if (!strcmp(name, "libtracefs")) {
[..]
}
-- Steve
|