GNOME Bugzilla – Bug 732292
add better meta-data support to NMConnection, enhancing more generic clients and setting-plugins
Last modified: 2018-04-05 21:23:22 UTC
NMConnection/libnm-util does not provide enough meta-data about the settings itself. That makes it tedious to add new properties, to extend/add NM-clients or setting plugins. I think the solution to bug 704866 is improving libnm-util. First the problem: Look how all the clients and settings plugins do the same stuff over and over again (and again). For example, keyfile and ifcfg-rh have (duplicated) approaches of converting the same properties to/fro string. When adding a new property, you have to define it to both settings plugins. Ok, keyfile has "some" logic to detect standard properties automatcially. But often that fails, and then we have to teach it manually. Just take for example what was necessary for adding one simple property: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=fad5472b343f89ce384c61e6cb40016880b9b70c . It neeed changes to cli, ifcfg-rh, and keyfile. nmtui doesn't support it yet, because we didn't add it. If nm-applet ever shall support it, we have to add it manually too. This is additional effort, and it is duplicated code/knowledge. Then, when adding a property (e.g. to nmcli), just look how "knowledge" you have to bring in there. Look at NM_SETTING_BOND_OPTION_MODE. This is an "enum" of values like "active-backup", "balence-rr", etc. The knowledge which mode exist is spread over src/devices/nm-device-bond.c, libnm-util/nm-setting-bond.c, cli, tui, nm-connection-editor. You cannot even ask libnm-util which are the known modes if you know you have a NM_SETTING_BOND_OPTION_MODE at hand. Let alone, handling the property generically for a simple set-from-string operation. Ok, you can set the string blindly and call NMSettingBond:verify() -- like the settings plugins do. But apparently that is not sufficient for cli, tui or nm-a (which want to show a meaning full error message or provide completion). Do we at least agree, that this is a huge(?) problem and causes never ending work? The solution is that NMConnection and NMSetting provide better metadata about what properties there are, what is their name, their type, their to_string(), from_string(), ... you name it. Everything we possibly need. Note that there are two kinds of settings. Those that have a static, well-known set of properties (lets call them "static settings"). Their properties are mostyl implemented as glib-properties. Almost all of our settings to date are of this kind. (NMSettingBond is a bit special, as it is effectively a static-setting, but implemented with a hash-table). There was bug rh#1032808 reworking that, but I stopped on that, because I was essentially adding all the metadata to NMSettingBond but not reusable for other settings. We do have quite some metadata of these properties due to the glib-properties. We can ask the setting which properties it has, we can ask the (glib) type, a description string. But it is not enough(!!). We need more. And we need to unify it with the "dynamic settings" so that client can access properties generically. "dynamic settings" are currently only NMSettingVPN. All their values are effectively strings, and we cannot validate them (bug 704866 I hear you). They are opaque values. We currently don't have them, but you could also image "hybrid settings", that contain a static set of properties and an (open) set of dynamics properties. We need to add more meta-data to the NMSetting. Let's add new functions that give us all we need. Let's call them nm_setting_meta_*(). The "key" for a property is still a (unique) name. We need to be able to do "everything" with a property what we need. Compare, set, get, to/fro string. The to/fro string functions must accept a "type" argument, because the syntax for ifcfg-rh might be different then nmcli. There could be "NM_SETTING_META_TO_STRING_DEFAULT", _DEBUG, _KEYFILE, _NMCLI, _IFCFG_RH. (of course, in many cases they will all return NM_SETTING_META_TO_STRING_DEFAULT). We need to get the default value, reset it to default, know if it can be NULL, etc. We need to know if a property is an enumerable (string). If it is, we need to get all known values. Do we need help text for the enum values? We need a concept of "property-type" that goes beyond GType. A G_TYPE_STRING is not enough to express what we need to know. We need to be aware of secrets (e.g. hide them in NM_SETTING_META_TO_STRING_DEBUG). And the final step is for the VPN plugins to allow a way to provide metadata for them, so that we actually know something about the dynamic settings too, beyond that they have opaque string values. This is the solution to 704866. Also note, that NM already tries to do this partly. Look at nm_setting_get_secret_flags(). Actually, nm_setting_get_secret_flags() is wrong in subtle ways because the function does not properly abstract between static settings and vpn (so that an assertion fails for me whenever NM requests secrets -- I tried to fix it, but it's a problem of wrong abstraction, I didn't know how to do it). We still will use glib properties internally, and reuse all the metadata that is saved there. And the base class, of course a suitable default implementation for most cases. Calling g_object_set() must have the same effect as nm_setting_meta_set_property(). But the former only works if you already know the property you have at hand and it does not work for NMSettingVPN. This way, we also don't break API and don't have to change clients. Something like http://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/cli/src/connections.c#n131 should not provided by libnm-util (just picking cli arbitrarily. Could be settings plugins, tui, nm-a, etc.)
I'm not sure I'd even bother with "meta", wouldn't nm_setting_set_property() be fine, if we intend for this to be used in addition to (or in replacement of) g_object_set_property()? Also, would the new function take GValues like g_object_get/set_property(), or use GVariant? WRT to the to/from string stuff, I'd rather keep plugin-specific knowledge out of the core (eg, NM_SETTING_META_TO_STRING_IFCFG_RH) and keep that in the plugins though. I'm not sure that a lot of code for to/from string could actually be shared, at least for the formatting bits for each plugin. Instead, what if we allowed a caller to specify "hooks" instead, that got called when doing the conversion? Those could be passed to the to/from string functions to transform the plugin-specific format to one the setting wanted.
(In reply to comment #1) > I'm not sure I'd even bother with "meta", wouldn't nm_setting_set_property() be > fine, if we intend for this to be used in addition to (or in replacement of) > g_object_set_property()? If you know the exact settings type, you can continue to use g_object_set_property(). This becomes interesting when handling settings generically. Then you want to use nm_setting_set_property() (which internally does the right thing). > Also, would the new function take GValues like g_object_get/set_property(), or > use GVariant? Probably, GValues are more suited -- as they are also used for g_object_get() and play together with GParamSpec. > WRT to the to/from string stuff, I'd rather keep plugin-specific knowledge out > of the core (eg, NM_SETTING_META_TO_STRING_IFCFG_RH) and keep that in the > plugins though. I'm not sure that a lot of code for to/from string could > actually be shared, at least for the formatting bits for each plugin. Instead, keyfile is the native configuration format of NM. It would be great if clients could read/write them too. E.g. import/export connection in nmcli via keyfile. another option could be "debug" string, which would be useful for nm_utils_log_connection_diff(). Mostly all format-types would just return the same string representation. > what if we allowed a caller to specify "hooks" instead, that got called when > doing the conversion? Those could be passed to the to/from string functions to > transform the plugin-specific format to one the setting wanted. hooks might be great. But having "default", "keyfile", "debug" would already be a good start. And important: a to/fro string that actually works (round-trip)
The first code has been pushed to jk/libnm-nmcli-metadata. It adds meta-data to libnm and uses that in nmcli. I will work through review comments from https://bugzilla.redhat.com/show_bug.cgi?id=1034126 (#c6 and #c7) shortly.
As danw also mentioned, we already have the internal class NMSettingProperty... that is actually the same as I suggested with "NMProperty". I'd just prefer the latter name, because NMSettingProperty sounds like a [property] NMSetting subclass... and because NMProperty could be done independently from NMSetting and could provide metadata for other types as well -- just like GParamSpec does not only work on a NMSetting.
I looked into using the NMSettingProperty, but it just complicates things and messes with the overrides. Moreover a function like nm_setting_class_set_valid_property_values (class, ...) would first look up a property, which is an extra step provided we have GParamSpec at hand when installing properties. I prefer to keep things plain and simple. > Maybe add wrapper functions so you don't need to muck with quarks directly, and to make it look more > like the existing property override/transform stuff. Added wrapper functions for setting up metadata _nm_setting_property_set_...() > We could now have an NMSetting-level function that does this using the new metadata: > if (!_nm_setting_validate_slist_property (setting, NM_SETTING_802_1X_EAP, priv->eap, error)) > return FALSE; I have added _nm_setting_validate_string_property() and _nm_setting_validate_slist_property() for validating properties. Pushed changes as a fixup commit and re-pushed jk/libnm-cli-metadata (re-based on master).
Overall I like this approach. Just some small questions... What was the plan for NM_SETTING_PROPERTY_METADATA_HASH? Couldn't that be determined by checking the GParamSpec->value_type == G_TYPE_HASH_TABLE already, like is done for nm_setting_property_is_boolean()? > cli: switch to using libnm properties' metadata in nmcli The patch replaces should_complete_files() but there's a lot more file properties before than after. In libnm-core only pac_file, ca_path, and phase2_ca_path are marked as 'filename' but the old code has the 'scheme'-based properties too like 'ca-cert', client-cert, etc and the 'team' setting's 'config' item. Perhaps we should instead mark some properties as "can be a filename but can be other stuff too"?
(In reply to Dan Williams from comment #6) > Overall I like this approach. Just some small questions... > > What was the plan for NM_SETTING_PROPERTY_METADATA_HASH? Couldn't that be > determined by checking the GParamSpec->value_type == G_TYPE_HASH_TABLE > already, like is done for nm_setting_property_is_boolean()? > Yes, it could. The function is meant for option properties in the form of key1=value1, key2=value2, but it appears all of those are of G_TYPE_HASH_TABLE type. Fixed accordingly. > > cli: switch to using libnm properties' metadata in nmcli > > The patch replaces should_complete_files() but there's a lot more file > properties before than after. In libnm-core only pac_file, ca_path, and > phase2_ca_path are marked as 'filename' but the old code has the > 'scheme'-based properties too like 'ca-cert', client-cert, etc and the > 'team' setting's 'config' item. Perhaps we should instead mark some > properties as "can be a filename but can be other stuff too"? I have added nm_setting_property_maybe_filename() for these properties. team and team-port "config" property should contain raw JSON data. So I special cased it just in nmcli (because it accepted file names too). Re-pushed squashed branch rebased on master.
+#define NM_SETTING_PROPERTY_METADATA_VALID_VALUES "metadata-valid-values" +#define NM_SETTING_PROPERTY_METADATA_FILENAME "metadata-filename" +#define NM_SETTING_PROPERTY_METADATA_MULTI "metadata-multi" +NM_AVAILABLE_IN_1_2 +gconstpointer nm_setting_property_get_metadata (NMSetting *setting, + const char *property_name, + const char *datum); this function is unused, I would not make it as public API. At least not yet, unless there is a good usecase. +NM_AVAILABLE_IN_1_2 +gboolean nm_setting_property_is_hash (NMSetting *setting, + const char *property_name); +NM_AVAILABLE_IN_1_2 +gboolean nm_setting_property_is_boolean (NMSetting *setting, + const char *property_name); wouldn't it be better to expose a GType? After all, every property we'll ever have will have a GType, so you can say: prop = nm_setting_property_get_gtype(setting,name); so you can do: if (nm_setting_property_get_gtype (setting, "PROP") == G_BOOLEAN) { }
The properties implementation currently does not work correctly for NMSettingVpn. You don't have to implement it from the start, just g_return_val_if_fail (!NM_IS_SETTING_VPN (setting)); then no user can use that (non-implemented) API for NMSettingVpn -- until we implement it.
(In reply to Thomas Haller from comment #8) > +#define NM_SETTING_PROPERTY_METADATA_VALID_VALUES "metadata-valid-values" > +#define NM_SETTING_PROPERTY_METADATA_FILENAME "metadata-filename" > +#define NM_SETTING_PROPERTY_METADATA_MULTI "metadata-multi" > +NM_AVAILABLE_IN_1_2 > +gconstpointer nm_setting_property_get_metadata (NMSetting *setting, > + const char > *property_name, > + const char *datum); > > this function is unused, I would not make it as public API. At least not > yet, unless there is a good usecase. > The function is not used because it is a library-convenience function, and there can't just be any user of that yet. But, I pushed a fixup removing the function if you prefer. > +NM_AVAILABLE_IN_1_2 > +gboolean nm_setting_property_is_hash (NMSetting *setting, > + const char > *property_name); > +NM_AVAILABLE_IN_1_2 > +gboolean nm_setting_property_is_boolean (NMSetting *setting, > + const char > *property_name); > > wouldn't it be better to expose a GType? > After all, every property we'll ever have will have a GType, so you can say: > > prop = nm_setting_property_get_gtype(setting,name); > > so you can do: > > if (nm_setting_property_get_gtype (setting, "PROP") == G_BOOLEAN) { > } > Added nm_setting_property_get_gtype(), but I would not remove _is_boolean(), _is_hash(). They are more friendly to users. >The properties implementation currently does not work correctly for NMSettingVpn. How come???
+ g_return_val_if_fail (NM_IS_SETTING (setting), G_TYPE_INVALID); + g_return_val_if_fail (property_name, FALSE); ^^^^^ gboolean nm_setting_property_is_boolean (NMSetting *setting, const char *property_name) { return nm_setting_property_get_gtype (setting, property_name) == G_TYPE_BOOLEAN; } >> The properties implementation currently does not work correctly for >> NMSettingVpn. > How come??? e.g Openvpn setting: [vpn] connection-type=tls remote=134.0.24.109 comp-lzo=yes cert-pass-flags=0 port=11197 nm_setting_property_is_boolean (my_ovpn, "comp-lzo") gives FALSE. nm_setting_property_is_hash (my_ovpn, "settings") gives TRUE. both is wrong.
(In reply to Thomas Haller from comment #11) > nm_setting_property_is_hash (my_ovpn, "settings") gives TRUE. should be: nm_setting_property_is_hash (my_ovpn, "data") gives TRUE.
(In reply to Thomas Haller from comment #11) > > + g_return_val_if_fail (NM_IS_SETTING (setting), G_TYPE_INVALID); > + g_return_val_if_fail (property_name, FALSE); > ^^^^^ > > gboolean > nm_setting_property_is_boolean (NMSetting *setting, const char > *property_name) > { > return nm_setting_property_get_gtype (setting, property_name) == > G_TYPE_BOOLEAN; > } > Fixed. > > >> The properties implementation currently does not work correctly for > >> NMSettingVpn. > > How come??? > > e.g Openvpn setting: > > [vpn] > connection-type=tls > remote=134.0.24.109 > comp-lzo=yes > cert-pass-flags=0 > port=11197 > > nm_setting_property_is_boolean (my_ovpn, "comp-lzo") gives FALSE. > nm_setting_property_is_hash (my_ovpn, "settings") gives TRUE. > > both is wrong. I don't like handling bond.options, vpn.data properties differently than other properties (and pretending the options inside are the actual properties). But Thomas seems to have a very strong opinion on that. So I have added additional two fixup commits ...
(In reply to Thomas Haller from comment #11) > > + g_return_val_if_fail (NM_IS_SETTING (setting), G_TYPE_INVALID); > + g_return_val_if_fail (property_name, FALSE); > ^^^^^ > > gboolean > nm_setting_property_is_boolean (NMSetting *setting, const char > *property_name) > { > return nm_setting_property_get_gtype (setting, property_name) == > G_TYPE_BOOLEAN; > } > > > > >> The properties implementation currently does not work correctly for > >> NMSettingVpn. > > How come??? > > e.g Openvpn setting: > > [vpn] > connection-type=tls > remote=134.0.24.109 > comp-lzo=yes > cert-pass-flags=0 > port=11197 > > nm_setting_property_is_boolean (my_ovpn, "comp-lzo") gives FALSE. > nm_setting_property_is_hash (my_ovpn, "settings") gives TRUE. > > both is wrong. Well, the stuff contained in 'data' and 'secrets' aren't NMSetting properties, so these return the correct result. That's why there's nm_setting_vpn_get_data_item() and such, and why they have to be strings. So 'comp-lzo' isn't actually a boolean anyway. In this case at least, I think the is_hash/is_boolean functions are doing the correct thing. It would be inconsistent to make them try to introspect into the VPN/bond options. Our hash table properties are obviously sub-optimal, but given that we can't have arbitrary setting properties they're the best we've got until we solve that larger issue. Also, if we keep the function, how about nm_setting_property_has_metadata() instead of nm_setting_property_metadata_implemented()?
Thomas, can you clarify what you meant like you did to me on IRC earlier today? Just for the record here.
(In reply to Dan Williams from comment #15) > Thomas, can you clarify what you meant like you did to me on IRC earlier > today? Just for the record here. I was talking about whether to expose a ~property~ "options" (NMSettingBond) and "data","secrets" (NMSettingVpn). That is in my opinion wrong, because the nm_setting_property*() functions should peek into those hashes. But anyway. In my opinion the purpose of this BZ is very much to add support for NMSettingVpn. I mean, ~meaningfull~ metadata support, beyond saying: "I got a "data" hash, but I don' know more". This will solve a very concrete problem: how to add VPN editing support to nmcli and nmtui (beyond editing hashes). Arguable, it is useful (on it's own) to move NmcPropertyFuncs from cli to libnm. The current branch is indeed doing that. It is useful, simply because it could be reused by other clients. Virtually every editor needs to do value<->string conversions. I really think we should do this right from the start (whatever that means :) ). In my opinion that means, that we add classes for the data types like: Property type classes like: NMMetaInteger NMMetaString NMMetaEnum NMMetaPassword NMMetaFilename NMMetaStringHash ... and also: NMMetaSetting NMMetaConnection and then you can ask NMMetaSetting: "what properties are there, and what can I do with them?". For our usual settings (NMSettingConnection) we can immediately construct these meta classes based on GObject properties. Something like: NMMetaSetting *nm_meta_setting_new_introspect_gobject (GType setting_type); For NMSettingBond we manually construct it, and for NMSettingVpn we make it loadable via VPN plugins.
(In reply to Thomas Haller from comment #16) > (In reply to Dan Williams from comment #15) > > Thomas, can you clarify what you meant like you did to me on IRC earlier > > today? Just for the record here. > > I was talking about whether to expose a ~property~ "options" (NMSettingBond) > and "data","secrets" (NMSettingVpn). That is in my opinion wrong, because > the nm_setting_property*() functions should peek into those hashes. Ok, that does make sense, and we also have existing virtual methods of NMSetting that already peek into the hashes, like nm_setting_get_secret_flags() and such that are handled by the subclass. > But anyway. In my opinion the purpose of this BZ is very much to add support > for NMSettingVpn. I mean, ~meaningfull~ metadata support, beyond saying: "I > got a "data" hash, but I don' know more". Yeah, agreed. > This will solve a very concrete problem: how to add VPN editing support to > nmcli and nmtui (beyond editing hashes). > > > Arguable, it is useful (on it's own) to move NmcPropertyFuncs from cli to > libnm. The current branch is indeed doing that. It is useful, simply because > it could be reused by other clients. Virtually every editor needs to do > value<->string conversions. Yeah. > I really think we should do this right from the start (whatever that means > :) ). That would be nice, yes. > In my opinion that means, that we add classes for the data types like: > > Property type classes like: > NMMetaInteger > NMMetaString > NMMetaEnum > NMMetaPassword > NMMetaFilename > NMMetaStringHash We have the following situations: 1) normal GObject properties where we can easily determine the time, min, max, and default values, and whether it's secret 2) normal GObject properties that have additional metadata like whether it's a file path; only filename comes to mind here so far 3) hash table GObject properties like vpn.secrets, vpn.data, bond.options, wired.s390-options which are always string::string hashes 4) ip.route-data and ip.address-data which is a hash of string::variant > ... > and also: > NMMetaSetting > NMMetaConnection Having a bunch of new classes seems pretty heavy; why couldn't the methods just exist on NMSetting and the subclasses? I think we can do all this within the existing object framework fairly simply still.
To push this idea further, what about instead of classes for types, we just use GType for all of them since that's all we really need here for the leaves of the property tree. I've always wanted to refer to properties via '.' notation, eg 'wired.mac-address' and 'bond.options.mode' or 'vpn.data.NAT Traversal Mode'. 3 or 4 levels of properties should be enough, if we go beyond 4 levels then we really need to stop and think about why we're nesting stuff so deeply. gboolean nm_connection_get_property_basic (NMConnection *connection, const char *property, gpointer *out_value); const char *mode = NULL; nm_connection_get_property_basic (connection, "bond.options.mode", &mode); or gboolean nm_connection_get_property_basic (NMConnection *connection, gpointer *out_value, const char *setting_name, ...); const char *mode = NULL; nm_connection_get_property_basic (connection, &mode, NM_SETTING_BOND_SETTING_NAME, NM_SETTING_BOND_OPTIONS, NM_SETTING_BOND_OPTION_MODE, NULL); or something similar. And then maybe g_hash_table_iter() style functions for iterating through a whole connection or setting's properties in the 2nd and 3rd level (eg, iterating through all possible values of 'bond.options' or 'vpn.data'). Basically, create our own thin abstraction layer over the NMSetting + GObject API which allows you to: 1) list all properties and sub-properties of the setting 2) get information about those properties and sub-properties, including type, min/max, default, flags (secret, filename, etc) 3) maybe even get/set property values too Maybe? What do people think?
(In reply to Dan Williams from comment #17) > (In reply to Thomas Haller from comment #16) ... > > and also: > > NMMetaSetting > > NMMetaConnection > > Having a bunch of new classes seems pretty heavy; why couldn't the methods > just exist on NMSetting and the subclasses? I think we can do all this > within the existing object framework fairly simply still. possbile. In principle both are same (and could be translated from one to the other): nm_setting_property_get_as_string(setting,name) { return nm_meta_get_string (nm_setting_get_property(setting,name)); } nm_meta_get_string(meta_property, target) { return nm_setting_property_get_as_string (target, nm_meta_get_name (meta_property)); } but from an API point of view, I think it's much neater to have: nm_meta_get_*(meta, target); nm_meta_set_*(meta, target, value); nm_meta_is_*(meta); nm_settting_get_property(setting, name); than having them all in NMSetting: nm_setting_property_get_*(setting, name); nm_setting_property_set_*(setting, name); nm_setting_property_is_*(setting, name); I also find it nicer: meta = nm_setting_get_prioperty(setting, name); if (nm_meta_is_filename (meta)) { nm_meta_filename_some_special_accessor ((NMMetaFilename *))meta, setting); } then: if (nm_setting_property_is_filename (setting, name)) { nm_setting_property_filename_some_special_accessor (setting, name); } (In reply to Dan Williams from comment #18) > Basically, create our own thin abstraction layer over the NMSetting + > GObject API which allows you to: > > 1) list all properties and sub-properties of the setting > 2) get information about those properties and sub-properties, including > type, min/max, default, flags (secret, filename, etc) > 3) maybe even get/set property values too I agree with this. You definitely need API to ask which properties exist at all. and that you access them by name. The "." notation looks like a nice detail.
I am gonna close this as obsolete. nmcli in the meantime uses https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/clients/common/nm-meta-setting-desc.c?id=3f969d3b5b95f3d0f4fc0ba7e8d42bdf85bdef38#n4978 , which significantly changed how nmcli works. It still doesn't fully implement this bug, because: - the meta-data approach is not public API, nor used outside of nmcli - it only works for properties that have corresponding GObject properties in the NMSetting* class. This is cumbersome, because it doesn't work for NMSettingVpn, NMSettingBond, and NMSettingUser. Also, in general, creating a NMSetting class and adding properties to it is too much boiler place and copy-paste. Optimally, you wouldn't need different GObject types to implement a new setting, and no GObject properties. While there is still to do, this RFE doesn't help in implementing it. If you need inspiration, you can read up on it, or better: look at the current code and think how it could be done better. Closing as obsolte.
Created attachment 370562 [details] [review] [patch] attach content of jk/libnm-nmcli-metadata-bgo732292 branch Applies to commit 88f3aba9bfaf8435bae8a4836f5fdb955c21ac2d