https://mail.gnome.org/archives/networkmanager-list/2014-March/msg00052.html (and followups) have some more suggestions.

several of the original suggestions already work.


A)

(A0) since NMSettingConnection.interface was added (http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=2d668d763e57a70dddb4683a00bcb7a961286cd7) you can omit it and NMSettingConnection:verify() will set it depending <type>.interace-name. I consider this actually a bug, because verify() should not modify a setting. But guess we have to live with it not to break existing code.

(A1) since merging http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=392d4e48316f3dd6faf06faeafc70a7cd177c2f8, the normalize functions that are applied after reading a setting will take care that NMSettingConnection:interface-name gets copied to <type>.interace-name.

So, now you can omit either one of the properties, and normalize() will fix the other one, respectively. This is not done by the keyfile-reader, but by normalize().


B)
Since http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9f8b7ff51dfd7cdb828fe5e7b50016c5587e8657 you can omit the "[bridge]" section, which gets deduced from "type=bridge" section.

This is done by the keyfile plugin, but we should move it to normalize() so that it is generally available.

C)
Since http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=bbdae85 , we can omit 'id' and 'uuid'.

Again, done by the keyfile reader, should be done by nm_connection_normalize().

I pushed branch th/bgo682052_normalize_settings for review. It extends normalization, so that both slave-type and type gets normalized if missing.

Note, that it's more useful to extend nm_connection_normalize() then do such normalization inside keyfile-reader itself. This way, it is more useful beyond keyfile.



Minimal (not very useful) connections now look like this:

Test_minimal_1:[connection]
Test_minimal_1:type=802-3-ethernet

Test_minimal_2:[802-3-ethernet]

Test_minimal_slave_1:[connection]
Test_minimal_slave_1:type=802-3-ethernet
Test_minimal_slave_1:master=br0
Test_minimal_slave_1:slave-type=bridge

Test_minimal_slave_2:[connection]
Test_minimal_slave_2:master=br0
Test_minimal_slave_2:[802-3-ethernet]
Test_minimal_slave_2:[bridge-port]

(In reply to comment #3)
> I pushed branch th/bgo682052_normalize_settings for review. It extends
> normalization, so that both slave-type and type gets normalized if missing.
> 
> Note, that it's more useful to extend nm_connection_normalize() then do such
> normalization inside keyfile-reader itself. This way, it is more useful beyond
> keyfile.


It also changes, that a slave connection *must* have the corresponding slave-port setting

(In reply to comment #3)
> Note, that it's more useful to extend nm_connection_normalize() then do such
> normalization inside keyfile-reader itself. This way, it is more useful beyond
> keyfile.

I agree. That's why I advocated the normalize() function in the first place.

Btw. repushed with fixes and 2 new commits.

note that
>> libnm-util: add nm_connection_get_settings() function
is used nowhere, I added it because I needed it initially. I think such a functionality should be available, shouldn't it?

(In reply to comment #6)
> note that
> >> libnm-util: add nm_connection_get_settings() function
> is used nowhere, I added it because I needed it initially. I think such a
> functionality should be available, shouldn't it?

Well, if you don't need it now, and no one has needed it before, then it sounds like no...



> nmtst: add nmtst_connection_normalize() function

>+	g_assert (success);
>+	g_assert_no_error (error);

it's better to put the g_assert_no_error() first, since it will show the error message if the normalization fails, rather than just "assertion failed: success".



> nmtst: add nmtst_keyfile_read_connection_from_file() function

This is keyfile-specific and doesn't seem like it belongs in nm-test-utils.h



> ifcfg-rh: refactor reader to verify() connection after type specific readers

"to NOT verify()"?

Also, it's weird to add a verify() to the end here and then remove it again in the very next commit...



> libnm-util: add _nm_setting_find_in_list_required() function

This adds NM_SETTING_ERROR_SETTING_NOT_FOUND but then the code actually uses NM_SETTING_ERROR_PROPERTY_NOT_FOUND. (But wouldn't NM_CONNECTION_ERROR_SETTING_NOT_FOUND be best anyway?)



> libnm-util: properly disconnect "notify" signal for settings in NMConnection

It would be better to change the creation of priv->settings to use a custom destroy func rather than g_object_unref(), which would do the signal disconnection and then unref the setting. Then we don't have to worry about disconnecting in every place where a setting might be removed.



> libnl-util: move validation of NMSettingConnection:type to NMSettingConnection:verify()

typo "libnl-util"

>+			g_set_error (error,
>+						 NM_SETTING_CONNECTION_ERROR,
>+						 NM_SETTING_CONNECTION_ERROR_TYPE_SETTING_NOT_FOUND,
>+						 _("requires presence of '%s' setting in the connection"),

indentation in a few places



> libnm-util: normalize slave-type/slave-port-settings of connections

>+		if (!s_port) {
>+			GType port_type_g = nm_connection_lookup_setting_type (port_type);

You can simplify this section by using nm_connection_create_setting(). (Likewise later in "normalize NMSettingConnection:type property")

>+		slave_type = nm_setting_connection_get_slave_type (s_con);
>+		if (   slave_type
>+		    && strcmp (slave_type, NM_SETTING_BRIDGE_SETTING_NAME)) {
>+			g_set_error (error,

it's an error if slave_type isn't set too

> libnm-util: add nm_connection_get_settings() function

It returns NMSetting**, which is pretty different than other API we're using in libnm-util.  Also I don't think it'll introspect well, if at all.  Better to use a GSList I think...

But yeah, I'd remove it unless there's an actual consumer.  We haven't ever needed it in nm-connection-editor or the applet or gnome-shell, or NM itself, so it's probably not very useful.


> nmtst: add nmtst_connection_normalize() function

The branch doesn't use _nmtst_connection_normalize_v() anywhere; I'd postpone adding the varargs variants until you add something that actually starts passing arguments to the normalize function.


> nmtst: add nmtst_keyfile_read_connection_from_file() function

This is keyfile specific.  It should be in the keyfile tests, not in the generic nm-test-utils.h header.


> nmtst: add nmtst_create_minimal_connection() function

There's test code in various places that creates minimal connections; maybe convert that over to this function too?


> libnm-util: properly disconnect "notify" signal for settings in NMConnection

If the settings have already been removed in dispose(), we don't need to remove them again in finalize().  If you want to be extra-sure, just g_assert (g_hash_table_size (priv->settings) == 0).  But dispose() will *always* be called before finalize().


> libnl-util: move validation of NMSettingConnection:type to NMSettingConnection:verify()

Indentation in nm-setting-connection.c::verify() when setting the GError.


> libnm-util: normalize slave-type/slave-port-settings of connections

Instead of building up a list of all settings to pass to _nm_setting_slave_type_detect_from_settings(), you can just re-use _nm_setting_slave_type_is_valid().  Then you can iterate the list once with g_hash_table_iter_next(), and when you find a slave setting, set slave-type and break:

nm-setting.c:

typedef struct {
	const char *sname;
	const char *pname;
} SlaveSetting;

const SlaveSetting slave_settings[] = {
	{ NM_SETTING_BOND_SETTING_NAME,   NULL },
	{ NM_SETTING_BRIDGE_SETTING_NAME, NM_SETTING_BRIDGE_PORT_SETTING_NAME },
	{ NM_SETTING_TEAM_SETTING_NAME,   NM_SETTING_TEAM_PORT_SETTING_NAME },
};

gboolean
_nm_setting_slave_type_is_valid (const char *slave_type, const char **out_port_type)
{
	const char *port_type = NULL;
	gboolean found = FALSE;

	if (slave_type) {
		for (i = 0; i < G_N_ELEMENTS (slave_settings); i++) {
			if (strcmp (slave_type, slave_settings[i].sname) == 0) {
				port_type = slave_settings[i].pname;
				found = TRUE;
				break;
			}
		}
	}

	if (out_port_type)
		*out_port_type = port_type;
	return found;
}

nm-connection.c:

	} else {
		NMConnectionPrivate *priv = NM_CONNECTION_GET_PRIVATE (self);
		NMSetting *setting;
		GHashTableIter h_iter;

		g_hash_table_iter_init (&h_iter, priv->settings);
		while (g_hash_table_iter_next (&h_iter, NULL, (void **) &setting)) {
			const char *name = nm_setting_get_name (setting);

			if (_nm_setting_slave_type_is_valid (name, NULL)) {
				g_object_set (s_con, NM_SETTING_CONNECTION_SLAVE_TYPE, name, NULL);
				return TRUE;
			}
		}
	}


I can't think of a case where a connection would have *two* kinds of slave settings, so this will be safe.  But it also implies that verify() should ensure there aren't two slave settings.  But that's also easily accomplished with the same _nm_setting_is_slave_setting() call.


> libnm-util: minor refactoring in NMSettingConnection:verify()

Let's save a strlen and do:

if (!priv->type[0]) {


> libnm-util: normalize NMSettingConnection:type property

We actually *can* detect the type from multiple base types.  But instead of doing it all this way, we could use connection priorities to do it.  We'd have to:

1) Increase setting priorities [2 - 4] by 1.
2) Make settings with priority 1 & 2 base types
3) Add setting priority 2 for settings that are the lower priority base type.  This group would contain Ethernet, GSM, and CDMA.  (GSM + CDMA because when Bluetooth DUN is used, BT is the base type, but regular modems use GSM/CDMA as the base type)
4) Verify() would ensure that there were never *two* settings of the same priority for priorities 0, 1, and 2.

_nm_setting_find_in_list_unique_base_type() would then change to check for priority 1 and 2 settings, and use the smallest-priority one it finds.  And rename the function since base types aren't unique :)

(In reply to comment #7)
> (In reply to comment #6)

> > libnm-util: properly disconnect "notify" signal for settings in NMConnection
> 
> It would be better to change the creation of priv->settings to use a custom
> destroy func rather than g_object_unref(), which would do the signal
> disconnection and then unref the setting. Then we don't have to worry about
> disconnecting in every place where a setting might be removed.

I think that is not possible because the GDestroyNotify argument has not user_data to pass on @self.



> You can simplify this section by using nm_connection_create_setting().
> (Likewise later in "normalize NMSettingConnection:type property")
> 
> >+		slave_type = nm_setting_connection_get_slave_type (s_con);
> >+		if (   slave_type
> >+		    && strcmp (slave_type, NM_SETTING_BRIDGE_SETTING_NAME)) {
> >+			g_set_error (error,
> 
> it's an error if slave_type isn't set too

No it isn't. !slave_type means that it can be normalized. NMSettingConnection:verify() will take care of that.

These checks in bridge-port and team-port are there to catch the following:

[connection]
slave-type=Y
[X-port]

e.g. having a slave-type=bond, with a [bridge-port].



Done to the rest.

(In reply to comment #8)

> > nmtst: add nmtst_connection_normalize() function
> 
> The branch doesn't use _nmtst_connection_normalize_v() anywhere; I'd postpone
> adding the varargs variants until you add something that actually starts
> passing arguments to the normalize function.

I would like to keep it. Currently we only support one parameter (NM_CONNECTION_NORMALIZE_PARAM_IP6_CONFIG_METHOD). But I think it is a nice syntax for the test function waiting to be used.


> > nmtst: add nmtst_create_minimal_connection() function
> 
> There's test code in various places that creates minimal connections; maybe
> convert that over to this function too?

Yes, fully agree. But in a second step? For now I care more about new/modified code to use the nmtst functions.



> > libnm-util: normalize slave-type/slave-port-settings of connections
> 
> Instead of building up a list of all settings to pass to
> _nm_setting_slave_type_detect_from_settings(), you can just re-use
> _nm_setting_slave_type_is_valid().  Then you can iterate the list once with
> g_hash_table_iter_next(), and when you find a slave setting, set slave-type and
> break:
> 
> nm-setting.c:
> 
> typedef struct {
>     const char *sname;
>     const char *pname;
> } SlaveSetting;
> 
> const SlaveSetting slave_settings[] = {
>     { NM_SETTING_BOND_SETTING_NAME,   NULL },
>     { NM_SETTING_BRIDGE_SETTING_NAME, NM_SETTING_BRIDGE_PORT_SETTING_NAME },
>     { NM_SETTING_TEAM_SETTING_NAME,   NM_SETTING_TEAM_PORT_SETTING_NAME },
> };


Initially I had a slave_settings array like this. Turned out to be more complicated that just hard code the known types in if-else. Also, we don't add lot's of new slave types anytime soon.


> gboolean
> _nm_setting_slave_type_is_valid (const char *slave_type, const char
> **out_port_type)
> {
>     const char *port_type = NULL;
>     gboolean found = FALSE;
> 
>     if (slave_type) {
>         for (i = 0; i < G_N_ELEMENTS (slave_settings); i++) {
>             if (strcmp (slave_type, slave_settings[i].sname) == 0) {
>                 port_type = slave_settings[i].pname;
>                 found = TRUE;
>                 break;
>             }
>         }
>     }
> 
>     if (out_port_type)
>         *out_port_type = port_type;
>     return found;
> }
> 
> nm-connection.c:
> 
>     } else {
>         NMConnectionPrivate *priv = NM_CONNECTION_GET_PRIVATE (self);
>         NMSetting *setting;
>         GHashTableIter h_iter;
> 
>         g_hash_table_iter_init (&h_iter, priv->settings);
>         while (g_hash_table_iter_next (&h_iter, NULL, (void **) &setting)) {
>             const char *name = nm_setting_get_name (setting);
> 
>             if (_nm_setting_slave_type_is_valid (name, NULL)) {
>                 g_object_set (s_con, NM_SETTING_CONNECTION_SLAVE_TYPE, name,
> NULL);
>                 return TRUE;
>             }
>         }
>     }
> 
> 
> I can't think of a case where a connection would have *two* kinds of slave
> settings, so this will be safe.  But it also implies that verify() should
> ensure there aren't two slave settings.  But that's also easily accomplished
> with the same _nm_setting_is_slave_setting() call.

For one you suggest a different implementation of _nm_setting_slave_type_is_valid using slave_settings array. I had such an array before, but as your example shows too, it is more lines of code + 3*sizeof(SlaveSetting) bytes of static memory.
I don't think that this implementation has any advantage. Also considering that we don't add new slave-types often or any time soon.

Having such a slave_setting array would make more sense as it is now, where there are two functions using the same knowledge (which I actually dislike). In your suggestion you drop _nm_setting_slave_type_detect_from_settings() making the slave_setting even less a win.


You suggest to drop _nm_setting_slave_type_detect_from_settings() and instead reimplement it in NMSettingConnection:verify() and NMConnection:_normalize_connection_slave_type(). Is this to save  creating the additional "GSList *all_settings" list in NMConnection:_normalize_connection_slave_type()?

Interesting that the container classes of glib don't offer a generic way to iterate over them. Seams overly useful to me.

I did it this way, because I dislike the duplication of "detecting a slave-port-setting". Both NMSettingConnection:verify() and NMConnection:_normalize_connection_slave_type() must come to the very same conclusion what they intend to normalize.

also:
        while (g_hash_table_iter_next (&h_iter, NULL, (void **) &setting)) {
            const char *name = nm_setting_get_name (setting);

            if (_nm_setting_slave_type_is_valid (name, NULL)) {
                g_object_set (s_con, NM_SETTING_CONNECTION_SLAVE_TYPE, name,
NULL);
                return TRUE;
            }
        }
@name will be something like "bridge-port", but _nm_setting_slave_type_is_valid() would expect "bridge". That would need a slightly different solution again.


Not sure how to improve on this. Needs some more discussion :)



> > libnm-util: normalize NMSettingConnection:type property
> 
> We actually *can* detect the type from multiple base types.  But instead of
> doing it all this way, we could use connection priorities to do it.  We'd have
> to:
> 
> 1) Increase setting priorities [2 - 4] by 1.
> 2) Make settings with priority 1 & 2 base types
> 3) Add setting priority 2 for settings that are the lower priority base type. 
> This group would contain Ethernet, GSM, and CDMA.  (GSM + CDMA because when
> Bluetooth DUN is used, BT is the base type, but regular modems use GSM/CDMA as
> the base type)
> 4) Verify() would ensure that there were never *two* settings of the same
> priority for priorities 0, 1, and 2.
> 
> _nm_setting_find_in_list_unique_base_type() would then change to check for
> priority 1 and 2 settings, and use the smallest-priority one it finds.  And
> rename the function since base types aren't unique :)


That sounds like a larger change. How about adding now normalization with *one* base_type. Later we can extend on that and add also normalization of "type" for multiple base-types.

For now, I renamed _nm_setting_find_in_list_unique_base_type() to _nm_setting_find_in_list_base_type() and added a FIXME comment saying that we want to support that later.


Other remarks fixed.

Repushed

I renamed the branch from th/bgo682052_normalize_settings to th/bgo696936_normalize_settings . No other changes

> nmtst: add nmtst_auto_g_unref and nmtst_auto_g_free variable attributes

We don't need this now do we? Just use gsystem-local-alloc.h



> nmtst: add nmtst_connection_normalize() function

>+#ifdef NM_CONNECTION_H

Just have nm-test-utils.h #include nm-connection.h. You don't want its content to depend on whether it's included before or after other things.



> nmtst: add nmtst_assert_connection_equals() function

>+	compare = nm_connection_diff (a, b, NM_SETTING_COMPARE_FLAG_EXACT, &out_settings);
...
>+	g_assert (compare);
...
>+	compare = nm_connection_compare (a, b, NM_SETTING_COMPARE_FLAG_EXACT);
>+	g_assert (compare);

It should not be possible for nm_connection_compare() to return FALSE if nm_connection_diff() returned TRUE, should it?



> libnm-util: properly disconnect "notify" signal for settings in NMConnection

Ah, good. Using g_hash_table_foreach_remove() also works for centralizing the signal-disconnection code.

>+_nm_connection_add_setting (NMConnection *connection, NMSetting *setting, gboolean might_exist)

The "might_exist" flag seems like an unnecessary optimization; you can just always check if it exists, and remove it if so. (And then you don't have to worry about the code breaking if someone passes FALSE when they should have passed TRUE.)



> libnm-util: normalize slave-type/slave-port-settings of connections

"Slave" and "port" are different names for the same thing, so you generally shouldn't use them both in the same name... (eg, NM_SETTING_CONNECTION_ERROR_SLAVE_PORT_SETTING_NOT_FOUND should be either NM_SETTING_CONNECTION_ERROR_SLAVE_SETTING_NOT_FOUND or NM_SETTING_CONNECTION_ERROR_PORT_SETTING_NOT_FOUND)

>+		s_port = nm_connection_create_setting(port_type);

space before "("

>+             "A connection with a '" NM_SETTING_BRIDGE_SETTING_NAME "' setting must have the slave-type set. Instead it is '%s'",

(in nm-setting-bridge-port.c). That should be NM_SETTING_BRIDGE_PORT_SETTING_NAME, right? And "must have the slave-type set to 'bridge'." Likewise in nm-setting-team-port.c

>+	g_assert (!nm_connection_verify (con, NULL));
>+	g_assert (!nm_connection_normalize (con, NULL, NULL, NULL));

It might be good to pass in GErrors and assert that you're getting back the expected error (so that test test doesn't accidentally pass because of some other bug).

(In reply to comment #12)
> > nmtst: add nmtst_auto_g_unref and nmtst_auto_g_free variable attributes
> 
> We don't need this now do we? Just use gsystem-local-alloc.h

Yes, before I wasn't aware of this functionality from libgsystem. I dropped this commit.



> > nmtst: add nmtst_connection_normalize() function
> 
> >+#ifdef NM_CONNECTION_H
> 
> Just have nm-test-utils.h #include nm-connection.h. You don't want its content
> to depend on whether it's included before or after other things.

nm-test-utils.h must always be included as last. Especially, because it behaves different depending on whether you include nm-logging.h.

Currently, I think that all tests do link with libnm-util, but the test utility should work without it too.


> > nmtst: add nmtst_assert_connection_equals() function
> 
> >+	compare = nm_connection_diff (a, b, NM_SETTING_COMPARE_FLAG_EXACT, &out_settings);
> ...
> >+	g_assert (compare);
> ...
> >+	compare = nm_connection_compare (a, b, NM_SETTING_COMPARE_FLAG_EXACT);
> >+	g_assert (compare);
> 
> It should not be possible for nm_connection_compare() to return FALSE if
> nm_connection_diff() returned TRUE, should it?

no. It should not. The assert is redundant. I know, this is against the testing principle of only asserting against the one thing you want to test. I do it because I use the opportunity to checking that nm_connection_diff() and nm_connection_compare() agree.


> > libnm-util: properly disconnect "notify" signal for settings in NMConnection
> 
> Ah, good. Using g_hash_table_foreach_remove() also works for centralizing the
> signal-disconnection code.


> >+_nm_connection_add_setting (NMConnection *connection, NMSetting *setting, gboolean might_exist)
> 
> The "might_exist" flag seems like an unnecessary optimization; you can just
> always check if it exists, and remove it if so. (And then you don't have to
> worry about the code breaking if someone passes FALSE when they should have
> passed TRUE.)

done


> > libnm-util: normalize slave-type/slave-port-settings of connections
> 
> "Slave" and "port" are different names for the same thing, so you generally
> shouldn't use them both in the same name... (eg,
> NM_SETTING_CONNECTION_ERROR_SLAVE_PORT_SETTING_NOT_FOUND should be either
> NM_SETTING_CONNECTION_ERROR_SLAVE_SETTING_NOT_FOUND or
> NM_SETTING_CONNECTION_ERROR_PORT_SETTING_NOT_FOUND)

done


> 
> >+		s_port = nm_connection_create_setting(port_type);
> 
> space before "("

done

> 
> >+             "A connection with a '" NM_SETTING_BRIDGE_SETTING_NAME "' setting must have the slave-type set. Instead it is '%s'",
> 
> (in nm-setting-bridge-port.c). That should be
> NM_SETTING_BRIDGE_PORT_SETTING_NAME, right? And "must have the slave-type set
> to 'bridge'." Likewise in nm-setting-team-port.c

correct. Fixed.



> >+	g_assert (!nm_connection_verify (con, NULL));
> >+	g_assert (!nm_connection_normalize (con, NULL, NULL, NULL));
> 
> It might be good to pass in GErrors and assert that you're getting back the
> expected error (so that test test doesn't accidentally pass because of some
> other bug).

added new commit
  "nmtst: add assertion functions for verify() connection"
and use those functions.


Pushed for re-review.

> +			             "A connection with a '" NM_SETTING_BRIDGE_PORT_SETTING_NAME "' setting must have the slave-type set to '" NM_SETTING_BRIDGE_SETTING_NAME "'. Instead it is '%s'",

Oh, I think all existing nm_connection_verify() errors are localized, so you should _() these ones too. (Maybe elsewhere in the branch too, I wasn't paying attention...). You'll also have to use "%s" to substitute in the setting names rather than splicing the defines in directly, because the tool that parses out the strings to fill in the .po files won't handle #defines.

(In reply to comment #13)
> > >+	g_assert (!nm_connection_verify (con, NULL));
> > >+	g_assert (!nm_connection_normalize (con, NULL, NULL, NULL));
> > 
> > It might be good to pass in GErrors and assert that you're getting back the
> > expected error (so that test test doesn't accidentally pass because of some
> > other bug).
> 
> added new commit
>   "nmtst: add assertion functions for verify() connection"
> and use those functions.

It's still not checking the *specific* errors. The assertion function should take a GQuark and a guint and call g_assert_error() to assert that it got, eg NM_SETTING_CONNECTION_ERROR/NM_SETTING_CONNECTION_ERROR_SLAVE_SETTING_NOT_FOUND, and not, eg, NM_CONNECTION_ERROR/NM_CONNECTION_ERROR_CONNECTION_TYPE_INVALID because of a typo earlier in the test function.

(In reply to comment #14)

comments addressed.

Error messages are now localized, and the test assert against error domain+code.

Repushed, see changes as fixup! commits

> libnm-util: properly disconnect "notify" signal for settings in NMConnection

We can put all the arguments to the g_hash_table_insert() on one line now.


> libnm-util: properly disconnect "notify" signal for settings in NMConnection

Should probably backport this to stable branches.

----

Should we create a helper in nm-connection for:

	g_hash_table_iter_init (&h_iter, priv->settings);
	while (g_hash_table_iter_next (&h_iter, NULL, (void **) &setting))
		all_settings = g_slist_prepend (all_settings, setting);

There are three users: _normalize_connection_type(), _normalize_connection_slave_type(), and _nm_connection_verify().  I believe the _nm_connection_verify() logic should be used (eg, always keep NMSettingConnection first) but it could be a lot cleaner in those three users.

(In reply to comment #16)
> > libnm-util: properly disconnect "notify" signal for settings in NMConnection
> 
> We can put all the arguments to the g_hash_table_insert() on one line now.

done.


> > libnm-util: properly disconnect "notify" signal for settings in NMConnection
> 
> Should probably backport this to stable branches.

agree.



> Should we create a helper in nm-connection for:
> 
>     g_hash_table_iter_init (&h_iter, priv->settings);
>     while (g_hash_table_iter_next (&h_iter, NULL, (void **) &setting))
>         all_settings = g_slist_prepend (all_settings, setting);
> 
> There are three users: _normalize_connection_type(),
> _normalize_connection_slave_type(), and _nm_connection_verify().  I believe the
> _nm_connection_verify() logic should be used (eg, always keep
> NMSettingConnection first) but it could be a lot cleaner in those three users.


_nm_connection_verify() does something special. So, two places only.

I was looking into this, and found that I disagree with commit 6cb6d3972558bb89cbb83ac1b87595dc33ed665a.

See new commits:

all: use nm_intern_utils_hash_values_to_slist() (master)
libnm-util: add nm_intern_utils_hash_values_to_slist()
all: add nm-utils-internal.h header

(In reply to comment #17)
> (In reply to comment #16)
> > > libnm-util: properly disconnect "notify" signal for settings in NMConnection
> > 
> > We can put all the arguments to the g_hash_table_insert() on one line now.
> 
> done.
> 
> 
> > > libnm-util: properly disconnect "notify" signal for settings in NMConnection
> > 
> > Should probably backport this to stable branches.
> 
> agree.
> 
> 
> 
> > Should we create a helper in nm-connection for:
> > 
> >     g_hash_table_iter_init (&h_iter, priv->settings);
> >     while (g_hash_table_iter_next (&h_iter, NULL, (void **) &setting))
> >         all_settings = g_slist_prepend (all_settings, setting);
> > 
> > There are three users: _normalize_connection_type(),
> > _normalize_connection_slave_type(), and _nm_connection_verify().  I believe the
> > _nm_connection_verify() logic should be used (eg, always keep
> > NMSettingConnection first) but it could be a lot cleaner in those three users.
> 
> 
> _nm_connection_verify() does something special. So, two places only.

what I meant is that we don't need the _nm_connection_verify() behavior. But I see use for nm_intern_utils_hash_values_to_slist().

Regardless of whether we like that utils function, I think 
"all: add nm-utils-internal.h header" is the right approach.

Can we prefix nm_intern_utils_hash_values_to_slist with an "_" like we do other places just to make clear that it's private?

Also we'll need to figure out what to do with this versus libnm/libnm-core too.  But we can do that in parallel.

ALso, do we really have to provide backwards compatible ABI for the nm_utils_get_private()?  THat's only used by ifcfg-rh and NM itself, and we don't support plugins that are out-of-sync with NM.  I think we can just drop it since the only users were internal?

Also, at least nm_intern_utils_hash_values_to_slist() coudl be 'static inline' and then we wouldn't have to export it.

(In reply to comment #19)
> Also we'll need to figure out what to do with this versus libnm/libnm-core too.
>  But we can do that in parallel.

We'll need to add all this new normalizing, etc, to libnm-core/libnm too of course. (In fact, we *don't* need to add it to libnm-util/libnm-glib.)

Anyway, libnm-core has no internally-private API; everything in libnm-core is visible to libnm/ and src/. (And so nm_utils_private_get(), etc, should go away, but I didn't think to do that before.)

> ALso, do we really have to provide backwards compatible ABI for the
> nm_utils_get_private()?

So first off, this branc needs to be rebased; the address label functionality has already been removed from libnm-util in master, and libnm-util's nm_utils_get_private() just does g_assert_not_reached().

As for preserving ABI:

> Even for the NMUtilsPrivateData provided functions we must preserve ABI
> and functional backward compatibility. So the only advantage of using
> the struct is that we can retire exported symbols. However, we still have
> to provide the functionality and binary interface.

That's wrong. Any call to nm_utils_get_private() from outside of the NM source tree is a programmer error, and could have any result, including instantly crashing. The only ABI we need to preserve is that the symbol exists. (And we only have to preserve that because otherwise Debian and other people running automated ABI-breakage-detecting tools will yell at us later.)

I rebased the branch on current master. The previous version is named th/bgo696936_normalize_settings-1.

This included porting the changes to libnm-util over to libnm-core.

Also, the first commits were heavily reworked:

972dfa8 libnm-core: add nm_utils_hash_values_to_slist()
8c6525c libnm: move declaration of NM_SETTING_SECRET_FLAGS_ALL to nm-core-intern.h
f211803 libnm-core: declare NM_SETTING_COMPARE_FLAG_INFERRABLE flag in "nm-core-intern.h"
79c4168 all: add nm-core-intern.h header

> all: add nm-core-intern.h header

generally agree that it makes sense to differentiate "private-to-libnm-core" from "private-to-NM". I might go for "internal" rather than "intern" though. It's only two letters more.

>    nm-core-intern.h is used to give NetworkManager core previleged

"privileged"

>+ * These functions should only be used by libnm-core, NM-core and test
>+ * functions.

And libnm.

>+const char *_nm_intern_setting_ip4_config_get_address_label      (NMSettingIP4Config *setting,

I'm not sure I like renaming these methods to include "intern"... the underscore prefix already indicates that it's internal. The "intern" is redundant and breaks the standard naming convention.

And already in the next commit, you're kinda dropping the convention anyway by not renaming "NM_SETTING_COMPARE_FLAG_INFERRABLE" to "NM_INTERN_SETTING_COMPARE_FLAG_INFERRABLE"...

> /* Private NMSettingIP4Config methods */
>-#include "nm-setting-ip4-config.h"
>-const char *nm_setting_ip4_config_get_address_label      (NMSettingIP4Config *setting, guint32 i);
>-gboolean    nm_setting_ip4_config_add_address_with_label (NMSettingIP4Config *setting, NMIP4Address *address, const char *label);

Need to remove the comment too



> libnm-core: add nm_utils_hash_values_to_slist()

Hm... so when we had the conversation about public-vs-internal API yesterday, I was speaking entirely in the abstract, and hadn't seen this function yet. For this, I feel like it should NOT be public API, because it's not actually NetworkManager-related functionality; it's just filling in a gap in GLib...



> keyfile: reader adds NMSettingConnection if missing

bad commit message style. "make the reader add NMSettingConnection if missing". Or just "add NMSettingConnection if missing".



> libnm-core: move validation of NMSettingConnection:type to NMSettingConnection:verify()

>+		s_base = nm_setting_find_in_list (all_settings, priv->type);
>+		if (!s_base) {
>+			g_set_error (error,

given that this commit comes after "add _nm_setting_find_in_list_required()", it's weird that you don't use it.



> libnm-core: minor refactoring in NMSettingConnection:verify()

Why? This adds an extra level of indentation that isn't needed. (If the "if" clause is true, then we "return FALSE" at the end, so moving everything inside the "else" has no effect.



> libnm-core: normalize NMSettingConnection:type property

> It can also create a (default) base setting.

Some base types have mandatory-to-set properties (eg, NMSettingWireless:ssid), and so this won't result in a valid connection. But as the code is now, normalize() would fail with "802-11-wireless.ssid: property is missing" in that case. I think it would make more sense for it to fail with an error about the 802-11-wireless setting itself being missing. (So, if the newly-created base setting doesn't verify(), then throw it away rather than adding it to the connection, and let the connection fail to verify because of the missing base setting later.)

>+	/* FIXME: currently, if there are more then one matching base types,

s/are/is/
s/then/than/
s/types/type/

:)



> keyfile/tests: add read_connection_from_file() utility function

would it make sense to add a "gboolean verifies" arg so you can use this to read connections that are expected to need normalization too?



> keyfile/tests: add reading minimal slave keyfiles

There are non-slave tests in this commit too.

(In reply to comment #22)
> > all: add nm-core-intern.h header
> 
> generally agree that it makes sense to differentiate "private-to-libnm-core"
> from "private-to-NM". I might go for "internal" rather than "intern" though.
> It's only two letters more.
>
>
> >+const char *_nm_intern_setting_ip4_config_get_address_label      (NMSettingIP4Config *setting,
> 
> I'm not sure I like renaming these methods to include "intern"... the
> underscore prefix already indicates that it's internal. The "intern" is
> redundant and breaks the standard naming convention.
> 
> And already in the next commit, you're kinda dropping the convention anyway by
> not renaming "NM_SETTING_COMPARE_FLAG_INFERRABLE" to
> "NM_INTERN_SETTING_COMPARE_FLAG_INFERRABLE"...

well, naming... certainly can be discussed. I named nm-core-"intern".h to mirror _nm_"intern"_*(), both could be changed to internAL. Or we could drop the _nm_intern_ prefix...??

Any other opinion? ... I don't really mind.


> > libnm-core: add nm_utils_hash_values_to_slist()
> 
> Hm... so when we had the conversation about public-vs-internal API yesterday, I
> was speaking entirely in the abstract, and hadn't seen this function yet. For
> this, I feel like it should NOT be public API, because it's not actually
> NetworkManager-related functionality; it's just filling in a gap in GLib...

How about renaming it to _nm_utils_hash_values_to_slist() (or _nm_intern_...) and expose it in another header file?

Maybe add another private header nm-core-extern.h (or "external", and maybe call the function _nm_extern_utils_hash_values_to_slist()). nm-core-extern.h should contain undocumented functions that are exported by libnm.so.

I think we should just export the symbol and not using nm_utils_get_private(). nm_util_get_private() does not really hide the function from the ABI (depending on what you define as ABI :) ). It still means we cannot ever remove or modify functionality (without breaking clients).


Well commit b9fef07fff7ecc806563162b8311a070e98b23e6 kinda did that:

-const NMUtilsPrivateData *
+gconstpointer
 nm_utils_get_private (void)
 {
-    return &data;
+    /* We told you not to use it! */
+    g_assert_not_reached ();
 }

I think this ~can~ be acceptable, because *only* NetworkManager service used this function. But it effectively breaks backward compatibly for libnm-util to NM versions pre-libnm-split -- which I think is almost acceptable to require libnm-util be *exactly* the same version as NetworkManager.

Or how about undoing that breaking aspect of b9fef07?



> > libnm-core: move validation of NMSettingConnection:type to NMSettingConnection:verify()
> 
> >+		s_base = nm_setting_find_in_list (all_settings, priv->type);
> >+		if (!s_base) {
> >+			g_set_error (error,
> 
> given that this commit comes after "add _nm_setting_find_in_list_required()",
> it's weird that you don't use it.

Later commits again rework the same code so that it works quite differently.


> > libnm-core: minor refactoring in NMSettingConnection:verify()
> 
> Why? This adds an extra level of indentation that isn't needed. (If the "if"
> clause is true, then we "return FALSE" at the end, so moving everything inside
> the "else" has no effect.

I dropped this commit (squashing the following commit).


> > libnm-core: normalize NMSettingConnection:type property
> 
> > It can also create a (default) base setting.
> 
> Some base types have mandatory-to-set properties (eg, NMSettingWireless:ssid),
> and so this won't result in a valid connection. But as the code is now,
> normalize() would fail with "802-11-wireless.ssid: property is missing" in that
> case. I think it would make more sense for it to fail with an error about the
> 802-11-wireless setting itself being missing. (So, if the newly-created base
> setting doesn't verify(), then throw it away rather than adding it to the
> connection, and let the connection fail to verify because of the missing base
> setting later.)

You are right. I added a major fixup! commit that only does this kind of normalization where we know it can work. Is that good now? Also added more tests.

commit
  "libnm-core: make failure cases for nm_connection_normalize() more
   robust against bugs"
tries to address the problem that we attempt normalization that fails later anyway.





> > keyfile/tests: add read_connection_from_file() utility function
> 
> would it make sense to add a "gboolean verifies" arg so you can use this to
> read connections that are expected to need normalization too?

Since commit
  "keyfile: let reader normalize() the connection, not only verify()"
the reader already does normalization (or fails).


also added new commits:

5d8558e libnm-core: fix NMSettingConnection:verify() not to modify
        interface-name
4ac2393 libnm-core: allow nm_setting_verify() to succeed individually
        without @all_settings
3f3a3dc libnm-core: add normalize of MTU for NMSettingInfiniband
1310069 fixup! libnm-core: make failure cases for nm_connection_normalize() 
        more robust against bugs
ba1b02d fixup! libnm-core: normalize NMSettingConnection:type property
643ca2f libnm-core: fix crash in NMSettingAdsl:verify()

(In reply to comment #23)
> I think we should just export the symbol and not using nm_utils_get_private().
> nm_util_get_private() does not really hide the function from the ABI (depending
> on what you define as ABI :) ). It still means we cannot ever remove or modify
> functionality (without breaking clients).

No, it doesn't. See comment 20.

At any rate, we don't need to export the symbol, because it only gets used from src/, and that links against libnm-core directly, so it has access to all of libnm-core's symbols.

> I think this ~can~ be acceptable, because *only* NetworkManager service used
> this function. But it effectively breaks backward compatibly for libnm-util to
> NM versions pre-libnm-split -- which I think is almost acceptable to require
> libnm-util be *exactly* the same version as NetworkManager.

We do require that.

(In reply to comment #24)
> (In reply to comment #23)
> > I think we should just export the symbol and not using nm_utils_get_private().
> > nm_util_get_private() does not really hide the function from the ABI (depending
> > on what you define as ABI :) ). It still means we cannot ever remove or modify
> > functionality (without breaking clients).
> 
> No, it doesn't. See comment 20.

Up to now, nm_utils_get_private() was only ever used by NM.
And you say you require libnm-util version and NM version to always match.

Given these two assumptions you are right.


However, I think that having libnm-util.so backward compatible also for NM might have been beneficial (1).

Also, I think that providing (private) utility functions to nmcli/nmtui could be useful. And then it could make sense that libnm.so provides backward compatibility to nmcli/nmtui and not requiring exactly matching versions (2).

The point was that to achieve (2), nm_utils_get_private() still requires us to keep an unchanged ABI and is no more private then directly exporting the functions.


But anyway. Question (1) is obsolete now the NM links statically to libnm-core.
Question (2) is avoided for now by not exposing anything public. _nm_utils_hash_values_to_slist() is now provided via nm-core-internal.h and not accessible by nmcli/nmtui.


> At any rate, we don't need to export the symbol, because it only gets used from
> src/, and that links against libnm-core directly, so it has access to all of
> libnm-core's symbols.
> 
> > I think this ~can~ be acceptable, because *only* NetworkManager service used
> > this function. But it effectively breaks backward compatibly for libnm-util to
> > NM versions pre-libnm-split -- which I think is almost acceptable to require
> > libnm-util be *exactly* the same version as NetworkManager.
> 
> We do require that.

Ok.



I did all the renaming you asked.

Repushed.

(In reply to comment #23)
> > Some base types have mandatory-to-set properties (eg, NMSettingWireless:ssid),
> > and so this won't result in a valid connection. But as the code is now,
> > normalize() would fail with "802-11-wireless.ssid: property is missing" in that
> > case. I think it would make more sense for it to fail with an error about the
> > 802-11-wireless setting itself being missing. (So, if the newly-created base
> > setting doesn't verify(), then throw it away rather than adding it to the
> > connection, and let the connection fail to verify because of the missing base
> > setting later.)
> 
> You are right. I added a major fixup! commit that only does this kind of
> normalization where we know it can work. Is that good now? Also added more
> tests.

Oh, I'd missed the fact that you explicitly excluded wifi already...

I don't think you should be hardcoding specific types there. Just create the setting, and see if it verifies. If it does, keep it, and if it doesn't, throw it out. Then it automatically does the right thing with any new settings types we add as well.

> commit
>   "libnm-core: make failure cases for nm_connection_normalize() more
>    robust against bugs"
> tries to address the problem that we attempt normalization that fails later
> anyway.

Sure, but I was talking about the fact that we'd fail with an error that didn't quite make sense. (Talking about a missing property in a setting that wasn't even there.)

> 5d8558e libnm-core: fix NMSettingConnection:verify() not to modify
>         interface-name

Ah, yeah, I did this in libnm-props-2 too. There's also a comment in _nm_connection_verify() that refers to the fact that NMSettingConnection:verify() modifies the connection, which needs to be edited now.

> 3f3a3dc libnm-core: add normalize of MTU for NMSettingInfiniband

So at one point during the libnm-props-* work, I had added an NMSetting:normalize() virtual method, so that we didn't have to cram all of that logic into NMConnection. The branch ended up going in a different direction, and those commits got dropped, but maybe they still make sense?

I pushed that work to danw/wip/normalize. Note that the final commit ("libnm-core: Use _nm_setting_normalize() for interface-name normalization") conflicts with what I eventually ended up doing in libnm-props-*, so you probably don't want to cherry-pick it, but it shows how it would work.

(This could also be fixed later rather than as part of this branch.)

> 1310069 fixup! libnm-core: make failure cases for nm_connection_normalize() 
>         more robust against bugs

>+		 * Reaching this line of code is actually a bug in verify(), but since it can be
>+		 * triggered from external, we don't want to g_warn() about it. */

Hm... I don't think that's correct. We don't want code that essentially does g_warning("the user passed bad data"), but that's not what's happening here. This is g_warning("nm_connection_normalize failed an internal consistency check"). The warning is triggered by the external data, but it's not *about* the external data, it's about a bug in the codepaths that should have been dealing with that external data.

> 643ca2f libnm-core: fix crash in NMSettingAdsl:verify()

This is basically unrelated and you can go ahead and commit it now.

Beyond what danw said....

> all: use _nm_utils_hash_values_to_slist()

Since you removed the g_strdup() for nm_settings_connection_get_seen_bssids(), you need to update the caller (get_settings_auth_cb) to not free the data values too.  Which is actually good, because we don't need to strdup() here at all, so lets save some memory.


> libnm-core: don't set GError on invalid @connection argument in _nm_connection_verify()

+g_return_val_if_fail (!error || !*error, NM_SETTING_VERIFY_ERROR);

This now requires 'error', and doesn't allow callers to pass NULL errors.  I'd imagine some callers might want to pass NULL and just get yes/no result for verify(), so I don't think we should require 'error'.


> keyfile/tests: test reading minimal keyfiles that needs normalization of type and slave-type
> libnm-core: add normalize of MTU for NMSettingInfiniband

I think any new tests should use g_test_add_func(), which is what we've been doing with other testcases at least.

> libnm-core: allow nm_setting_verify() to succeed individually without @all_settings

In the commit message, "togehter" -> "together"

(In reply to comment #26)
> (In reply to comment #23)
> > > Some base types have mandatory-to-set properties (eg, NMSettingWireless:ssid),
> > > and so this won't result in a valid connection. But as the code is now,
> > > normalize() would fail with "802-11-wireless.ssid: property is missing" in that
> > > case. I think it would make more sense for it to fail with an error about the
> > > 802-11-wireless setting itself being missing. (So, if the newly-created base
> > > setting doesn't verify(), then throw it away rather than adding it to the
> > > connection, and let the connection fail to verify because of the missing base
> > > setting later.)
> > 
> > You are right. I added a major fixup! commit that only does this kind of
> > normalization where we know it can work. Is that good now? Also added more
> > tests.
> 
> Oh, I'd missed the fact that you explicitly excluded wifi already...
> 
> I don't think you should be hardcoding specific types there. Just create the
> setting, and see if it verifies. If it does, keep it, and if it doesn't, throw
> it out. Then it automatically does the right thing with any new settings types
> we add as well.

It happens seldom enough that we add a new connection type. I think it is simpler to just hard code those few cases.

For "normalize()" we don't have to be overly generic. We support a few hard-coded cases that we anticipate, other settings just fail verification.
Normalization is to fix connections from legacy clients and to add a few convenience fixups (e.g. ensure IPvX settings, completion of base setting).


> > commit
> >   "libnm-core: make failure cases for nm_connection_normalize() more
> >    robust against bugs"
> > tries to address the problem that we attempt normalization that fails later
> > anyway.
> 
> Sure, but I was talking about the fact that we'd fail with an error that didn't
> quite make sense. (Talking about a missing property in a setting that wasn't
> even there.)

One way to get around this, would be to clone the setting before attempting to normalize it. And if normalization still fails, restore the previous state and return the original error message.

Downside: additional overhead for each normalization.



> > 5d8558e libnm-core: fix NMSettingConnection:verify() not to modify
> >         interface-name
> 
> Ah, yeah, I did this in libnm-props-2 too. There's also a comment in
> _nm_connection_verify() that refers to the fact that
> NMSettingConnection:verify() modifies the connection, which needs to be edited
> now.

comment fixed.


> > 3f3a3dc libnm-core: add normalize of MTU for NMSettingInfiniband
> 
> So at one point during the libnm-props-* work, I had added an
> NMSetting:normalize() virtual method, so that we didn't have to cram all of
> that logic into NMConnection. The branch ended up going in a different
> direction, and those commits got dropped, but maybe they still make sense?
> 
> I pushed that work to danw/wip/normalize. Note that the final commit
> ("libnm-core: Use _nm_setting_normalize() for interface-name normalization")
> conflicts with what I eventually ended up doing in libnm-props-*, so you
> probably don't want to cherry-pick it, but it shows how it would work.
> 
> (This could also be fixed later rather than as part of this branch.)

We already talked about this when I added nm_connection_normalize(). I still think that is a bad idea: https://bugzilla.redhat.com/show_bug.cgi?id=979425#c18

In any case, lets do that later.


> > 1310069 fixup! libnm-core: make failure cases for nm_connection_normalize() 
> >         more robust against bugs
> 
> >+		 * Reaching this line of code is actually a bug in verify(), but since it can be
> >+		 * triggered from external, we don't want to g_warn() about it. */
> 
> Hm... I don't think that's correct. We don't want code that essentially does
> g_warning("the user passed bad data"), but that's not what's happening here.
> This is g_warning("nm_connection_normalize failed an internal consistency
> check"). The warning is triggered by the external data, but it's not *about*
> the external data, it's about a bug in the codepaths that should have been
> dealing with that external data.

I don't agree. g_critical/g_return and g_warning are essentially asserts. An untrusted user must never be able to trigger an assertion failure.


> > 643ca2f libnm-core: fix crash in NMSettingAdsl:verify()
> 
> This is basically unrelated and you can go ahead and commit it now.

commited.

(In reply to comment #27)
> Beyond what danw said....
> 
> > all: use _nm_utils_hash_values_to_slist()
> 
> Since you removed the g_strdup() for nm_settings_connection_get_seen_bssids(),
> you need to update the caller (get_settings_auth_cb) to not free the data
> values too.  Which is actually good, because we don't need to strdup() here at
> all, so lets save some memory.

added fixup commit (and fixed a memleak along the way: fe6b002f0f06cefa801b1d0a8603b95b52d27c62 already merged to master)


> > libnm-core: don't set GError on invalid @connection argument in _nm_connection_verify()
> 
> +g_return_val_if_fail (!error || !*error, NM_SETTING_VERIFY_ERROR);
> 
> This now requires 'error', and doesn't allow callers to pass NULL errors.  I'd
> imagine some callers might want to pass NULL and just get yes/no result for
> verify(), so I don't think we should require 'error'.

hm, I think it is right. This inverse logic of "return_if_fail" confuses me every time too :)


> > keyfile/tests: test reading minimal keyfiles that needs normalization of type and slave-type
> > libnm-core: add normalize of MTU for NMSettingInfiniband
> 
> I think any new tests should use g_test_add_func(), which is what we've been
> doing with other testcases at least.

done.

Added two new commits:

  d70064c tests: refactor tests to use g_test framework (g_test_add_func)
  b012ff6 core: add compatibility wrapper for g_test_add_data_func_full()
          to nm-glib-compat.h



> > libnm-core: allow nm_setting_verify() to succeed individually without @all_settings
> 
> In the commit message, "togehter" -> "together"

done



Repushed

(In reply to comment #28)
> > I don't think you should be hardcoding specific types there. Just create the
> > setting, and see if it verifies. If it does, keep it, and if it doesn't, throw
> > it out. Then it automatically does the right thing with any new settings types
> > we add as well.
> 
> It happens seldom enough that we add a new connection type. I think it is
> simpler to just hard code those few cases.

I disagree that it's simpler, and note that we might add connection types for all of the devices that are currently "generic" in the future.

> I don't agree. g_critical/g_return and g_warning are essentially asserts. An
> untrusted user must never be able to trigger an assertion failure.

*NO* user should ever be able to trigger an assertion failure. But that is equivalent to saying "NM should not have bugs". When there are bugs, correct actions on the part of the user may result in assertions being hit.

The question to ask is not "can a user cause this code to be reached?", it's "if this code is reached, does that indicate a bug in NetworkManager which needs to be fixed?". If nm_connection_verify() returns NORMALIZABLE_ERROR when the connection is not actually normalizable then that's a bug, and we need to fix it. Thus, it is correct to warn/assert/whatever.

I'm fine with the rest of it now, once you and danw work out comment 30.

(In reply to comment #30)
> (In reply to comment #28)
> > > I don't think you should be hardcoding specific types there. Just create the
> > > setting, and see if it verifies. If it does, keep it, and if it doesn't, throw
> > > it out. Then it automatically does the right thing with any new settings types
> > > we add as well.
> > 
> > It happens seldom enough that we add a new connection type. I think it is
> > simpler to just hard code those few cases.
> 
> I disagree that it's simpler, and note that we might add connection types for
> all of the devices that are currently "generic" in the future.

added two fixups:

  1c0b019 fixup! libnm-core: normalize NMSettingConnection:type property
  8442464 fixup! libnm-core: normalize NMSettingConnection:type property


> > I don't agree. g_critical/g_return and g_warning are essentially asserts. An
> > untrusted user must never be able to trigger an assertion failure.
> 
> *NO* user should ever be able to trigger an assertion failure. But that is
> equivalent to saying "NM should not have bugs". When there are bugs, correct
> actions on the part of the user may result in assertions being hit.
> 
> The question to ask is not "can a user cause this code to be reached?", it's
> "if this code is reached, does that indicate a bug in NetworkManager which
> needs to be fixed?". If nm_connection_verify() returns NORMALIZABLE_ERROR when
> the connection is not actually normalizable then that's a bug, and we need to
> fix it. Thus, it is correct to warn/assert/whatever.

and another fixup for this too.


Repushed

looks good

(In reply to comment #32)
>   1c0b019 fixup! libnm-core: normalize NMSettingConnection:type property

 I would have just passed NULL for all_settings. And I definitely don't think you need to add the setting itself to all_settings first. But it's fine as-is if you like it that way.

(In reply to comment #33)
> (In reply to comment #32)
> >   1c0b019 fixup! libnm-core: normalize NMSettingConnection:type property
> 
>  I would have just passed NULL for all_settings. And I definitely don't think
> you need to add the setting itself to all_settings first. But it's fine as-is
> if you like it that way.

I prefer it that way then :)

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



I think we can close this bug now. You really can create quite minimal keyfiles and normalization can handle the requested cases. For any other/remaining issues, let's start a new bug.

*** Bug 722295 has been marked as a duplicate of this bug. ***