(In reply to comment #0)
> Note that this pattern is not entirely save. Best would be

"saFe". And why isn't it?

> But there are
> still other leakages unfixed. For example, NMAgentManager, NMDBusManager,
> NMAuthManager, NMManager, NMSettings, NMPlatform.

Since we're defining a destructor for each singleton anyway, it should nm_log_dbg() if the singleton's ref_count > 1. (And eventually we can change it to nm_log_warn().)



>    all: add macro NM_DEFINE_SINGLETON_GETTER()

There's no reason to define NM_DEFINE_SINGLETON_GETTER_FULL(), since nothing needs it. Just simplify things and get rid of it.



>    core: declare nm_dns_manager_get() using NM_DEFINE_SINGLETON_GETTER()

>-	dns_mgr = nm_dns_manager_get ();
>-	g_assert (dns_mgr != NULL);
>+	nm_dns_manager_get ();

Hm... if main() isn't going to be disposing the singleton itself, then I don't think there's any reason for it to be creating/getting it itself. It can just let it be created by the first thing that tries to use it.

(Likewise for other singletons later.)



>    core: declare nm_session_monitor_get() using NM_DEFINE_SINGLETON_GETTER()

if nm_session_monitor_get() isn't supposed to be visible outside of nm-session-monitor.c, you can just put the prototype there (in the .c file, not the .h file)



>    auth: destruct singleton instance of NMAuthManager on exit
>    dbus: destruct singleton instance of NMDBusManager on exit

"destroy"

(In reply to comment #1)
> (In reply to comment #0)
> > Note that this pattern is not entirely save. Best would be
> 
> "saFe". And why isn't it?

It is safe if subscriber has a known shorter lifetime then the singleton.
It is not safe if a singletons subscribes to another, then the order of their destruction is not strictly defined and it is unsafe.

I added a fixup! commit so that the getter implementation asserts about multiple instantiation of the singleton.

> > But there are
> > still other leakages unfixed. For example, NMAgentManager, NMDBusManager,
> > NMAuthManager, NMManager, NMSettings, NMPlatform.
> 
> Since we're defining a destructor for each singleton anyway, it should
> nm_log_dbg() if the singleton's ref_count > 1. (And eventually we can change it
> to nm_log_warn().)

A singleton can be referenced by another singleton, hence it is legit that the ref_count is higher in the destructor. But I changed to log "disown" only with ref_count>1.


> >    all: add macro NM_DEFINE_SINGLETON_GETTER()
> 
> There's no reason to define NM_DEFINE_SINGLETON_GETTER_FULL(), since nothing
> needs it. Just simplify things and get rid of it.

done


> >    core: declare nm_dns_manager_get() using NM_DEFINE_SINGLETON_GETTER()
> 
> >-	dns_mgr = nm_dns_manager_get ();
> >-	g_assert (dns_mgr != NULL);
> >+	nm_dns_manager_get ();
> 
> Hm... if main() isn't going to be disposing the singleton itself, then I don't
> think there's any reason for it to be creating/getting it itself. It can just
> let it be created by the first thing that tries to use it.
> 
> (Likewise for other singletons later.)

done.


> >    core: declare nm_session_monitor_get() using NM_DEFINE_SINGLETON_GETTER()
> 
> if nm_session_monitor_get() isn't supposed to be visible outside of
> nm-session-monitor.c, you can just put the prototype there (in the .c file, not
> the .h file)

done


> >    auth: destruct singleton instance of NMAuthManager on exit
> >    dbus: destruct singleton instance of NMDBusManager on exit
> 
> "destroy"

done



Rebased and repushed.

> core: declare XXXXX using NM_DEFINE_SINGLETON_GETTER()

Note that some of these objects do initialization when they are created, and they are now created a later (and a *lot* later in the case of the DHCP, VPN, and supplicant managers).

The only one I'm a bit worried about is the supplicant manager.  It does some D-Bus calls to check capabilities of the supplicant, and in the case of wired 802.1x it will only start up right before it gets configured.  I think we need to test wired 802.1x to make sure the capabilities check doesn't happen concurrently with other operations.

(also these patches will make the VPN and DHCP managers do some initializations much later as well, including printing out what VPN plugins there are.  I don't think that's a functional problem at all, but we'll have to be careful with singletons in the future to make sure they are OK with late initialization).

The rest looks OK to me.

(In reply to comment #3)
> > core: declare XXXXX using NM_DEFINE_SINGLETON_GETTER()
> 
> Note that some of these objects do initialization when they are created, and
> they are now created a later (and a *lot* later in the case of the DHCP, VPN,
> and supplicant managers).
> 
> The only one I'm a bit worried about is the supplicant manager.  It does some
> D-Bus calls to check capabilities of the supplicant, and in the case of wired
> 802.1x it will only start up right before it gets configured.  I think we need
> to test wired 802.1x to make sure the capabilities check doesn't happen
> concurrently with other operations.
> 
> (also these patches will make the VPN and DHCP managers do some initializations
> much later as well, including printing out what VPN plugins there are.  I don't
> think that's a functional problem at all, but we'll have to be careful with
> singletons in the future to make sure they are OK with late initialization).
> 
> The rest looks OK to me.

I was aware of that and it's the reason, why the first version of the branch did not remove calling the getter early inside main().

But do you see an actual problem with it that needs fixing?

If this change causes problems, then it merely points out a bug that was already there.
If a singleton needs some warm-up time, then it must be implemented in a way to handle requests that come early. For example, NMAuthManager transparently queues early requests in nm_auth_manager_polkit_authority_check_authorization().

We could add an explicit _init() call for the singletons that we want to initialize early. Eg, have main() call nm_vpn_manager_init(), which will actually create the singleton, and nm_vpn_manager_get() would just return it.

(In reply to comment #5)
> We could add an explicit _init() call for the singletons that we want to
> initialize early. Eg, have main() call nm_vpn_manager_init(), which will
> actually create the singleton, and nm_vpn_manager_get() would just return it.

I'd prefer _setup(), which is also used by nm_auth_manager_setup(), nm_linux_platform_setup(). Branch th/rh1066697_reload_config will add nm_config_setup() too.

In the past we often used _new(), (for example currently there is nm_config_new()). I really dislike _new() functions that do 
»···g_assert (!singleton);
»···singleton = NM_CONFIG (g_object_new (NM_TYPE_CONFIG, NULL));



anyway, is this branch good to go?

A singleton must appear to be ready when it is returned by _get().

If it needs some initialization, it must either initialize synchronously(!) (in _get() or _setup()) or it must transparently proxy any requests until it is ready.

Otherwise every caller of _get() would have to check, if the singleton is still initializing. Leaving the complexity to the caller.

Not that I like the extensive usage of macros. But it simplifies the handling here.
I am fine with the branch.

Merged as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=74a2c64060c9fc2f540d1aa5614343768933db5f