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

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

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.

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.

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)

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.

Added XP_BEOS endif comment, and asking for review.

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 :)

Same patch but with the extra space removed.

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 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.

Will do.

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.

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

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.

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.