GNOME Bugzilla – Bug 740409
[review bg/hostname-bgo740409]: use /etc/hostname instead of NM configuration file in keyfile plugin
Last modified: 2015-11-24 12:10:41 UTC
ifcfg-rh plugin now uses /etc/hostname to store hostname. Do the same for keyfile plugin and use configuration file only as a fallback. Code is in branch jk/hostname-plugins.
>> utils: add nm_utils_file_set_contents() g_file_set_contents() writes the file atomically (first to a temporary file and rename), which is great. But there is a short time where the labels of the file are wrong. I would rather copy the entire g_file_set_contents() function to NM and set the labels on the temporary file before renaming(). And isn't there a library function to copy the selinux labels from an existing file? That could be used to copy the labels from the file to the temporary before renaming. >> ifcfg-rh: use nm_utils_file_set_contents() in ifcfg-rh's plugin_set_hostname() you could pass an error variable to set_contents an print it in the warning. >> keyfile: read/write hostname from/to /etc/hostname instead of NM config file I think this is related to bug 699458. What was the motivation for this bug and does it try to solve something different? See also the patch https://bugzilla.gnome.org/show_bug.cgi?id=699458#c9 The other patch I did back then had the same problem as this one. Reading a hostname via the setting plugins might be doable (just iterate over the plugins until one of them returns a hostname). But writing shows that having setting plugins manage the hostname is just wrong. Imagine you have plugin1,plugin2 enabled. Look at pk_hostname_cb(). We call set_hostname for all of them. Should we rather iterate until one plugin signals success (obviously, keyfile would always succeed)? As it is now, ifcfg-rh and keyfile will both end up writing the hostname to /etc/hostname. That is wrong. It is wrong to let setting plugins write/read the hostname. For example, on my system I disable ifcfg-rh plugin, because I prefer keyfile settings. But that does not mean I don't want the Fedora specific ways to write hostnames. This is all so overly complicated and could be so simple. Have one global function: void set_hostname(const char *hostname) { #if WRITE_TO_ETC set_hostname_write_to_etc(hostname); #endif #if WRITE_TO_HOSTNAMECTL set_hostname_write_to_hostnamectl(hostname); #endif #if WRITE_TO_WHATEVER set_hostname_write_to_whatever (hostname); #endif // and always write it to NM.conf (too). // urgh, why is it keyfile.hostname and not main.hostname?? set_hostname_write_to_nm_conf(hostname); } and reading hostname similarly. Also, in face of bug 732941, maybe we should instead write /var/lib/NetworkManager/hostname, and symlink /etc/hostname to it? > man: update keyfile hostname description In general I don't think its wrong to have hostname in NM.config. But it should not be keyfile specific. Why not main.hostname?
(In reply to comment #1) > It is wrong to let setting plugins write/read the hostname. > For example, on my system I disable ifcfg-rh plugin, because I prefer keyfile > settings. But that does not mean I don't want the Fedora specific ways to write > hostnames. Yeah, this should probably be distro-specific, not plugin-specific... Although, at this point, does any distro we support use anything other than /etc/hostname by default? > // and always write it to NM.conf (too). ... > Also, in face of bug 732941, maybe we should instead write > /var/lib/NetworkManager/hostname, and symlink /etc/hostname to it? No to both of those; NM does not own the hostname, it just provides an API for getting and setting it. And more specifically, on systemd-based systems, "hostnamed" owns /etc/hostname, and we should talk to that rather than rewriting /etc/hostname ourselves. (danw/wip/hostnamed has some old, untested code for that)
Fedora, SUSE, Debian, Ubuntu, and Arch all use /etc/hostname. Gentoo might use /etc/conf.d/hostname according to https://www.gentoo.org/doc/en/handbook/handbook-x86.xml?part=1&chap=8 so perhaps the 'ifnet' plugin would want to write the hostname there instead. I think it's reasonable to stop saving the hostname at the first plugin that reports success. If no plugin reports success, then NM will fall back to 'keyfile' which will try to write to /etc/hostname. If that fails then we just return an error. I don't think it's worth trying to save it anywhere else than /etc/hostname if no other plugin handles it. For backwards compat reasons, ifcfg-rh should still *read* from /etc/sysconfig/network, and keyfile should still read from NetworkManager.conf, but I don't think we care about saving changes back to those files. In the end you'd have: Fedora: ifcfg-rh clears /etc/sysconfig/network's HOSTNAME, and returns "can't write hostname"; keyfile writes to /etc/hostname Debian/Ubuntu: ifupdown returns "cant' write hostname" and keyfile handles it to /etc/hostname Arch: they just use keyfile, so no change Suse: I think they just use keyfile if they still use NM at all (I think they use wicked now?) Gentoo: ifnet would write to /etc/conf.d/hostname and report success --- keyfile could also detect if hostnamed is running and just send the hostname there instead of /etc/hostname too.
I pushed branch bg/hostname-bgo740409 which contains the following changes: - refactored the code for dealing with hostname files, moving it from plugins to core. The method used to persist the hostname can be selected at compile time between "default", "suse" and "ifnet". - added support for hostnamed. If hostnamed is available it always overrides other methods.
>> core: move handling of hostname from plugins to core read_sysconfig_hostname(void) ^^^ we do spaces here. read_sysconfig_hostname(void): I think _LOGW() should only be Debug-level. Missing file doesn't seem worth a warning. dispose() - g_free (priv->hostname); + g_clear_pointer (&priv->hostname, g_free); can you squash "settings: reference inotify helper singleton" into "core: move handling of hostname from plugins to core" write_hostname_to_file(): could you add a FIXME comment: /* FIXME: g_file_set_contents() writes first to a temporary file and renames * it atomically. We should hack g_file_set_contents() to set the SELINUX * labels before renaming the file. */ + } else { + hostname_eol = g_strdup_printf ("%s\n", hostname); + } no braces here :) could you pass an error argument to g_file_set_contents() and log it in nm_log_warn (LOGD_SETTINGS, "Could not save hostname: failed to create/open + char *hostname; + char *hostname_file; + gboolean hostname_file_ifnet; + GFileMonitor *hostname_monitor; + GFileMonitor *dhcp_monitor; + guint hostname_monitor_id; + guint dhcp_monitor_id; + gulong ih_event_id; + int sc_network_wd; could we rename those to indicate that they are all related to hostname? maybe even: struct { char *value; GFileMonitor *file_monitor; guint file_monitor_id; GFileMonitor *ifnet_monitor; ... } hostname; +#if defined (HOSTNAME_PERSIST_SUSE) + priv->hostname_file = HOSTNAME_FILE_SUSE; + priv->hostname_file_ifnet = FALSE; +#elif defined (HOSTNAME_PERSIST_IFNET) + priv->hostname_file = HOSTNAME_FILE_IFNET; + priv->hostname_file_ifnet = TRUE; +#else + priv->hostname_file = HOSTNAME_FILE_DEFAULT; + priv->hostname_file_ifnet = FALSE; +#endif instead have a #if defined(HOSTNAME_PERSIST_SUSE) #define HOSTNAME_FILE HOSTNAME_FILE_SUSE #define HOSTNAME_IS_IFNET FALSE #elif ... all the behavior is determined at compiletime. I guess that is what we want. But then I would replace + if (priv->dhcp_monitor_id && hostname_is_dynamic ()) { + return NULL; + } else if (priv->hostname_file_ifnet) { + return read_hostname_ifnet (priv->hostname_file); + } else { by "#if defined"
Repushed with fixups for Thomas' comments.
>> Based on 'danw/wip/hostnamed' branch by Dan Winship <danw@gnome.org> Dan usually uses his redhat.com address for NM. I think --with-hostname-persist should auto detect the default based on whether the plugins are enabled. Otherwise, looks good at a quick glance.
(In reply to Thomas Haller from comment #7) > >> Based on 'danw/wip/hostnamed' branch by Dan Winship <danw@gnome.org> > > Dan usually uses his redhat.com address for NM. (Actually, I only switched from @gnome to @redhat a few months ago [after hacking up an alias so that git will use @redhat for work-related stuff and @gnome for non-work-related stuff]. The hostnamed branch is old enough that the commits there would have still been using @gnome, which is presumably where Beniamino got it from. But yeah, use @redhat.)
Pushed branch bg/hostname-bgo740409 with the following changes: - modified configure.ac to select a default value based on enabled plugins when --with-hostname-persist is not explicitly set - changed Dan's mail address in commit message
>> settings: add hostnamed support @self could be disposed before on_proxy_acquired(). While not likely, you could just avoid that by taking a reference in + g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, 0, NULL, + HOSTNAMED_SERVICE_NAME, HOSTNAMED_SERVICE_PATH, + HOSTNAMED_SERVICE_INTERFACE, NULL, + (GAsyncReadyCallback) on_proxy_acquired, self); IMO g_dbus_proxy_new_for_bus_finish() usually does not fail (does it?). So, creating the proxy successfully does not tell you anything about whether the service is running or not. I think, this always ends up using hostnamed.
(In reply to Thomas Haller from comment #10) > >> settings: add hostnamed support > > @self could be disposed before on_proxy_acquired(). While not likely, you > could just avoid that by taking a reference in > > + g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, 0, NULL, > + HOSTNAMED_SERVICE_NAME, > HOSTNAMED_SERVICE_PATH, > + HOSTNAMED_SERVICE_INTERFACE, NULL, > + (GAsyncReadyCallback) on_proxy_acquired, > self); Ok. > > IMO g_dbus_proxy_new_for_bus_finish() usually does not fail (does it?). So, > creating the proxy successfully does not tell you anything about whether the > service is running or not. I think, this always ends up using hostnamed. After some tests, it seems that g_dbus_proxy_new_for_bus_finish() can fail in some situations but, as you suggested, a successful result doesn't mean that the service is actually running. I've added a check to verify that the service has an owner and this seems to properly detect if the service is available. Changes pushed to branch bg/hostname-bgo740409
settings: add hostnamed support + if (!g_dbus_proxy_get_name_owner (priv->hostname.hostnamed_proxy)) { + priv->hostname.value = g_strdup (hostname); two leaks above^^ on_proxy_acquired() tries now to detect once whether to use hostnamed or not. It does not reconsider that decision afterwards. That seems good to me. Only thing, I would log only one line with <info> priority: one of: "hostname: hostnamed not used" (which implies !g_dbus_proxy_get_name_owner) "hostname: use hostnamed" "hostname: hostnamed not used as proxy creation failed with %s"
(In reply to Thomas Haller from comment #12) > two leaks above^^ Fixed, thanks. > > Only thing, I would log only one line with <info> priority: > one of: > "hostname: hostnamed not used" (which implies !g_dbus_proxy_get_name_owner) > "hostname: use hostnamed" > "hostname: hostnamed not used as proxy creation failed with %s" Ok, pushed a fixup.
>> core: move handling of hostname from plugins to core + while (g_io_channel_read_line (channel, &str, NULL, NULL, NULL) != G_IO_STATUS_EOF) { + if (!strncmp (str, pattern, pattern_len)) { + if (!strncmp (str + pattern_len, "\"yes\"", 5)) + dynamic = TRUE; + break; + } + g_free (str); I understand that this was copied from existing code, but this leaks @str. Also, it seems that @str can be NULL (instead of ""). also + if (!strncmp (str, pattern, pattern_len)) { + if (!strncmp (str + pattern_len, "\"yes\"", 5)) is the same as + if (g_str_has_prefix (str, "DHCLIENT_SET_HOSTNAME=\"yes\"")) actually, I would instead: + g_strstrip (str); + if (!g_strcmp (str, "DHCLIENT_SET_HOSTNAME=\"yes\"")) + if (g_str_has_prefix (all_lines[i], "hostname")) { + tmp = strstr (all_lines[i], "="); + tmp++; Again copied from read_hostname(), but I would: if (g_str_has_prefix (all_lines[i], "hostname=")) { tmp = all_lines[STRLEN ("hostname=") + 1]; also note that read_hostname() can be removed in the next commit as well. I would ensure that nm_settings_get_hostname() doesn't ever return "". "" is not a valid hostname.
(In reply to Thomas Haller from comment #14) > >> core: move handling of hostname from plugins to core > > + while (g_io_channel_read_line (channel, &str, NULL, NULL, NULL) != > G_IO_STATUS_EOF) { > + if (!strncmp (str, pattern, pattern_len)) { > + if (!strncmp (str + pattern_len, "\"yes\"", 5)) > + dynamic = TRUE; > + break; > + } > + g_free (str); > > > I understand that this was copied from existing code, but this leaks @str. > Also, it seems that @str can be NULL (instead of ""). Good catch. > > > also > + if (!strncmp (str, pattern, pattern_len)) { > + if (!strncmp (str + pattern_len, "\"yes\"", 5)) > is the same as > + if (g_str_has_prefix (str, "DHCLIENT_SET_HOSTNAME=\"yes\"")) > actually, I would instead: > + g_strstrip (str); > + if (!g_strcmp (str, "DHCLIENT_SET_HOSTNAME=\"yes\"")) > Actually there is a small difference (the first version returns after the first line containing the variable, the other two only when the variable is set to yes) but probably multiple instances of the variable are not allowed in the configuration file, so it doesn't matter. I will use the last version. > > + if (g_str_has_prefix (all_lines[i], "hostname")) { > + tmp = strstr (all_lines[i], "="); > + tmp++; > > Again copied from read_hostname(), but I would: > > if (g_str_has_prefix (all_lines[i], "hostname=")) { > tmp = all_lines[STRLEN ("hostname=") + 1]; > > also note that read_hostname() can be removed in the next commit as well. > > I would ensure that nm_settings_get_hostname() doesn't ever return "". "" is > not a valid hostname. Changed, thanks.
(In reply to Beniamino Galvani from comment #15) > (In reply to Thomas Haller from comment #14) > > >> core: move handling of hostname from plugins to core > > > > + while (g_io_channel_read_line (channel, &str, NULL, NULL, NULL) != > > G_IO_STATUS_EOF) { > > + if (!strncmp (str, pattern, pattern_len)) { > > + if (!strncmp (str + pattern_len, "\"yes\"", 5)) > > + dynamic = TRUE; > > + break; > > + } > > + g_free (str); > > > > > > I understand that this was copied from existing code, but this leaks @str. > > Also, it seems that @str can be NULL (instead of ""). > > Good catch. > > also > > + if (!strncmp (str, pattern, pattern_len)) { > > + if (!strncmp (str + pattern_len, "\"yes\"", 5)) > > is the same as > > + if (g_str_has_prefix (str, "DHCLIENT_SET_HOSTNAME=\"yes\"")) > > actually, I would instead: > > + g_strstrip (str); > > + if (!g_strcmp (str, "DHCLIENT_SET_HOSTNAME=\"yes\"")) > > > > Actually there is a small difference (the first version returns after the > first line containing the variable, the other two only when the variable is > set to yes) but probably multiple instances of the variable are not allowed > in the configuration file, so it doesn't matter. I will use the last version. Ah, I missed that. Seems it would be best to let the last occurrence win. How about the fixup! commit I pushed on your branch? > > + if (g_str_has_prefix (all_lines[i], "hostname")) { > > + tmp = strstr (all_lines[i], "="); > > + tmp++; > > > > Again copied from read_hostname(), but I would: > > > > if (g_str_has_prefix (all_lines[i], "hostname=")) { > > tmp = all_lines[STRLEN ("hostname=") + 1]; > > > > also note that read_hostname() can be removed in the next commit as well. > > > > I would ensure that nm_settings_get_hostname() doesn't ever return "". "" is > > not a valid hostname. > > Changed, thanks. you removed the test for reading "src/settings/plugins/ifnet/tests/hostname". That seems ok, read_hostname_ifnet() seams simple enough. But then also remove the hostname test file. (or restore the test to test the new function in nm-settings.c)
(In reply to Thomas Haller from comment #16) > (In reply to Beniamino Galvani from comment #15) > > > > Actually there is a small difference (the first version returns after the > > first line containing the variable, the other two only when the variable is > > set to yes) but probably multiple instances of the variable are not allowed > > in the configuration file, so it doesn't matter. I will use the last version. > > Ah, I missed that. Seems it would be best to let the last occurrence win. > How about the fixup! commit I pushed on your branch? Looks good, thanks. > you removed the test for reading "src/settings/plugins/ifnet/tests/hostname". > That seems ok, read_hostname_ifnet() seams simple enough. > But then also remove the hostname test file. > (or restore the test to test the new function in nm-settings.c) Ok, file removed.
> core: move handling of hostname from plugins to core I'd change the "ifnet" name to "arch" or something like that, since if we're making hostname reading dependent on the distro and not the plugin, I guess there's no reason to keep it called 'ifnet'. I'd rename set_hostname() to write_hostname(), or even just consolidate the two functions. set_hostname makes me wonder whether it sets the machine hostname or writes the file... Also, I'd make the hook names a bit more generic. Like instead of sysconfig_hostname_clear -> hostname_postwrite_hook and sysconfig_hostname_read -> hostname_read_fallback_hook. And possibly best of all, let's just drop the /etc/sysconfig/network HOSTNAME handling entirely. I looks like it's completely dead in 'initscripts' in Fedora 19 and RHEL7 and later, and since this branch is only going to git master, I think the time has come kill this. Yay! Remove code! dispose() has some whitespace issues when disconnecting hostname.dhcp_monitor_id. > settings: add hostnamed support In set_hostname(), "gs_unref_variant *var = NULL;" and then you don't need to worry about unrefing it. And the arguments to g_variant_new() should be space-aligned to the opening parenthesis of g_variant_new() too. Also, we can use _nm_dbus_signal_connect() now (see 1a0bc83c398b6dff2631c80480ed655e6e187459) to simplify the GDBus properties changed handling. Looking good overall though, thanks!
(In reply to Dan Williams from comment #18) > > core: move handling of hostname from plugins to core > > In set_hostname(), "gs_unref_variant *var = NULL;" and then you don't need > to worry about unrefing it. And the arguments to g_variant_new() should be > space-aligned to the opening parenthesis of g_variant_new() too. I fixed this and the other points above and repushed. > > Also, we can use _nm_dbus_signal_connect() now (see > 1a0bc83c398b6dff2631c80480ed655e6e187459) to simplify the GDBus properties > changed handling. I'm having some troubles replacing this: g_signal_connect (priv->hostname.hostnamed_proxy, "g-properties-changed", G_CALLBACK (hostnamed_properties_changed), self); with this: _nm_dbus_signal_connect (priv->hostname.hostnamed_proxy, "PropertiesChanged", G_VARIANT_TYPE ("(a{sv})"), G_CALLBACK (nmdbus_properties_changed), self); Is this what you were suggesting? With the second version the callback is never invoked and I suspect that the problem is that _nm_dbus_signal_connect() connects to signals only for the interface the proxy is for ("org.freedesktop.hostname1"), while the "PropertiesChanged" signal is delivered on the interface "org.freedesktop.DBus.Properties".
>> settings: don't return empty string in nm_settings_get_hostname() could just be merged into "core: move handling of hostname from plugins to core" What is with NM_SYSTEM_CONFIG_INTERFACE_PROP_HOSTNAME, NM_SYSTEM_CONFIG_INTERFACE_HOSTNAME and priv->hostname, plugin_get_hostname() in src/settings/plugins/keyfile/plugin.c? Seems they are unused now, aren't they? That means we ignore now keyfile.hostname configuration setting? I'd be ok with that change, but then lets update the manual page for NM.config to say that this configuration setting is now unused/deprecated.
(In reply to Thomas Haller from comment #20) > >> settings: don't return empty string in nm_settings_get_hostname() > > could just be merged into > "core: move handling of hostname from plugins to core" Okay. I had to squash fixups to merge the two commits and then pushed everything to the new branch bg/hostname-bgo740409-squashed. > What is with NM_SYSTEM_CONFIG_INTERFACE_PROP_HOSTNAME, > NM_SYSTEM_CONFIG_INTERFACE_HOSTNAME and priv->hostname, > plugin_get_hostname() in > src/settings/plugins/keyfile/plugin.c? Seems they are unused now, aren't > they? > > That means we ignore now keyfile.hostname configuration setting? I'd be ok > with that change, but then lets update the manual page for NM.config to say > that this configuration setting is now unused/deprecated. I updated the man page for this and also removed some leftover references to hostname handling in plugins.
> core: move handling of hostname from plugins to core +#if defined(HOSTNAME_PERSIST_ARCH) + hostname = read_hostname_arch (priv->hostname.file); +#else +#if defined(HOSTNAME_PERSIST_SUSE) ... +#endif ... +#endif Should just use #elif defined(HOSTNAME_PERSIST_SUSE)/#else/#endif here. > settings: remove hostname handling from plugins typedef struct { - GFileMonitor *hostname_monitor; - GFileMonitor *dhcp_monitor; - char *hostname; } SCPluginIfcfgPrivate; GObject is going to yell at runtime because you're trying to allocate a zero-size private structure. The two fixes are to add an 'int dummy' variable to the struct, or just remove the whole private struct entirely since we don't need it. I vote for removal :) Also since you've removed everything from dispose() in the SUSE plugin you can remove the dispose function entirely too. Also, a follow-on (eg we can do this later after this merge) is to consolidate the NMInotifyHelper into the ifcfg-rh plugin, since nothing else needs it anymore. Lastly, there's two places in the SUSE and ifupdown plugin that do this: case NM_SYSTEM_CONFIG_INTERFACE_PROP_CAPABILITIES: - g_value_set_uint (value, NM_SYSTEM_CONFIG_INTERFACE_CAP_MODIFY_HOSTNAME); + g_value_set_uint (value, 0); I'd use NM_SYSTEM_CONFIG_INTERFACE_CAP_NONE here instead of 0 to make it clear that the plugin has no capabilities. > settings: add hostnamed support Yeah, you're right about _nm_dbus_signal_connect(). One problem I see here: systemd-hostnamed is bus-activated and quits when it's not used. So what's going to happen here is that creating the proxy might spawn hostnamed automatically. But that's a bit fragile, and there's also the possibility that by the time we try to request the owner it has quit (though technically because we're single-threaded, the owner will always be valid until the next trip through the event loop). Basically, I don't think we can just create a proxy to see if we're supposed to use it or not. Instead what I think we should do is something like: 1) create the proxy, hook up properties changed signals 2) Get the hostname from systemd-hostnamed 3) if getting the hostname succeeds, then we're using hostnamed 4) if getitng the hostname fails with an error fall back to PERSIST_ARCH or PERSIST_SUSE or /etc/hostname directly Step #2 is the crucial step, becuase this will 100% positively tell us whether hostnamed can be used or not, or whether it's been 'systemctl mask'-ed by the user even if installed. Just creating the proxy doesn't necessarily tell us that.
(In reply to Dan Williams from comment #22) > > core: move handling of hostname from plugins to core > > +#if defined(HOSTNAME_PERSIST_ARCH) > + hostname = read_hostname_arch (priv->hostname.file); > +#else > > +#if defined(HOSTNAME_PERSIST_SUSE) > ... > +#endif > ... > +#endif > > Should just use #elif defined(HOSTNAME_PERSIST_SUSE)/#else/#endif here. The code before the last #endif must also be used when HOSTNAME_PERSIST_SUSE; using #elif/#else/#endif would change the semantics. > > settings: remove hostname handling from plugins > GObject is going to yell at runtime because you're trying to allocate a > zero-size private structure. The two fixes are to add an 'int dummy' > variable to the struct, or just remove the whole private struct entirely > since we don't need it. I vote for removal :) > > Also since you've removed everything from dispose() in the SUSE plugin you > can remove the dispose function entirely too. Good catch; after removing this I noticed that the SUSE plugin is now empty so it seems a good idea to remove it. If anybody thinks that we should keep it instead I can just drop that commit. > Also, a follow-on (eg we can do this later after this merge) is to > consolidate the NMInotifyHelper into the ifcfg-rh plugin, since nothing else > needs it anymore. Ok. > I'd use NM_SYSTEM_CONFIG_INTERFACE_CAP_NONE here instead of 0 to make it > clear that the plugin has no capabilities. Ok. > > settings: add hostnamed support > Instead what I think we should do is something like: > > 1) create the proxy, hook up properties changed signals > 2) Get the hostname from systemd-hostnamed > 3) if getting the hostname succeeds, then we're using hostnamed > 4) if getitng the hostname fails with an error fall back to PERSIST_ARCH or > PERSIST_SUSE or /etc/hostname directly > > Step #2 is the crucial step, becuase this will 100% positively tell us > whether hostnamed can be used or not, or whether it's been 'systemctl > mask'-ed by the user even if installed. Just creating the proxy doesn't > necessarily tell us that. I implemented it this way now and I also realized that other components called get_property() to get the hostname after the initialization of the object but before on_proxy_acquired() callback was run. In such situation the proxy was NULL and hostname files not initialized and so we hit some GLib warnings. I made the proxy initialization synchronous and this problem should not happen anymore. Pushed branch bg/hostname-bgo740409
>> settings: remove ifcfg-suse plugin I think this is right to do, but your change to configure.ac mean that a user who used to compile with `./configure --enable-ifcfg-suse` will no longer get "hostname_persist=suse". I think you need to continue to parse --enable-ifcfg-suse, only to detect suse-hostname handling. Otherwise, users must update their build-scripts. While at it, isn't this wrong: [test -z "$hostname_persist" -a "$distro_plugins" = "ifnet"] and should be something like [test -z "$hostname_persist" -a "$distro_plugins" = *"ifnet"* ] or even better: [test -z "$hostname_persist" -a "$enable_ifnet" = "yes" ]
>> trivial: factor out monitor initialization from nm_settings_new() could we reserve the word "trivial" in the subject line for patches that are really absolutely trivial? Like renaming or moving code? wow, this branch removes 800+ LOC ... nice! Otherwise, LGTM
(In reply to Thomas Haller from comment #24) > >> settings: remove ifcfg-suse plugin > > I think this is right to do, but your change to configure.ac mean that a > user who used to compile with `./configure --enable-ifcfg-suse` will no > longer get "hostname_persist=suse". > > I think you need to continue to parse --enable-ifcfg-suse, only to detect > suse-hostname handling. Otherwise, users must update their build-scripts. How about the fixup I pushed to the branch? It re-adds --enable-ifcfg-suse support, marked as deprecated, and uses it to infer hostname_persist=suse. > While at it, isn't this wrong: > > [test -z "$hostname_persist" -a "$distro_plugins" = "ifnet"] > > and should be something like > > [test -z "$hostname_persist" -a "$distro_plugins" = *"ifnet"* ] > > or even better: > > [test -z "$hostname_persist" -a "$enable_ifnet" = "yes" ] We can select the right persist method only when there is a single distro-specific setting plugin enabled; if there are more of them we should stick to the default persist method. (In reply to Thomas Haller from comment #25) > >> trivial: factor out monitor initialization from nm_settings_new() > > could we reserve the word "trivial" in the subject line for patches that are > really absolutely trivial? Like renaming or moving code? Subject updated, thanks.
(In reply to Beniamino Galvani from comment #26) > (In reply to Thomas Haller from comment #24) LGTM now
It seems fine on first look. But I am going to go through more deeply and do some tests yet.
1.) Is it necessary to load the hostname upon daemon startup? It activates systemd-hostnamed and slows down bootup. Wouldn't it be possible to load it only when someone asks? 2.) NM now returns an empty string when /etc/hostname is missing. Is that desired behavior? systemd-hostnamed sees the actually set hostname (the transient hostname, presumably from dhcp) -- shouldn't we do the same? Otherwise looks fine. Tested on Fedora & Debian with systemd.
(In reply to Lubomir Rintel from comment #29) > 1.) Is it necessary to load the hostname upon daemon startup? It activates > systemd-hostnamed and slows down bootup. Wouldn't it be possible to load it > only when someone asks? Yes, it should be possible to load the hostname when get_property() and nm_settings_get_hostname() are called for the first time; the only user of the property at the moment seems to be DHCP client code. > 2.) NM now returns an empty string when /etc/hostname is missing. Is that > desired behavior? systemd-hostnamed sees the actually set hostname (the > transient hostname, presumably from dhcp) -- shouldn't we do the same? hostnamed exposes both Static and Transient hostname properties, the former is the hostname stored in /etc/hostname while the latter is the runtime hostname set in kernel, which can be different from the static one when the hostname is configured from DHCP. Since now NM has always read the static hostname and this branch doesn't change the behavior; I think it's the right thing to do since the hostname value is used by DHCP clients to send the host-name option to servers and this probably should be the statically configured hostname.
(In reply to Beniamino Galvani from comment #30) > (In reply to Lubomir Rintel from comment #29) > > 1.) Is it necessary to load the hostname upon daemon startup? It activates > > systemd-hostnamed and slows down bootup. Wouldn't it be possible to load it > > only when someone asks? > > Yes, it should be possible to load the hostname when get_property() > and nm_settings_get_hostname() are called for the first time; the only > user of the property at the moment seems to be DHCP client code. If it would be a performance issue to ask hostnamed for the hostname during startup, that would bad (of hostnamed). I'd guess that during boot hostnamed will be asked several times and is anyway running. > > 2.) NM now returns an empty string when /etc/hostname is missing. Is that > > desired behavior? systemd-hostnamed sees the actually set hostname (the > > transient hostname, presumably from dhcp) -- shouldn't we do the same? > > hostnamed exposes both Static and Transient hostname properties, the > former is the hostname stored in /etc/hostname while the latter is the > runtime hostname set in kernel, which can be different from the static > one when the hostname is configured from DHCP. > > Since now NM has always read the static hostname and this branch > doesn't change the behavior; I think it's the right thing to do since > the hostname value is used by DHCP clients to send the host-name > option to servers and this probably should be the statically > configured hostname. Since DHCP can set the transient hostname, it seems very right to use the static hostname.
Pushed three more commits: 63fa0ca settings/example: remove example settings plugin ea9f43b keyfile: refactor reading "keyfile.unmanaged-devices" setting 7bba3aa keyfile: refactor dispose() of SCPluginKeyfile These don't have to block merging of the branch. If the first part is good, merge it to master, we can finish the later part afterwards... but maybe, those 3 patches are uncontroversial enough to be handled quickly(??) (IMO, I would prefer an approach where we get branches done after a while... we can make them AAA rating in later branches)
(In reply to Thomas Haller from comment #32) > Pushed three more commits: > > 63fa0ca settings/example: remove example settings plugin > ea9f43b keyfile: refactor reading "keyfile.unmanaged-devices" setting > 7bba3aa keyfile: refactor dispose() of SCPluginKeyfile These commits LGTM.
Maybe we should log a warning when the keyfile plugin finds a deprecated & unused 'hostname' key in the config? That's all I've got, the rest LGTM and we should merge this ASAP.
(In reply to Dan Williams from comment #34) > Maybe we should log a warning when the keyfile plugin finds a deprecated & > unused 'hostname' key in the config? Done, thanks. Also, when we switched from generic values to distro names for the --with-hostname-persist option, the 'ifnet' value was incorrectly renamed to Arch instead of Gentoo. Fixed that too and repushed.
Merged to master as 135999c2ec567683b6d71cc328584a2126167144.
Hi, I don't see this patch in any recent releases, is that expected?
(In reply to Laurent Bigonville from comment #37) > Hi, > > I don't see this patch in any recent releases, is that expected? Yes. The change got merged to master branch and since then there was no major release (the next release containing this fix will be 1.2.0). The fix was *not* backported to the stable branch 1.0, and thus the fix was not part of any of the recent (minor) releases from 1.0.x.