apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: svn commit: r905970 - in /apr/apr/trunk: file_io/unix/filedup.c file_io/unix/open.c file_io/unix/readwrite.c include/apr_file_io.h include/arch/unix/apr_arch_file_io.h
Date Wed, 17 Feb 2010 09:48:38 GMT
On Wed, Feb 03, 2010 at 10:18:04AM -0000, Paul Querna wrote:
> Author: pquerna
> Date: Wed Feb  3 10:17:57 2010
> New Revision: 905970
> 
> URL: http://svn.apache.org/viewvc?rev=905970&view=rev
> Log:
> Add two new features to APR Files:
>  - When opened with normal rotating flag, every 60 seconds the file will check
>    if the file it is writing to has changed inode (ie, been replaced/moved).
>  - When opened with the manual rotating flag, the consumer must call the check,
>    but can provider the current timestamp, to avoid a apr_time call.
> 
> This is based off of the patch from Brian, but I've modified it for style, and 
> adding the manual rotation flag after discussion with brian at the httpd 
> hackathon.

Why do this in APR?  This seems like a fairly obscure feature to try to 
shoehorn into apr_file_*.  Combinatorial explosion of API features is a 
dangerous thing: how does this interact with the _XTHREAD, _DELONCLOSE 
flags?  What about if _EXCL is set in flags?

Lack of API docs for the semantics of APR_FOPEN_ROTATING and 
APR_FOPEN_MANUAL_ROTATE make it hard to understand what this is doing 
without reading the svn message, likewise for:

APR_DECLARE(apr_status_t) apr_file_rotating_check(apr_file_t *thefile);
APR_DECLARE(apr_status_t) apr_file_rotating_manual_check(apr_file_t *thefile, apr_time_t time);

Is this an advisory flag which will be ignored on platforms which don't 
support it, or mandatory where I can expect ENOTIMPL if it's not 
supported?

More inline:

> --- apr/apr/trunk/file_io/unix/readwrite.c (original)
> +++ apr/apr/trunk/file_io/unix/readwrite.c Wed Feb  3 10:17:57 2010
...
>  
> +static apr_status_t do_rotating_check(apr_file_t *thefile, apr_time_t now)
> +{
> +    apr_size_t rv = APR_SUCCESS;
> +
> +    if ((now - thefile->rotating->lastcheck) >= thefile->rotating->timeout)
{
> +        apr_finfo_t new_finfo;
> +        apr_pool_t *tmp_pool;
> +
> +        apr_pool_create(&tmp_pool, thefile->pool);
> +
> +        rv = apr_stat(&new_finfo, thefile->fname,
> +                      APR_FINFO_DEV|APR_FINFO_INODE, tmp_pool);
> +
> +        if (rv != APR_SUCCESS || 
> +            new_finfo.inode != thefile->rotating->finfo.inode ||
> +            new_finfo.device != thefile->rotating->finfo.device)  {
> +
> +            if (thefile->buffered) {
> +                apr_file_flush(thefile);
> +            }
> +
> +            close(thefile->filedes);

Dropping errors from _flush or close() here means potential silent data 
loss on failure.

> +            thefile->filedes = -1;
> +
> +            if (thefile->rotating->perm == APR_OS_DEFAULT) {
> +                thefile->filedes = open(thefile->fname,
> +                                        thefile->rotating->oflags,
> +                                        0666);
> +            }
> +            else {
> +                thefile->filedes = open(thefile->fname,
> +                                        thefile->rotating->oflags,
> +                                        apr_unix_perms2mode(thefile->rotating->perm));
> +            }

FD_CLOEXEC needs to be reinstated here if necessary.

Re-opening the file invalidates a bunch of the state in the file object; 
as per apr_os_file_put() this will all needs to be reset (eof_hit, 
ungetchar, buffering state etc).

Regards, Joe

Mime
View raw message