Last Comment Bug 318217 - Change ssl_EmulateSendFile to use NSPR 4.1 features
: Change ssl_EmulateSendFile to use NSPR 4.1 features
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: All Windows XP
: P2 enhancement (vote)
: 3.11.1
Assigned To: Wan-Teh Chang
: Jason Reid
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-29 18:49 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2006-04-24 17:52 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (21.49 KB, patch)
2005-11-30 13:57 PST, Wan-Teh Chang
nelson: review+
julien.pierre: review+
Details | Diff | Splinter Review
Alternate patch (21.91 KB, patch)
2005-11-30 14:07 PST, Wan-Teh Chang
nelson: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2005-11-29 18:49:58 PST
In bug 39011, regarding libSSL's new function ssl_EmulateSendFile, Wan-Teh wrote:

> NSPR 4.0 and earlier incorrectly assume that the mem mapping alignment 
> boundary is the page size.  
> To work around this NSPR bug, you have to make an OS-dependent call. 
> When you upgrade to NSPR 4.1 you will be able to call the new function
> PR_GetMemMapAlignment() instead. (But you might as well use the new 
> PR_EmulateSendFile function in NSPR 4.1 directly.)

I think NSS is now using NSPR 4.1.x, and so we should consider making the
changes Wan-Teh suggested.
Comment 1 Wan-Teh Chang 2005-11-30 13:57:37 PST
Created attachment 204598 [details] [diff] [review]
Proposed patch

This will eliminate all the code in lib/ssl/emulate.c.

See bug 34920 comment 2 on how PR_EmulateAcceptRead
and PR_EmulateSendFile should be used, and the transmitfile
method should be defined.

The only design issue is whether you want to have
PR_EmulateAcceptRead and PR_EmulateSendFile directly
in the ssl_methods structure, or you want to define
trivial ssl_AcceptRead and ssl_SendFile methods that
simply call PR_EmulateAcceptRead and PR_EmulateSendFile.
Let me know which you prefer.

I've verified that PR_EmulateAcceptRead and ssl_EmulateAcceptRead
are essentially the same.  I still need to compare
PR_EmulateSendFile with ssl_EmulateSendFile to make sure
they are essentially the same.  But please go ahead and
review this patch.
Comment 2 Wan-Teh Chang 2005-11-30 14:07:27 PST
Created attachment 204601 [details] [diff] [review]
Alternate patch

This is the other design I described.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2006-01-18 14:49:16 PST
Comment on attachment 204598 [details] [diff] [review]
Proposed patch

I prefer this patch.  r=nelson
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-01-18 14:50:07 PST
Comment on attachment 204601 [details] [diff] [review]
Alternate patch

This patch is also acceptable, but I think the other patch is preferable.
Comment 5 Wan-Teh Chang 2006-01-18 15:07:39 PST
I checked in the proposed patch on the NSS trunk (3.12).

Removing emulate.c;
/cvsroot/mozilla/security/nss/lib/ssl/emulate.c,v  <--  emulate.c
new revision: delete; previous revision: 1.7
done
Checking in manifest.mn;
/cvsroot/mozilla/security/nss/lib/ssl/manifest.mn,v  <--  manifest.mn
new revision: 1.14; previous revision: 1.13
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.44; previous revision: 1.43
done
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.46; previous revision: 1.45
done
Comment 6 Nelson Bolyard (seldom reads bugmail) 2006-04-05 09:37:41 PDT
Comment on attachment 204598 [details] [diff] [review]
Proposed patch

I would like to eliminate any unnecessary libssl source code differences between the trunk and the 3.11 branch, so I am requesting second review on this patch for the 3.11 branch.
Comment 7 Nelson Bolyard (seldom reads bugmail) 2006-04-05 10:19:59 PDT
reopening, so this will appear on our radar for 3.11.1
Comment 8 Julien Pierre 2006-04-11 21:55:44 PDT
Comment on attachment 204598 [details] [diff] [review]
Proposed patch

Looks fine. I didn't review the NSPR functions, only the NSS usage.
Comment 9 Nelson Bolyard (seldom reads bugmail) 2006-04-24 17:52:44 PDT
I back ported this fix to the NSS 3.11 branch.

Note You need to log in before you can comment on or make changes to this bug.