Closed Bug 849078 Opened 7 years ago Closed 7 years ago

Change UnixSocketImpl::SetNonblockFlags to UnixSocketImpl::SetSocketFlags

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: qdot, Assigned: qdot)

Details

Attachments

(1 file, 3 obsolete files)

Bug 836523 removed our O_NONBLOCK set from SetNonblockFlags, meaning bluetooth's accept() blocks the IOThread.
Comment on attachment 722602 [details] [diff] [review]
Patch 1 (v1) - Readd O_NONBLOCK set for sockets

Review of attachment 722602 [details] [diff] [review]:
-----------------------------------------------------------------

Since accept() is now being called only in OnFileCanReadWithoutBlocking(), I think it should be fine without setting O_NONBLOCK. Also, we should change the function name if it does not 'SetNonblockFlags' anymore.
Attachment #722602 - Flags: review?(echou)
Ok, original issue happened due to a mis-rebase on bug 843868 that brought the accept() back into Accept(), and misunderstanding of function names that we should've changed in Bug 836523. Changing this to change function name to something more relevant.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Summary: O_NONBLOCK set removed from UnixSocketImpl::SetNonblockFlags → Change UnixSocketImpl::SetNonblockFlags to UnixSocketImpl::SetSocketFlags
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Attachment #722602 - Attachment is obsolete: true
Attachment #722626 - Flags: review?(echou)
Comment on attachment 722626 [details] [diff] [review]
Patch 1 (v2) - Change SetNonblockFlags to SetSocketFlags

Review of attachment 722626 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Kyle, I just left a quick question in the comment. Pleaes have me review again after it's answered. Thanks.

::: ipc/unixsocket/UnixSocket.cpp
@@ +165,3 @@
>    void Close();
>  
>    /** 

nit: trainling whitespace

@@ +596,4 @@
>      return;
>    }
>  
> +  if (!SetSocketFlags()) {

Question: Do we really need to call SetSocketFlags() here? In case of listening socket, flags should have been already set in Accept(). And, if you'd like to set flags to the newly created socket, I think this should be called after mFd.reset(client_fd).

Please correct me if I misunderstand your idea. Thanks.
Attachment #722626 - Flags: review?(echou)
(In reply to Eric Chou [:ericchou] [:echou] from comment #5)

> @@ +596,4 @@
> >      return;
> >    }
> >  
> > +  if (!SetSocketFlags()) {
> 
> Question: Do we really need to call SetSocketFlags() here? In case of
> listening socket, flags should have been already set in Accept(). And, if
> you'd like to set flags to the newly created socket, I think this should be
> called after mFd.reset(client_fd).

Your point is valid, I did need to add SetSocketFlags after we get the new mFd from accept, as well as keeping it where we create the server descriptor to listen on. That said, where you're pointing it out here is where I added it in Connect(), since it was never getting called there in the first place (used to be, at least, I thought). This was we're covering all of the places we get new file descriptors now. Will put in a new patch with this.
Attachment #722626 - Attachment is obsolete: true
Attachment #722651 - Flags: review?(echou)
Comment on attachment 722651 [details] [diff] [review]
Patch 1 (v3) - Change SetNonblockFlags to SetSocketFlags

Agh. Something wrong with patch.
Attachment #722651 - Flags: review?(echou)
Attachment #722653 - Flags: review?(echou)
Comment on attachment 722653 [details] [diff] [review]
Patch 1 (v4) - Change SetNonblockFlags to SetSocketFlags

Review of attachment 722653 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Thanks!
Attachment #722653 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/4f155de58dbf
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.