318217 - Change ssl_EmulateSendFile to use NSPR 4.1 features

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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]

Attachment: 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]

Attachment: Alternate patch

This is the other design I described.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 204598 [details] [diff] [review]
Proposed patch

I prefer this patch.  r=nelson

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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]

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

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

reopening, so this will appear on our radar for 3.11.1

Comment 8

[Julien Pierre]

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

I back ported this fix to the NSS 3.11 branch.