server/log.c:351, function open_error_log() ``` if ((rc = apr_file_open(&s->error_log, fname, APR_APPEND | APR_WRITE | APR_CREATE | APR_LARGEFILE, APR_OS_DEFAULT, p)) != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, ap_server_conf, APLOGNO(00091) "%s: could not open error log file %s.", ap_server_argv0, fname); return DONE; // from APR_INCOMPLETE, cannot open -> DONE } ``` modules/loggers/mod_log_config.c:1889, function check_log_dir() ``` apr_status_t rv = apr_stat(&finfo, dir, APR_FINFO_TYPE, p); cls->directive = NULL; /* Don't check this config_log_state again */ if (rv == APR_SUCCESS && finfo.filetype != APR_DIR) rv = APR_ENOTDIR; if (rv != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_STARTUP|APLOG_EMERG, rv, s, APLOGNO(02297) "Cannot access directory '%s' for log file '%s' " "defined at %s:%d", dir, cls->fname, directive->filename, directive->line_num); return !OK; // from APR_INCOMPLETE, cannot open -> !OK } ``` Both errors can be seemed as file cannot be opened, but the handling is totally different. I believe the same errors should be handled in the same way.
Hi, I don't get your point. The first opens a file for writing, and create it if needed. If it fails, it logs something. The 2nd one checks if a directory already exists. If not, it logs something. For me, this is NOT a "file cannot be opened" or created check. Could you elaborate, or even propose a patch of what you think would be better, to help us understand?
Created attachment 37392 [details] Change the failure error code from DONE to !OK
(In reply to Christophe JAILLET from comment #1) > Hi, > > I don't get your point. > > The first opens a file for writing, and create it if needed. > If it fails, it logs something. > > The 2nd one checks if a directory already exists. > If not, it logs something. > For me, this is NOT a "file cannot be opened" or created check. > > > Could you elaborate, or even propose a patch of what you think would be > better, to help us understand? Sure, you are right about their functionalities. My point is about their handling of similar errors should be consistent, i.e., In the case of failing to open file/directory, it should consistently handle similar errors by returning!OK or DONE, rather than have different levels of errors. To me, !OK or DECLINED will be better to unify these similar errors, since common error code APR_INCOMPLETE returned by both functions. More specifically, the first function name is ap_open_logs, which suggests its key function is to open logs. So if it fails, !OK or something like DECLINED would better express the current condition. It is worth mentioning that changing DONE to !OK is totally safe. Only three other places invoke this function and check it with != OK. To this end, I would like to propose a patch here.