Open
Bug 888581
Opened 10 years ago
Updated 1 year ago
Allow integrating NSPR/NSS sockets into non-NSPR event loops
Categories
(NSPR :: NSPR, defect)
Tracking
(Not tracked)
REOPENED
4.10.3
People
(Reporter: mitr, Unassigned)
Details
Attachments
(2 files, 1 obsolete file)
3.66 KB,
patch
|
wtc
:
review-
rrelyea
:
review+
|
Details | Diff | Splinter Review |
3.35 KB,
patch
|
Details | Diff | Splinter Review |
Event-driven applications that want to use NSS for TLS can currently do so only by designing the event loop around PR_Poll(), because NSS needs to be able to remap PR_POLL_{READ,WRITE} to reflect e.g. the actual direction necessary for performing renegotiations; this remapping (through a PRPollFN) happens only implicitly within PR_Poll() and is not otherwise available to applications. Changing the primary event loop is an extremely invasive requirement; it makes it impossible to use NSS together with software assuming a different event loop (e.g. the one in GLib), or to encapsulate NSS use in a module without requiring users of the module to know about NSS. To allow using NSPR sockets in other event loops built around select(), poll() or similar calls, applications need 1. the handle to use 2. a way to remap the flags. This is essentially the mirror of PR_{Create,Destroy}SocketPollFd() 1. The handle is currently available through PR_FileDesc2NativeHandle(), which is in a "private" header file. I'll attach a patch that proposes a more limited version, PR_SocketPollingHandle(), documented in a way that should avoid the UNIX/Windows semantics differences. 2. The flags are available by directly calling fd->methods->poll; proposed PR_SocketPollingStatus() both encapsulates this and makes the semantics documented. The patches don't do anything fundamentally new, they would just allow callers to use officially supported and documented interfaces instead of private undocumented ones. There certainly are alternative approaches - e.g. 1. Just formally bless PR_FileDesc2NativeHandle() and leave it up to the caller to be careful about non-sockets. 2a. Add PR_NativePollingStatus() that returns <poll.h> POLL* flags instead of PR_POLL_*, making it easier for UNIX callers to map the flags. 2b. In addition to PR_SocketPollingStatus(), add also PR_SocketPollingResult() that helps mapping back from the poll() direction returned on the underlying socket to the application-relevant I/O direction.
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #769341 -
Attachment is patch: true
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Updated•10 years ago
|
Attachment #769341 -
Flags: review?(wtc)
Reporter | ||
Updated•10 years ago
|
Attachment #769342 -
Flags: review?(wtc)
Reporter | ||
Updated•10 years ago
|
Attachment #769341 -
Flags: review?(rrelyea)
Reporter | ||
Updated•10 years ago
|
Attachment #769342 -
Flags: review?(rrelyea)
Comment 2•10 years ago
|
||
Comment on attachment 769341 [details] [diff] [review] Add PR_SocketPollingHandle() r+ My only reservation is ptio.h. I think we should probably return -1 if pthreads are used. In practice I don't think this is an issue as I don't think we have any more platforms that no longer support native threads. wtc do you have an opinion?
Attachment #769341 -
Flags: review?(rrelyea) → review+
Comment 3•10 years ago
|
||
Comment on attachment 769342 [details] [diff] [review] Add PR_SocketPollingStatus() r+ rrelyea. Unlike the other function, since this uses the underlying poll function it should be safe for the ptio case.
Attachment #769342 -
Flags: review?(rrelyea) → review+
Comment 4•10 years ago
|
||
both patches checked in: https://hg.mozilla.org/projects/nspr/rev/1bd2f00745d3 https://hg.mozilla.org/projects/nspr/rev/77c4ee30c1fe
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.10.2
Comment 5•10 years ago
|
||
Comment on attachment 769341 [details] [diff] [review] Add PR_SocketPollingHandle() Review of attachment 769341 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. I prefer just use the existing PR_FileDesc2NativeHandle function. ::: pr/src/io/prsocket.c @@ +1552,5 @@ > +PR_SocketPollingHandle(PRFileDesc *fd) > +{ > + if (!_pr_initialized) _PR_ImplicitInitialization(); > + /* Should this refuse a non-socket fd? */ > + return PR_FileDesc2NativeHandle(fd); Why don't you use PR_FileDesc2NativeHandle? In general there is nothing wrong with using a function from "private/pprio.h" because the header is exported. The "private" directory name is unfortunate. It is similar to the "friend" access in C++.
Comment 6•10 years ago
|
||
Comment on attachment 769342 [details] [diff] [review] Add PR_SocketPollingStatus() Review of attachment 769342 [details] [diff] [review]: ----------------------------------------------------------------- ::: pr/include/prio.h @@ +2057,5 @@ > + * should wait for on the underlying OS socket. On failure, the value > + * is -1. > + ************************************************************************ > + */ > +NSPR_API(PRInt32) PR_SocketPollingStatus(PRFileDesc *fd, PRInt16 in_flags, We should remove "Socket" from the function name because the |fd| can also be other kinds of file descriptors, for example, pipes or even regular files. Just wanted to confirm: this function is just syntactic sugar, right? Because you can already invoke the |poll| method directly.
Comment 7•10 years ago
|
||
Reopening, because we want to get a NSPR release completed, but Wan-Teh would like more time to properly review the function signatures etc. I've temporarily backed out the patches for the release process. https://hg.mozilla.org/projects/nspr/rev/b3bb2fe7f716 https://hg.mozilla.org/projects/nspr/rev/4e13f425d282
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 4.10.2 → 4.10.3
Comment 8•10 years ago
|
||
I understand that Miloslav needs to respond to Wan-Teh's comment, prior to proceeding with this work. I'll postpone relanding the patch until there's progress and agreement on the API.
Comment 9•10 years ago
|
||
Comment on attachment 769341 [details] [diff] [review] Add PR_SocketPollingHandle() Review of attachment 769341 [details] [diff] [review]: ----------------------------------------------------------------- Miloslav: thank you very much for the patch. NSPR users should use PR_FileDesc2NativeHandle() for this purpose. Some headers in NSPR's "private" directory are exported, public headers. "private/pprio.h" is such a header. The "private" directory name comes from the NSPR-only days. In a mixed NSPR/native environment, some of the functions declared in these headers are crucial. So, please feel free to use PR_FileDesc2NativeHandle(). Do you have any suggestion on changes to the comments in "private/pprio.h" to clarify this issue? ::: pr/src/pthreads/ptio.c @@ +4633,5 @@ > } /* PR_FileDesc2NativeHandle */ > > +PR_IMPLEMENT(PROsfd) PR_SocketPollingHandle(PRFileDesc *fd) > +{ > + if (!_pr_initialized) _PR_ImplicitInitialization(); This _PR_ImplicitInitialization() call is not necessary because the PRFileDesc input argument implies NSPR is already initialized.
Attachment #769341 -
Flags: review?(wtc) → review-
Comment 10•10 years ago
|
||
Comment on attachment 769342 [details] [diff] [review] Add PR_SocketPollingStatus() Review of attachment 769342 [details] [diff] [review]: ----------------------------------------------------------------- Miloslav: thank you for the patch. It is OK to call fd->methods->poll directly. It seems useful to add a PR_SocketPollingStatus function to clarify it's legal to call the fd->methods->poll method. fd->methods->poll was originally intended for internal use by PR_Poll. But it is now crucial for interfacing to a non-NSPR event polling system. I suggest renaming this function "PR_GetPollingStatus" or "PR_GetPollStatus" because 1) a function name should have a verb, and 2) the function can also be used on pipes on Unix. ::: pr/include/prio.h @@ +2057,5 @@ > + * should wait for on the underlying OS socket. On failure, the value > + * is -1. > + ************************************************************************ > + */ > +NSPR_API(PRInt32) PR_SocketPollingStatus(PRFileDesc *fd, PRInt16 in_flags, Please declare this function after PR_SendTo. This matches the ordering of the sendto and poll methods in the PRIOMethods structure. ::: pr/src/pthreads/ptio.c @@ +4645,4 @@ > if (fd) fd->secret->md.osfd = handle; > } /* PR_ChangeFileDescNativeHandle*/ > > +PR_IMPLEMENT(PRInt32) PR_SocketPollingStatus(PRFileDesc *fd, PRInt16 in_flags, This function should be defined, once, in nspr/pr/src/io/priometh.c. Define it after PR_SendTo. @@ +4647,5 @@ > > +PR_IMPLEMENT(PRInt32) PR_SocketPollingStatus(PRFileDesc *fd, PRInt16 in_flags, > + PRInt16 *out_flags) > +{ > + if (!_pr_initialized) _PR_ImplicitInitialization(); This _PR_ImplicitInitialization() call is also not necessary because the PRFileDesc input argument implies NSPR is already initialized.
Attachment #769342 -
Flags: review?(wtc) → review-
Comment 11•10 years ago
|
||
Miloslav: could you review this patch? Thanks. I think "PR_GetPollingFlags" may be a good name for this function. Let me know what you think of the function name and the comments. I included a link to the NSPR article "NSPR Poll Method".
Attachment #769342 -
Attachment is obsolete: true
Attachment #8361332 -
Flags: review?(mitr)
Comment 12•1 year ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: wtc → nobody
Updated•1 year ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•