[Bug 64665] New: Inconsistent error handling when file cannot be opened

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 64665] New: Inconsistent error handling when file cannot be opened

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64665

            Bug ID: 64665
           Summary: Inconsistent error handling when file cannot be opened
           Product: Apache httpd-2
           Version: 2.5-HEAD
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: major
          Priority: P2
         Component: Core
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ---

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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64665] Inconsistent error handling when file cannot be opened

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64665

Christophe JAILLET <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |NEEDINFO

--- Comment #1 from Christophe JAILLET <[hidden email]> ---
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?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64665] Inconsistent error handling when file cannot be opened

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64665

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #2 from [hidden email] ---
Created attachment 37392
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37392&action=edit
Change the failure error code from DONE to !OK

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64665] Inconsistent error handling when file cannot be opened

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64665

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |NEW

--- Comment #3 from [hidden email] ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]