GNOME Bugzilla – Bug 765645
[PATCH] If /etc/hostname is a symbolic link, the hostname is not given to sethostname(2) or read back correctly
Last modified: 2016-05-12 15:56:20 UTC
Created attachment 326813 [details] [review] Patch to resolve the symbolic link before attaching the file monitor Since 55a07b4ca4 has supported /etc/hostname being a symbolic link. In this case NM will write through the link to the target file. However, NM currently still attaches a file monitor to /etc/hostname, rather than the link target. The effect of this is that sethostname(2) is never called, and the hostname cannot be read back with the org.freedesktop.DBus.Properties.Get dbus API. The attached patch fixes the issue by resolving the symbolic link before attaching the file monitor, in a similar behaviour to that of write_hostname .
The patch has minor style issues, and link_path is leaked. Also, readlink() / g_file_read_link() will read the symbolic links, but if it's a relative path, it will not resolve that to an absolute path. I fixed that on master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=7f7e1eb60b383a1b7b8c46105718748af2b84c66 Anyway, ... let's take one step back first :) (1) As is, write_hostname() will follow a symlink one time, and then replace the target of the symlink. Is that what we want? I would expect we want to follow the symlink all the way down (by calling realpath). On the other hand, what if realpath cannot resolve because the symlink points into a non-existing directory? Then writing will fail. I merged patch 55a07b4ca4, and Joel already talked about the use-case (https://mail.gnome.org/archives/networkmanager-list/2016-January/msg00075.html). But it's still unclear to me what we really want to do with respect to above questions. (2) If you monitor symlinks, then we would have to account also for all symlinks along the way. That is, if you have multiple indirections, each symlink would warrant a monitor. That is of course doable, if we want to affort all those monitors.
Created attachment 326887 [details] [review] Patch to resolve the symbolic link before attaching the file monitor (v2) v2 patch
I've rebased the patch on the current master branch, and tried to fix the style issues, and added a g_free . In answer to (1) my rationale was explained in that e-mail, though a full resolve would work ok. We would just have to be careful about sym-link loops and such. In answer to (2), in theory it could be an issue, but in practice I don't see it being a common problem. We could add multiple file monitors if anyone ever needed them.
We could say that we support only one symbolic-link indirection for the hostname file; that should be enough to implement read-only /etc and other use cases. And maybe we should add a warning when we detect that the target of the first link is another link so that users can quickly spot that there is something wrong and things will not work as expected. Otherwise, if we decide to support an arbitrary number of symlinks we must be prepared to handle the complexity of multiple monitors, but I prefer to avoid it.
Created attachment 327183 [details] [review] Patch to resolve the symbolic link before attaching the file monitor (v3)
Beniamino Galvani: Added a new version with a warning if there is a second level symbolic link.
v3 merged to master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=860606012145b42590e389655e4e4e3fec86e1d1 Thank you, Joel.
Thanks Thomas!