On 06/09/2017 09:21 AM, Andreas Weigel wrote: > Alex Rousskov wrote: >> I would _not_ change that terminology now because >> all the renaming will obfuscate your true fix and make it more difficult >> to understand/review.
> I have to disagree to that one. Renaming obfuscated your true fix. It is a fact. It is not an opinion that we can agree or disagree with. > Looking at the Code for 3.5 I was very confused by the combination of > Config.workers and opt_no_daemon. The names of options and functions did > their best to add to the confusion. I agree that a lot of things can be fixed and improved. Just do not do those fixes and improvements in the tiny patch fixing --foreground. Do it separately, after the --foreground code is fixed. This is _not_ one of those cases where fixing something is impossible without rewriting/renaming a lot of related code. Moreover, if rewriting/renaming a lot of related code was necessary for you to arrive at the correct fix, it still does not imply that you cannot separate the two after the fact. Yes, that separation can be viewed as "overhead", but that "overhead" may be necessary to move things along in a collaborative project. Convincing others that your changes are desirable is an (expensive) part of the Squid game. FWIW, I often spend more resources convincing others that a patch should be committed then on actually making that patch! > I think we agreed that due to > historical reasons, the -N option is doing something different now > (v4/trunk) from what its description is implying. My (v4/trunk) patch > therefore changes the names of this option and the function to retrieve > its value to be more clear, as well as the description of the feature. What is more clear for you may look wrong to others. This is a separate issue/problem. It should be solved in a separate patch (which I, FWIW, would welcome and promise to review). > I do think that my changes are all related to the same thing, namely > allowing a "no daemon" mode with --foreground while retaining the old > -N functionality, but calling the latter by a more apt name. They are related but they hurt understanding of the core fix and they can be done separately. Thus, they should be done separately. See my patch review for details. > I do not expect my patch to be applicable to v3.5. Too many > changes there plus the --foreground option is not even available here. Ah, I thought it was. Thank you for correcting my understanding. There is no need to change v3.5 then! This change does not affect the core of my "divide and conquer" arguments. >> When daemonized, Squid master process does not support reconfiguration >> at all. There are plans to change that, but those difficult changes are >> completely outside your project scope. > My impression was that the current v4/trunk branches do already allow > exactly that They do not. You are confusing worker reconfiguration with master reconfiguration. Kid/worker reconfiguration is supported. Master reconfiguration is not supported (unless that master is also a worker). IIRC, the context of this part of the discussion is whether one can reconfigure the number of kids. One cannot. You did not change that, and you do not need to worry about that. > With the --foreground option working as expected, however, a supervising > process may send signals to its initial child process, which currently > (v4/trunk) will always be the squid master. Thereby, the PID file (and > all possible race conditions associated with it) are not even necessary. This is a common misunderstanding. The PID file is necessary for many things beyond supervisor's signaling needs. For example, it is impossible to reliably start an SMP Squid instance without a PID file. > While this does not seem to be the intended interface, it IMHO is an > improvement to determining the process ID by reading some PID file and > works great for supervision schemes that do not like daemons. If you know the master PID and just need to send it some signal, then you do not need a PID file. In many other use cases, you (and Squid itself) do need that file. (OT: The right signal to do X is not a part of Squid's interface, so a supervisor sending raw signals to Squid is using private Squid details that may change. The public interface is "squid -k X". That -k interface has its own problems, but those problems do not change the fact that -k is the official signaling interface.) > PS: I do understand your reluctance to change function/variable names > and/or description of command line options. I still think it is better > to have those updated, Agreed. We disagree _where_ that update should happen, not whether it should happen. > even if it may slightly obfuscate the actual > behavioral change. I also believe in the broken window theorem and try > to at least fix some of those from time to time. Your patch was replacing broken windows during a shootout with a murderer. Not a good strategy, even if you believe in the broken window theory! > I once [...] encountered diverse > variables/macros referring to the same concept but used differently > throughout the code (Squid_MaxFd, SQUID_MAXFD), eventually caused a > dangerous segfault (see bugs.squid-cache.org/show_bug.cgi?id=4650 if you > are interested -- the patch I proposed there also tried to fix the issue > by introducing various renames, which is probably the reason that no one > ever cared to look at it ;-/ ) FYI: IIRC, I did look at that patch but could not comment because the patch scope was defined as "the changes I proposed [in a large comment discussing various problems]" and the patch itself appeared to contain a mix of renames and bug fixes. It would take me a long time to understand the issue and to separate the core changes from the renames. I do not have that time, unfortunately. The lack of progress on that bug does not mean that your changes were wrong or ignored! It means that you need to do more than to improve code. You need to convince others that your improvements should be committed. That takes more than writing code. The associated overheads are unfortunate, but there is no other way that works here AFAIK. Thank you, Alex. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
