joerg-krause writes
Since commit b87516ae07e719941c25f72b2ccf234903fd2839 onvolumechange does not work anymore for calling shell scripts with root permissions, e.g. onvolumechange = sudo /etc/on-volume-change.sh.
As upmpdcli needs to be run as user upmpdcli this makes calling scripts or commands with root privileges more difficult.
Is there any advantage to use upmpdclis built-in ExecCmd instead of system() (which was used before commit b87516ae07e719941c25f72b2ccf234903fd2839).
medoc92 writes
Some historical implementations of system() were abominations, and I have learnt the hard way to avoid this call like the plague. Maybe things have changed though, but I’d rather solve the problem with ExecCmd ? What is the symptom ? And how do you avoid sudo asking for a password by the way ?
joerg-krause writes
> And how do you avoid sudo asking for a password by the way ?I am adding the user upmpdcli to sudoers: upmpdcli ALL=(ALL) NOPASSWD: ALL.
> Maybe things have changed though, but I'd rather solve the problem with ExecCmd ? What is the symptom ?I am okay with this. The problem is that which() fails, if the command starts with sudo, so I guess it could be as easy as allowing the command to begin with the string sudo.
joerg-krause writes
I would like to change the handling of onvolumechange back to how onplay and co. are handled. If system() does not work properly for some reason, we can still decide to fix ExecCmd to allow sudo.
I’ll prepare a PR soon.
medoc92 writes
J�rg Krause writes:
> I would like to change the handling of onvolumechange back to how onplay and
> co. are handled. If system() does not work properly for some reason, we can
> still decide to fix ExecCmd to allow sudo.
>
> I'll prepare a PR soon.Sorry this is taking a bit of time, but I’m going to check why sudo does not work with ExecCmd in the next few days. So please just wait a little more.
jf
joerg-krause writes
Sorry, for not being patient enough. I did had the time to dig any deeper into ExecCmd neither. The failure happens in which(), so probably it has to be fixed there. Many thanks for having a look!
humarf writes
Sorry to break into your discussion. But if this is problematic can’t you run sudo command in your script?
Best regards humarf
joerg-krause writes
@humarf I could, but it’s tedious to add sudo to every command. And besides, the script is also called from processes running as root, so sudo is not necessary for them.
humarf writes
sudo ( command 1 … command n )
should work and is applied in a second. Of course it should be fixed but at least you have an easy work around for this until jf is happy with what he thinks is the best solution. You could also create a wrapper script that runs sudo command. Then you don’t have a sudo command in the script when it’s already called with root privileges.
I also use onvolumechange and getvolume scripts - that’s why I stepped into this discussion. Fortunately I do not need root privileges for them.
Best regards humarf
medoc92 writes
J�rg Krause writes:
> @humarf I could, but it's tedious to add sudo to every command. And besides,
> the script is also called from processes running as root, so sudo is not
> necessary for them.I just forgot something when replacing "system", it will be fixed in the hour, no need to look for complicated workarounds :) This has nothing to do with sudo by the way, it would happen in any case where onvolumechange has additional parameters (not a simple command name).
medoc92 writes
Joerg could you please check that the current git code works for you ? I’ll then make a release.
joerg-krause writes
@medoc92 It works! Many thanks for fixing!