You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2007/10/11 09:43:37 UTC
Tagging 1.2.* Sat night or Sun a.m.
Depending on my weekend.
I found the bug, I'll commit the fix friday after I vet it a bit more.
Win32 apr_file_dup2's new flags bits for stdio channels were the buggers,
needed to mask those off in appropriate ways, but I'm not done validating
the fix.
I've committed to trunk the build changes that let us almost seamlessly
transition from .dsp to .vcproj, for x86 and x64. [Actually, they are
seamless here because we don't try stuffing /D "SYM=With a space" at
the resource compiler. httpd is not so lucky].
I don't plan to backport the /D WINNT change for x86, or the Win9x
targets. Save that for 1.3. But there's no sense in run time tests
of 9x for x64 ;-) Otherwise, it goes over to 1.2 as-in trunk.
So the dsp improvements roll on to apr-iconv and apr-util for releases
there too. We should have a really ready-to-rock x64 build schema, all
it still needs is a top level Makefile.win to make things easy, which
I'm about to crib from httpd (including ssl detection logic for apr 1.3,
but nothing that rich for the other trees.)
So what of y'all - anything else to slide in before the weekend? If
you are still at it Saturday, be sure to holler so I don't accidentally
roll right over anyone's toes.
Bill
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 10/12/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Lucian Adrian Grijincu wrote:
> > Actually there's a 0.9.4 to 0.9.5 change that is documented as such
> > (see the CHANGES file):
> >
> > *) Define apr_off_t as long rather than as off_t on platforms with a
> > 32-bit off_t to prevent incompatibility with packages such as
> > Perl which redefine the size of off_t via _FILE_OFFSET_BITS on
> > some platforms. [Ben Reser <ben reser.org>]
> >
> > So either that change broke the rules (if these rules were applicable
> > back when the change was made) or my proposed changes are compatible
> > with the rules.
>
> The rules didn't apply to 0.9, and never have.
>
> 1.0.0 heralded our current versioning restrictions, which if you aren't
> already familiar, can be found here;
>
> http://apr.apache.org/versioning.html
>
> DOH - just struck me what's going on here... yes, the eariliest possible
> opportunity to adopt 64 bit ino values is apr 2.0.0
>
OK, I admit that having sizeof(apr_ino_t)==64 needs to go in 2.0.0
(because it would break any program that uses apr_ino_t which is 32
bit large on all 32 bit systems I've used). I was talking about
something else.
This is a problem I actually ran into and I know another developer did so too.
http://apr.apache.org/docs/apr/1.2/structapr__finfo__t.html
struct apr_finfo_t
{
....
apr_ino_t inode
....
const char * fname
const char * name
}
I was wrongfully compiling my binary with _FILEOFFSET_BITS=64 (this
decision was imposed by some not-controlled-by-me factors, but this
could also happen if someone works with a library that depends on
_FILEOFFSET_BITS=64). I was also using the libapr that came with
Ubuntu x86, and that one was compiled with _FILEOFFSET_BITS=32.
The bug: what APR wrote in "name" I read in the "fname" member.
This is because apr thought inode was 4 bytes long and my binary
thought it was 8 bytes long. The 4 byte difference lead to the bug.
This was behaving normal on Windows: what APR wrote in "name" I read in "name".
So I had to do an ifdef to get a "portable" behavior:
#ifdef WINDOWS
val = finfo.name;
#else
val = finfo.fname;
#endif
I'm suggesting having ino_t maintain it's current size*, but typedef
it to something that is immutable with respect to _FILEOFFSET_BITS'
value.
* current size == the size of ino_t given all the flags and extraflags
used when compiling libapr. If someone built a custom libapr with
_FILEOFFSET_BITS=64, they won't loose ABI compatibility.
My patch only affects people that want to use a system provided libapr
compiled with _FILEOFFSET_BITS=32 while they build their program with
_FILEOFFSET_BITS=64.
I'm only insisting because I had the idea that you misunderstood me.
If I'm wrong, I'll stop bugging you about this; I don't want to become
"the annoying guy that keeps insisting on stupid things".
Joe Orton agreed that this is a bug of the header files:
http://mail-archives.apache.org/mod_mbox/apr-dev/200709.mbox/%3c20070917081310.GA26963@redhat.com%3e
--
Lucian.
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Lucian Adrian Grijincu wrote:
> Actually there's a 0.9.4 to 0.9.5 change that is documented as such
> (see the CHANGES file):
>
> *) Define apr_off_t as long rather than as off_t on platforms with a
> 32-bit off_t to prevent incompatibility with packages such as
> Perl which redefine the size of off_t via _FILE_OFFSET_BITS on
> some platforms. [Ben Reser <ben reser.org>]
>
> So either that change broke the rules (if these rules were applicable
> back when the change was made) or my proposed changes are compatible
> with the rules.
The rules didn't apply to 0.9, and never have.
1.0.0 heralded our current versioning restrictions, which if you aren't
already familiar, can be found here;
http://apr.apache.org/versioning.html
DOH - just struck me what's going on here... yes, the eariliest possible
opportunity to adopt 64 bit ino values is apr 2.0.0
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 10/11/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Lucian Adrian Grijincu wrote:
> >
> > Do my patches against configure.in, apr.hnw and apr.hw fixing the
> > apr_ino_t ( http://issues.apache.org/bugzilla/show_bug.cgi?id=43417 )
> > issue have a chance of getting accepted (aka should I try to test them
> > on other configurations to see whether they break something on some
> > systems)?
>
> Not according to folks interpretation of our versioning policy, it seems
> this patch must wait for 2.0.0. You would be breaking ABI for anyone
> building against a 32 bit APR build.
>
> If folks can prove APR 1.0.0 forward have promised that it's internally
> always built _FILE_OFFSET_BITS=64 then I'd entertain changing the type,
As far as I can see (from the 1.2.x tag head) APR is built by default
with _FILE_OFFSET_BITS=32. Also there is a desire to prevent ABI
changes when _FILE_OFFSET_BITS is defined as 64.
Actually there's a 0.9.4 to 0.9.5 change that is documented as such
(see the CHANGES file):
*) Define apr_off_t as long rather than as off_t on platforms with a
32-bit off_t to prevent incompatibility with packages such as
Perl which redefine the size of off_t via _FILE_OFFSET_BITS on
some platforms. [Ben Reser <ben reser.org>]
So either that change broke the rules (if these rules were applicable
back when the change was made) or my proposed changes are compatible
with the rules.
I'm doing the same thing: trying to find a type that is compatible
with ino_t with the flags with which apr is built and typedef
apr_ino_t to that compatible type.
> (an external included-headers apparent bug as opposed to an internal bug)
> but again according to folks interpretation of the versioning policy,
> this would have to be based on apr_off_t or a type we already declare,
> as we cannot add an apr_ino_t until 1.3.0.
>
http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/include/apr_file_info.h?view=markup
has this line:
typedef apr_uint64_t apr_ino_t;
so the type is present in the current 1.2.x head.
--
Lucian
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 10/12/07, Lucian Adrian Grijincu <lu...@gmail.com> wrote:
> On 10/12/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> > William A. Rowe, Jr. wrote:
> > >
> > > WORSE you just f***ed up the user who chose a different order for their
> > > includes, which sucks to be them. Please don't do such nonsense to them.
> >
> > Belay that assessment, I see you snuck this in after the header protect
> > macro ;-)
> >
> > It's still not acceptable to add apr_ino_t until 1.3.0 no matter how it's
> > hidden.
> >
>
> It's not added. it's present in apr_file_info.h since 1.2.0
>
here's the proof:
http://svn.apache.org/viewvc/apr/apr/tags/1.2.0/include/apr_file_info.h?view=markup
(with some simplification) this is the code that introduces the
apr_ino_t type in 1.2.0:
#if (defined WIN32) || (defined NETWARE)
typedef apr_uint64_t apr_ino_t;
#else
typedef ino_t apr_ino_t;
#endif
> > I had gone so far as to offer no outward facing explanation of the 1.3.0
> > solution to win32 handles, but they could pass a constant which would
> > achieve the effect. That too was vetoed, so we simply won't add a new
> > interface no matter how cloaked it is :)
> >
(if I'm interpreting correctly the versioning rules) I agree that the veto:
If I code an application that uses APR I know that whichever patch
version in a minor release I use, I'll be able to build my application
(that is, if I only use the APR exposed API and don't mingle with
internal implementation specific stuff). That is, if I code, build and
test against 1.2.13 I know I will be able to build it if I use 1.2.0,
1.2.1, ... 1.2.12, or any version that is released after 1.2.13.
If 1.2.13 adds a new macro identifier and I use it in my code, I will
break source compatibility with earlier patch versions from the 1.2
minor release: I won't be able to compile.
We don't add a new type! apr_ino_t is present in apr_file_info.h since
1.2.0 (I haven't checked exactly which version introduces the type, if
you want me to, I'll look it up).
What I did is replace the
typedef ino_t apr_ino_t;
with a _FILEOFFSET_BITS ignorant
typedef sometype_t apr_ino_t;
sometype_t needs to be determined at ./configure time. It depends on
the platform and the flags with which APR is built.
If the type is to be determined at ./configure time, it must be
defined in apr.h.
If we provide the type in apr.h we'll break the rules, because some
developer will forget to include apr_file_info.h somewhere in his
project and use apr_ino_t.
If because of whichever reason he must use an earlier patch version of
APR his app will break at compile time. BAD!
To prevent such a case I used a bit of preprocessor sorcery: only
define apr_ino_t if apr_file_info.h was already included.
In all prior versions of the 1.2 minor release, if a developer wants
to use apr_ino_t he'll have to #include "apr_file_info.h".
With my changes in place this rule is still enforced. The only way
apr_ino_t will be exposed in a manner incompatible with prior patch
versions is if a developer explicitly defines APR_FILE_INFO_H before
including apr.h.
But that developer can shoot himself in the head for all I care. He's
breaking an (albeit unwritten) rule that one should not mess with
internal library thingies.
So in conclusion:
There is no ABI change for 1.2.x.
There is no new type.
No priorly existing type is exposed in a new manner.
World peace :)
--
Lucian.
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 10/12/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> William A. Rowe, Jr. wrote:
> >
> > WORSE you just f***ed up the user who chose a different order for their
> > includes, which sucks to be them. Please don't do such nonsense to them.
>
> Belay that assessment, I see you snuck this in after the header protect
> macro ;-)
>
> It's still not acceptable to add apr_ino_t until 1.3.0 no matter how it's
> hidden.
>
It's not added. it's present in apr_file_info.h since 1.2.0
> I had gone so far as to offer no outward facing explanation of the 1.3.0
> solution to win32 handles, but they could pass a constant which would
> achieve the effect. That too was vetoed, so we simply won't add a new
> interface no matter how cloaked it is :)
>
> Bill
>
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
William A. Rowe, Jr. wrote:
>
> WORSE you just f***ed up the user who chose a different order for their
> includes, which sucks to be them. Please don't do such nonsense to them.
Belay that assessment, I see you snuck this in after the header protect
macro ;-)
It's still not acceptable to add apr_ino_t until 1.3.0 no matter how it's
hidden.
I had gone so far as to offer no outward facing explanation of the 1.3.0
solution to win32 handles, but they could pass a constant which would
achieve the effect. That too was vetoed, so we simply won't add a new
interface no matter how cloaked it is :)
Bill
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Lucian Adrian Grijincu wrote:
> On 10/12/07, Joe Orton <jo...@redhat.com> wrote:
>> On Thu, Oct 11, 2007 at 10:14:29AM -0500, William Rowe wrote:
>>> Lucian Adrian Grijincu wrote:
>>>> Do my patches against configure.in, apr.hnw and apr.hw fixing the
>>>> apr_ino_t ( http://issues.apache.org/bugzilla/show_bug.cgi?id=43417 )
>>>> issue have a chance of getting accepted (aka should I try to test them
>>>> on other configurations to see whether they break something on some
>>>> systems)?
>>> Not according to folks interpretation of our versioning policy, it seems
>>> this patch must wait for 2.0.0. You would be breaking ABI for anyone
>>> building against a 32 bit APR build.
>> Lucian's patches do not change the ABI of the library which is built (at
>> least by design, unless you are talking about some problem with the
>> implementation which I'm missing).
Gotcha, then we'll be able to fix to make this consistent.
>> In the case where APR is built *without* _FILE_OFFSET_BITS=64, and an
>> external application includes APR headers *with* _FILE_OFFSET_BITS=64
>> defined, an APR library ABI would be used by the application which does
>> not match the real ABI, and all bets were off. Lucian's patch fixes
>> that.
Right - and this is all the worse because it's not opaque, and the user
is allocating the space for the structure that apr fills in. Yuck.
>> Exposing apr_ino_t from apr.h rather than only from apr_file_info.h is
>> certainly an API extension and so cannot be done in 1.2.x.
Exactly, we need to use an existing apr_foo_t to accomplish the same thing
until 1.3.0
> after the last line in apr.h which is the end of the header-protection macros:
> #endif /* APR_H */
>
> we insert the next lines:
>
> #ifdef APR_FILE_INFO_H //aka we are included from apr_file_info.h
>
> #ifndef APR_FILE_INFO_H__APR_H_or_whatever_name_you_find_suitable
> #define APR_FILE_INFO_H__APR_H_or_whatever_name_you_find_suitable
>
> typedef apr_sometype_t apr_ino_t;
>
> #endif //APR_FILE_INFO_H__APR_H_or_whatever_name_you_find_suitable
>
> #endif //APR_FILE_INFO_H
Nope - it's still in the public namespace if they include apr_file_info.h.
WORSE you just f***ed up the user who chose a different order for their
includes, which sucks to be them. Please don't do such nonsense to them.
Bill
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Oct 12, 2007 at 06:44:15PM +0300, Lucian Adrian Grijincu wrote:
> On 10/12/07, Joe Orton <jo...@redhat.com> wrote:
...
> > Exposing apr_ino_t from apr.h rather than only from apr_file_info.h is
> > certainly an API extension and so cannot be done in 1.2.x.
>
> Would something like this work?
> after the last line in apr.h which is the end of the header-protection macros:
> #endif /* APR_H */
>
> we insert the next lines:
Hah, very nice! I think that would work, yes. It would certainly win a
genius evil hack award :)
I think I'd rather see the non-evil patch settle on the trunk for a bit
first, anyway, this isn't an urgent issue for the next 1.2.x release.
joe
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 10/12/07, Joe Orton <jo...@redhat.com> wrote:
> On Thu, Oct 11, 2007 at 10:14:29AM -0500, William Rowe wrote:
> > Lucian Adrian Grijincu wrote:
> > >
> > >Do my patches against configure.in, apr.hnw and apr.hw fixing the
> > >apr_ino_t ( http://issues.apache.org/bugzilla/show_bug.cgi?id=43417 )
> > >issue have a chance of getting accepted (aka should I try to test them
> > >on other configurations to see whether they break something on some
> > >systems)?
> >
> > Not according to folks interpretation of our versioning policy, it seems
> > this patch must wait for 2.0.0. You would be breaking ABI for anyone
> > building against a 32 bit APR build.
>
> Lucian's patches do not change the ABI of the library which is built (at
> least by design, unless you are talking about some problem with the
> implementation which I'm missing).
>
> In the case where APR is built *without* _FILE_OFFSET_BITS=64, and an
> external application includes APR headers *with* _FILE_OFFSET_BITS=64
> defined, an APR library ABI would be used by the application which does
> not match the real ABI, and all bets were off. Lucian's patch fixes
> that.
>
> Exposing apr_ino_t from apr.h rather than only from apr_file_info.h is
> certainly an API extension and so cannot be done in 1.2.x.
>
Would something like this work?
after the last line in apr.h which is the end of the header-protection macros:
#endif /* APR_H */
we insert the next lines:
#ifdef APR_FILE_INFO_H //aka we are included from apr_file_info.h
#ifndef APR_FILE_INFO_H__APR_H_or_whatever_name_you_find_suitable
#define APR_FILE_INFO_H__APR_H_or_whatever_name_you_find_suitable
typedef apr_sometype_t apr_ino_t;
#endif //APR_FILE_INFO_H__APR_H_or_whatever_name_you_find_suitable
#endif //APR_FILE_INFO_H
This would prevent apr_ino_t from being exposed from apr.h.
It's exposed only when apr_file_info.h was included beforehand.
It doesn't matter if apr.h is included before apr_file_info.h. The
file is reparsed and because this code is outside apr.h's protection
macros the typedef makes it to the compilation stage.
--
Lucian.
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Oct 11, 2007 at 10:14:29AM -0500, William Rowe wrote:
> Lucian Adrian Grijincu wrote:
> >
> >Do my patches against configure.in, apr.hnw and apr.hw fixing the
> >apr_ino_t ( http://issues.apache.org/bugzilla/show_bug.cgi?id=43417 )
> >issue have a chance of getting accepted (aka should I try to test them
> >on other configurations to see whether they break something on some
> >systems)?
>
> Not according to folks interpretation of our versioning policy, it seems
> this patch must wait for 2.0.0. You would be breaking ABI for anyone
> building against a 32 bit APR build.
Lucian's patches do not change the ABI of the library which is built (at
least by design, unless you are talking about some problem with the
implementation which I'm missing).
In the case where APR is built *without* _FILE_OFFSET_BITS=64, and an
external application includes APR headers *with* _FILE_OFFSET_BITS=64
defined, an APR library ABI would be used by the application which does
not match the real ABI, and all bets were off. Lucian's patch fixes
that.
Exposing apr_ino_t from apr.h rather than only from apr_file_info.h is
certainly an API extension and so cannot be done in 1.2.x.
joe
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Lucian Adrian Grijincu wrote:
>
> Do my patches against configure.in, apr.hnw and apr.hw fixing the
> apr_ino_t ( http://issues.apache.org/bugzilla/show_bug.cgi?id=43417 )
> issue have a chance of getting accepted (aka should I try to test them
> on other configurations to see whether they break something on some
> systems)?
Not according to folks interpretation of our versioning policy, it seems
this patch must wait for 2.0.0. You would be breaking ABI for anyone
building against a 32 bit APR build.
If folks can prove APR 1.0.0 forward have promised that it's internally
always built _FILE_OFFSET_BITS=64 then I'd entertain changing the type,
(an external included-headers apparent bug as opposed to an internal bug)
but again according to folks interpretation of the versioning policy,
this would have to be based on apr_off_t or a type we already declare,
as we cannot add an apr_ino_t until 1.3.0.
Other opinions?
Bill
Re: Tagging 1.2.* Sat night or Sun a.m.
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 10/11/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Depending on my weekend.
>
> I found the bug, I'll commit the fix friday after I vet it a bit more.
> Win32 apr_file_dup2's new flags bits for stdio channels were the buggers,
> needed to mask those off in appropriate ways, but I'm not done validating
> the fix.
>
> I've committed to trunk the build changes that let us almost seamlessly
> transition from .dsp to .vcproj, for x86 and x64. [Actually, they are
> seamless here because we don't try stuffing /D "SYM=With a space" at
> the resource compiler. httpd is not so lucky].
>
> I don't plan to backport the /D WINNT change for x86, or the Win9x
> targets. Save that for 1.3. But there's no sense in run time tests
> of 9x for x64 ;-) Otherwise, it goes over to 1.2 as-in trunk.
>
> So the dsp improvements roll on to apr-iconv and apr-util for releases
> there too. We should have a really ready-to-rock x64 build schema, all
> it still needs is a top level Makefile.win to make things easy, which
> I'm about to crib from httpd (including ssl detection logic for apr 1.3,
> but nothing that rich for the other trees.)
>
> So what of y'all - anything else to slide in before the weekend? If
> you are still at it Saturday, be sure to holler so I don't accidentally
> roll right over anyone's toes.
>
Do my patches against configure.in, apr.hnw and apr.hw fixing the
apr_ino_t ( http://issues.apache.org/bugzilla/show_bug.cgi?id=43417 )
issue have a chance of getting accepted (aka should I try to test them
on other configurations to see whether they break something on some
systems)?
--
Lucian