276330 - BeOS does not need the extra seeking introduced by bug 4090

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[tqh]

User-Agent:       Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.7.5) Gecko/20041219 Firefox/1.0
Build Identifier: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.7.5) Gecko/20041219 Firefox/1.0

From bug 4090:
'PR_Open() has a flag "PR_APPEND" which the document claims
will open the file with the offset at the end of the file.
This is not currently implemented on Win32.'

UNIX and BeOS should do the right thing without that patch.

Reproducible: Always

Comment 1

[tqh]

Attachment: adds BeOS to ifdefs

Comment 2

[tqh]

Forgot to mention that patch is taken from AVIARY-branch.

Comment 3

[Niels Reedijk]

Patch looks good. I'll try to apply it later. If it applies cleanly in the MAIN
branch, you should find someone to review it. I know whom to contact for a
superreview.

Comment 4

[Niels Reedijk]

1. Did you test the patch? does it work? How can you test whether it works or not?
2. What extra value does it give? Is it a worthwhile performance patch? 

(not to discourage you, but I think the ifdefs are rather ugly. At the other
hand, they're there anyway, so that's not an issue either). 

Finally, one stylistic remark: In the first hunk
mozilla/nsprpub/pr/src/io/prfile.c the #endif is marked with XP_UNIX in a
comment. You should mention XP_BEOS there as well. 

Comment 5

[tqh]

1. I've applied it and it runs ok. I havn't looked at any log-file to see it in
action. But we're using the exact same function for opening with APPEND and
BeBook says the same so I've read up how it should work in a perfect work. I
have never heard from any other project or port using POSIX to have problem with
APPEND so I'm quite confident it's ok. I should at least expect Apache to have
it in it's code. Anyone has an easy way to check this? A log-file generated
while running firefox perhaps?

2. It removes unnecessary code, always strive for perfection. Also I wanted to
get it of my mind and down in a bug instead as I'm moving from AVIARY to trunk.
Notice that the bug is set to minor as it  is just 'the right way' for BeOS, but
doesn't really change much. You should read the defines as 'if windows or OS/2
or other platform without append'.

(Btw, you missed the double space in the first hunk :P)

Comment 6

[tqh]

Btw, don't worry about discouraging me. I appreciate comments. Ifdefs are by
nature a bit ugly, but needed in many many places. I think it would be uglier
for us to have code that just adds bloat though.

Comment 7

[tqh]

Attachment: added the XP_BEOS comment.

Added XP_BEOS endif comment, and asking for review.

Comment 8

[Sergei Dolgov]

Comment on attachment 171437 [details] [diff] [review]
added the XP_BEOS comment.

r=sergei_d
For nicety, person who will check that in, should remove extra space before ||
in last #if
:)

Comment 9

[tqh]

Attachment: Removed extra space

Same patch but with the extra space removed.

Comment 10

[tqh]

Comment on attachment 171518 [details] [diff] [review]
Removed extra space

Ran the nspr test 'append -d' and it passed so I guess it's time for sr (and
checkin would be nice too)

Comment 11

[Wan-Teh Chang]

Comment on attachment 171518 [details] [diff] [review]
Removed extra space

tqh, thanks for the patch.

I want to ask you to make a change, and I
apologize for forgetting to ask you to do
it before.

In your patch, please replace all instances of

    #if !(defined(XP_UNIX) || defined(XP_BEOS))

by

    #if !defined(_PR_HAVE_O_APPEND)

and add

    #define _PR_HAVE_O_APPEND

to nsprpub/pr/include/md/_unixos.h and _beos.h.

This will make it easier to do this for another
platform if it also has O_APPEND or an equivalent.

Comment 12

[tqh]

Will do.

Comment 13

[tqh]

Attachment: _PR_HAVE_O_APPEND with comments (checked in after editing)

Added reference to this bug in comment and also tried to describe the define in
the md headers.

Can't test the UNIX change, but BeOS is ok.

Comment 14

[Wan-Teh Chang]

Patch checked in on the NSPR tip (NSPR 4.6)
and NSPR_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 Beta).

Comment 15

[Wan-Teh Chang]

Comment on attachment 171547 [details] [diff] [review]
_PR_HAVE_O_APPEND with comments (checked in after editing)

r=wtc.	I made some editing changes before
checking this in.

Comment 16

[Wan-Teh Chang]

I ran "append -d" with the "WIN95" configuration
of NSPR on Windows.  The test passed.

Most Unix platforms do not use the code in prfile.c.
(Their file functions are implemented in ptio.c.)
So I didn't test the patch on Unix.