In the attached patch I've attempted to clean up the log levels used a little bit. I am not saying these are all the changes that could/should be done but we can start with this patch. So please review only these changes for now and additional changes can be done in further patches.
I tried to follow the MWG - memory allocation errors should be LOG_CRIT and errors induced by the authenticating user should be LOG_NOTICE. For the rest I just tried to somehow make them more reasonable. Mostly I am lowering the level but there are some cases where the level is raised too.
It also addresses the https://fedorahosted.org/linux-pam/ticket/63
Please review,
On St, 2016-06-15 at 16:46 +0200, Tomas Mraz wrote:
In the attached patch I've attempted to clean up the log levels used a little bit. I am not saying these are all the changes that could/should be done but we can start with this patch. So please review only these changes for now and additional changes can be done in further patches.
I tried to follow the MWG - memory allocation errors should be LOG_CRIT and errors induced by the authenticating user should be LOG_NOTICE. For the rest I just tried to somehow make them more reasonable. Mostly I am lowering the level but there are some cases where the level is raised too.
It also addresses the https://fedorahosted.org/linux-pam/ticket/63
Please review,
Ping? Any comments?
On Po, 2016-06-20 at 17:14 +0200, Tomas Mraz wrote:
On St, 2016-06-15 at 16:46 +0200, Tomas Mraz wrote:
In the attached patch I've attempted to clean up the log levels used a little bit. I am not saying these are all the changes that could/should be done but we can start with this patch. So please review only these changes for now and additional changes can be done in further patches.
I tried to follow the MWG - memory allocation errors should be LOG_CRIT and errors induced by the authenticating user should be LOG_NOTICE. For the rest I just tried to somehow make them more reasonable. Mostly I am lowering the level but there are some cases where the level is raised too.
It also addresses the https://fedorahosted.org/linux-pam/ticket/63
Please review,
Ping? Any comments?
If there is no comment by Friday, I'll commit this.
Hi,
sorry for beeing late, was on conferences and didn't found the time to answer...
On Wed, Jun 15, Tomas Mraz wrote:
diff --git a/modules/pam_access/pam_access.c b/modules/pam_access/pam_access.c index 3ac1ad0..f3ea35a 100644 --- a/modules/pam_access/pam_access.c +++ b/modules/pam_access/pam_access.c @@ -402,7 +402,7 @@ login_access (pam_handle_t *pamh, struct login_info *item) (void) fclose(fp); } else if (errno == ENOENT) { /* This is no error. */
- pam_syslog(pamh, LOG_WARNING, "warning: cannot open %s: %m",
- pam_syslog(pamh, LOG_INFO, "warning: cannot open %s: %m", item->config_file); } else { pam_syslog(pamh, LOG_ERR, "cannot open %s: %m", item->config_file);
I disagree here. The message is warning, so the log level should not be info. Or the message should be adjusted. But, if a user configures pam_access to use a configuration file, and this file does not exist, this should be more than LOG_INFO. That's not a message which is printed regular, it's only printed if the user made a mistake in the configuration. So I wouldn't change this priority.
Else I'm fine with the changes.
Thorsten
On Čt, 2016-06-30 at 14:06 +0200, Thorsten Kukuk wrote:
Hi,
sorry for beeing late, was on conferences and didn't found the time to answer...
On Wed, Jun 15, Tomas Mraz wrote:
diff --git a/modules/pam_access/pam_access.c b/modules/pam_access/pam_access.c index 3ac1ad0..f3ea35a 100644 --- a/modules/pam_access/pam_access.c +++ b/modules/pam_access/pam_access.c @@ -402,7 +402,7 @@ login_access (pam_handle_t *pamh, struct login_info *item) (void) fclose(fp); } else if (errno == ENOENT) { /* This is no error. */
- pam_syslog(pamh, LOG_WARNING, "warning: cannot open %s:
%m",
- pam_syslog(pamh, LOG_INFO, "warning: cannot open %s: %m",
item->config_file); } else { pam_syslog(pamh, LOG_ERR, "cannot open %s: %m", item-
config_file);
I disagree here. The message is warning, so the log level should not be info. Or the message should be adjusted. But, if a user configures pam_access to use a configuration file, and this file does not exist, this should be more than LOG_INFO. That's not a message which is printed regular, it's only printed if the user made a mistake in the configuration. So I wouldn't change this priority.
OK, that makes sense, I will leave it as is.
pam-developers@lists.fedorahosted.org