[haiku-commits] Re: haiku: hrev48985 - in src: kits/interface system/libroot/posix/malloc_debug kits/tracker add-ons/kernel/debugger/demangle

  • From: Dario Casalinuovo <b.vitruvio@xxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Mon, 6 Apr 2015 02:02:25 +0200

Hi Michael,

What is certainly not correct however is quitting the BMediaRoster which

deletes the gDormantNodeManager and then doing
gDormantNodeManager->PutAddOn() from the MediaAddonServer.


I guess there were some reason to add this, but who originally created the
media_addon_server relied on the static object to destroy it. As you are
suggesting it looks like the cleanest solution out there. Anyway i hope
that we can avoid in any case to interfere with the BMediaRoster, beginning
from reconnecting it in the backend instead to let the app quit it when the
media services are shut down. I believe the Haiku code should be the first
example of correct use of it's API.

In general this whole thing is rather messy:

* The gDormantNodeManager is created in the BMediaRosterEx constructor.
* The gDormantNodeManager is deleted in BMediaRoster (not BMediaRosterEx)
destructor.


I agree, i've realized it too in my work to improve the media_kit, this
also apply to registration messages which are done in the same places you
are pointing to. While there's not much difference in pratice (since after
all the BMediaRosterEx is just something to hide implementation details and
it's the class returned by BMediaRoster::Roster()) i think this should be
cleaned up for a 'principle' question by removing the BMediaRosterEx
constructor.

* The MediaAddonServer uses this global directly which makes it non-obvious
that its lifetime is managed by BMediaRoster[Ex] and therefore is coupled
with the Lock()/Quit() sequence.


Also the media_server make use of it in a place, i think this functionality
could be just composite into the BMediaRosterEx or at limit give a method
to access it but make the variable private anyway. Good catch anyway!

If the whole thing can just be left to the static object mentioned by
Dario (I assume it's the MediaInitializer in MediaRoster.cpp?) then that
might be the cleanest solution.


I just think that it's what happen after QuitRequested() return and the app
is unloaded, so no need to do it explicitly.

From looking around MediaRoster.cpp there seems to be a duplicate
InitDataExchange() call both in the constructor of the static object and in
the constructor of BMediaRosterEx. Can the one in the BMediaRosterEx
constructor be removed? Or would it make more sense having it done lazily
and remove it from the static object instead?


For me it would make more sense to establish the connection lazily. This
because an app may make use of the BMediaRoster but create it only in
certain situations which depending on the app may happen rarely, and until
the client attempt to create the roster we don't need the message passing
stuff to be up.

If there's not problem i would add those changes to my media_kit patchset
as i've already changed this part of code and i hope we will not make merge
conflicts and double work eventually.

Regards,
Dario
--
« Nullius addictus iurare in verba magistri, quo me cumque rapit tempestas,
deferor hospes. »

Other related posts: