apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c include/apr_file_io.h
Date Mon, 08 Mar 2010 20:53:42 GMT
On Mon, Mar 8, 2010 at 9:59 AM, Graham Leggett <minfrin@sharp.fm> wrote:
> On 08 Mar 2010, at 12:54 AM, Jeff Trawick wrote:
>
>> On Sun, Mar 7, 2010 at 10:24 AM,  <minfrin@apache.org> wrote:
>>>
>>> Author: minfrin
>>> Date: Sun Mar  7 15:24:36 2010
>>> New Revision: 920017
>>>
>>> URL: http://svn.apache.org/viewvc?rev=920017&view=rev
>>> Log:
>>> Backport r920016:
>>> Enable platform specific support for the opening of a file or
>>> pipe in non blocking module through the APR_FOPEN_NONBLOCK flag.

>>> +#ifdef O_NONBLOCK
>>> +    if (flag & APR_FOPEN_NONBLOCK) {
>>> +        oflags |= O_NONBLOCK;
>>> +    }
>>
>> #else result is APR_ENOTIMPL?
>
> Hmmm, the existing code follows this pattern, as below, and if we decide to
> change the pattern then we need to change this behaviour throughout the rest
> of the code, and probably the rest of APR too.

For APR_FOPEN_NONBLOCK, if the caller asks for it but APR doesn't know
how to implement it, should it succeed?  Would it possibly/definitely
break the program to pretend success?

(Maybe this isn't a practical concern -- no known platforms have this
issue -- but other APR code supports multiple variations of the
non-block flag.)

> I'm not sure I like returning
> APR_ENOTIMPL without an API present for a caller to confirm whether these
> functions work on a particular platform.

Some existing APIs can return not-implemented even though they don't
have a feature check macro.

>
> #ifdef O_BINARY
>    if (flag & APR_FOPEN_BINARY) {
>        oflags |= O_BINARY;
>    }
> #endif

AFAIK, O_BINARY is irrelevant on real *ix (i.e., we always satisfy
that request even if the flag doesn't exist) and useful on Cygwin,
where O_BINARY is defined and required.  I guess the hole would be
some *ix-ish programming environment that can be configured externally
to translate text files (like Cygwin) but doesn't support O_BINARY?  I
don't think that is a good example.

>
> #ifdef O_NONBLOCK
>    if (flag & APR_FOPEN_NONBLOCK) {
>        oflags |= O_NONBLOCK;
>    }
> #endif
>
> #ifdef O_CLOEXEC
>    /* Introduced in Linux 2.6.23. Silently ignored on earlier Linux kernels.
>     */
>    if (!(flag & APR_FOPEN_NOCLEANUP)) {
>        oflags |= O_CLOEXEC;
> }
> #endif

There's a fallback below for when APR_FOPEN_NOCLEANUP is requested but
O_CLOEXEC isn't defined.

>
> #if APR_HAS_LARGE_FILES && defined(_LARGEFILE64_SOURCE)
>    oflags |= O_LARGEFILE;
> #elif defined(O_LARGEFILE)
>    if (flag & APR_FOPEN_LARGEFILE) {
>        oflags |= O_LARGEFILE;
>    }
> #endif

This might be more interesting., though perhaps when the
configure-time logic to turn on large file support is out of sync with
this code?  I'm not sure...

Mime
View raw message