$XDG_RUNTIME_DIR is the place where to put socket files these days.
It seems good practice to create a sub-directory as well, perhaps we create more files there in the future.
The fallback to write in /tmp remains for the rare occasions where $XDG_RUNTIME_DIR cannot be used. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2222
-- Commit Summary --
* prefer XDG_RUNTIME_DIR over /tmp for the socket
-- File Changes --
M src/socket.c (31)
-- Patch Links --
https://github.com/geany/geany/pull/2222.patch https://github.com/geany/geany/pull/2222.diff
One other use case: I want to be able to bind-mount the socket directory into a container to open files from within the container in the Geany instance on the host, but I do not want to share /tmp with the container.
A good idea, but the implementation without the random number on the socket name means that a user can't open two Geanys with differing config files because they each will try to open the same socket.
b4n requested changes on this pull request.
I like the idea, but I'm a little worried about what [the spec](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html...) says on lifetime of files in that directory: *Files in this directory MAY be subjected to periodic clean-up. To ensure that your files are not removed, they should have their access time timestamp modified at least once every 6 hours of monotonic time or the 'sticky' bit should be set on the file.*
This suggests our socket link could disappear on us after 6 hours if we're not careful, doesn't it? That could be a problem and the source of weird bugs.
@@ -439,20 +441,35 @@ static gint socket_fd_open_unix(const gchar *path)
return -1; }
- /* fix for #1888561: - * in case the configuration directory is located on a network file system or any other - * file system which doesn't support sockets, we just link the socket there and create the - * real socket in the system's tmp directory assuming it supports sockets */ - real_path = g_strdup_printf("%s%cgeany_socket.%08x", - g_get_tmp_dir(), G_DIR_SEPARATOR, g_random_int()); + /* Try to place the socket in XDG_RUNTIME_DIR, according to XDG Base + * Directory Specification, see + * https://specifications.freedesktop.org/basedir-spec/latest */ + real_dir = g_build_filename(g_get_user_runtime_dir(), "geany", NULL); + if (g_mkdir_with_parents(real_dir, 0755) == 0)
The [spec](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html...) says that *The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700. The lifetime of the directory MUST be bound to the user being logged in. It MUST be created when the user first logs in […]*. This tells us first that we don't need the `with_parents()` part as the runtime dir must already exist, and then even if it didn't have to it must be created with mode 0700, not 0755. I suggest we use `g_mkdir(real_dir, 0700)`, so we only create it when it's "safe". Also note that [`g_get_user_runtime_dir()`](https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions...) is slightly annoying in that it returns `$XDG_CACHE_HOME` if `$XDG_RUNTIME_DIR` isn't set, and that directory does *not* have cleanup semantics similar to /tmp or `$XDG_RUNTIME_DIR`.
kugel- commented on this pull request.
@@ -439,20 +441,35 @@ static gint socket_fd_open_unix(const gchar *path)
return -1; }
- /* fix for #1888561: - * in case the configuration directory is located on a network file system or any other - * file system which doesn't support sockets, we just link the socket there and create the - * real socket in the system's tmp directory assuming it supports sockets */ - real_path = g_strdup_printf("%s%cgeany_socket.%08x", - g_get_tmp_dir(), G_DIR_SEPARATOR, g_random_int()); + /* Try to place the socket in XDG_RUNTIME_DIR, according to XDG Base + * Directory Specification, see + * https://specifications.freedesktop.org/basedir-spec/latest */ + real_dir = g_build_filename(g_get_user_runtime_dir(), "geany", NULL); + if (g_mkdir_with_parents(real_dir, 0755) == 0)
I use `_with_parents()` in case the geany directory already exists. I could also use mkdir() and handle EEXIST but this seemed more convenient.
For the runtime directory, I assume it already exists, since it's provided by the spec. If that's not the case I think we cuoldn't create it since users generally don't have sufficient permissions for /var/run/user.
I also considered that the 0700 requirement applies to the $XDG_RUNTIME_DIR itself. A quick survey on my system shows that the subdirectories are inconsistent, but the systemd subdirectory (and systemd manages $XDG_RUNTIME_DIR) has 0755. I guess it doesn't matter much. Can change to 0700 if you want.
``` srw-rw-rw- 1 kugel kugel 0 16. Jul 06:57 bus drwx------ 2 kugel kugel 60 17. Jul 16:14 dconf drwxr-xr-x 2 kugel kugel 40 18. Jul 07:13 geany drwx------ 2 kugel kugel 140 16. Jul 06:57 gnupg srw------- 1 kugel kugel 0 16. Jul 06:57 kdeinit5__0 srwxr-xr-x 1 kugel kugel 0 16. Jul 06:57 klauncherdKXvtl.1.slave-socket -rw-r--r-- 1 kugel kugel 74 16. Jul 06:57 KSMserver__0 srwxr-xr-x 1 kugel kugel 0 16. Jul 06:57 kwallet5.socket drwxr-xr-x 2 kugel kugel 60 16. Jul 06:57 Nextcloud drwxr-xr-x 2 kugel kugel 60 16. Jul 06:57 p11-kit srw-rw-rw- 1 kugel kugel 0 16. Jul 06:57 pipewire-0 drwx------ 2 kugel kugel 100 16. Jul 06:57 pulse drwxr-xr-x 3 kugel kugel 100 16. Jul 06:57 systemd drwx------ 2 kugel kugel 40 18. Jul 21:56 xpra ```
Good point. I didn't find exact information unfortunately. Just this one SO discussion: https://unix.stackexchange.com/questions/356302/sticky-bit-and-socket-files-...
That suggests that 1) the 6h requirement applies to real files (and I tend to follow since the spec makes a distinction between files and file objects in the same paragraph), and 2) no implementation actually implements the periodic cleanup (which is a MAY requirement).
On the other hand, the spec guarantees that $XDG_RUNTIME_DIR itself exists until the last logout.
So given that and that the spec expressively tells to put sockets in that directory, I would say it's not an issue to worry about.
@b4n do you still request changes? If yes, which exactly?
@kugel- pushed 1 commit.
625ae44a1a6f2a3680ff173ab8aa3ce6ef5a6321 Use utils_mkdir() to create the geany directory.
b4n approved this pull request.
Apart from the nitpicking below, LGTM
if (utils_is_file_writable(real_path) != 0) { /* if real_path is not writable for us, fall back to ~/.config/geany/geany_socket_*_* */ /* instead of creating a symlink and print a warning */ g_warning("Socket %s could not be written, using %s as fallback.", real_path, path); SETPTR(real_path, g_strdup(path)); } - /* create a symlink in e.g. ~/.config/geany/geany_socket_hostname__0 to /tmp/geany_socket.499602d2 */ + /* create a symlink in e.g. ~/.config/geany/geany_socket_hostname__0 to + * /var/run/user/1000/geany/geany_socket_hostname__0 */
the filename in /var in this comment is not correct anymore
* in case the configuration directory is located on a network file system or any other
- * file system which doesn't support sockets, we just link the socket there and create the - * real socket in the system's tmp directory assuming it supports sockets */ - real_path = g_strdup_printf("%s%cgeany_socket.%08x", - g_get_tmp_dir(), G_DIR_SEPARATOR, g_random_int()); + /* Try to place the socket in XDG_RUNTIME_DIR, according to XDG Base + * Directory Specification, see + * https://specifications.freedesktop.org/basedir-spec/latest. + * If that fails, we try to use /tmp as a fallback. The last resort + * is the configuration directory. But the other directories + * are preferred in case the configuration directory is located on + * a network file system or any other file system which doesn't + * support sockets (see #1888561). Append a random int to + * prevent clashes with other instances on the system. */ + real_dir = g_build_filename(g_get_user_runtime_dir(), "geany", NULL); + val = utils_mkdir(real_dir, FALSE);
ideally you'd increase the cope of the `saved_errno` variables en use that as it's a better variable name.
[on periodic cleanup]
OK fair enough. It'd actually be kind of strange to clean up sockets (but who knows). Let's just hope it's OK for now then, and not worry about it further.
@kugel- pushed 1 commit.
a9eb7cb02185eca9a1788c5f151ff63275a88919 Update outdated comment and use a more descriptive variable for mkdir result.
kugel- commented on this pull request.
if (utils_is_file_writable(real_path) != 0) { /* if real_path is not writable for us, fall back to ~/.config/geany/geany_socket_*_* */ /* instead of creating a symlink and print a warning */ g_warning("Socket %s could not be written, using %s as fallback.", real_path, path); SETPTR(real_path, g_strdup(path)); } - /* create a symlink in e.g. ~/.config/geany/geany_socket_hostname__0 to /tmp/geany_socket.499602d2 */ + /* create a symlink in e.g. ~/.config/geany/geany_socket_hostname__0 to + * /var/run/user/1000/geany/geany_socket_hostname__0 */
fixed
@b4n Thanks for the review. I'll squash-merge this soon if you approve.
elextr requested changes on this pull request.
@@ -439,20 +441,35 @@ static gint socket_fd_open_unix(const gchar *path)
return -1; }
- /* fix for #1888561: - * in case the configuration directory is located on a network file system or any other - * file system which doesn't support sockets, we just link the socket there and create the - * real socket in the system's tmp directory assuming it supports sockets */ - real_path = g_strdup_printf("%s%cgeany_socket.%08x", - g_get_tmp_dir(), G_DIR_SEPARATOR, g_random_int()); + /* Try to place the socket in XDG_RUNTIME_DIR, according to XDG Base + * Directory Specification, see + * https://specifications.freedesktop.org/basedir-spec/latest */ + real_dir = g_build_filename(g_get_user_runtime_dir(), "geany", NULL); + if (g_mkdir_with_parents(real_dir, 0755) == 0) + { + basename = g_path_get_basename(path); + real_path = g_build_filename(real_dir, basename, NULL); + }
As I said in comment, this tries to create the same socket if the user runs geany with two different configs.
(geany:20745): Geany-WARNING **: 12:28:53.641: Failed to bind IPC socket (/run/user/1000/geany/geany_socket_fred9__0): 98: Address already in use
b4n commented on this pull request.
@@ -439,20 +441,35 @@ static gint socket_fd_open_unix(const gchar *path)
return -1; }
- /* fix for #1888561: - * in case the configuration directory is located on a network file system or any other - * file system which doesn't support sockets, we just link the socket there and create the - * real socket in the system's tmp directory assuming it supports sockets */ - real_path = g_strdup_printf("%s%cgeany_socket.%08x", - g_get_tmp_dir(), G_DIR_SEPARATOR, g_random_int()); + /* Try to place the socket in XDG_RUNTIME_DIR, according to XDG Base + * Directory Specification, see + * https://specifications.freedesktop.org/basedir-spec/latest */ + real_dir = g_build_filename(g_get_user_runtime_dir(), "geany", NULL); + if (g_mkdir_with_parents(real_dir, 0755) == 0) + { + basename = g_path_get_basename(path); + real_path = g_build_filename(real_dir, basename, NULL); + }
This should already be fixed in the current version of the changes, it uses the same basename as it used to, including the random integer (which is admittedly not necessarily the smartest idea as there's a faint chance of clashes, but well, it seems to have served us good enough 'til now).
b4n approved this pull request.
LGTM
elextr commented on this pull request.
@@ -439,20 +441,35 @@ static gint socket_fd_open_unix(const gchar *path)
return -1; }
- /* fix for #1888561: - * in case the configuration directory is located on a network file system or any other - * file system which doesn't support sockets, we just link the socket there and create the - * real socket in the system's tmp directory assuming it supports sockets */ - real_path = g_strdup_printf("%s%cgeany_socket.%08x", - g_get_tmp_dir(), G_DIR_SEPARATOR, g_random_int()); + /* Try to place the socket in XDG_RUNTIME_DIR, according to XDG Base + * Directory Specification, see + * https://specifications.freedesktop.org/basedir-spec/latest */ + real_dir = g_build_filename(g_get_user_runtime_dir(), "geany", NULL); + if (g_mkdir_with_parents(real_dir, 0755) == 0) + { + basename = g_path_get_basename(path); + real_path = g_build_filename(real_dir, basename, NULL); + }
Oh, I just noticed this morning I hadn't clicked the submit button on this comment for nearly two weeks, since nobody had mentioned it had been fixed I submitted it.
Ok, LGBI
elextr approved this pull request.
@b4n, yes there is a small chance of a clash but its so small ... or the process id could be included or proper UUIDs but hey, I doubt it worth it.
Merged #2222 into master.
github-comments@lists.geany.org