th/nm-default-bgo752857

Created attachment 308391 [details] [review]
[PATCH] fixup! all: include internal headers with quotes

Looks good to me, only some nitpicks:

> core: order destruction of singleton instances

    This is only a crude method to get the lifetime of singleton instances
    right by default. Having singeltons ref other singletons to keep them

s/singeltons/singletons/

+inline void __attribute__((destructor))
+_nm_singleton_instance_destroy (void)

static void? or is inline needed in this case?

Also pushed a trivial fixup and added a patch as attachment (I didn't push a fixup for it because it has conflicts with following commits).

(In reply to Beniamino Galvani from comment #2)
> Created attachment 308391 [details] [review] [review]
> [PATCH] fixup! all: include internal headers with quotes
> 
> Looks good to me, only some nitpicks:
> 

Thanks. Fixed after your comments and included attachment 308391 [details] [review].

Rebased and repushed.

>+		if (instance->ref_count > 1)
>+			nm_log_dbg (LOGD_CORE, "disown %s singleton (%p)", G_OBJECT_TYPE_NAME (instance), instance);

I know this is in the old code too, but I'm not sure why; what is it supposed to be telling you?

>+#define NM_DEFINE_SINGLETON_REGISTER_DESTRUCTION TRUE

This seems unnecessary...

And I don't think you actually need nm_singleton_instance_register_destruction(). (Eg, compare against my version in bug 622927; though I should have made nm_singletons_destroy() be a destructor rather than being called explicitly.)



>+#if defined (NETWORKMANAGER_COMPILATION) && NETWORKMANAGER_COMPILATION == 2
>+
>+/* the header is used inside src/, where additional
>+ * headers are available. */

That's non-obvious. It might be better to have a different or additional define...

Related to that; if you knew whether the current directory was a library directory or a daemon/client directory, then you could automatically include either <glib/gi18n-lib.h> or <glib/gi18n.h> as appropriate.

(In reply to Dan Winship from comment #4)
> >+#define NM_DEFINE_SINGLETON_REGISTER_DESTRUCTION TRUE
> 
> This seems unnecessary...

Yeah, I don't see why we'd need both ways?

> And I don't think you actually need
> nm_singleton_instance_register_destruction(). (Eg, compare against my
> version in bug 622927; though I should have made nm_singletons_destroy() be
> a destructor rather than being called explicitly.)

Agreed; shouldn't we just be destroying all the singletons we create at exit, instead of having them register themselves?

> >+#if defined (NETWORKMANAGER_COMPILATION) && NETWORKMANAGER_COMPILATION == 2
> >+
> >+/* the header is used inside src/, where additional
> >+ * headers are available. */
> 
> That's non-obvious. It might be better to have a different or additional
> define...

How about NETWORKMANAGER_DAEMON_COMPILATION?  I'd rather have a different/additional define than overloading this one.

The rest of the patches LGTM.

(In reply to Dan Winship from comment #4)
> >+		if (instance->ref_count > 1)
> >+			nm_log_dbg (LOGD_CORE, "disown %s singleton (%p)", G_OBJECT_TYPE_NAME (instance), instance);
> 
> I know this is in the old code too, but I'm not sure why; what is it
> supposed to be telling you?

The "destructor" only unrefs the singleton. Previously, without ordering of destruction, it was not at all uncommon, that this does not yet drop the ref-count to zero.
So, you sould first see a "disown" message and later "destroy" (which is ok).
OTOH, some singletons were leaked, so we would not actually ever see the "destroy" message.

Now with ordering the destruction of singletons, we would even more expect that the singletons shut down in the right order and the last reference gets droped.

If you now see the message, something is odd.


> >+#define NM_DEFINE_SINGLETON_REGISTER_DESTRUCTION TRUE
> 
> This seems unnecessary...

Dropped.


> And I don't think you actually need
> nm_singleton_instance_register_destruction(). (Eg, compare against my
> version in bug 622927; though I should have made nm_singletons_destroy() be
> a destructor rather than being called explicitly.)

You basically merged the singleton for destruction into _singleton_instance_weak_ref_cb(). I did that now too (see fixup!) and renamed the function.

Note that NMPlatform does not get registered for destruction -- as before.

How is this?


> >+#if defined (NETWORKMANAGER_COMPILATION) && NETWORKMANAGER_COMPILATION == 2
> >+
> >+/* the header is used inside src/, where additional
> >+ * headers are available. */
> 
> That's non-obvious. It might be better to have a different or additional
> define...

How about the fixup! that adds NM_NETWORKMANAGER_COMPILATION_INSIDE_DAEMON ?



> Related to that; if you knew whether the current directory was a library
> directory or a daemon/client directory, then you could automatically include
> either <glib/gi18n-lib.h> or <glib/gi18n.h> as appropriate.

Added commit "nm-default: include i18n headers via "nm-default.h""




Repushed.

(In reply to Dan Williams from comment #5)
> (In reply to Dan Winship from comment #4)
> > >+#define NM_DEFINE_SINGLETON_REGISTER_DESTRUCTION TRUE
> > 
> > This seems unnecessary...
> 
> Yeah, I don't see why we'd need both ways?

Dropped.

> > And I don't think you actually need
> > nm_singleton_instance_register_destruction(). (Eg, compare against my
> > version in bug 622927; though I should have made nm_singletons_destroy() be
> > a destructor rather than being called explicitly.)
> 
> Agreed; shouldn't we just be destroying all the singletons we create at
> exit, instead of having them register themselves?

Addressed in fixup commit? Better?

> > >+#if defined (NETWORKMANAGER_COMPILATION) && NETWORKMANAGER_COMPILATION == 2
> > >+
> > >+/* the header is used inside src/, where additional
> > >+ * headers are available. */
> > 
> > That's non-obvious. It might be better to have a different or additional
> > define...
> 
> How about NETWORKMANAGER_DAEMON_COMPILATION?  I'd rather have a
> different/additional define than overloading this one.

It's still only one define, with named flags. How is that?

(In reply to Thomas Haller from comment #6)
> (In reply to Dan Winship from comment #4)
> > >+		if (instance->ref_count > 1)
> > >+			nm_log_dbg (LOGD_CORE, "disown %s singleton (%p)", G_OBJECT_TYPE_NAME (instance), instance);

> If you now see the message, something is odd.

OK, so it should probably be reworded to indicate that?

>+#if (NETWORKMANAGER_COMPILATION) & NM_NETWORKMANAGER_COMPILATION_INSIDE_DAEMON

That looks weird...

I don't think you need a bitfield. At the moment, we only want to distinguish daemon, libraries, and "other", which are mutually exclusive.

> nm-default: include i18n headers via "nm-default.h"

pushed three fixups to danw/nm-default-bgo752857

Why aren't we destroying the NMPlatform instance on exit?  That seems to be the only reason for the register_destruction() stuff.  From a quick look NMPlatform itself doesn't care about destruction, and NMLinuxPlatform doesn't care much.  By this point all listeners should have unregistered their signal handlers, and the Platform should probably be the last thing destroyed since it doesn't depend on any other objects.

danw's fixups look OK to me.

(In reply to Dan Winship from comment #8)
> (In reply to Thomas Haller from comment #6)
> > (In reply to Dan Winship from comment #4)
> > > >+		if (instance->ref_count > 1)
> > > >+			nm_log_dbg (LOGD_CORE, "disown %s singleton (%p)", G_OBJECT_TYPE_NAME (instance), instance);
> 
> > If you now see the message, something is odd.
> 
> OK, so it should probably be reworded to indicate that?

Reworded.


> >+#if (NETWORKMANAGER_COMPILATION) & NM_NETWORKMANAGER_COMPILATION_INSIDE_DAEMON
> 
> That looks weird...

Changed to ==


> I don't think you need a bitfield. At the moment, we only want to
> distinguish daemon, libraries, and "other", which are mutually exclusive.
> 
> > nm-default: include i18n headers via "nm-default.h"
> 
> pushed three fixups to danw/nm-default-bgo752857

Thanks, I took them, and removed your branch. Rebased all to master.



(In reply to Dan Williams from comment #9)
> Why aren't we destroying the NMPlatform instance on exit?

Correct, when unifying several singleton implementations (by adding NM_SINGLETON_DEFINE_GETTER()), we did not do everything.
 - NMManager is still defined differently
 - we leak several singletons, although we defined a destructor.
 - NMPlatform is known not to destroy.

It's todo, but not that urgent either.


> That seems to be
> the only reason for the register_destruction() stuff. 

Not really. It is the reason for the "gboolean register_destruction" argument of nm_singleton_instance_register().

Do you want to drop _nm_singleton_instance_register_destruction() altogether? How should that be? 



> From a quick look
> NMPlatform itself doesn't care about destruction, and NMLinuxPlatform
> doesn't care much.  By this point all listeners should have unregistered
> their signal handlers, and the Platform should probably be the last thing
> destroyed since it doesn't depend on any other objects.

We still leak some singletons. Let's fix that first, before destroying NMPlatform too. danw/gdbus-bgo622927 has some relevant changes for NMManager there...

lgtm

> Correct, when unifying several singleton implementations (by adding
> NM_SINGLETON_DEFINE_GETTER()), we did not do everything.
>  - NMManager is still defined differently
>  - we leak several singletons, although we defined a destructor.
>  - NMPlatform is known not to destroy.
> 
> It's todo, but not that urgent either.
> 
> 
> > That seems to be
> > the only reason for the register_destruction() stuff. 
> 
> Not really. It is the reason for the "gboolean register_destruction"
> argument of nm_singleton_instance_register().
> 
> Do you want to drop _nm_singleton_instance_register_destruction()
> altogether? How should that be? 
> 
> 
> 
> > From a quick look
> > NMPlatform itself doesn't care about destruction, and NMLinuxPlatform
> > doesn't care much.  By this point all listeners should have unregistered
> > their signal handlers, and the Platform should probably be the last thing
> > destroyed since it doesn't depend on any other objects.
> 
> We still leak some singletons. Let's fix that first, before destroying
> NMPlatform too. danw/gdbus-bgo622927 has some relevant changes for NMManager
> there...

Added another fixup, removing the @register_destruction singleton. NMPlatform is still leaked, but there is a FIXME comment.
Other advantage: we log now a line: "disown NMPlatform instance" which indicates the leakage too.

LGTM

merged to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=796ec5c038abd07a0fecd52794bb9ec4e6b3d11d