You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Jeff Trawick <tr...@gmail.com> on 2010/03/07 23:54:38 UTC
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./
file_io/unix/open.c include/apr_file_io.h
On Sun, Mar 7, 2010 at 10:24 AM, <mi...@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.
I don't believe that versioning rules allow this to be added to the
1.4.x branch.
>
> Modified:
> apr/apr/branches/1.4.x/ (props changed)
> apr/apr/branches/1.4.x/file_io/unix/open.c
> apr/apr/branches/1.4.x/include/apr_file_io.h
>
> Modified: apr/apr/branches/1.4.x/file_io/unix/open.c
> URL: http://svn.apache.org/viewvc/apr/apr/branches/1.4.x/file_io/unix/open.c?rev=920017&r1=920016&r2=920017&view=diff
> ==============================================================================
> --- apr/apr/branches/1.4.x/file_io/unix/open.c (original)
> +++ apr/apr/branches/1.4.x/file_io/unix/open.c Sun Mar 7 15:24:36 2010
> @@ -135,6 +135,12 @@
> }
> #endif
>
> +#ifdef O_NONBLOCK
> + if (flag & APR_FOPEN_NONBLOCK) {
> + oflags |= O_NONBLOCK;
> + }
#else result is APR_ENOTIMPL?
> +#endif
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c
include/apr_file_io.h
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 3/7/2010 4:54 PM, Jeff Trawick wrote:
> On Sun, Mar 7, 2010 at 10:24 AM, <mi...@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.
>
> I don't believe that versioning rules allow this to be added to the
> 1.4.x branch.
Agreed. Perhaps apr-util remains volatile? apr is definitely not.
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c include/apr_file_io.h
Posted by Graham Leggett <mi...@sharp.fm>.
On 09 Mar 2010, at 4:53 PM, William A. Rowe Jr. wrote:
>> In theory, when people try apr v1.5 for the first time, we're likely
>> to find out. I suspect it may be worth assuming support is present,
>> and then if it turns out that a platform that is still used doesn't
>> support it, we can then reintroduce the ifdef (and ENOTIMPL).
>
> -1 veto, please revert the backport.
>
> If you are experimenting on users, can we please keep that activity to
> trunk? Some of us here are committed to offering developers a stable
> library of features they can count on. Thanks in advance.
You're vetoing a patch that I haven't even written yet??? Based on us
"experimenting" on a branch that hasn't been released yet???
What I was suggesting was to remove the #ifdef and make the assumption
that O_NONBLOCK is supported on all unix platforms on apr v1.5, and
fixing this assertion if it turns out to not be true *before*
releasing v1.5.0. If you believe this is too onerous on users that's
entirely fine, but then say so. There is no need to wave around a -1
in the air, I am perfectly happy to accept your objection without
being bashed over the head with a veto.
Regards,
Graham
--
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c
include/apr_file_io.h
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 3/9/2010 5:52 AM, Graham Leggett wrote:
> On 09 Mar 2010, at 1:46 PM, Jeff Trawick wrote:
>
>>> Hmmm - in that case it may make sense to drop the ifdef entirely,
>>> and if a
>>> unix platform is found to not support O_BLOCK, we can then make a
>>> call then
>>> as what to do. The ifdef could in theory be solving a problem we
>>> don't have.
>>
>> I mis-worded this slightly -- I should have said "maybe no known
>> platforms have this issue". I have no idea what the answer is.
>
> In theory, when people try apr v1.5 for the first time, we're likely
> to find out. I suspect it may be worth assuming support is present,
> and then if it turns out that a platform that is still used doesn't
> support it, we can then reintroduce the ifdef (and ENOTIMPL).
-1 veto, please revert the backport.
If you are experimenting on users, can we please keep that activity to
trunk? Some of us here are committed to offering developers a stable
library of features they can count on. Thanks in advance.
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c include/apr_file_io.h
Posted by Graham Leggett <mi...@sharp.fm>.
On 09 Mar 2010, at 1:46 PM, Jeff Trawick wrote:
>> Hmmm - in that case it may make sense to drop the ifdef entirely,
>> and if a
>> unix platform is found to not support O_BLOCK, we can then make a
>> call then
>> as what to do. The ifdef could in theory be solving a problem we
>> don't have.
>
> I mis-worded this slightly -- I should have said "maybe no known
> platforms have this issue". I have no idea what the answer is.
In theory, when people try apr v1.5 for the first time, we're likely
to find out. I suspect it may be worth assuming support is present,
and then if it turns out that a platform that is still used doesn't
support it, we can then reintroduce the ifdef (and ENOTIMPL).
Regards,
Graham
--
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./
file_io/unix/open.c include/apr_file_io.h
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, Mar 9, 2010 at 6:43 AM, Graham Leggett <mi...@sharp.fm> wrote:
> On 08 Mar 2010, at 10:53 PM, Jeff Trawick wrote:
>
>>> 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.)
>
> Hmmm - in that case it may make sense to drop the ifdef entirely, and if a
> unix platform is found to not support O_BLOCK, we can then make a call then
> as what to do. The ifdef could in theory be solving a problem we don't have.
I mis-worded this slightly -- I should have said "maybe no known
platforms have this issue". I have no idea what the answer is.
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c include/apr_file_io.h
Posted by Graham Leggett <mi...@sharp.fm>.
On 08 Mar 2010, at 10:53 PM, Jeff Trawick wrote:
>> 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.)
Hmmm - in that case it may make sense to drop the ifdef entirely, and
if a unix platform is found to not support O_BLOCK, we can then make a
call then as what to do. The ifdef could in theory be solving a
problem we don't have.
Regards,
Graham
--
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c
include/apr_file_io.h
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 3/13/2010 5:44 PM, Bojan Smojver wrote:
> On Fri, 2010-03-12 at 13:30 +0000, Joe Orton wrote:
>> I'd expect to see some description of exactly what the new APR_FOPEN_*
>> flag changes w.r.t. method calls; does it affect just read/write, what
>> about flush, sync, etc? Can I presume POSIX semantics w.r.t.
>> O_NONBLOCK in open()? The questions are kind of endless if the
>> documentation isn't there.
>
> One way to figure this out for sure would be to enhance the test suite
> so it exercises relevant tests using non blocking semantics, I guess.
+1
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./
file_io/unix/open.c include/apr_file_io.h
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2010-03-12 at 13:30 +0000, Joe Orton wrote:
> I'd expect to see some description of exactly what the new APR_FOPEN_*
> flag changes w.r.t. method calls; does it affect just read/write, what
> about flush, sync, etc? Can I presume POSIX semantics w.r.t.
> O_NONBLOCK in open()? The questions are kind of endless if the
> documentation isn't there.
One way to figure this out for sure would be to enhance the test suite
so it exercises relevant tests using non blocking semantics, I guess.
--
Bojan
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./
file_io/unix/open.c include/apr_file_io.h
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2010-03-13 at 17:54 +0200, Graham Leggett wrote:
> Based on what you've written above, the #else ENOTIMPL seems to be the
> way to go. If it isn't supported, you get explicit confirmation of the
> fact
Probably the safe thing to do. Probably won't get many hits, but
nevertheless.
--
Bojan
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c
include/apr_file_io.h
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 3/13/2010 9:54 AM, Graham Leggett wrote:
>
> To zoom out a bit to put this into context, right now APR has no support
> for non blocking behaviour at all, and this is the problem I would like
> to fix. Using apr_os_file_put(), while functional, isn't portable, and
> as a result a user of the API is forced to have "if windows do this, if
> unix do that". This is APR's job, not the job of the application using APR.
You are aboslutely right; whatever goes into the apr_file api for nonblocking
file behavior must be portable, work consistently and produce identical results
on all platforms. What I think Joe (and I) have been suggesting is that this
API does not look thought out from the perspective you have identified above.
> My understanding is that these are well understood under unix, are there
> unix platforms out there that *don't* support non blocking behaviour?
It is not surprising how distinct the BSD, Linux and Solaris "standard"
behaviors are; why else does virtually every die-hard APR developer rely
on a copy of Stevens "Unix Network Programming" on the shelf?
> In the Windows case we can certainly return ENOTIMPL, and the same for
> Netware (the Netware people can change that to be otherwise if it isn't
> true).
Why would we? I'm under the impression that file I/O completion had
existed on WindowsNT for about 2 decades. It need not block. Now define
nonblocking behavior, and we are all set. I suspect that the entire unix
file read/write set of API's, including write_complete etc, need to be
reviewed in light of the introduction of this flag.
I guess I'm perceiving this as throwing a unix feature at APR to see if
it sticks, and I'm not fond of that approach to such junk expansion of APR.
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c include/apr_file_io.h
Posted by Graham Leggett <mi...@sharp.fm>.
On 12 Mar 2010, at 3:30 PM, Joe Orton wrote:
> Well, right, this the point I was making - it should be no different
> at
> run-time; i.e. you can achieve exactly what you want already without
> adding a new interface which uses up one of the remaining bits in the
> APR_FOPEN_* bitmask, must be maintained in perpuity, won't work on
> platform X and hence confuses users, etc etc.
>
> But specifically, if I'm writing an app which depends on being able to
> do non-blocking IO to a fifo, I *need* the API guarantee that
> O_NONBLOCK
> works. You can get that with open()+apr_os_file_put(), but you can't
> get that with the API added to APR, which has no such guarantee. So
> from that perspective, adding _NONBLOCK to APR is little use.
>
> To be clear; I don't see a problem with adding new flags so long as
> they
> come with documented API constraints/guarantees. It's the idea of
> adding a seemingly deliberately undocumented platform-specific flag
> which seems wrong. Does that make more sense?
To zoom out a bit to put this into context, right now APR has no
support for non blocking behaviour at all, and this is the problem I
would like to fix. Using apr_os_file_put(), while functional, isn't
portable, and as a result a user of the API is forced to have "if
windows do this, if unix do that". This is APR's job, not the job of
the application using APR.
Based on what you've written above, the #else ENOTIMPL seems to be the
way to go. If it isn't supported, you get explicit confirmation of the
fact, and the caller can then make a decision as to what to do at that
point (leave out the flag, punt, whatever).
>> Are there specific semantics you are referring to, or are you just
>> assuming they are missing? In the case of read and write,
>> APR_STATUS_IS_EAGAIN already gives you the semantics you want. Or
>> are you referring to something else?
>
> I'd expect to see some description of exactly what the new APR_FOPEN_*
> flag changes w.r.t. method calls; does it affect just read/write, what
> about flush, sync, etc? Can I presume POSIX semantics w.r.t.
> O_NONBLOCK
> in open()? The questions are kind of endless if the documentation
> isn't
> there.
My understanding is that these are well understood under unix, are
there unix platforms out there that *don't* support non blocking
behaviour?
What I'm trying to ascertain is whether we are trying to solve a
problem that isn't there. I'm not convinced we need the ifdef
O_NONBLOCK at all in the unix case.
In the Windows case we can certainly return ENOTIMPL, and the same for
Netware (the Netware people can change that to be otherwise if it
isn't true).
Regards,
Graham
--
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./
file_io/unix/open.c include/apr_file_io.h
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Mar 09, 2010 at 12:26:08PM +0200, Graham Leggett wrote:
> On 08 Mar 2010, at 11:26 PM, Joe Orton wrote:
>
> >APR_FOPEN_LARGEFILE is intended to be (and is documented as)
> >"advisory"
> >rather than mandatory.
> >
> >Exposing a NONBLOCK flag without attaching *any* semantics to it
> >w.r.t.
> >subsequent read/write calls seems completely wrong. If you want
> >platform-specific behaviour and don't care how badly it interacts with
> >other apr_file_* interfaces then use native open() and
> >apr_os_file_put()
> >to get the APR wrapper.
>
> This makes no sense at all.
>
> If I open a file using native open() setting flags appropriately,
> and then use apr_os_file_put(), followed by apr_file_read and
> apr_file_write, how is that in any way different to using
> apr_file_open(), setting flags appropriately, and then follow with
> apr_file_read and apr_file_write?
Well, right, this the point I was making - it should be no different at
run-time; i.e. you can achieve exactly what you want already without
adding a new interface which uses up one of the remaining bits in the
APR_FOPEN_* bitmask, must be maintained in perpuity, won't work on
platform X and hence confuses users, etc etc.
But specifically, if I'm writing an app which depends on being able to
do non-blocking IO to a fifo, I *need* the API guarantee that O_NONBLOCK
works. You can get that with open()+apr_os_file_put(), but you can't
get that with the API added to APR, which has no such guarantee. So
from that perspective, adding _NONBLOCK to APR is little use.
To be clear; I don't see a problem with adding new flags so long as they
come with documented API constraints/guarantees. It's the idea of
adding a seemingly deliberately undocumented platform-specific flag
which seems wrong. Does that make more sense?
> Are there specific semantics you are referring to, or are you just
> assuming they are missing? In the case of read and write,
> APR_STATUS_IS_EAGAIN already gives you the semantics you want. Or
> are you referring to something else?
I'd expect to see some description of exactly what the new APR_FOPEN_*
flag changes w.r.t. method calls; does it affect just read/write, what
about flush, sync, etc? Can I presume POSIX semantics w.r.t. O_NONBLOCK
in open()? The questions are kind of endless if the documentation isn't
there.
Regards, Joe
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c include/apr_file_io.h
Posted by Graham Leggett <mi...@sharp.fm>.
On 08 Mar 2010, at 11:26 PM, Joe Orton wrote:
> APR_FOPEN_LARGEFILE is intended to be (and is documented as)
> "advisory"
> rather than mandatory.
>
> Exposing a NONBLOCK flag without attaching *any* semantics to it
> w.r.t.
> subsequent read/write calls seems completely wrong. If you want
> platform-specific behaviour and don't care how badly it interacts with
> other apr_file_* interfaces then use native open() and
> apr_os_file_put()
> to get the APR wrapper.
This makes no sense at all.
If I open a file using native open() setting flags appropriately, and
then use apr_os_file_put(), followed by apr_file_read and
apr_file_write, how is that in any way different to using
apr_file_open(), setting flags appropriately, and then follow with
apr_file_read and apr_file_write?
Are there specific semantics you are referring to, or are you just
assuming they are missing? In the case of read and write,
APR_STATUS_IS_EAGAIN already gives you the semantics you want. Or are
you referring to something else?
Regards,
Graham
--
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./
file_io/unix/open.c include/apr_file_io.h
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Mar 08, 2010 at 03:53:42PM -0500, Jeff Trawick wrote:
> On Mon, Mar 8, 2010 at 9:59 AM, Graham Leggett <mi...@sharp.fm> wrote:
...
> > #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...
APR_FOPEN_LARGEFILE is intended to be (and is documented as) "advisory"
rather than mandatory.
Exposing a NONBLOCK flag without attaching *any* semantics to it w.r.t.
subsequent read/write calls seems completely wrong. If you want
platform-specific behaviour and don't care how badly it interacts with
other apr_file_* interfaces then use native open() and apr_os_file_put()
to get the APR wrapper.
Regards, Joe
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./
file_io/unix/open.c include/apr_file_io.h
Posted by Jeff Trawick <tr...@gmail.com>.
On Mon, Mar 8, 2010 at 9:59 AM, Graham Leggett <mi...@sharp.fm> wrote:
> On 08 Mar 2010, at 12:54 AM, Jeff Trawick wrote:
>
>> On Sun, Mar 7, 2010 at 10:24 AM, <mi...@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...
Re: svn commit: r920017 - in /apr/apr/branches/1.4.x: ./ file_io/unix/open.c include/apr_file_io.h
Posted by Graham Leggett <mi...@sharp.fm>.
On 08 Mar 2010, at 12:54 AM, Jeff Trawick wrote:
> On Sun, Mar 7, 2010 at 10:24 AM, <mi...@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.
>
> I don't believe that versioning rules allow this to be added to the
> 1.4.x branch.
True, let me revert. It's been confusing with apr v1.4 being released
but apr-util v1.4 not.
>> +#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. 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.
#ifdef O_BINARY
if (flag & APR_FOPEN_BINARY) {
oflags |= O_BINARY;
}
#endif
#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
#if APR_HAS_LARGE_FILES && defined(_LARGEFILE64_SOURCE)
oflags |= O_LARGEFILE;
#elif defined(O_LARGEFILE)
if (flag & APR_FOPEN_LARGEFILE) {
oflags |= O_LARGEFILE;
}
#endif
Regards,
Graham
--