WIP: Move Mumble Socket and Overlay to $XDG_RUNTIME_DIR/mumble/

[carlocastoldi]

Moving MumbleSocket and MumbleOverlayPipe to a dedicated subdirectory keeps the runtime directory clean and allows flatpak applications to use the overlay by giving access only to Mumble's subdirectory. It also moves the default directory to /run/user/$UID/mumble/ when $XDG_RUNTIME_DIR is not set.

Fixes #5951

See also: flathub/com.valvesoftware.Steam#273

Checks

• My commits follow the commit guidelines

[carlocastoldi]

I thought of using QtStandardPaths, but if $XDG_RUNTIME_DIR is not set it defaults to unpredictable directories. From the docs it should point to /run/user/$UID/, but with me it defaulted to /tmp/runtime-carlo/

Use of QtStandardPaths::RuntimeLocation

		QString runtimePath = QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation);
		QString mumbleRuntimePath = QDir(runtimePath).absolutePath() + QLatin1String("/mumble/");
		QDir().mkpath(mumbleRuntimePath);
		pipepath = QDir(mumbleRuntimePath).absoluteFilePath(QLatin1String("MumbleOverlayPipe"));

[Krzmbrzl]

What is that tags file that has been added in this PR?

[Krzmbrzl]

It might make sense to create a new function for obtaining the respective path so we don't have to write the logic in multiple places (with the potential of screwing up one of the times or one version getting outdated because it was overlooked).
I would propose writing a function returning a std::string and everything that needs something else can convert from that.

[Krzmbrzl]

I suppose that old overlays won't work with new Mumble versions and vice versa after this PR. I guess that's fine as it seems like an unlikely scenario.

However, existing IPC scripts will also stop working, if XDG_RUNTIME is not set. But then again, from your description it seemed that this special case was ill-defined anyway, so I wouldn't consider it likely that a lot of scripts exist for this special case...
Thus, this should be fine as well 🤷

[carlocastoldi]

I would propose writing a function returning a std::string and everything that needs something else can convert from that.

Yeah I like the idea. Where would I put this util function, tho? I'm not accustomed to Mumble's code organization, yet

[carlocastoldi]

Btw: do you want me to include in this PR the fix for OverlayTest? So that it can be run again? I found it super useful to test the change of this PR in a flatpak environment

[carlocastoldi]

I'm afraid OverlayTest does not actually test anything, does it? CI gets stuck

[davidebeatrici]

Well... how can it work if Mumble is not running?

[carlocastoldi]

Well... how can it work if Mumble is not running?

That is not even the issue. OverlayTest is an application for humans to check whether the overlay is properly displayed or not. I disabled it by default, but it is being built anyway.

[carlocastoldi]

Just a quick glance. I have the feeling that there must be a less convoluted way of doing things. For one we can use C++ to implement those functions and then simply ensure that they can be called through a C interface. That should clear the code up already.

If you ask me, i think having the code for all platform in the same file makes it unnecessary long, hard to read and review. I would have sticked to have different file for each platform, which were then selected, based on the OS, in the CMakeLists.txt files.

I'll have to look at this in more detail at some point though...

Anyhow, I'll be waiting for you detailed review so that we can discuss this further

[Krzmbrzl]

I have not forgotten this, but I still don't have the time to look into this

[Krzmbrzl]

sigh... let's try to ignore that it took me half a year to take another look at this. Big sorry for that 🙈

I had another look at the current implementation and I think most of the complexity comes from the fact that doing the path-related logic in C is a bit of a PITA.
Therefore, my suggestion for creating a properly maintainable implementation would be this, would be to use C++ for implementing all the logic and instead of writing cpp wrappers for the C implementations, we'll do it the other way around. So the hierarchy I'm having in mind would be something like

namespace mumble {
namespace overlay {

	std::string getIPCPath();

	std::string getSocketPath();

	std::string getPipePath();

	std::string createSocket();

	std::string createPipe();

}
}


extern "C" {

char *mumble_overlay_create_and_get_socket();

char *mumble_overlay_create_and_get_pipe();

}

The respective wrappers would then simply call the C++ implementation under the hood, allocate a new char buffer of the appropriate size, copy the contents over and then return that freshly allocated buffer for use in C world.

Inside the C++ implementations, we can then make use of Qt (for lack of C++17 support in Mumble) to do the filesystem operations (in particular things like querying the user's home directory) and then only do the actual creation of the pipe and socket in a platform dependent manner. That seems to save a lot of complexity that I am currently seeing in the C implementation.

In order for all to work together, I think the easiest solution would be to make the overlay utils part of the shared codebase - that is, the code living directly under src/ - and then link the shared target to the overlay one. In order for the inclusion of the header to work, we probably have to do something like

#ifdef __cplusplus
// C++ declarations here -> including namespaces

extern "C" {
#endif

// C declarations here

#ifdef __cplusplus
}
#endif 

which is not terribly pretty but should get the job done, without devastating readability 🤔

This would of course mean, that we can then never build the overlay client without building the shared target, which in turn brings many new dependencies (not least of which is Qt). However, I don't really see a scenario where someone would want to build the overlay client, but not Mumble itself (at least the client), so I don't think this would be a problem.

And once we make the switch to C++17, we could even create another, standalone, static library that makes uses of std::filesystem to replace the Qt parts that I suggest using for the implementation.

Thoughts?
/CC @davidebeatrici

@carlocastoldi do you even still want to work on this? If not, I could totally understand.

[davidebeatrici]

Alternatively, we could use cwalk.

[carlocastoldi]

I rather like the idea of using a third party C library (cwalk) instead of choosing to entangle the overlay code with client's.
I personally worked on #5967 to decouple the overlay build from client's so that I could easily build a flatpak extension for mumble's overlay (see com.valvesoftware.Steam.Utility.MumbleOverlay) . I could see similar needs for other package formats (e.g. 32bit overlay)

carlocastoldi do you even still want to work on this? If not, I could totally understand.

I sure want. It's just a matter of time. I recently started a new job so I have fewer free time to spend on this.
But I'd like to close this PR myself ^^

P.S. a little reminder: I started all of this work so that it would be possible to have the overlay on flatpak steam games. The next major work I would like to work on is to move away the overlay from using shared memory, onto exclusively using local sockets (branch#socket-overlay-not-chunked and branch#socket-overlay-chunked)

[Krzmbrzl]

Alright - using a 3rdParty library would also be fine for me. However it seems like cwalk doesn't provide a function to query the user's home directory - unless normalizing ~ will yield the expected result. But have a look at it and decide for yourself.

Another option could be to create a completely separate target that implements this functionality and that the overlay and the Mumble client both link to. Then inside this function we could use even simply use C++17 (making sure to not accidentally export STL symbols).
That way, the client and the overlay would remain unentangled as bother would simply rely on that external library.

[carlocastoldi]

If I wanted to use cwalk, would it be ok for you if I used it only in ipc_utils.c for the C backend? I would then keep exporting them in C++ with IPCUtils.cpp.

However it seems like cwalk doesn't provide a function to query the user's home directory

True. However this is only needed in BSD/MacOS and it's a matter of 5 easy lines. I don't see it as a deal breaker. Moreover, the process for retrieving the home not so different from retrieving the runtime directory on Linux.

Also: are we sure both BSD and MacOS suggest using the home directory for storing this kind of files? Isn't there a standard similar to what we have on Linux?
I mean.. it's not something we have to discuss over on this PR, but the need for retrieving the home directory for BSD/Mac might just disappear in a next PR

[carlocastoldi]

Hell. I gave it a try and included cwalk into the PR. Now IPC utils uses an external library to handle OS-specific paths

[carlocastoldi]

I had an hard time figuring out whether it was possible to compile and link cwalk both in 32 and 64bits in the same build as a 3rdparty library/dependency.
For this reason I opted to use snfprintf(). It basically does the same think as cwk_path_join(), but with no path resolution. I don't feel like that's a a must for this PR, however.

P.S. i marked this PR as WIP as at the moment commits and code is not merge-ready.

[carlocastoldi]

Finally, since Windows doesn't rely on an actual "path" for sockets, i didn't even have to handle platform specific separators.

At this point, I'd like to request whether the code structure of this PR is roughly ok or not. Is the refactoring you asked when suggesting to write "a function returning a std::string" as you intended? Do you like the approach of having a C backend with a C++ frontend? Is the code separation ok?

Thank you in advance!