Opened 8 years ago

Closed 6 years ago

#5129 closed enhancement (implemented)

Avoid fcntl(O_NONBLOCK) calls on Linux

Reported by: nickm Owned by:
Priority: Low Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: easy tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We already use the SOCK_CLOEXEC extesion to socket/accept4/open/socketpair on Linux so that we can create a socket/fd and make it close-on-exec in a single syscall. In 0.2.4.x, we might want to do the same for opening a nonblocking socket.

Child Tickets

Attachments (3)

ticket5129.patch (9.8 KB) - added by piet 7 years ago.
ticket5129.patch.v2 (11.1 KB) - added by piet 6 years ago.
ticket5129.patch.v3 (13.2 KB) - added by piet 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by nickm

Keywords: easy added

comment:2 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:3 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:4 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:5 Changed 7 years ago by nickm

Type: defectenhancement

Changed 7 years ago by piet

Attachment: ticket5129.patch added

comment:6 Changed 7 years ago by piet

Status: newneeds_review

Wrote nonblocking versions of tor_open_socket() and tor_accept_socket() and used them throughout the code.

comment:7 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

This looks good. It should check the return values from set_socket_nonblocking() and from fcntl.

Unfortunately, it no longer applies cleanly to master, and will therefore need some hand-cleanup before I can merge it.

Changed 6 years ago by piet

Attachment: ticket5129.patch.v2 added

comment:8 Changed 6 years ago by piet

Status: needs_revisionneeds_review

This should do the trick, hopefully.

comment:9 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

Okay, looks better. Three things I want to do before merging this:

  • The eventdns changes should be wrapped in an #if 1 or something -- eventdns.c is imported code (snarfed from Libevent 1.x and then patched sporadically with fixes from later Libevent versions), and we should minimize our changes to it. In fact, unless the old code is _wrong_ , we should really just leave it alone.
  • In several places, the patch uses an #ifdef _WIN32 to decide whether to call close() or closesocket(). Ideally, we should have a function or macro for that. This would be tor_close_socket, but without the socket accounting.
  • This needs a changes/ file.

I'll see if I can do these in the next day or two, unless you want to.

comment:10 Changed 6 years ago by piet

I can make the changes if you want. Not sure when I'll have the time, but it will be some time this week.
I'm not sure whether I understand your first point correctly, though. You want to wrap the changed code in the #if branch, and leave the old code in the #else branch, so it won't be compiled but is still around for future patches? Is that correct?

comment:11 Changed 6 years ago by nickm

I think that sounds good, yes.

Changed 6 years ago by piet

Attachment: ticket5129.patch.v3 added

comment:12 Changed 6 years ago by piet

Status: needs_revisionneeds_review

comment:13 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Thanks, and sorry about the delay! (I hope the next version of trac sends notification mails when somebody uploads an attachment.)

I merged your patch as ebd4ab1506517b32ee3bd5bd1597b4373aa56ee7 ; fixed a bug as e25eb35f11b0419fe50791b3760f1d4016bf8a8b; and added tests in b8d9c8403718b2dca577947d2f5d9248ab87961e, d6adf055828283dda2ca9c0c77a80348778c45df, and bcc39c466688e41ac8f58e5589e854a916769a14. Thanks!

Note: See TracTickets for help on using tickets.