The attached patch does the following.. * add ability to change user mediabrowser runs as instead of hardcoding it. This has the init system drop the user privileges instead of relying on daemon. This is also the reason I had to initialize the pidfile * remove setting LC_ALL and LANG, is this needed? * suggest using sysrc instead of echoing to /etc/rc.conf which is less fool proof (I just learned about this cool tool =] )
Maintainer CC'd
Created attachment 149308 [details] Patch to mediabrowser port
Created attachment 149310 [details] Patch to plexmediaserver port
Created attachment 149311 [details] Patch to plexmediaserver port keep fixing typos I made =/
The reason I set LC_ALL and LANG is that without these being set (as per my jail that mediabrowser is running on), mediabrowser gets a System.ArgumentNullException (handled by a try/catch, but seen if you run with mono --trace=N:nothing). The code that is failing was get_posix_locale was returning NULL in mono/mono/metadata/locales.c, which was caught in mcs/class/corlib/System.Globalization/CultureInfo.cs.
Lol, k way over my head. Guess we shouldn't remove them then =]
I guess the LC_ALL and LANG settings should first check if they are already set, and only set them to "C" if they are not (do not override them if already set)
Created attachment 149312 [details] Patch to plexmediaserver port fixed
Also... stop calling it plexmediaserver in your patch title ;)
Whoops, I just kept clicking the auto-suggestion in my browser... I guess it's getting too late for me..
Also, I deliberate didn't provide the flexibility to set user/group, since the FreeBSD UIDs and GIDs files have been updated to define the user/group mediabrowser, and that gets installed when the package is installed. Given that, do you still think we should provide this flexibility? Why would anyone need it?
Also, is there something wrong with relying on daemon to run mono with a specific user? It avoids having to initialise the pid file, and ensures the pid file is cleaned up (deleted) when the programs stops. Using your method, the pid file will continue to sit there even after the service has stopped (presumably with the old pid number in it?)
Yes, this is standard; users may want to run certain daemons as certain users. Even if you don't put the hints there a FreeBSD user could set mediabrowser_user and it would break things. the init system would try to run "command" as that user, which would fail because whatever user they set can't run "daemon" that forks to the mediabrowser user.
True that the pidfile doesn't get cleaned. Are you saying daemon as root does clean the pidfile? Either way, this seems like a minor problem compared to not using the standard %%PORTNAME%%_user method built into the init system.
Yes, using daemon does clean the pid file when the service is stopped. Also, in your patch the pid file is created with ownership of mediabrowser_user the first time the user runs the service. If they try and run as a different user after that, it will fail since they won't have write access to the pid file. Can the script be made to be flexible to that (I guess we can define a stop_postcmd that cleans up (deletes) the pidfile... that will fix both of these things).
we could either take it out of the "if" so the pidfile is always reinitialized with the correct user. or, cleanup the pidfile in a postcmd. If we don't do anything and a FreeBSD user did change the user mediabrowser ran as, (and they'd need to change the ownership of the data_dir). They'd encounter this message, which I think they could figure out. daemon: pidfile ``/var/run/mediabrowser.pid'': Permission denied /usr/local/etc/rc.d/mediabrowser: WARNING: failed to start mediabrowser #### I think it'd be fine to clean the pidfile, or not.
Lets clean it up, its... cleaner...
Lol, k. I'll see what I can cook up real quick. After that going to bed so feel free to work on it or I can work on it tomorrow.
Created attachment 149313 [details] Patch to mediabrowser port hopefully the last one, lol
another alternative is we create the directory /var/run/mediabrowser owned by mediabrowser_user. I think this would allow daemon to clean the pid but an empty folder would be left instead. see net-p2p/btsync as an example.
I quite like the way we have it at the moment. Sure it means having a start_precmd and a stop_postcmd, but its quite neat.
k, either way works. just giving an alternative.
Created attachment 149380 [details] PR194959: improvements to mediabrowser port (with maintainer edits) I have made some minor edits to these suggested improvements. Maintainer approval granted to commit (this version of the patch).
Okay. I'll commit this later today. Personally I have reservations about changing to "sysrc". Most ports, and the FreeBSD Handbook, just say "add 'foo_enable="YES"' to /etc/rc.conf." sysrc is only part of 9.2+. 9.0 is supported until 2016/12/31, and 9.1 is supported until 2014/12/31. Your call, Ben.
Created attachment 149381 [details] PR194959: improvements to mediabrowser port (with maintainer edits) Thanks for the heads up Adam. I have removed the change to pkg-message that mentioned sysrc, in favour of having accurate comments on all supported versions of FreeBSD.
Committed. Thanks, Josh and Ben.
A commit references this bug: Author: adamw Date: Thu Nov 13 21:37:06 UTC 2014 New revision: 372539 URL: https://svnweb.freebsd.org/changeset/ports/372539 Log: - Allow overriding the user/group as which the service is run - Don't clobber LC_ALL/LANG - Add a deinstallation message PR: 194959 Submitted by: joshruehlig@gmail.com / maintainer Approved by: Ben Woods (maintainer) Changes: head/multimedia/mediabrowser/files/mediabrowser.in head/multimedia/mediabrowser/pkg-deinstall