Re: snmpd(8): turn snmpd_env the only global

[Jeremie Courreges-Anglas]

Rafael Zalamena  writes:

> This diff removes all "extern struct snmpd *" lines from source files,
> replaces all 'env' occurences with 'snmpd_env' and adds the extern
> declaration for snmpd_env in the snmpd.h header.
>
> With this diff we only need to guarantee that this variable is set,
> we avoid shadowing other 'env' variables and we diminish the confusion
> about this env variable thing.
>
> We need this diff (or something with the same effect) to proceed with
> fork+exec, because jca@ found out that traphandler child process do not
> set env. We could alternatively just set env in traphandler p_init() and
> remove snmpd_env, however it looks cleaner to me to not have all this
> extern in all .c files and have a propper name for 'env'.

It looks cleaner to me too.  IIUC we could go further and remove all
local env variables, ps_env, cs_env, etc.  Anyway, I think it's already
an improvement.

> ok?

Looks fine and works fine in my tests, ok jca@

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

snmpd(8): turn snmpd_env the only global

[Rafael Zalamena]

This diff removes all "extern struct snmpd *" lines from source files,
replaces all 'env' occurences with 'snmpd_env' and adds the extern
declaration for snmpd_env in the snmpd.h header.

With this diff we only need to guarantee that this variable is set,
we avoid shadowing other 'env' variables and we diminish the confusion
about this env variable thing.

We need this diff (or something with the same effect) to proceed with
fork+exec, because jca@ found out that traphandler child process do not
set env. We could alternatively just set env in traphandler p_init() and
remove snmpd_env, however it looks cleaner to me to not have all this
extern in all .c files and have a propper name for 'env'.

ok?


Index: kroute.c
===
RCS file: /home/obsdcvs/src/usr.sbin/snmpd/kroute.c,v
retrieving revision 1.33
diff -u -p -r1.33 kroute.c
--- kroute.c3 Sep 2016 15:45:02 -   1.33
+++ kroute.c22 Oct 2016 22:20:00 -
@@ -44,8 +44,6 @@
 
 #include "snmpd.h"
 
-extern struct snmpd*env;
-
 struct ktable  **krt;
 u_intkrt_size;
 
@@ -173,8 +171,9 @@ kr_init(void)
, sizeof(opt)) == -1)
log_warn("%s: SO_USELOOPBACK", __func__);   /* not fatal */
 
-   if (env->sc_rtfilter && setsockopt(kr_state.ks_fd, PF_ROUTE,
-   ROUTE_MSGFILTER, >sc_rtfilter, sizeof(env->sc_rtfilter)) == -1)
+   if (snmpd_env->sc_rtfilter && setsockopt(kr_state.ks_fd, PF_ROUTE,
+   ROUTE_MSGFILTER, _env->sc_rtfilter,
+   sizeof(snmpd_env->sc_rtfilter)) == -1)
log_warn("%s: ROUTE_MSGFILTER", __func__);
 
/* grow receive buffer, don't wanna miss messages */
Index: mib.c
===
RCS file: /home/obsdcvs/src/usr.sbin/snmpd/mib.c,v
retrieving revision 1.80
diff -u -p -r1.80 mib.c
--- mib.c   17 Nov 2015 12:30:23 -  1.80
+++ mib.c   22 Oct 2016 22:26:52 -
@@ -58,8 +58,6 @@
 #include "snmpd.h"
 #include "mib.h"
 
-extern struct snmpd*env;
-
 /*
  * Defined in SNMPv2-MIB.txt (RFC 3418)
  */
@@ -255,7 +253,7 @@ mib_sysor(struct oid *oid, struct ber_oi
 int
 mib_getsnmp(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
 {
-   struct snmp_stats   *stats = >sc_stats;
+   struct snmp_stats   *stats = _env->sc_stats;
long longi;
struct statsmap {
u_int8_t m_id;
@@ -316,7 +314,7 @@ mib_getsnmp(struct oid *oid, struct ber_
 int
 mib_setsnmp(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
 {
-   struct snmp_stats   *stats = >sc_stats;
+   struct snmp_stats   *stats = _env->sc_stats;
long longi;
 
if (ber_get_integer(*elm, ) == -1)
@@ -354,11 +352,11 @@ mib_engine(struct oid *oid, struct ber_o
 {
switch (oid->o_oid[OIDIDX_snmpEngine]) {
case 1:
-   *elm = ber_add_nstring(*elm, env->sc_engineid,
-   env->sc_engineid_len);
+   *elm = ber_add_nstring(*elm, snmpd_env->sc_engineid,
+   snmpd_env->sc_engineid_len);
break;
case 2:
-   *elm = ber_add_integer(*elm, env->sc_engine_boots);
+   *elm = ber_add_integer(*elm, snmpd_env->sc_engine_boots);
break;
case 3:
*elm = ber_add_integer(*elm, snmpd_engine_time());
@@ -375,7 +373,7 @@ mib_engine(struct oid *oid, struct ber_o
 int
 mib_usmstats(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
 {
-   struct snmp_stats   *stats = >sc_stats;
+   struct snmp_stats   *stats = _env->sc_stats;
long longi;
struct statsmap {
u_int8_t m_id;
@@ -697,7 +695,7 @@ mib_hrdevice(struct oid *oid, struct ber
 
/* Get and verify the current row index */
idx = o->bo_id[OIDIDX_hrDeviceEntry];
-   if (idx > (u_int)env->sc_ncpu)
+   if (idx > (u_int)snmpd_env->sc_ncpu)
return (1);
 
/* Tables need to prepend the OID on their own */
@@ -748,7 +746,7 @@ mib_hrprocessor(struct oid *oid, struct 
 
/* Get and verify the current row index */
idx = o->bo_id[OIDIDX_hrDeviceEntry];
-   if (idx > (u_int)env->sc_ncpu)
+   if (idx > (u_int)snmpd_env->sc_ncpu)
return (1);
else if (idx < 1)
idx = 1;
@@ -766,9 +764,9 @@ mib_hrprocessor(struct oid *oid, struct 
 * The percentage of time that the system was not
 * idle during the last minute.
 */
-   if (env->sc_cpustates == NULL)
+   if (snmpd_env->sc_cpustates == NULL)
return (-1);
-   cptime2 = env->sc_cpustates + (CPUSTATES * (idx - 1));
+   cptime2 = snmpd_env->sc_cpustates + (CPUSTATES * (idx - 1));
val = 100 -