Bug 247247 - www/squid: fails to start when dir for pidfile does not exist (e.g. /var/run is a tmpfs)
Summary: www/squid: fails to start when dir for pidfile does not exist (e.g. /var/run ...
Status: New
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-ports-bugs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-14 08:06 UTC by Walter von Entferndt
Modified: 2020-07-05 08:37 UTC (History)
1 user (show)

See Also:
bugzilla: maintainer-feedback? (timp87)


Attachments
patch for squid's service script (8.69 KB, patch)
2020-06-14 08:06 UTC, Walter von Entferndt
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Walter von Entferndt 2020-06-14 08:06:03 UTC
Created attachment 215535 [details]
patch for squid's service script

System: FreeBSD 12.1-RELEASE-p6
squid: 4.10 from pkg (not built myself)
fstab: none /var/run tmpfs rw,size=2M,noexec,gid=0,uid=0 0 0

Hello Tim!

By default, squid's pidfile is /var/run/squid/squid.pid.

The dir /var/run/squid gets neither checked nor created by the service script /etc/rc.d/squid.

Many users will create that directory when they run into that bug (and not report it), but that fixes it only if /var/run resides on stable storage -- in contrast, a common recommendation is a tmpfs like given above, and this implies it is empty after every reboot.  Thus, the service script should check and create that dir, if neccessary.

My suggestions for appropiate defaults are:
pidfile=/var/run/squid.pid (why does squid need it's own directory? worker threads?)
chdir=/var/cache/squid
    The admin might have special ZFS parameters set for caches (e.g. relaxed
    metadata redundancy, no snapshots).
    2nd many would agree to keep /usr and /var narrow.
    Remember 30 years ago, when each and every tarball installed itself into
    /usr/gprolog, /usr/X11, /usr/squid,... nobody likes that nowadays.

Appended is the patch to my fixed service script; it is by no means perfect, but you may want to copy&paste some parts.  I removed comments where the information they give is brilliantly clear (on 1st sight, even for beginners) from the code.

The patch was produced with 'diff -U 7 etc/rc.d/squid{.orig,}' from /usr/local.

With kind regards, Torsten
Comment 1 timp87 2020-06-17 07:07:28 UTC
(In reply to t.eichstaedt from comment #0)
Hi, Torsten! Thank you for the report!

> pidfile=/var/run/squid.pid (why does squid need it's own directory? worker threads?)

Oh, I don't remember why it was placed in a separate dir. Probably some kind of legacy. I'll take a look at svn history. The suggestion looks good to me.


> chdir=/var/cache/squid
This is exactly what I wanted to do back in 2015 https://lists.freebsd.org/pipermail/freebsd-ports/2015-July/099740.html
But people told me that /var/cache is intended for different "type"(?) of cache.
Well, read the thread :)

> Appended is the patch to my fixed service script
I'll review it soon. And probably get back with questions ;)
Comment 2 timp87 2020-06-19 13:02:47 UTC
BTW, here is why /var/run/squid exists https://svnweb.freebsd.org/ports?view=revision&revision=391555
I think I'll add some handling into rc script
Comment 3 Walter von Entferndt 2020-06-20 16:10:20 UTC
(In reply to timp87 from comment #2)
Thx, very interesting.  The question is which is the usual standard use-case: server or workstation.  I guess the latter, but I'm perfectly ok with defaults adjusted for server, since I can change Squid's home to /var/cache/squid. 2nd I do not agree to this guy's argument concerning short- vs. long-term caches, but that's all not important.

Please check that it's not neccessary to call squid_configtest() (at least) when squid is stopped.  I missed that, my patch is buggy at least here.
Comment 4 timp87 2020-07-02 08:29:02 UTC
Follow https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=247397

I have taken some ideas from your patch. Follow https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=247397.
I didn't use set_rcvar like in your patch because it's not the way how porters handbook describes rc script creation. It's to keep rc script clear to even new users. Also I'm not sure if it's ideologically right to use set_rcvar in ports external rc scripts.
Comment 5 Walter von Entferndt 2020-07-02 10:56:02 UTC
Hmm.  The use of 'set_rcvar()' makes such scripts to produce their own documentation, which is very friendly to users.  You do not have to /read/ the script, but can /ask/ it how to use it.  If the Porters Handbook does not tell to use it, I'll not hesitate to file in a bug report on that ;)  IMHO, if a given standard (template) is insufficient, it's up to us to fix that (and communicate why), right?  Remember the principles of XP: communication, feedback, simplicity, /courage/

Eventually, it's your decision.  It's about what is more important to you: a user-friendly script or a "clean" script.  IMHO ease of maintainance does not suffer from using 'set_rcvar()'.  The readability does, but not to such a degree that I would say don't use it.
Thx for your time!
Comment 6 timp87 2020-07-02 13:22:27 UTC
(In reply to Walter von Entferndt from comment #5)
Just looked into svn history. Seems like I'm wrong and set_rcvar() is something we should use.
I'm gonna start thread in mailing list about porters handbook and rc.subr(8) man page
Comment 7 timp87 2020-07-02 18:15:14 UTC
(In reply to timp87 from comment #6)
Done. Let's see what people think about set_rcvar() use
Comment 8 Walter von Entferndt 2020-07-02 19:33:17 UTC
(In reply to timp87 from comment #7)
Sorry I forgot: please add me to the CC.  And after all: you also fixed the endless loop waiting for the pid, didn't you? ;) Cheers, cu.
Comment 9 timp87 2020-07-05 08:37:28 UTC
(In reply to Walter von Entferndt from comment #8)
Sorry, I'm too slow.
The whole thread is available here http://freebsd.1045724.x6.nabble.com/set-rcvar-function-use-td6403176.html