| Message ID | OF8270EB2F.B681BF4F-ON86258134.0009D902-86258134.0009E63A@notes.na.collabserv.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Dan, > So what will the actual result of these changes be? Is this going to > change where our messages show up? This only changes the priority of the messages logged. Currently, we log all HBRT output at LOG_INFO priority. After this change, we will log HBRT messages that contain the string "error" as LOG_ERROR and those that contain "warn" as LOG_WARNING (both are higher priorities than LOG_INFO). So, nothing will change in terms of log locations at this stage. However, once this is done, I'd like to propose a second change, to the distributions' packing of opal-prd. The idea would be that: - all log output will go to a new log file, /var/log/opal-prd.log - log messages of LOG_WARNING and LOG_ERROR *also* go to the standard syslog paths (generally /var/log/syslog). This means that we get all the HBRT output stored, but only include higher-priority messages in the standard system logs. How does that sound? Cheers, Jeremy
> - all log output will go to a new log file, /var/log/opal-prd.log > > - log messages of LOG_WARNING and LOG_ERROR *also* go to the standard > syslog paths (generally /var/log/syslog). I'm good with that. We just need to get PE to update their FFDC collection scripts to grab the new file once it shows up. I would suggest you add a couple more cases in your compares. We've got these constant in HB that are used a lot (though not all over) as the first 2 characters in our traces. #define ERR_MRK "E>" #define FAIL_MRK "F>" #define WARN_MRK "W>" #define INFO_MRK "I>" -- Dan Crowell Senior Software Engineer - Power Systems Enablement Firmware IBM Rochester: t/l 553-2987 dcrowell@us.ibm.com From: Jeremy Kerr <jk@ozlabs.org> To: Daniel M Crowell <dcrowell@us.ibm.com> Cc: skiboot@lists.ozlabs.org, Frederic Bonnard <FREDERIC@fr.ibm.com> Date: 06/02/2017 10:15 PM Subject: Re: [PATCH RFC 05/12] opal-prd: implement basic heuristic for hservice_puts loglevels Hi Dan, > So what will the actual result of these changes be? Is this going to > change where our messages show up? This only changes the priority of the messages logged. Currently, we log all HBRT output at LOG_INFO priority. After this change, we will log HBRT messages that contain the string "error" as LOG_ERROR and those that contain "warn" as LOG_WARNING (both are higher priorities than LOG_INFO). So, nothing will change in terms of log locations at this stage. However, once this is done, I'd like to propose a second change, to the distributions' packing of opal-prd. The idea would be that: - all log output will go to a new log file, /var/log/opal-prd.log - log messages of LOG_WARNING and LOG_ERROR *also* go to the standard syslog paths (generally /var/log/syslog). This means that we get all the HBRT output stored, but only include higher-priority messages in the standard system logs. How does that sound? Cheers, Jeremy
Hi Dan, >> - all log output will go to a new log file, /var/log/opal-prd.log >> >> - log messages of LOG_WARNING and LOG_ERROR *also* go to the standard >> syslog paths (generally /var/log/syslog). > > I'm good with that. We just need to get PE to update their FFDC > collection scripts to grab the new file once it shows up. > > > I would suggest you add a couple more cases in your compares. We've got > these constant in HB that are used a lot (though not all over) as the > first 2 characters in our traces. > #define ERR_MRK "E>" > #define FAIL_MRK "F>" > #define WARN_MRK "W>" > #define INFO_MRK "I>" Oh, that's great then. If one of these is present, is it going to be at the beginning of the puts() string? Would it be worthwhile removing the strcasestr(.., "error"/"warning"), and *just* using these instead? That would prevent the potential for false classification. We'd probably map both FAIL_MRK and ERR_MRK to LOG_ERROR. Or is FAIL_MRK more of a warning? Or a notice ("normal but significant condition")? Cheers, Jeremy
> be at the beginning of the puts() string? Yes, those are always at the beginning. > Would it be worthwhile removing the strcasestr(.., "error"/"warning"), Yeah, not sure what you'll get with those. > Or is FAIL_MRK more of a warning? Or a notice ("normal but significant condition")? I've honestly never seen a F> in any trace I've looked at. I suspect we copied these in from the FSP and just never used it. I would equate FAIL with ERR -- Dan Crowell Senior Software Engineer - Power Systems Enablement Firmware IBM Rochester: t/l 553-2987 dcrowell@us.ibm.com From: Jeremy Kerr <jk@ozlabs.org> To: Daniel M Crowell <dcrowell@us.ibm.com> Cc: Frederic Bonnard <FREDERIC@fr.ibm.com>, skiboot@lists.ozlabs.org Date: 06/03/2017 12:20 AM Subject: Re: [PATCH RFC 05/12] opal-prd: implement basic heuristic for hservice_puts loglevels Hi Dan, >> - all log output will go to a new log file, /var/log/opal-prd.log >> >> - log messages of LOG_WARNING and LOG_ERROR *also* go to the standard >> syslog paths (generally /var/log/syslog). > > I'm good with that. We just need to get PE to update their FFDC > collection scripts to grab the new file once it shows up. > > > I would suggest you add a couple more cases in your compares. We've got > these constant in HB that are used a lot (though not all over) as the > first 2 characters in our traces. > #define ERR_MRK "E>" > #define FAIL_MRK "F>" > #define WARN_MRK "W>" > #define INFO_MRK "I>" Oh, that's great then. If one of these is present, is it going to be at the beginning of the puts() string? Would it be worthwhile removing the strcasestr(.., "error"/"warning"), and *just* using these instead? That would prevent the potential for false classification. We'd probably map both FAIL_MRK and ERR_MRK to LOG_ERROR. Or is FAIL_MRK more of a warning? Or a notice ("normal but significant condition")? Cheers, Jeremy
diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c index 6ac8a20..c10803d 100644 --- a/external/opal-prd/opal-prd.c +++ b/external/opal-prd/opal-prd.c @@ -267,7 +267,14 @@ extern int call_run_command(int argc, const char **argv, char **o_outString); void hservice_puts(const char *str) { - pr_log(LOG_INFO, "HBRT: %s", str); + int priority = LOG_INFO; + + if (strcasestr(str, "error")) + priority = LOG_ERR; + else if (strcasestr(str, "warn")) + priority = LOG_WARNING; + + pr_log(priority, "HBRT: %s", str); }