Bug 64665 - Inconsistent error handling when file cannot be opened
Summary: Inconsistent error handling when file cannot be opened
Status: NEW
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: Core (show other bugs)
Version: 2.5-HEAD
Hardware: PC Linux
: P2 major (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-13 09:12 UTC by legendt
Modified: 2020-08-15 15:21 UTC (History)
1 user (show)



Attachments
Change the failure error code from DONE to !OK (1.64 KB, patch)
2020-08-15 15:20 UTC, legendt
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description legendt 2020-08-13 09:12:59 UTC
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.
Comment 1 Christophe JAILLET 2020-08-15 05:44:45 UTC
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?
Comment 2 legendt 2020-08-15 15:20:54 UTC
Created attachment 37392 [details]
Change the failure error code from DONE to !OK
Comment 3 legendt 2020-08-15 15:21:10 UTC
(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.