Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:  closed
 Priority:  High |  Milestone:  Tor:
Component:  Core Tor/Tor |  0.2.9.x-final
 Severity:  Normal   |Version:
 Keywords:  modularity, tor-modularity,  | Resolution:
  TorCoreTeam201605, TorCoreTeam-|  implemented
  postponed-201604, review-group-1   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by cypherpunks):

 Replying to [comment:29 cypherpunks]:
 > `pubsub_subscriber_t *r = tor_malloc_zero(sizeof(r));`
 >
 > Nice.
 This was indeed the cause and ticket:19038#comment:1 contains a patch
 based on your suggestion.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:  closed
 Priority:  High |  Milestone:  Tor:
Component:  Core Tor/Tor |  0.2.9.x-final
 Severity:  Normal   |Version:
 Keywords:  modularity, tor-modularity,  | Resolution:
  TorCoreTeam201605, TorCoreTeam-|  implemented
  postponed-201604, review-group-1   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by cypherpunks):

 `pubsub_subscriber_t *r = tor_malloc_zero(sizeof(r));`

 Nice.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:  closed
 Priority:  High |  Milestone:  Tor:
Component:  Core Tor/Tor |  0.2.9.x-final
 Severity:  Normal   |Version:
 Keywords:  modularity, tor-modularity,  | Resolution:
  TorCoreTeam201605, TorCoreTeam-|  implemented
  postponed-201604, review-group-1   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by arma):

 {{{util/pubsub/pubsub_basic: [forking] *** Error in `./src/test/test':
 free(): invalid next size (fast): 0xf7890ee0 ***}}}

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:  closed
 Priority:  High |  Milestone:  Tor:
Component:  Core Tor/Tor |  0.2.9.x-final
 Severity:  Normal   |Version:
 Keywords:  modularity, tor-modularity,  | Resolution:
  TorCoreTeam201605, TorCoreTeam-|  implemented
  postponed-201604, review-group-1   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-
Changes (by nickm):

 * status:  merge_ready => closed
 * resolution:   => implemented


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  merge_ready
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604, review-group-1   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by nickm):

 Thanks David! Squashing & Merging.  Calling the other stuff "can fix later
 if needed".

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  merge_ready
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604, review-group-1   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-
Changes (by dgoulet):

 * status:  needs_review => merge_ready


Comment:

 lgtm;

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604, review-group-1   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-
Changes (by nickm):

 * status:  needs_revision => needs_review


Comment:

 >DECLARE/DEFINE change hasn't been changed so I'm assuming the current
 branch needs a revision.

 just did that in 8b75263e35ec5c217525fa67a019852b6b541ffc ; thanks for the
 reminder.

 >Also, the whole need for a subscriber_data_t structure hasn't been
 addressed. I'm fine if you prefer it that way instead of making the caller
 choose whatever it want to pass.

 I think I did this already in ceadd497525120d0f77be7b5755ca5bc7015f326
 though.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_revision
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604, review-group-1   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 * `DECLARE/DEFINE` change hasn't been changed so I'm assuming the current
 branch needs a revision.

 * Also, the whole need for a `subscriber_data_t` structure hasn't been
 addressed. I'm fine if you prefer it that way instead of making the caller
 choose whatever it want to pass.

 I'm currently happy with the latest commits addressing some issues. I'll
 let cypherpunks here maybe continue his/her review.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by cypherpunks):

 Replying to [comment:20 nickm]:
 > > Why gratuitously polluting the namespace of every consumer of
 "pubsub.h"?
 >
 > The implementation macros use these functions.  The alternative would be
 to rename them and stick them in the macros.  That didn't seem to have
 much point IMO.

 Errh.  Why would you need to rename them?  AFAICS, you could simply put
 them all at the beginning of the IMPLEMENT macro, for example.

 In "pubsub.c" you only need the types.  It would be great if it were
 possible to put them in a header and include this from both "pubsub.c" and
 the IMPLEMENT macro, but that doesn't work, right?  In any case,
 repetition can be avoided if the include inside the macro is scripted (a
 pre-preprocessing step).

 Hmm.  Well, at least the function prototypes can be easily hidden away in
 the macro.

 > > Yeah, no.
 >
 > Please spell it out for me.

 Oh, I think I quoted too much and confused you.  I actually think it's
 fine now with the "locked" guard.  When I said "no" I was actually
 referring only to this part:
 > we can allow it later if needed.

 And I qualified that: "unless...".  And I believe you already agree with
 me in this, that's why you added the "locked" thing, right?  But 2
 examples anyway:
 * Call unsubscribe while inside a notification handler: `_del_keeporder`
 moves the array content and reduces the length; `_FOREACH` is oblivious.
 * Call subscribe while inside a notification handler: `_insert` moves the
 array content, relocates memory (this one is not be a problem in single-
 threading? there's one further indirection? not sure, will check again
 later), and increases the length; `_FOREACH` is oblivious.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by nickm):

 > Why gratuitously polluting the namespace of every consumer of
 "pubsub.h"?

 The implementation macros use these functions.  The alternative would be
 to rename them and stick them in the macros.  That didn't seem to have
 much point IMO.

 > Yeah, no.

 Please spell it out for me.

 >  s/DEFINE/DECLARE/g.

 Ah, that one.  Sounds reasonable.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by cypherpunks):

 Replying to [comment:17 nickm]:
 > Oh, wrt the "quite a number of symbols declared outside of any macro" --
 which symbols did you have in mind?

 Well, all of them!  To wit:
 * pubsub_subscriber_fn_t
 * pubsub_subscriber_t
 * pubsub_topic_t
 * pubsub_subscribe_
 * pubsub_unsubscribe_
 * pubsub_clear_
 * pubsub_notify_fn_t
 * pubsub_notify_

 These are implementation details, nobody should be using them.  So, why
 even allow it?  Why gratuitously polluting the namespace of every consumer
 of "pubsub.h"?

 Replying to [comment:18 nickm]:
 > Okay, both of the above issues should be 'resolved'.  I've solved the
 concurrent-modification problem by just forbidding it for now; we can
 allow it later if needed.

 Yeah, no.  Not unless you change the data structure behind.  I've looked a
 bit at the smrtlist functions/macros now, and indeed they are just
 relocatable arrays, so that's certainly not going to fly against reentrant
 calls (not something easy to avoid once you have this convenient callback
 machinery; call trees can grow complicated pretty quickly).

 (PS: You said nothing about s/DEFINE/DECLARE/g.)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-
Changes (by nickm):

 * status:  needs_revision => needs_review


Comment:

 Okay, both of the above issues should be 'resolved'.  I've solved the
 concurrent-modification problem by just forbidding it for now; we can
 allow it later if needed.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_revision
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by nickm):

 I fixed some of the simpler issues.  I'm going to wontfix the following
 ones:
* The static/public stuff.  I think that I got it right, as cpunks
 describes above, but it'll be easy enough to fix later if we need it
 different.
* Function naming stuff; I don't mind the names.  Also they're easy
 enough to change later if somebody makes a patch.
* Yes, foo_subscriber_t is only a tag.
* Low-valued priorities meaning "do this first" is all over POSIX; I'm
 not changing that.

 That leaves these as my remaining items:
   * Seeing if I can mess with the type definitions so that you don't need
 to declare a struct for non-compound message types.
   * The "interesting" interactions as described above.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_revision
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by nickm):

 Oh, wrt the "quite a number of symbols declared outside of any macro" --
 which symbols did you have in mind?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_revision
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by cypherpunks):

 Replying to [comment:14 dgoulet]:
 > Replying to [comment:13 cypherpunks]:
 > > It's supposed to be an inter-module facility, remember? Think about
 it.
 >
 > It is but that doesn't mean it has to be used inter-module _all_ the
 time thus controlling the linkage here is cheap and desirable imo.

 Sure, and I conceded as much in the my sentence. But my reminder was also
 directed at your item 2. Your feeling is wrong ;).

 I'll spell it out, now that I've read the whole thing.

 nickm's data structures seem fairly decoupled, I can see a few different
 possibilities.  Let's see if I understood it correctly. (You correct me
 nickm.)

 The more straightforward usage would be 1 publisher, n subscribers.
 {{{
 T-pubsub.h:
 #include "pubsub.h

 DECLARE_PUBSUB_TOPIC(T)

 [...]

 T-publishers-private.h:
 #include "T-pubsub.h"
 DECLARE_NOTIFY_PUBSUB_TOPIC(static, T)

 [...]

 T-publisher.c:
 #include "T-pubsub.h"
 #include "T-publisher-private.h"

 IMPLEMENT_PUBSUB_TOPIC(static, T)

 Use T_notify() and T_clear() here.  But maybe also T_subscribe()
 and
 T_unsubscribe().

 [...]

 any-T-subscriber.c:
 #include "T-publisher.h"

 Implement your T_subscriber_fn_t here, or maybe grab a generic one
 from
 somewhere else (maybe the publisher provides one).

 Use T_subscribe() and T_unsubscribe() here.
 }}}

 But ISTM that n publishers, n subscribers is also possible.
 {{{
 another-T-publisher.c:
 #include "T-pubsub.h"
 #include "T-publisher-private.h"

 // Note: no IMPLEMENT.

 Use T_notify() and T_clear() here.  But maybe also T_subscribe()
 and
 T_unsubscribe().

 [...]
 }}}

 The choice of where to put the definition for `T_event_data_t` and
 `T_subscriber_data_t` seems quite open, since the functions in "pubsub.c"
 only shuffle pointers around and never dereference them.  So it
 effectively depends on what the notification handler does with its
 arguments (plus maybe style and mental health).

 If the T-publisher(s) need to send actual data with the event (the event
 is more than just a "signal"), then, logically, T-publisher(s) and
 T-subscribers need to have the definition for `T_event_data_t` available.
 So it should probably go on the public "T-pubsub.h" header.

 If not, then I don't see why you couldn't always pass NULL.

 If there's a generic T-notification handler somewhere, then the definition
 for `T_subscriber_data_t` should probably be alongside.  If instead every
 T-subscriber implements his own T-notification handler then the definition
 can be in the c unit of each; hell, they could even be all different.

 ---

 Further review comments:

 - I don't think you use "tor_queue.h" included in "pubsub.h".

 - I don't like the `T_subscriber_fn_t` name (nor the "generic" type name
 nor the argument name).  A name that conveys "T notification handler", or
 similar, would be better IMHO.  But T_notification_handler_fn_t is a tiny
 bit unwieldy.

 - Why does `pubsub_subscriber_fn_t` take a single `void*` argument?  Why
 not `(void*, void*)`?  I see that it's going to be casted anyway, but is
 there any reason?

 - In `T_call_the_notify_fn_`, maybe document that the reason this function
 exists is so that the function pointer can be casted back to the
 appropriate type (noting that is not necessary to cast any of the 2 data
 pointers, in C).

 - `T_subscriber_t` is just used as a type tag, memory is never accessed as
 a `T_subscriber_t` (it can't: the type is never defined, only declared),
 instead it is always cast to and from `pubsub_subscriber_t` before and
 after accessing.  Correct?

 - In "pubsub.h", there are quite a number of symbols declared outside of
 any macro.  Why didn't they go inside the implementation macro?  Where
 else are they needed?

 - "Funny" that low priority means high priority ;).

 - Did you think about "interesting" interactions? E.g. 

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_revision
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by dgoulet):

 Replying to [comment:13 cypherpunks]:
 > Replying to [comment:12 dgoulet]:
 > > * I think a `pubsub_free_` would be useful because right this leaks
 the `static pubsub_topic_t name ## _topic_ = { NULL, 0 };`.
 >
 > dgoulet, dude!
 Ahah... I really thought this one was a pointer. Nvm :)

 >
 > > 2) Why is `DEFINE_PUBSUB_TOPIC` and `DEFINE_NOTIFY_PUBSUB_TOPIC` are
 separated? I feel like _not_ having a notify function defined will end up
 breaking `IMPLEMENT_PUBSUB_TOPIC` so maybe merge them together?
 > >
 > > 3) Is there a reason why only the `notify` and `clear` can have a
 different linkage? I can see myself wanting _all_ the functions `static`.
 >
 > It's supposed to be an inter-module facility, remember? Think about it.

 It is but that doesn't mean it has to be used inter-module _all_ the time
 thus controlling the linkage here is cheap and desirable imo.

 >
 > (Though, about the linkage: it may actually be useful to have the option
 anyway.)
 >
 > (Also: `DEFINE_*` should really be `DECLARE_*`, definition is the same
 as implementation.)

 Agree.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_revision
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-

Comment (by cypherpunks):

 Replying to [comment:12 dgoulet]:
 > * I think a `pubsub_free_` would be useful because right this leaks the
 `static pubsub_topic_t name ## _topic_ = { NULL, 0 };`.

 dgoulet, dude!

 > 2) Why is `DEFINE_PUBSUB_TOPIC` and `DEFINE_NOTIFY_PUBSUB_TOPIC` are
 separated? I feel like _not_ having a notify function defined will end up
 breaking `IMPLEMENT_PUBSUB_TOPIC` so maybe merge them together?
 >
 > 3) Is there a reason why only the `notify` and `clear` can have a
 different linkage? I can see myself wanting _all_ the functions `static`.

 It's supposed to be an inter-module facility, remember? Think about it.

 (Though, about the linkage: it may actually be useful to have the option
 anyway.)

 (Also: `DEFINE_*` should really be `DECLARE_*`, definition is the same as
 implementation.)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-+-
 Reporter:  nickm|  Owner:  nickm
 Type:  enhancement  | Status:
 Priority:  High |  needs_revision
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,  |Version:
  TorCoreTeam201605, TorCoreTeam-| Resolution:
  postponed-201604   |  Actual Points:
Parent ID:   | Points:  medium
 Reviewer:  dgoulet  |Sponsor:
 |  SponsorS-can
-+-
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 Some code review and then after some thoughts. Btw, this is neat, it's the
 kind of thing I would use once merged!

 * Seems to be an extra space typo in `pubsub_notify_`
 {{{
   ++ topic->n_events_fired;
 }}}

 * In `pubsub_notify_`, seems `n_ok` is basically useless counter there.

 * In `pubsub_notify_`, the comment says: `Return the number of nonzero
 return values`. But it returns 1 or 0 depending on if we a bad one
 happened, not `n_bad` which is what the comment suggests.

 * I think a `pubsub_free_` would be useful because right this leaks the
 `static pubsub_topic_t name ## _topic_ = { NULL, 0 };`. We should be able
 to fully clear the memory linked to a unused pubsub.

 * Why are we keeping "notify flags" if they are not used at all. Do you
 plan having some or just "in case"?

 * Imagine tens of thousands time a topic being notified every 60 seconds.
 I worry that this will overflow regularly. So `(2^32 / 1) == 429496`
 minutes which is 7158 hours and thus after running for 298 days, wrap
 time. Ok, pretty large but still that could have undesired consequences in
 the future if unnoticed. Pretty cheap to have it set to `uint64_t`.
 {{{
 unsigned n_events_fired;
 }}}

 * Do we want `pubsub_clear_` to reset `n_events_fired` ?

 == Thoughts:

 1) Could it be possible to use void data pointer instead of
 `_subscriber_data_t` and `_event_data_t`. Not sure it's really good to
 force every user to create "data" wrappers. If they need one because for
 instance they need a lot of information being passed along, the caller
 should define one by itself. I suspect most of the time we'll pass back a
 `connection` object or some single data structure. So we could do
 something like this instead of more data structure which seems more simple
 and telling:

 {{{
 dir_connection_t new_dir_conn;
 [...]
 dirconn_open_subscribe(dir_conn_has_opened, NULL, 0, 0)
 [...]
 dirconn_open_notify(new_dir_conn)
 }}}

 2) Why is `DEFINE_PUBSUB_TOPIC` and `DEFINE_NOTIFY_PUBSUB_TOPIC` are
 separated? I feel like _not_ having a notify function defined will end up
 breaking `IMPLEMENT_PUBSUB_TOPIC` so maybe merge them together?

 3) Is there a reason why only the `notify` and `clear` can have a
 different linkage? I can see myself wanting _all_ the functions `static`.

 4) One part that will be annoying is the `foobar_subscriber_t` object that
 `pubsub_subscribe_()` returns. A user would have to keep them somewhere
 and index them using function pointer/data pointer pair. I wonder how we
 could make this easier. At least, `pubsub_clear_` will be useful for just
 cleaning everything instead of using all subscriber object. What about an
 "alias" like `foobar_unsubscribe_all()` which brings a clearer semantic
 imo.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs