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
