Last Comment Bug 924376 - Make PR_ImportTCPSocket() a public, officially supported function
: Make PR_ImportTCPSocket() a public, officially supported function
Status: NEW
:
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: 4.10.1
: All All
: -- normal (vote)
: ---
Assigned To: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-08 06:12 PDT by Miloslav Trmač
Modified: 2013-12-03 07:56 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
PR_ImportTCPSocket.patch (2.50 KB, patch)
2013-12-03 07:56 PST, Miloslav Trmač
no flags Details | Diff | Splinter Review

Description Miloslav Trmač 2013-10-08 06:12:52 PDT
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130917102605

Steps to reproduce:

Please make PR_ImportTCPSocket() officially supported, i.e. move it out of the private/ subdirectory.

Having this function available would make it easier to integrate NSS/NSPR with existing server code, where the listen/select/accept loop wouldn't need to know about NSS at all; only the accept()ed OS-level socket would be imported into NSS.

This is related to bug #888581.
Comment 1 Kai Engert (:kaie) 2013-11-26 08:01:17 PST
(a) do you suggest to export PR_ImportTCPSocket, only? What about PR_ImportUDPSocket?

(b) function headers in prio.h appear to have complete documentation headers. This change would require a patch that adds such documentation, too.

(c) I don't know how strict the documentation requirements of NSPR are, but ideally the public NSPR reference would have to get extended to document this API.

(d) In file pprio.h, above the declaration of PR_ImportTCPSocket, there's a comment section that explains the special expectations of NSPR. In my understanding, once you import a file handle into NSPR, then NSPR owns the file handle and may change it's attributes. Is such existing behaviour acceptable for your usage scenario? Do you agree that the importing application would have to stop using OS functions on the socket after the import call, but use NSPR APIs, only?

(e) It seems that PR_ImportTCPSocket doesn't work for IPv6 on Windows. This would have to be documented in the API description.

It seems the primary work here is one of documentation and description of assumptions and limitations of the API.
Comment 2 Miloslav Trmač 2013-12-03 07:56:23 PST
Created attachment 8341752 [details] [diff] [review]
PR_ImportTCPSocket.patch

(In reply to Kai Engert (:kaie) from comment #1)
> (a) do you suggest to export PR_ImportTCPSocket, only? What about
> PR_ImportUDPSocket?

Only TCP for now.  There is a fairly widespread practice of dealing with TCP sockets at the native level (with POSIX, using the for (;;){ accept();fork(); } iditom for daemons; with OpenSSL, doing accept() and connect() using the OS API and then SSL_set_fd() with an established TCP connection), and integrating NSS into such programs with little disruption is useful.

This is has not been the case for UDP, or at least not to such extent.  Now that NSS supports DTS, it might make more sense, but I don't have any experience with this.  Given the difference in connection establishment between TCP and UDP, I'm not sure that allowing the native-connect/NSPR-usage split for UDP is as necessary.

> (b) function headers in prio.h appear to have complete documentation
> headers. This change would require a patch that adds such documentation, too.
Attached a proof-of-concept patch.
 
> (c) I don't know how strict the documentation requirements of NSPR are, but
> ideally the public NSPR reference would have to get extended to document
> this API.
Yes, this would go to https://developer.mozilla.org/en-US/docs/NSPR_API_Reference/I_O_Functions .

OTOH https://developer.mozilla.org/en-US/docs/NSPR_API_Reference starts with
> I am inclined to delay ANY edits to the text, until this massive text is migrated here (with redirects and all) so that this text becomes the "only" source for NSPR API Reference.

> (d) In file pprio.h, above the declaration of PR_ImportTCPSocket, there's a
> comment section that explains the special expectations of NSPR. In my
> understanding, once you import a file handle into NSPR, then NSPR owns the
> file handle and may change it's attributes. Is such existing behaviour
> acceptable for your usage scenario? Do you agree that the importing
> application would have to stop using OS functions on the socket after the
> import call, but use NSPR APIs, only?
Yes, that's fine.

> (e) It seems that PR_ImportTCPSocket doesn't work for IPv6 on Windows. This
> would have to be documented in the API description.
That's unfortunate.  This seems to be only required for knowing what socket type to pass to AcceptEx; wouldn't it be possible to get this socket type
using iAddressFamily from getsockopt( SO_PROTOCOL_INFO ), as described at http://msdn.microsoft.com/en-us/library/windows/desktop/ms738544%28v=vs.85%29.aspx and http://msdn.microsoft.com/en-us/library/windows/desktop/ms741675%28v=vs.85%29.aspx ?  I'm afraid I have very little Windows programming experience.
> 
> It seems the primary work here is one of documentation and description of
> assumptions and limitations of the API.

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