Implement openURL on Linux #642
Conversation
|
Heya @falkTX and @jsakkine just want to make sure you agree with my choice of "xdg-open" here as best bet to open a URL on Linux. Thanks! |
|
xdg-open is the right tool to use, but I am not sure if |
|
Great - this is why I ask! Thanks |
baconpaul
force-pushed
the
linux-browser-638
branch
2 times, most recently
from
0f1547f to
fdc530b
Compare
|
@falkTX this works no problem and is indeed better! Looks right to me too. Can you give a quick peek? Thanks! |
src/linux/UserInteractionsLinux.cpp
Outdated
| @@ -31,6 +33,13 @@ namespace Surge | |||
|
|
|||
| void openURL(const std::string &url) | |||
| { | |||
| if (vfork()==0) | |||
| { | |||
| if (execlp("xdg-open", "xdg-open", url.c_str(), (char*)NULL) < 0) | |||
There was a problem hiding this comment.
last argument can be nullptr
I would just test this in a system without xdg-open or purposefully renaming things here in order to make it call something that does not exist, and see how that behaves.
if that works, then we should be fine.
|
30+ years of typing NULL is gonna be a hard habit to break. I’ll fix before I sweep. I accidentalky tested what you said in my first version where I had a typo in xdg home. Nothing bad happened but of course the url didn’t open. I considered saying something to stderr but decided against it. I think thats ok - not sure what happens to stderr from a child process in hosts and also seems it would force a copy of some system resource into the child which is why you suggested vfork Thanks again for the reviews!! |
|
I gotta say, reading the vfork man page more carefully it really seems like a worse choice than fork I agree @jsakkine - but I've never written linux audio code. @falkTX why the vfork vs fork? |
|
vfork is the function to use when we deal with audio plugins and realtime stuff. |
|
We can ignore the part about file descriptors, as we do not care about them here. |
|
Right; so "make a copy on write version of the entire process space" vs "block until I hit exec one instruction later" is the tradeoff and ardour picked the second. Part of me thinks @jsakkine may explain how much better the linux copy on write at fork time is now than 5 years ago when the ardour team wrote that (and he's probably correct). So I dunno. Plus it doesn't really matter in this instance; this is when a user does menu / open manual and it starts firefox or what not. Not a performance-moment event. So I'm tempted to follow what ardour does and use vfork just so our linux users can read the manual! Or follow more standard code and use fork just so our linux users can read the manual! @jsakkine your thoughts? |
|
On modern systems the performance difference is very modest but yes in some cases it probably makes sense as @falkTX stated (or alternatively using raw clone() with CLONE_VFORK flag) and in 99% of cases it does not. Nice thing in constrained memory systems when using vfork() is that it does not temporarily commit memory for a new process. Actually, if you don't have MMU in your CPU you have to use vforkO. I wasn't saying that please don't use it. I was just wondering was there any particular reason for opening on URL :) I'd guess that either would be seamless to use here... |
|
Ok cool! Like I said I learned just today that there was a second fork! (Learn something new every day). So wasn’t sure if your question was a “no don’t do this” version of why or so “why” version of why! I’ll change null to nullptrr and merge Thanks for the looks here. Really appreciate us being careful with this software |
Implement openURL using xdg-open, the freedesktop.org command to open a resource in the default browser or viewer. Closes surge-synthesizer#638 menu manual on linux broken Incorporate @falkTX comments to use vfork rather than system
baconpaul
force-pushed
the
linux-browser-638
branch
from
fdc530b to
9e205e8
Compare
|
You should stick to fork() unless you block all the signals before calling vfork() because otherwise the signal handlers might be executed in the context of the child process. We need to fix this by blocking the signals. Also if the parent process has any threads using vfork() can be nasty because they continue to execute in the child context. Fixing this issue requires a full audit on how Surge uses threads and make appropriate quirks around the codebase. Sorry, was a long time since I've given a thought to vfork(). Took time to recall its dark sides but yes for these reasons you should not use it unless you have a hard reason to use it. If you have a really good reason, go for it, but it should be this way around, not as the default choice for everything. |
|
Looked at GLIBC source code. Looks like unless I got it wrong somehow that GLIBC always fallbacks to regular fork() when you call vfork(). I don't question this much because it doesn't really play well with multi-threading. Still for correctness sake this should be fixed by replacing vfork() with fork(). |
|
OK! I'll probably be back in there soon anyway doing the open my patches area; I will fix it then. thanks! |
|
I do not get why we are backtracking now, I thought the reasons for using vfork were pretty clear... |
|
Ok I'll leave it then. It's only one menu item after all! |
|
@baconpaul, It really should be fixed. |
Implement openURL using xdg-open, the freedesktop.org command to open a resource in the default browser or viewer. Closes surge-synthesizer#638 menu manual on linux broken Incorporate @falkTX comments to use vfork rather than system Former-commit-id: a20270b229645745950f006946d1741f2aa8e884 [formerly 5c5ac54] Former-commit-id: 04736b8a465b80425cfcca5d2b1ab6f94a8ceb69 Former-commit-id: 08662f5d55de796b010939fe96896468c2b63f24
Implement openURL using xdg-open, the freedesktop.org command to
open a resource in the default browser or viewer.
Closes #638 menu manual on linux broken