Opened 5 months ago

Last modified 3 weeks ago

#23750 needs_revision enhancement

Isolate libevent usage to a few locations

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: refactoring, technical-debt, review-group-31
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

With #21841, we restricted openssl header usage to a small number of modules. We should do the same with libevent.

Child Tickets

Change History (4)

comment:1 Changed 5 months ago by nickm

I've started some work here as isolate_libevent. Let's take a look and maybe merge it when 0.3.3 rolls around.

comment:2 Changed 4 weeks ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final
Status: assignedneeds_review

comment:3 Changed 3 weeks ago by nickm

Keywords: review-group-31 added

comment:4 Changed 3 weeks ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

From commit 6c5a4bef4e77cf37:

  • I would propose that mainloop_event_new() validates at least the cb argument passed with a tor_assert() (seems it can't be NULL nor it makes sense to be NULL).

I would also document the function that the userdata ownership is passed to the returned mainloop_event_.

  • Can we use mainloop_event_activate() if the event was never scheduled/added to the main loop?
  • Can I use tv = NULL in mainloop_event_schedule() to tell it "now" or tv must be a valid pointer?
  • I would document what this does if called multiple time: mainloop_event_cancel().
  • Maybe we should have this using the FREE_AND_NULL macro scheme? mainloop_event_free()

From commit 3edf64a95f91d9be:

  • This function threadpool_register_reply_event() doesn't use the nice mainloop_event_t API and I can see that is because we create an event with specific flags (EV_PERSIST | EV_READ).


And then it goes on using event_add() directly. Couldn't we remove all this and make it that it would use that mainloop interface created prior?

  • Also, why is threadpool_register_reply_event() is taking a base event, can't we abstract that to always use tor_libevent_get_base() or that doesn't play nice in multi thread?
Note: See TracTickets for help on using tickets.