On Sat, Oct 22, 2016 at 08:14:16PM +0200, Jeremie Courreges-Anglas wrote: > Rafael Zalamena <rzalam...@gmail.com> writes: > > > On Fri, Oct 21, 2016 at 01:26:36PM +0200, Jeremie Courreges-Anglas wrote: > >> Rafael Zalamena <rzalam...@gmail.com> writes: > >> > On Fri, Oct 14, 2016 at 06:47:09PM +0200, Rafael Zalamena wrote: > >> >> On Mon, Sep 26, 2016 at 03:45:59PM +0200, Rafael Zalamena wrote: > >> >> ---snip--- > >> > > >> > I got feedback from jca@ that the trap handler wasn't working, so after > >> > trying to reproduce the problem myself I found one 'env' global variable > >> > that was not being set and the child process was dying silently. > >> > (thanks jca@ !) > >> > > >> > Instead of depending on snmpe.c:snmpe env initialization (p_init), I'm > >> > now calling smi_setenv() to do that in the main() function so all > >> > children > >> > get the same behaviour. Also note that we don't have an 'extern' env in > >> > smi.c anymore. > >> > > >> > ok? > >> > >> Works fine here, but then I don't understand the relationship between > >> static struct snmpd *env in smi.c and struct snmpd *env in snmpe.c. > > > > smi.c had a "extern struct snmpd *env" and that variable was only being > > set during the snmpe initialzation (p_init). Since with fork+exec the > > child process runs entirely from scratch (no memory / socket sharing with > > the parent process), we need to set it somewhere else. > > > > It is a known problem that everyone that used to set things in the p_init > > and expected it to work for everyother process was wrong. sunil@ found this > > the hard-way when he found out that p_env wasn't being set for his process > > and he noticed that now p_init is ran in the child process already. Before > > fork+exec the p_init() functions were run by the parent process. > > I understand this, but... > > > To fix the current problem I made the 'env' for smi.c to be a local file > > global variable and set it in the main() process for every child. > > right now there are mixed uses of a global 'env' variable, a global > 'snmpd_env' variable, some local 'env' variables set using ps_env or > cs_env fields. I fear that throwing another *file-local* 'env' variable > in the mix makes the code harder to follow.
Short answer: Yes, you are correct to note this and I think now that it is probably better to write another diff to solve this problem. I'll get back at this diff later. Brief background: With the httpd(8) and relayd(8) we had the same problem: every file depended on some functions setting a global env and it was being set by some p_init() that now is not called anymore because of fork+exec. This case: Before fork+exec snmpe made the favor to set his global env in p_init which every file uses and traphandler was also using it indirectly. Now that we do fork+exec traphandler process don't get env anymore, however traphandler seems to be only using smi.c, so the new diff addresses this problem. I agree with you that this is not the final solution, but it is not the objective of this diff to solve this problem. If we don't feel that it is safe to proceed with this correction (properly set env for all files even for traphandler), we must write another diff that only handles this problem. Just to clarify: I talked with reyk@ about this global env variables in the last hackathon, and we reached the conclusion that the best way to handle this is to use the ps_env whenever is possible, however since a lot of functions don't get access to ps, we must decide what does less changes to the daemon: 1) Use a single global variable (look at the httpd(8) commits); 2) Keep using the env (relayd(8) case); > > Also, why would smi.c be special? > > kroute.c:47:extern struct snmpd *env; > mib.c:61:extern struct snmpd *env; > mps.c:48:extern struct snmpd *env; > timer.c:45:extern struct snmpd *env; > trap.c:42:extern struct snmpd *env; > usm.c:45:extern struct snmpd *env;
