GNOME Bugzilla – Bug 749648
[review] make ipvx.dns-options having an distinct default value [th/default-dns-options-bgo749648]
Last modified: 2015-06-05 10:37:56 UTC
together with bug 695383, I think certain properties should have a distinct default value. Adding that to ipv4.dns-options and ipv6.dns-options. And a few related fixes.
http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/default-dns-options-bgo749648
> libnm: make dns-options support an "undefined" default value + for (i = 0; strv[i]; i++) { + if ( _nm_utils_dns_option_validate (strv[i], NULL, NULL, FALSE, NULL) + && _nm_utils_dns_option_find_idx (priv->dns_options, strv[i]) < 0) The indentation seems wrong here. > cli: show dns-options default value +DEFINE_GETTER_WITH_DEFAULT(nmc_property_ipv6_get_dns_options, NM_SETTING_IP_CONFIG_DNS_OPTIONS, !nm_setting_ip_config_has_dns_options ((NMSettingIPConfig *) setting)) Missing whitespace before first '('. Just a doubt, is it possible to set ipvX.dns-options as empty (non-default) from cli?
(In reply to Beniamino Galvani from comment #2) > > libnm: make dns-options support an "undefined" default value > > + for (i = 0; strv[i]; i++) { > + if ( _nm_utils_dns_option_validate > (strv[i], NULL, NULL, FALSE, NULL) > + && _nm_utils_dns_option_find_idx > (priv->dns_options, strv[i]) < 0) > > The indentation seems wrong here. > > > cli: show dns-options default value > > +DEFINE_GETTER_WITH_DEFAULT(nmc_property_ipv6_get_dns_options, > NM_SETTING_IP_CONFIG_DNS_OPTIONS, !nm_setting_ip_config_has_dns_options > ((NMSettingIPConfig *) setting)) > > Missing whitespace before first '('. fixed. > Just a doubt, is it possible to set ipvX.dns-options as empty (non-default) > from cli? Yes, but it's not very nice. setting default: $ nmcli connection modify eth0 ipv4.dns-options "" setting empty: $ nmcli connection modify eth0 ipv4.dns-options " " there was a bug that edit mode didn't work correctly. In course of fixing that, I did some other refactoring too... Repushed.
LGTM
> ifcfg-rh: add svGetValueFull() function + * svGetValueFull() will return empty values if that is the value fo the @key. */ fo -> for, I think? > libnm: make dns-options support an "undefined" default value + * is empty, there are two similar (but differenciated) states. "differentiated" Also, maybe instead of adding an argument to the clear function, you could add a new "gboolean nm_setting_property_set_empty(setting, <property_name>)". That way we could use that for other values that we can't change the function signatures for (like addresses/routes), and keep the DNS options function signatures consistent with the other ones?
(In reply to Dan Williams from comment #5) > > ifcfg-rh: add svGetValueFull() function > > + * svGetValueFull() will return empty values if that is the value fo the > @key. */ > > fo -> for, I think? > > > > libnm: make dns-options support an "undefined" default value > > + * is empty, there are two similar (but differenciated) states. > > "differentiated" fixed the two above. > Also, maybe instead of adding an argument to the clear function, you could > add a new "gboolean nm_setting_property_set_empty(setting, > <property_name>)". That way we could use that for other values that we > can't change the function signatures for (like addresses/routes), and keep > the DNS options function signatures consistent with the other ones? do you mean adding a generic function o NMSetting base class? That would be bug 732292 and a much larger piece of work. Until we add proper generic support to settings, we can continue to add functions that are property specific. They don't conflict with the overall goal of having a generic way to treat all properties. The comparison with "the other ones" is not entirely correct, because those others not have a distinct "default" value. In that regard they behave differently, and it is not necessarily wrong, that the clear() function differs too. Anyway. I don't mind the details of these non-generic functions. How should I adjust them?
(In reply to Jiri Klimes from comment #7) > LGTM Thank you. rebased on master. Seems the only missing question is about the public API for the clear function (@dcbw).
(In reply to Thomas Haller from comment #6) > (In reply to Dan Williams from comment #5) > > > ifcfg-rh: add svGetValueFull() function > > > > + * svGetValueFull() will return empty values if that is the value fo the > > @key. */ > > > > fo -> for, I think? > > > > > > > libnm: make dns-options support an "undefined" default value > > > > + * is empty, there are two similar (but differenciated) states. > > > > "differentiated" > > fixed the two above. > > > > Also, maybe instead of adding an argument to the clear function, you could > > add a new "gboolean nm_setting_property_set_empty(setting, > > <property_name>)". That way we could use that for other values that we > > can't change the function signatures for (like addresses/routes), and keep > > the DNS options function signatures consistent with the other ones? > > do you mean adding a generic function o NMSetting base class? That would be > bug 732292 and a much larger piece of work. > > Until we add proper generic support to settings, we can continue to add > functions that are property specific. They don't conflict with the overall > goal of having a generic way to treat all properties. > > > The comparison with "the other ones" is not entirely correct, because those > others not have a distinct "default" value. In that regard they behave > differently, and it is not necessarily wrong, that the clear() function > differs too. > Anyway. I don't mind the details of these non-generic functions. How should > I adjust them? Others like DNS servers, addresses, routes, etc are all arrays and could have distinct "default" vs empty values though, right?
(In reply to Dan Williams from comment #9) > (In reply to Thomas Haller from comment #6) > > (In reply to Dan Williams from comment #5) > > The comparison with "the other ones" is not entirely correct, because those > > others not have a distinct "default" value. In that regard they behave > > differently, and it is not necessarily wrong, that the clear() function > > differs too. > > Anyway. I don't mind the details of these non-generic functions. How should > > I adjust them? > > Others like DNS servers, addresses, routes, etc are all arrays and could > have distinct "default" vs empty values though, right? Yes, they could, but currently they have no distinct default value. It might be difficult to add a different default-value now, because it could change the meaning of some existing settings. That would need careful consideration. Also, for "addresses" etc., I don't see a strong use-case for a distinct "default value" yet. Probably it will never make sense to configure default-addresses in a global config file. I add it here especially for dns-options, because it's new API and I could imagine a future use-case to configure them system-wide. I want to keep that possibility open, not implement it now (but it would be easy to do). The API (nm_setting_ip_config_clear_dns_options) should be consistent, but I'd be fine with the current attempt. What do you think?
Merged the branch to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=ec972ad3057fa14aec060715ca2e06121482ecb5 If the public API still needs rework, please reopen.