Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (athena.apache.org: domain of trawick@gmail.com designates
 209.85.160.178 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=mime-version:in-reply-to:references:date:message-id:subject:from:to
         :content-type:content-transfer-encoding;
        b=oZqrRU1aCPmZw2NkTPlW1wqIJ2PZcIKtxCwy1tz2roZCoJE3My68qiyfW3v47E4LhF
         2oap9yW6CgT4PGsZzBOUVtadVJRdpAHEiEG/suTaTIk261FrzY50WGbgbDVquofAiGT2
         862OT6833AFyBs6/B/eekzhVPbW5iwcsFHYgA=
MIME-Version: 1.0
In-Reply-To: <63818FB9-89D1-44D9-883D-A0B6A6DE7AB2@sharp.fm>
References: <20100307152436.70C7A23888DD@eris.apache.org>
	 <cc67648e1003071454r4a9074beud1ccb490456d7c2f@mail.gmail.com>
	 <63818FB9-89D1-44D9-883D-A0B6A6DE7AB2@sharp.fm>
Date: Mon, 8 Mar 2010 15:53:42 -0500
Message-ID: <cc67648e1003081253n4dd13ca5sa83222dfc6edaa2c@mail.gmail.com>
Subject: Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./
	file_io/unix/open.c include/apr_file_io.h
From: Jeff Trawick <trawick@gmail.com>
To: dev@apr.apache.org
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

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, =A0<minfrin@apache.org> wrote:
>>>
>>> Author: minfrin
>>> Date: Sun Mar =A07 15:24:36 2010
>>> New Revision: 920017
>>>
>>> URL: http://svn.apache.org/viewvc?rev=3D920017&view=3Drev
>>> 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
>>> + =A0 =A0if (flag & APR_FOPEN_NONBLOCK) {
>>> + =A0 =A0 =A0 =A0oflags |=3D O_NONBLOCK;
>>> + =A0 =A0}
>>
>> #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 r=
est
> 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
> =A0 =A0if (flag & APR_FOPEN_BINARY) {
> =A0 =A0 =A0 =A0oflags |=3D O_BINARY;
> =A0 =A0}
> #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
> =A0 =A0if (flag & APR_FOPEN_NONBLOCK) {
> =A0 =A0 =A0 =A0oflags |=3D O_NONBLOCK;
> =A0 =A0}
> #endif
>
> #ifdef O_CLOEXEC
> =A0 =A0/* Introduced in Linux 2.6.23. Silently ignored on earlier Linux k=
ernels.
> =A0 =A0 */
> =A0 =A0if (!(flag & APR_FOPEN_NOCLEANUP)) {
> =A0 =A0 =A0 =A0oflags |=3D 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)
> =A0 =A0oflags |=3D O_LARGEFILE;
> #elif defined(O_LARGEFILE)
> =A0 =A0if (flag & APR_FOPEN_LARGEFILE) {
> =A0 =A0 =A0 =A0oflags |=3D O_LARGEFILE;
> =A0 =A0}
> #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...