This request was not resolved in time for the current release.
Red Hat invites you to ask your support representative to
propose this request, if still desired, for consideration in
the next release of Red Hat Enterprise Linux.

Pushed a branch th/rh1066697_reload_config for review.

This does *not* implement reloading of dns-configuration.

It adds some ground work to NMConfig to enable reloading.

As a show-case, I implemented reloading of the [connectivity] section.

> config: fix leaked GError when no default config file exists
> config: replace g_warning() by nm_log_warn()

I think these could be committed right away if you wanted



> config: add nm_config_setup() to initialize config singleton

I would keep nm_config_new() as it was (for consistency with all of the other singletons NM creates), and rename your new nm_config_new() to nm_config_new_internal() or something like that (and it doesn't need to be publicly visible).

>+	NMConfigOptions cli;

If "NMConfigOptions" is specifically for carrying option values passed in on the command line, then having something about that in its name would be good. "NMConfigCommandLineOptions" or whatever.

>+	/* These three are hidden for now, and should eventually just go away. */

Now might be a good time for that...



> config: add support for reloading of configuration

This commit is very confusing, since the code doesn't make any sense without the body of the function being there. (Particularly the fact that you create "new" and then immediately destroy it.)

Maybe just adding a "FIXME" comment explaining how it will work later would be enough.


> connectivity: fix early release of self pointer in nm_connectivity_check_cb()

FWIW, this is not actually a bug; g_async_result_get_source_object() refs the source object before returning it,even though the GAsyncResult is holding a ref on the source object itself. So it's 100% safe to unref self at that point, and it ensures that we don't forget to unref it later. (Basically, g_async_result_get_source_object() was designed badly, and we have to deal with it.) Anyway, your patch doesn't break anything, but it's not needed either. Maybe we could just add a comment "/* drop the unnecessary extra ref that get_source_object() added */" or something?



> connectivity: make NM_CONNECTIVITY_URI property immutable (G_PARAM_CONSTRUCT)

>-		                      G_PARAM_READWRITE |
>+		                      G_PARAM_READWRITE | G_PARAM_CONSTRUCT |
> 		                      G_PARAM_STATIC_STRINGS));

I'm trying to move toward one G_PARAM option per line, so add a newline?



> connectivity: ensure that parameters of asynchronous functions don't change

Would it be simpler to just cancel any in-progress check if the config changes?

>+	void **u = user_data;
>+	GSimpleAsyncResult *simple = u[0];
>+	ConfigData *data = u[1];

Eww!

You can store the ConfigData on the GSimpleAsyncResult using g_object_set_data() if you want. That would be a little nicer.

>+	if (response && !response)
>+		response = NULL;

presumably should be "!*response", but would it be better to normalize it the other way around, since you keep wanting it to be "" rather than NULL in nm_connectivity_check_cb()?



> config: implement reloading of connectivity parameters

It seems like NMConfig::config-changed's "changes" argument only needs to be a flags, not a HashTable...

OTOH, I'm not sure the other kinds of changes will break down into neat groups like "connectivity", so it might make more sense to just make each config option be a property, and just use notify:: on them. (This would also be half of the work needed to implement a config D-Bus interface, which we've talked about...)

(In reply to Dan Winship from comment #3)
> > config: fix leaked GError when no default config file exists
> > config: replace g_warning() by nm_log_warn()
> 
> I think these could be committed right away if you wanted

Done.


> > config: add nm_config_setup() to initialize config singleton
> 
> I would keep nm_config_new() as it was (for consistency with all of the
> other singletons NM creates), and rename your new nm_config_new() to
> nm_config_new_internal() or something like that (and it doesn't need to be
> publicly visible).

On a quick look I cannot find other singletons that follow that (IMO awkward) pattern as NMConfig does.

For most singletons you only call nm_x_get(). In this case it is not possible because we need to pass additional parameters. Having a init-singleton function called nm_x_new() is confusing and not consistent with all our other nm_y_new() functions which actually do what the name promisses.

I think nm_config_setup() is a suitable name. Maybe nm_config_init(), nm_config_get_init(), etc.

Having nm_config_new() exposed could be useful for tests. Currently the tests unref the singleton and call nm_config_init() again...
Removing nm_config_new() from the header file seems like little win.

With this change all functions except nm_config_setup() and nm_config_get() work independent of the singleton instance.


> >+	NMConfigOptions cli;
> 
> If "NMConfigOptions" is specifically for carrying option values passed in on
> the command line, then having something about that in its name would be
> good. "NMConfigCommandLineOptions" or whatever.


Renamed.



> >+	/* These three are hidden for now, and should eventually just go away. */
> 
> Now might be a good time for that...

Maybe not? It doesn't hurt that much. I dislike changing behaviour (even for upcoming 1.0)


> > config: add support for reloading of configuration
> 
> This commit is very confusing, since the code doesn't make any sense without
> the body of the function being there. (Particularly the fact that you create
> "new" and then immediately destroy it.)
> 
> Maybe just adding a "FIXME" comment explaining how it will work later would
> be enough.
> 
> 
> > connectivity: fix early release of self pointer in nm_connectivity_check_cb()
> 
> FWIW, this is not actually a bug; g_async_result_get_source_object() refs
> the source object before returning it,even though the GAsyncResult is
> holding a ref on the source object itself. So it's 100% safe to unref self
> at that point, and it ensures that we don't forget to unref it later.
> (Basically, g_async_result_get_source_object() was designed badly, and we
> have to deal with it.) Anyway, your patch doesn't break anything, but it's
> not needed either. Maybe we could just add a comment "/* drop the
> unnecessary extra ref that get_source_object() added */" or something?

added code comment.


> > connectivity: make NM_CONNECTIVITY_URI property immutable (G_PARAM_CONSTRUCT)
> 
> >-		                      G_PARAM_READWRITE |
> >+		                      G_PARAM_READWRITE | G_PARAM_CONSTRUCT |
> > 		                      G_PARAM_STATIC_STRINGS));
> 
> I'm trying to move toward one G_PARAM option per line, so add a newline?


done



> > connectivity: ensure that parameters of asynchronous functions don't change
> 
> Would it be simpler to just cancel any in-progress check if the config
> changes?
>
> 
> >+	void **u = user_data;
> >+	GSimpleAsyncResult *simple = u[0];
> >+	ConfigData *data = u[1];
> 
> Eww!
> 
> You can store the ConfigData on the GSimpleAsyncResult using
> g_object_set_data() if you want. That would be a little nicer.

I dislike g_object_set_data() because the keys are strings, that are not private. You are left to choose a "key-that-nobody-else-chooses".
Yes, this is not a real world problem, because we can simply ensure that this clash does not happen. I just dislike it.

Note that I do not dislike @u too much, because by looking at the code I can easily see what it does and verify correctness. Would you prefer a declared struct?


Actuall thy, first when I started addingis ref-counted ConfigData, I did not fully realize that the only place we need to ref it is nm_connectivity_check_cb().

So yes, instead we could

(1) as you said, cancel in-progess async action. All this async stuff in general is not trivial either. So, adding cancelling might be significantly less code, but still complex.

(2) create a copy of the 3 parameters for every async call and pass them on in @u/g_object_set_data()/struct. Downside: copy strings on every call.


I quite like immutable, refcounted types. I also like the current version. Downside of current version is that it adds lots of code, we could do it shorter.


Opinions?




> >+	if (response && !response)
> >+		response = NULL;
> 
> presumably should be "!*response", but would it be better to normalize it
> the other way around, since you keep wanting it to be "" rather than NULL in
> nm_connectivity_check_cb()?

I considered that before. But I think it's better to keep it NULL. It's clearer to mean "not-set".


> > config: implement reloading of connectivity parameters
> 
> It seems like NMConfig::config-changed's "changes" argument only needs to be
> a flags, not a HashTable...

I think a hashtable is useful later for [1] (see below).

> OTOH, I'm not sure the other kinds of changes will break down into neat
> groups like "connectivity", so it might make more sense to just make each
> config option be a property, and just use notify:: on them. (This would also
> be half of the work needed to implement a config D-Bus interface, which
> we've talked about...)

I think it's best to have *one* atomic config-changed signal that tells you all you need to know.

For example, I dislike the following:

"connectivity" consists of 3 properties that are related. So you subscribe to all 3 notify:: signals. You receive the first signal. What to do? Do now all 3 properties have consistently the new value? Or should you wait until you get the last notification? How would you know that you have a consistent state? And even if NMConfig guarantees you to only raise the 3 notify signals after setting all 3 properties in one step, you will receive 3 signals, but the last 2 ones don't actually change anything.

Note that later there might be cases where the same problem arises for parameters that are not so tightly related as connectivity. E.g. suppose we support reloading of connection.dns and connection.dhcp. Some listener might actually need both properties together in a consistent state.



[1] Another thing is, how will we support reloading some opaque values? E.g. there is no function/glib-property for nm_config_get_keyfile_hostname() (keyfile.hostname).

Sidenote: currently keyfile parses the config-file itself and ignores /etc/NM/conf.d/ (OMG)... keyfile should instead use nm_config_get_value().

By passing a @changes hashset (not flags) with the config-changed signal, we can explicitly provide information which groups/keys changed. This would be sufficient to notify about config-changes of opaque values.

But still opaque values would need a way to hook NMConfig to merge the relevant parts of @new. I think that should be doable nicely. E.g. have keyfile subscribe to NMConfig and takes responsibility of merging the keyfile-related opaque values, e.g. by obtaining @current and @new, it selects the values that should be updated/removed.

(In reply to Thomas Haller from comment #4)
> > I would keep nm_config_new() as it was (for consistency with all of the
> > other singletons NM creates), and rename your new nm_config_new() to
> > nm_config_new_internal() or something like that (and it doesn't need to be
> > publicly visible).
> 
> On a quick look I cannot find other singletons that follow that (IMO
> awkward) pattern as NMConfig does.

Hm... I guess it's just NMConfig and NMManager; all of the others don't require any initial setup args, so main() just uses the _get() function like everything else does.

> > >+	/* These three are hidden for now, and should eventually just go away. */
> > 
> > Now might be a good time for that...
> 
> Maybe not? It doesn't hurt that much. I dislike changing behaviour (even for
> upcoming 1.0)

I consider it to be a bug that those options ever existed, so I would like to get rid of them.

> Note that I do not dislike @u too much, because by looking at the code I can
> easily see what it does and verify correctness. Would you prefer a declared
> struct?

yes (assuming you stick with this approach)

> (1) as you said, cancel in-progess async action. All this async stuff in
> general is not trivial either. So, adding cancelling might be significantly
> less code, but still complex.

Yeah, I'm not sure how complicated it would be.

> (2) create a copy of the 3 parameters for every async call and pass them on
> in @u/g_object_set_data()/struct. Downside: copy strings on every call.

I agree that copying the strings every time seems pointless, since they'll almost never change. Although it has the advantage of being very simple...

> I think it's best to have *one* atomic config-changed signal that tells you
> all you need to know.
> 
> For example, I dislike the following:
> 
> "connectivity" consists of 3 properties that are related. So you subscribe
> to all 3 notify:: signals. You receive the first signal. What to do? Do now
> all 3 properties have consistently the new value? Or should you wait until
> you get the last notification? How would you know that you have a consistent
> state?

You would know that you have a consistent state because NMConfig would have called g_object_freeze_notify(), updated and notified all the properties, and then called g_object_thaw_notify(). So when you receive the first notification, all three properties would have been updated.

> And even if NMConfig guarantees you to only raise the 3 notify
> signals after setting all 3 properties in one step, you will receive 3
> signals, but the last 2 ones don't actually change anything.

But your code already deals with the possibility that the signal will be emitted even though nothing changed...

> Sidenote: currently keyfile parses the config-file itself and ignores
> /etc/NM/conf.d/ (OMG)... keyfile should instead use nm_config_get_value().

yes

> But still opaque values would need a way to hook NMConfig to merge the
> relevant parts of @new. I think that should be doable nicely. E.g. have
> keyfile subscribe to NMConfig and takes responsibility of merging the
> keyfile-related opaque values, e.g. by obtaining @current and @new, it
> selects the values that should be updated/removed.

All this "merging" just seems like an unnecessary complication. NMConfig could just clear out its old values, read in the new ones, and then emit "config-changed". And then, eg, NMConnectivity already has the old values cached, so it can just compare the current NMConfig values against those ones to see if anything changed. And any code that just calls nm_config_get_*() every time rather than caching the values doesn't even need to listen for the :config-changed signal.

(In reply to Dan Winship from comment #5)
> (In reply to Thomas Haller from comment #4)
> > > I would keep nm_config_new() as it was (for consistency with all of the
> > > other singletons NM creates), and rename your new nm_config_new() to
> > > nm_config_new_internal() or something like that (and it doesn't need to be
> > > publicly visible).
> > 
> > On a quick look I cannot find other singletons that follow that (IMO
> > awkward) pattern as NMConfig does.
> 
> Hm... I guess it's just NMConfig and NMManager; all of the others don't
> require any initial setup args, so main() just uses the _get() function like
> everything else does.

now I feel an urge to "fix" NMManager too... :)


> > > >+	/* These three are hidden for now, and should eventually just go away. */
> > > 
> > > Now might be a good time for that...
> > 
> > Maybe not? It doesn't hurt that much. I dislike changing behaviour (even for
> > upcoming 1.0)
> 
> I consider it to be a bug that those options ever existed, so I would like
> to get rid of them.

I do agree. But I tend to keep them. Other opinions?


> > Note that I do not dislike @u too much, because by looking at the code I can
> > easily see what it does and verify correctness. Would you prefer a declared
> > struct?
> 
> yes (assuming you stick with this approach)
> 
> > (1) as you said, cancel in-progess async action. All this async stuff in
> > general is not trivial either. So, adding cancelling might be significantly
> > less code, but still complex.
> 
> Yeah, I'm not sure how complicated it would be.
> 
> > (2) create a copy of the 3 parameters for every async call and pass them on
> > in @u/g_object_set_data()/struct. Downside: copy strings on every call.
> 
> I agree that copying the strings every time seems pointless, since they'll
> almost never change. Although it has the advantage of being very simple...

I don't strongly oppose other options. I just like it best as it is now. Other opinions? Except see [1] below



> > I think it's best to have *one* atomic config-changed signal that tells you
> > all you need to know.
> > 
> > For example, I dislike the following:
> > 
> > "connectivity" consists of 3 properties that are related. So you subscribe
> > to all 3 notify:: signals. You receive the first signal. What to do? Do now
> > all 3 properties have consistently the new value? Or should you wait until
> > you get the last notification? How would you know that you have a consistent
> > state?
> 
> You would know that you have a consistent state because NMConfig would have
> called g_object_freeze_notify(), updated and notified all the properties,
> and then called g_object_thaw_notify(). So when you receive the first
> notification, all three properties would have been updated.
>
> > And even if NMConfig guarantees you to only raise the 3 notify
> > signals after setting all 3 properties in one step, you will receive 3
> > signals, but the last 2 ones don't actually change anything.
> 
> But your code already deals with the possibility that the signal will be
> emitted even though nothing changed...

right. I noticed that :)

Are you arguing that it is better to move the relevant properties over to be glib-properties, and then raise notify:: signals?



> > But still opaque values would need a way to hook NMConfig to merge the
> > relevant parts of @new. I think that should be doable nicely. E.g. have
> > keyfile subscribe to NMConfig and takes responsibility of merging the
> > keyfile-related opaque values, e.g. by obtaining @current and @new, it
> > selects the values that should be updated/removed.
> 
> All this "merging" just seems like an unnecessary complication. NMConfig
> could just clear out its old values, read in the new ones, and then emit
> "config-changed". And then, eg, NMConnectivity already has the old values
> cached, so it can just compare the current NMConfig values against those
> ones to see if anything changed. And any code that just calls
> nm_config_get_*() every time rather than caching the values doesn't even
> need to listen for the :config-changed signal.

I would not do that. "Reloading" configuration requires to take very specific steps for each parameter that you want to support reloading. You cannot just replace the internal g_key_file and expect that it works magically.

So, I would take an opt-in approach and merge exactly those parameters that we actually support.

Also, it seems wrong that NMConfig blindly swaps its config without tightly controlling it.


[1] How about NMConfig exposing an immutable gobject NMConfigSettings with those config parameters that are subject to be reloaded. NMConnectivity would not cache the connectivity parameters itself. It would simply ref the current configuration. And on the async-call it would pass on yet another ref to the NMConfigSetting.
On config-changes, nm_config_new() swaps this NMConfigSettings to a new instance and raises the changes notification.

On reload nothing in NMConfig changes, except the NMConfigSettings is swapped.

Regarding the opaque parameters, on reload listeners (e.g. keyfile) can subscribe and control the options that are included in the new NMConfigSettings.

(In reply to Thomas Haller from comment #6)
> Are you arguing that it is better to move the relevant properties over to be > glib-properties, and then raise notify:: signals?

I think we eventually want gobject properties, but not necessarily now. (And even if we had them, that doesn't automatically mean that notify:: would be a better solution than :config-changed.)

> [1] How about NMConfig exposing an immutable gobject NMConfigSettings with
> those config parameters that are subject to be reloaded.

Sure. Maybe rename NMConfig to NMConfigManager in that case...

> NMConnectivity
> would not cache the connectivity parameters itself. It would simply ref the
> current configuration. And on the async-call it would pass on yet another
> ref to the NMConfigSetting.

It doesn't need to cache the whole NMConfigSetting; it just calls nm_config_get_connectivity_interval() when it's created, and then every time the interval times out, it calls nm_config_manager_get_settings() (or whatever it's called), uses the URL from the NMConfigSettings, and passes the NMConfigSettings to the callback. The callback gets the response text from that NMConfigSettings and compares that against the response.

On :config-change (or notify::connectivity-interval or whatever), NMConnectivity just needs to call nm_config_get_connectivity_interval() again; it doesn't matter at that point whether the URL and response text changed or not.

So, again, I'm not sure we really need any complex "reloading" infrastructure. I think in any situation where an option or set of options does have complex reloading logic, it will take less code to deal with it entirely on the "consumer" end of the option than it would to have NMConfig try to be clever on its behalf.

I repushed the branch for review.


I added a class NMConfigData and nm_config_get_data(NMConfig*) returns it.

NMConfigData are those parameters that we support changing. Other then that, the NMConfig instance does not change. When reloading it only swaps the NMConfigData instance.

NMConfigData itself also does not change.


First I let NMConnectivity reference NMConfigData directly, but that was not simpler (less code). Also, it couples those two classes which is not desirable. How it is now, NMConnectivity can work just fine without NMConfig, but they can work together.


How is this?

> connectivity: add code comment to nm_connectivity_check_cb()

s/save/safe

> config: add nm_config_setup() to initialize config singleton

I guess I was thinking that NMConfigData would encapsulate all the actual data read on-disk, and be immutable.  Then like you say, NMConfig would swap the data pointer out and emit some signals.  But that means, why do we need to create an entire new NMConfig object just to re-read the data?  Couldn't NMConfigData handle parsing the command-line options, except that the options are stored by NMConfig itself to pass to the data when creating it?  My though is that NMConfigData would be the "final" combination of all the data, command-line and config file.

> trivial: fix white space and line-break in nm-connectivity.c
> connectivty: refactor converting connectivity states to string
> connectivity: fix race checking connectivity when setting online

These seem like nice cleanups, and are independent of the rest.  Maybe merge them earlier than the rest, if everyone agrees?

> connectivity: refactor handling parameters of NMConnectivity

whitespace in nm_connectivity_set_online() for _run_check_ensure_scheduled().


> config: implement reloading of connectivity parameters

It seems somewhat complicated to have three different "new" functions here... Nothing calls nm_connectivity_new() anymore, so why couldn't we just combine nm_connectivity_new() and new_with_config()?

Also, why does the connectivity need to be independent of NMConfig?  If only the singleton NMConfig is accessible to NetworkManager stuff, shouldn't there be no problem with depending on it?

(In reply to Dan Williams from comment #9)
> > connectivity: add code comment to nm_connectivity_check_cb()
> 
> s/save/safe

Done


> > config: add nm_config_setup() to initialize config singleton
> 
> I guess I was thinking that NMConfigData would encapsulate all the actual
> data read on-disk, and be immutable. Then like you say, NMConfig would swap
> the data pointer out and emit some signals.  But that means, why do we need
> to create an entire new NMConfig object just to re-read the data?  Couldn't
> NMConfigData handle parsing the command-line options, except that the
> options are stored by NMConfig itself to pass to the data when creating it? 
> My though is that NMConfigData would be the "final" combination of all the
> data, command-line and config file.

I don't see it that way. From where a certain setting originates (command line, NM.conf, conf.d/) is of not much relevance and the NMConfig API also does not expose that (currently).

Reloading configuration only makes sense in the meaning: cherry-pick the new information, with some/most properties being not actually reloadable. Reloading NMConfig should not change any non-reloadable settings.

The point is to have a reasonable API to NMConfig with regard to reloading. Now it is very simple: all that can change during reloading is NMConfigData. And it updates atomically (and you can compare with @old_data). And everything is immutable (except the NMConfig-NMConfigData relation).

I am not sure what you are suggesting.


Btw. NMConfig also has opaque values accessible via nm_config_get_value(). But I have a clear idea how the above can be extended to support reloading of such properties.

In this case, nm_config_get_value() after a reload would still return the unchanged values (NMConfig really is immutable). But we have a corresponding nm_config_data_get_value() -- which itself is immutable. This one gets populated during a reload/merge by subscribers to NMConfig which handle "populate NMConfigData with reloadable-opaque-values".


> > trivial: fix white space and line-break in nm-connectivity.c
> > connectivty: refactor converting connectivity states to string
> > connectivity: fix race checking connectivity when setting online
> 
> These seem like nice cleanups, and are independent of the rest.  Maybe merge
> them earlier than the rest, if everyone agrees?

maybe we can get this branch done soon? :)

> 
> > connectivity: refactor handling parameters of NMConnectivity
> 
> whitespace in nm_connectivity_set_online() for _run_check_ensure_scheduled().

done


> > config: implement reloading of connectivity parameters
> 
> It seems somewhat complicated to have three different "new" functions
> here... Nothing calls nm_connectivity_new() anymore, so why couldn't we just
> combine nm_connectivity_new() and new_with_config()?

I think, an unused nm_connectivity_new() would not hurt. From an aesthetic POV, I think every instantiable class should have such a *_new() function.

Anyway, I removed it. See fixup!


> Also, why does the connectivity need to be independent of NMConfig?  If only
> the singleton NMConfig is accessible to NetworkManager stuff, shouldn't
> there be no problem with depending on it?

It doesn't need to. But it decreases coupling between NMConnectivity and NMConfig. Lower coupling leads usually to better design. In this case it makes NMConnectivty usable beyond its current use (including easier testing).
Actually, in a previous attempt I tried to replaced the 3 connectivity properties by a reference to NMConfigData. That did not seem shorter nor simpler.

btw. there is also nm_config_set_ethernet_no_auto_default() which modifies NMConfig. Correctly, the no-auto-config-part should move to NMConfigData and calling nm_config_set_ethernet_no_auto_default() does a swap of NMConfigData and emits the config-changed signal.

(In reply to Thomas Haller from comment #10)
> > > config: add nm_config_setup() to initialize config singleton
> > 
> > I guess I was thinking that NMConfigData would encapsulate all the actual
> > data read on-disk, and be immutable. Then like you say, NMConfig would swap
> > the data pointer out and emit some signals.  But that means, why do we need
> > to create an entire new NMConfig object just to re-read the data?  Couldn't
> > NMConfigData handle parsing the command-line options, except that the
> > options are stored by NMConfig itself to pass to the data when creating it? 
> > My though is that NMConfigData would be the "final" combination of all the
> > data, command-line and config file.
> 
> I don't see it that way. From where a certain setting originates (command
> line, NM.conf, conf.d/) is of not much relevance and the NMConfig API also
> does not expose that (currently).
> 
> Reloading configuration only makes sense in the meaning: cherry-pick the new
> information, with some/most properties being not actually reloadable.
> Reloading NMConfig should not change any non-reloadable settings.
> 
> The point is to have a reasonable API to NMConfig with regard to reloading.
> Now it is very simple: all that can change during reloading is NMConfigData.
> And it updates atomically (and you can compare with @old_data). And
> everything is immutable (except the NMConfig-NMConfigData relation).
> 
> I am not sure what you are suggesting.

What I mean is, I'm not sure why there should ever be two NMConfig objects at the same time, like in nm_config_reload().  I believe that makes things more complicated than they need to be.  There's nothing wrong with the singleton stuff, even for the testcases.  We just need to make sure that the last reference to the singleton clears out the singleton pointer in finalize().

When reloading, we have three sources of information:

1) command-line parameters
2) loaded-from-disk configuration
3) parsed/combined configuration

#3 is only ever the combination of #1 and #2.  We already have #1 from main() and that will never change.

So what would have done is something like:

1) split out the logic in nm_config_new() that loads stuff from disk and merges in the command-line options

2) create an *internal* NMConfigData structure that contains all the actual config variable members

3) make #1 return an NMConfigData populated from disk + command line

4) add a "diff" function for NMConfigData to determine changes

5) on reload, get a new disk+CLI NMConfigData, diff that to the existing NMConfigData, swap pointers, free the old one, and emit a signal with the differences

6) listeners to NMConfig::config-changed then re-read the data they care about

I think that's simpler and flatter than the existing code, and just as flexible.  Once created, NMConfigData can still be immutable even.

> > Also, why does the connectivity need to be independent of NMConfig?  If only
> > the singleton NMConfig is accessible to NetworkManager stuff, shouldn't
> > there be no problem with depending on it?
> 
> It doesn't need to. But it decreases coupling between NMConnectivity and
> NMConfig. Lower coupling leads usually to better design. In this case it
> makes NMConnectivty usable beyond its current use (including easier testing).
> Actually, in a previous attempt I tried to replaced the 3 connectivity
> properties by a reference to NMConfigData. That did not seem shorter nor
> simpler.

I don't see a huge difference for testing between creating a small NMConfigData emulator and an NMConfig emulator, if we're going to standalone test NMConnectivity.  We already do that for NMDevice in at least one place, and it's an established pattern elsewhere too.  I think having two objects complicates things more than it should.

> config: add nm_config_setup() to initialize config singleton

I'd rather pass the whole GOptionContext to nm_config_cmd_line_options_create_entires() and rename that to *_add_entries().

(In reply to Dan Williams from comment #12)
> (In reply to Thomas Haller from comment #10)
> > > Also, why does the connectivity need to be independent of NMConfig?  If only
> > > the singleton NMConfig is accessible to NetworkManager stuff, shouldn't
> > > there be no problem with depending on it?
> > 
> > It doesn't need to. But it decreases coupling between NMConnectivity and
> > NMConfig. Lower coupling leads usually to better design. In this case it
> > makes NMConnectivty usable beyond its current use (including easier testing).
> > Actually, in a previous attempt I tried to replaced the 3 connectivity
> > properties by a reference to NMConfigData. That did not seem shorter nor
> > simpler.
> 
> I don't see a huge difference for testing between creating a small
> NMConfigData emulator and an NMConfig emulator, if we're going to standalone
> test NMConnectivity.  We already do that for NMDevice in at least one place,
> and it's an established pattern elsewhere too.  I think having two objects
> complicates things more than it should.

I cannot follow. Which two objects are you referring to?



> > config: add nm_config_setup() to initialize config singleton
> 
> I'd rather pass the whole GOptionContext to
> nm_config_cmd_line_options_create_entires() and rename that to
> *_add_entries().

done.

(In reply to Thomas Haller from comment #13)
> (In reply to Dan Williams from comment #12)
> > (In reply to Thomas Haller from comment #10)
> > > > Also, why does the connectivity need to be independent of NMConfig?  If only
> > > > the singleton NMConfig is accessible to NetworkManager stuff, shouldn't
> > > > there be no problem with depending on it?
> > > 
> > > It doesn't need to. But it decreases coupling between NMConnectivity and
> > > NMConfig. Lower coupling leads usually to better design. In this case it
> > > makes NMConnectivty usable beyond its current use (including easier testing).
> > > Actually, in a previous attempt I tried to replaced the 3 connectivity
> > > properties by a reference to NMConfigData. That did not seem shorter nor
> > > simpler.
> > 
> > I don't see a huge difference for testing between creating a small
> > NMConfigData emulator and an NMConfig emulator, if we're going to standalone
> > test NMConnectivity.  We already do that for NMDevice in at least one place,
> > and it's an established pattern elsewhere too.  I think having two objects
> > complicates things more than it should.
> 
> I cannot follow. Which two objects are you referring to?

NMConfig and NMConfigData.

I had proposed that NMConfigData was private to NMConfig itself, and thus NMConnectivity only had access to the NMConfig object.  I understood your objection to that proposal to be that having NMConnectivity coupled to NMConfig is better, and it makes NMConnectivity testing easier.

My response is that I think having two objects for the configuration bits (NMConfig and NMConfigData) is more complicated than any testing code for NMConnectivity would be.  There are simple encapsulated ways of handling standalone testing for NMConnectivity, if we should choose to do that.

I think it's a simpler API to just have a singleton NMConfig with GObject properties for the various configuration settings, than it is to have both NMConfig *and* a separate NMConfigData object that other stuff like NMConnectivity has to use.  NMConnectivity already has to watch NMConfig, so it seems like the data should just come from there.  If no external object ever uses NMConfigData outside of the function that it calls nm_config_get_data() in, why does the extra object need to exist externally to NMConfig?

I think there is a misunderstanding of *why* I think this should be this way:


I see NMConfig as a simple Key-Value-Store (with some accessors).

All this is about about having a convenient interface to NMConfig. NMConfig should be immutable (except its NMConfigData instance -- which by itself is immutable again).

(actually, NMConfig is not immutable as of now, consider that another bug).

Immutable data structures are the greatest thing ever. When using NMConfig you know that it will not change its properties when re-reading them later. It is a pleasant interface for a (reloadable) data-store.

It is also about atomicity: a reload will *atomically* swap the NMConfigData reference in once step.

(your suggestion with having mutable properties on NMConfig gives an ugly interface when changing several properties and then raising notify signals one by one. Yes, you can freeze/thaw the signals, but for a consumer who cares about a *set* of properties, this is still inconvenient. And the interface itself is not atomic (it is only a promise that we will raise the changes signals after doing all changes together).

Did you see how nice the config-changed signal? You get the previous NMConfigData, and the new one. And it changed atomically.

A listener who does not support reloading can keep a reference to the NMConfigData and use that one instead!

The interface makes it very clear which properties are subject to reload and which are not.


NMConfigData is not at all about where the setting comes from (commandline, NM.conf, conf.d/). We hardly care about that part.


Could it be done differently? Sure. But it wouldn't be so damn neat.





> I understood your objection to that proposal to be that having 
> NMConnectivity coupled to NMConfig is better, and it makes NMConnectivity 
> testing easier.

Having NMConnectivity coupled is worse, not better. And has no upsides.

This is an entirely unrelated discussion:

Why does NMConnectivity have a copy of the 3 properties (uri,resp,interval), when it simply could reference NMConfig/NMConfigData (or call nm_config_get()) and use the properties there. Yes it could, but that does not simplify anything.

If NMConnectivity would use many parameters from NMConfig, the situation would quite likely be different! But as it is, it's simpler to just copy the 3 parameters.

I *did* that alternative before, and there are two reasons why that was no win:
- I want NMConfig to be the plain settings (i.e. I did not want to do URI validation in NMConfig, it should return the unaltered setting from the config). So, whenever accessing nm_config_get_connectivity_uri() I had to re-validate needlessly the property.
- Depending on which configuration options changes, we have to start/stop connectivity checking and/or reschedule the interval. That meant you have to keep track what was going on before, so that you can react correctly on config-change.

Both points are doable (obviously). But it was actually simpler, to decouple both. Now NMConnectivity works perfectly alone, and on config-change I reset all properties. Decoupled. No downsides.




But all this has nothing to do with NMConfigData private or not.

It is certainly not about testing either. Creating a stub for NMConfig is trivial either way -- especially it is trivial to stub NMConfigData (because NMConfigData is just a plain object with settings). But still that is no argument *for* coupling NMConnectivty to NMConfig.

And when NMConnectivity accesses its parameters, it does not matter whether it calls: nm_config_get_conn_uri(nm_config_get()) or nm_config_data_get_conn_uri(nm_config_get_data(nm_config_get()).

Branch rebased on current master

So code speaks louder than words I think, so I pushed dcbw/config with what I would have done here, heavily based off your work though.

It keeps NMConfigData, and NMConfigData is still immutable once created.  NMConfig has the same external API in my branch as your branch (at least for consumers like NMConnectivity).

Some notable differences:

1) command-line options are stored in an NMConfigData object which lives as long as NMConfig does, is never changed, and is immutable

2) there are two stored NMConfigData objects: (a) command-line immutable overrides, and (b) config read from disk merged with command-line overrides at construct time. Still once an NMConfigData has been created, it is never changed and cannot ever be changed.

3) NMConfigData no longer needs a reference (even a weak one) to NMConfig; there's no reason for that

4) When reloading, instead of creating a new NMConfig object (which was one of my main objects to this branch in the first place) a new *NMConfigData* object is created and swapped for the old data if it has changed

5) nm_config_data_diff() function returns a GHashTable of changed keys to be passed to listeners in NMConfig::config-changed

dcbw/config:

> config: add most properties to NMConfigData

reiterating my earlier comment, I think we should drop the command-line connectivity options rather than porting them over to the new framework. We've never claimed to guarantee API compatibility of the command line, and 1.0 is a fine time to drop those options.


> config: add new config data method that merges other config

The .c definition calls the log args "override_log_level" and "override_log_domain", but the .h prototype leaves out the "override". I think it's better with "override".

The behavior between properties is inconsistent:

  - For plugins, dhcp-client, dns-mode, debug, connectivity-uri, and
    connectivity-response; if it is set to any value in @override, then we use
    that value, otherwise we use the value from @keyfile.

    (except that the dns-mode part has a cut-and-paste error and actually
    checks if dhcp-client is set, not if dns-mode is set)

  - For monitor-connection-files, if it is set to TRUE in @override, then
    we use that value, otherwise we use the value from @keyfile, even if
    @override explicitly set it to FALSE. Likewise for connectivity-interval,
    we obey @override unless it's explicitly 0, in which case we ignore it.

  - For log-level and log-domain, we don't look at @override at all

(I guess, as it happens, none of this matters, but that means that the function is much less generic then it looks, and probably ought to just be nm_config_data_new_from_config_file_and_command_line_options() or whatever. I assumed originally that the @override arg was going to be used to handle the merging-multiple-keyfiles-together part of NMConfig initialization...)


> config: add most properties to NMConfigData

The --debug command line argument is not the same thing as the main.debug config option.


> core: add handler for supported signals

This reminds me that we ought to port to g_unix_signal_add(), and then we can probably get rid of the whole nm-posix-signals stuff too.

>+_handle_signal (gpointer user_data)
>+{
>+	int signo = GPOINTER_TO_INT (user_data);
>+
>+	g_assert (signo == SIGHUP);

If we're going to assert that the signal is SIGHUP, then the function should be called "handle_sighup" or something.

>+	nm_log_info (LOGD_CORE, "caught signal %d, not supported yet.", signo);

"caught SIGHUP..."


> config: add support for reloading of configuration

> (heavily modified by dcbw)

If it's "heavily" modified then I'd --reset-author and say "based on a patch by thaller"

>+GHashTable *
>+nm_config_data_diff (NMConfigData *self, NMConfigData *other)

I still think a hash table is overkill...

>+	if (g_strcmp0 (priv->log.domains, other_priv->log.domains))
>+		CHANGED (LOG_LEVEL);

CHANGED (LOG_DOMAIN)

>+nm_config_reload (NMConfig *self)

So just to make sure I'm following along correctly, the semantics are that values specified on the command line (except for --log-level and --log-domain?) continue to override the conf files, right?

That seems correct, although also potentially confusing, since NM may have been running for a long time and the user might not remember what command line options had been specified. It would be cool if NMConfig could warn if an option changed in the config files, but was being ignored because of the command line options...

Similar-ish-ly, what if the user calls SetLogging, and then reloads the config? Which value of log-level/log-domain should be kept? I can see arguments for doing things various ways, but I think the simplest is to just do something simple, and log a message if that might not be what the user intended.

(In reply to Dan Williams from comment #17)
> So code speaks louder than words I think, so I pushed dcbw/config with what
> I would have done here, heavily based off your work though.

I rebased both branches to current master.

now:

origin/th/rh1066697_reload_config
origin/th/rh1066697_reload_config-dcbw


previous version are there for backup as well (with suffix "-1").





I don't agree with dcbw/config. But let's discuss that on IRC, too much was written already.

Note that my last comment/question on the dcbw/config review (about deciding whether command-line args still override config files after a reload) applies to th/rh1066697_reload_config too.

Updated again:

Now we have:

original branches after comment 17:
  - th/rh1066697_reload_config-1
  - th/rh1066697_reload_config-dcbw-1

branches after comment 19:
  - th/rh1066697_reload_config-2
  - th/rh1066697_reload_config-dcbw-2

th/rh1066697_reload_config-2 rebased on current master:
  - th/rh1066697_reload_config

Rebased on master. Added new commits that rework NMConfig.

I know, some of the later patches rework earlier patches.
So creates nm_config_reload() first a new NMConfig instance, later it is reworked only to create a new NMConfigData instance.

I'd rather not spend that time in beautifying the git-history. (The patches are not *that* terrible). Is that acceptable?

> config: add nm_config_setup() to initialize config singleton

I don't think we need nm_config_try_get(), because even though it's called from a signal handler, those handlers don't ever get set up until long after the NMConfig singleton is created.

> config: add new NMConfigData class

I don't think we need to keep priv->config (and the weak ref) around anymore, since there's only ever one NMConfig object?  Thus NMConfigData->config will always be the same as the config singleton.

> config: use NMConfigData in NMConfig

Could we rename config_data0 -> override_config_data or something like that, which indicates that since it came from CLI stuff it overrides any config files?

> connectivity: refactor handling parameters of NMConnectivity

soap_uri -> soup_uri?


More review to come...  but I like this branch a lot better!

(In reply to Dan Williams from comment #23)
> > config: add nm_config_setup() to initialize config singleton
> 
> I don't think we need nm_config_try_get(), because even though it's called
> from a signal handler, those handlers don't ever get set up until long after
> the NMConfig singleton is created.

removed

> > config: add new NMConfigData class
> 
> I don't think we need to keep priv->config (and the weak ref) around
> anymore, since there's only ever one NMConfig object?  Thus
> NMConfigData->config will always be the same as the config singleton.

removed

> > config: use NMConfigData in NMConfig
> 
> Could we rename config_data0 -> override_config_data or something like that,
> which indicates that since it came from CLI stuff it overrides any config
> files?

config_data0 is not at all about about CLI or where the configuration comes from. The origin of a configuration property (CLI, conf.d, or NM.conf) is not tracked and always lost.

config_data0 is the very first (original) configuration, after calling nm_config_setup(). It is a snapshot of the original configuration. Unless users support reloading of configuration and handle it appropriately, they should completely ignore nm_config_get_data() and only look at nm_config_get_data0().

For example, ifupdown does not support reloading of the ifupdown.managed setting:
 value = nm_config_data_get_value (nm_config_get_data0 (nm_config_get ()),
                                   IFUPDOWN_KEY_FILE_GROUP, 
                                   IFUPDOWN_KEY_FILE_KEY_MANAGED,
                                   &error);


> > connectivity: refactor handling parameters of NMConnectivity
> 
> soap_uri -> soup_uri?

fixed.

Rebased on master. The connectivity patches were heavily changed, after conflicts with new patches on master.

(In reply to Thomas Haller from comment #24)
> (In reply to Dan Williams from comment #23)
> > > config: use NMConfigData in NMConfig
> > 
> > Could we rename config_data0 -> override_config_data or something like that,
> > which indicates that since it came from CLI stuff it overrides any config
> > files?
> 
> config_data0 is not at all about about CLI or where the configuration comes
> from. The origin of a configuration property (CLI, conf.d, or NM.conf) is
> not tracked and always lost.
> 
> config_data0 is the very first (original) configuration, after calling
> nm_config_setup(). It is a snapshot of the original configuration. Unless
> users support reloading of configuration and handle it appropriately, they
> should completely ignore nm_config_get_data() and only look at
> nm_config_get_data0().

Ah, I didn't get that at first.  That seems OK then.


> connectivity: refactor handling parameters of NMConnectivity

The g_object_notify() calls in _set_property_*** are redundant I think, because they are called from set_property(), and any time something is set via set_property() glib will automatically issue a notification (yes, even if it's set to the same value again, but whatever).  Plus GObject will queue them so that they get emitted after all property updates are done.

It also might be worth allocating the ConCheckCbData with g_slice_new() which typically doesn't get as fragmented as g_new()/g_malloc().

Also, instead of all the callback == run_check_complete things, how about an inline like "is_internal_check() that just does that to make things clearer?

> config: move no-auto-default to NMConfigData

Could we rename _reset_config_data() to _set_config_data() instead?  "reset" to me means either clearing it out completely, or returning it to default values, which that function doesn't do.  Instead what it does is take ownership of the new data.

That's all I've got.  Much better!  Lets get danw to review the connectivity parts early in the branch though.

(In reply to Dan Williams from comment #26)
> (In reply to Thomas Haller from comment #24)
> 
> > connectivity: refactor handling parameters of NMConnectivity
> 
> The g_object_notify() calls in _set_property_*** are redundant I think,
> because they are called from set_property(), and any time something is set
> via set_property() glib will automatically issue a notification (yes, even
> if it's set to the same value again, but whatever).  Plus GObject will queue
> them so that they get emitted after all property updates are done.

In a later commit ("config: implement reloading of connectivity parameters"), _config_changed_cb() calls these _set_property_() functions and then we need to raise the notifications.


> It also might be worth allocating the ConCheckCbData with g_slice_new()
> which typically doesn't get as fragmented as g_new()/g_malloc().

done


> Also, instead of all the callback == run_check_complete things, how about an
> inline like "is_internal_check() that just does that to make things clearer?

done


> > config: move no-auto-default to NMConfigData
> 
> Could we rename _reset_config_data() to _set_config_data() instead?  "reset"
> to me means either clearing it out completely, or returning it to default
> values, which that function doesn't do.  Instead what it does is take
> ownership of the new data.

done


repushed branch, with a few bugfixes (see fixup! commits).

The branch could really use some squashing...


>    connectivity: refactor handling parameters of NMConnectivity

This loses some of the changes from danw/connectivity-bgo742823. In particular, the pending_checks variable was there so that if you call set_online() and then call check_async() immediately after, then it will skip the periodic connectivity check, since it would be redundant with the manual check.

It also loses some of the debug logging.

>+	/* Only update the state, if the call was done from external, or if the periodic check
>+	 * is still the one that called this async check. */

I don't think this is necessary; whether you have connectivity is a fact which is independent of the URI you are using to check for it. (Besides, it's a race condition anyway; the response might come back right before you change the config URI rather than right afterward.) Also also, this code is never going to get tested. So I'd just remove it.


>    fixup! connectivity: refactor handling parameters of NMConnectivity

Part of this is actually a fixup for "connectivity: make NMConnectivity:dispose() reentrant"


>    config: use NMConfigData in NMConfig

"config_data0" is non-obvious. (To me it suggests "completely zeroed-out NMConfigData".) How about "config_data_orig" or "initial_config_data" or something like that?


>    config: add support for reloading of configuration

I reiterate my earlier assertion that it's silly to use a GHashTable for "changes", when all you need is a flags argument.


>    config: implement reloading of connectivity parameters

>+	changed |= _set_property_response (self, nm_config_data_get_connectivity_response (new_data), FALSE);
>+	if (changed) {

NMConfig is already doing the work of figuring out whether the connectivity data changed or not, but NMConnectivity is ignoring @changes and re-checking whether it changed itself. Only one of them should be doing that work.

>+	 * When resetting one of the properties externally, the connection
>+	 * to the NMConfig instance is released and the NMConnectivity instance
>+	 * again becomes entirely independent from NMConfig.

No code in NM ever sets any of NMConnectivity's properties after construction. NMConnectivity should just always get its info from NMConfig. Its properties could be made read-only.

(In reply to Dan Winship from comment #28)
> The branch could really use some squashing...
> 
> 
> >    connectivity: refactor handling parameters of NMConnectivity
> 
> This loses some of the changes from danw/connectivity-bgo742823. In
> particular, the pending_checks variable was there so that if you call
> set_online() and then call check_async() immediately after, then it will
> skip the periodic connectivity check, since it would be redundant with the
> manual check.

Are you referring to @pending_checks? I restored the same functionality using a @initical_check_obsoleted variable. The problem with @pending_checks was, it is wrong in case of the race-condition mentioned below.

> 
> It also loses some of the debug logging.

It lost the line in nm_connectivity_set_online(). I restored it.

> 
> >+	/* Only update the state, if the call was done from external, or if the periodic check
> >+	 * is still the one that called this async check. */
> 
> I don't think this is necessary; whether you have connectivity is a fact
> which is independent of the URI you are using to check for it. (Besides,
> it's a race condition anyway; the response might come back right before you
> change the config URI rather than right afterward.) Also also, this code is
> never going to get tested. So I'd just remove it.

I think it does, because there is a harmful race. Suppose you have connectivity wrongly configured at first:

 - start async check with old parameters (call1).
 - update parameters and re-start check with new parmaters (call2)
 - call2 returns successfully, sets ONLINE
 - call1 returns unsuccessfully, sets OFFLINE.

with "the response might come back right before you change the config URI rather than right afterward" there is no problem:

 - start async check with old parameters (call1).
 - call1 returns unsuccessfully, sets OFFLINE
 - update parameters and re-start check with new parmaters (call2)
 - call2 returns successfully, sets ONLINE


> >    fixup! connectivity: refactor handling parameters of NMConnectivity
> 
> Part of this is actually a fixup for "connectivity: make
> NMConnectivity:dispose() reentrant"

fixed


> >    config: use NMConfigData in NMConfig
> 
> "config_data0" is non-obvious. (To me it suggests "completely zeroed-out
> NMConfigData".) How about "config_data_orig" or "initial_config_data" or
> something like that?

renamed to priv->config_data_orig and nm_config_get_data_orig().


> >    config: add support for reloading of configuration
> 
> I reiterate my earlier assertion that it's silly to use a GHashTable for
> "changes", when all you need is a flags argument.

added commit "config: use flags argument in config-changed signal instead ..." [1]


> >    config: implement reloading of connectivity parameters
> 
> >+	changed |= _set_property_response (self, nm_config_data_get_connectivity_response (new_data), FALSE);
> >+	if (changed) {
> 
> NMConfig is already doing the work of figuring out whether the connectivity
> data changed or not, but NMConnectivity is ignoring @changes and re-checking
> whether it changed itself. Only one of them should be doing that work.

added commit "config: don't duplicate check for changes in _config_changed_cb()" [1]


> >+	 * When resetting one of the properties externally, the connection
> >+	 * to the NMConfig instance is released and the NMConnectivity instance
> >+	 * again becomes entirely independent from NMConfig.
> 
> No code in NM ever sets any of NMConnectivity's properties after
> construction. NMConnectivity should just always get its info from NMConfig.
> Its properties could be made read-only.

The complexity of this change comes because properties can now change after construction.

I prefer it this way, because NMConnectivity is usable and testable independent of NMConfig... if only there were some tests



Rebased and pushed.


[1] I we agree to keep the new patches, I will rebase them and fixup the implementation from the beginning. However, I would rather drop them entirely.

(In reply to Thomas Haller from comment #29)
> with "the response might come back right before you change the config URI
> rather than right afterward" there is no problem:

ah, I missed that you were always automatically starting a new check whenever the URI changed.

> > No code in NM ever sets any of NMConnectivity's properties after
> > construction. NMConnectivity should just always get its info from NMConfig.
> > Its properties could be made read-only.
> 
> The complexity of this change comes because properties can now change after
> construction.
> 
> I prefer it this way, because NMConnectivity is usable and testable
> independent of NMConfig... if only there were some tests

In that case, we can go the other way, and make NMConnectivity always use explicitly-set property values, and know nothing about NMConfig. NMManager would read the initial values from NMConfig and pass them to nm_connectivity_new(), and then when config-changed was emitted, NMManager would set NMConnectivity's properties to the new values.

(In reply to Dan Winship from comment #30)
> (In reply to Thomas Haller from comment #29)

> > > No code in NM ever sets any of NMConnectivity's properties after
> > > construction. NMConnectivity should just always get its info from NMConfig.
> > > Its properties could be made read-only.
> > 
> > The complexity of this change comes because properties can now change after
> > construction.
> > 
> > I prefer it this way, because NMConnectivity is usable and testable
> > independent of NMConfig... if only there were some tests
> 
> In that case, we can go the other way, and make NMConnectivity always use
> explicitly-set property values, and know nothing about NMConfig. NMManager
> would read the initial values from NMConfig and pass them to
> nm_connectivity_new(), and then when config-changed was emitted, NMManager
> would set NMConnectivity's properties to the new values.

Agree, that makes a bit more sense, because the manager should delegate the configuration changes to where they belong.

Added 3 new commits:

74bb453 config: implement reloading of connectivity parameters
4c5cf56 connectivity: add nm_connectivity_set()
1bb00b8 connectivity: make NMConnectivity independent of NMConfig

is that better?

compare "th/rh1066697_reload_config" vs. "th/rh1066697_reload_config-4" branch



keep or drop
  "use flags argument in config-changed signal instead of a hash table"
?

> 1bb00b8 connectivity: make NMConnectivity independent of NMConfig

>-	priv->connectivity = nm_connectivity_new ();
> 	g_signal_connect (priv->connectivity, "notify::" NM_CONNECTIVITY_STATE,

need to move the g_signal_connect() too

> 4c5cf56 connectivity: add nm_connectivity_set()

I would have just used g_object_set()...

As it is, it's bringing back the state where NMConfig and NMConnectivity are both checking for changes.

> 74bb453 config: implement reloading of connectivity parameters

there's a stray #include addition left in nm-connectivity.c

> is that better?

yes

> keep or drop
>   "use flags argument in config-changed signal instead of a hash table"

i vote for keep

(In reply to Dan Winship from comment #32)
> > keep or drop
> >   "use flags argument in config-changed signal instead of a hash table"
> 
> i vote for keep

I vote for keep too.

> fixup! connectivity: refactor handling parameters of NMConnectivity

initical -> initial

> config: add support for reloading of configuration

Maybe the signal should carry [ new_data, changes, old_data ]?  That would make it simpler for some listeners as they wouldn't have to call nm_config_get_data() if they don't need the structure after the signal is over (like the manager for connectivity updates).

> connectivity: make NMConnectivity independent of NMConfig

Yeah, the signal and 'new' should be together, though why should it be moved to _init() instead of _new() when other stuff (settings, policy) is handled in _new()?  Either way I don't really care, as long as both are together.

(In reply to Dan Winship from comment #32)
> > 4c5cf56 connectivity: add nm_connectivity_set()
> 
> I would have just used g_object_set()...
> 
> As it is, it's bringing back the state where NMConfig and NMConnectivity are
> both checking for changes.

Yeah, that would be simpler.  You unschedule/reschedule the connectivity check a couple times, but since these things won't change often thats OK.

all done, and repushed

merged http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=master

this refactors NMConfig to support reload in principle. It also adds reloading of NMConnectivity parameters.

This is the foundation of actually fixing this bug. The actual reloading of the main.dns setting is *not* yet implemented.

(In reply to Thomas Haller from comment #36)
> merged
> http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=master

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=f9e4af2d86251f4b9947fea3198f9aabf1f89ed5

reloading NMConfig is supported now in principle.

the original BZ talks about reloading DNS setting specifically. This will be fixed by https://bugzilla.redhat.com/show_bug.cgi?id=1062301#c6 .

it also talks about writing out the DNS configuration anew (on SIGUSR1). That also sounds very much like a duplicate of bug 1062301.


I close this as duplicate now.

*** This bug has been marked as a duplicate of bug 1062301 ***

Merged to nm-1-0 (post 1.2 release)

e224d96 config,connectivity: merge branch 'lr/nm-1-0-rh1066697_reload_config'
d9f372f config: use flags argument in config-changed signal instead of a hash table
0efcf3c config: make NMConfig implement GInitable
41598c1 config: move no-auto-default to NMConfigData
b85c3c4 config: refactor merging no_auto_default
9809eb4 config: move keyfile values to NMConfigData
c9aca6f config: add new function nm_config_data_diff()
0ca8273 config: move main_file and description to NMConfigData
30a4786 config: refactor reloading not to create a second NMConfig instance
c2d7454 config: minor refactoring to highlight mutable property no_auto_default of NMConfig
8ba5e06 config: refactor read_entire_config() to merge command line options
f166c00 config: refactor to inject NMConfigCmdLineOptions to NMConfig constructor
ca6f06d config: refactor nm_config_new() by extracting function read_entire_config()
cfc435b config: refactor nm_config_new() by extracting function _get_config_dir_files()
83edb5a config/trivial: rename variables for configuration file
67c4398 config: refactor read_config() to make it independent from NMConfig
244cc01 config: implement reloading of connectivity parameters
b18de23 connectivity: make NMConnectivity independent of NMConfig
63293bb config: add support for reloading of configuration
3a5fb56 config: add handler for SIGHUP and a reload-configuration stub
a56fe5a config: use NMConfigData in NMConfig
5a7506a config: add new NMConfigData class
b4ad743 config: add nm_config_setup() to initialize config singleton
d165559 config/trivial: fix returning FALSE instead of NULL in nm_config_new()
0799617 config: forward declare NMConfig in nm-types.h
9d5b4ba connectivity: add logging macros to nm-connectivity.c
b4d7ac7 connectivity: refactor handling parameters of NMConnectivity
fc6f45e connectivity: refactor converting connectivity states to string
6f4bf28 connectivity: add missing G_PARAM_CONSTRUCT for NM_CONNECTIVITY_URI property
da87866 connectivity/trivial: fix white space and line-break in nm-connectivity.c
5929a4c connectivity: make NMConnectivity:dispose() reentrant
72d06eb connectivity: add code comment to nm_connectivity_check_cb()