Reyk Floeter <[email protected]> writes:
> On Thu, Nov 10, 2016 at 12:57:13AM +0100, Jeremie Courreges-Anglas wrote:
>>
>> The following diff adds support for listening multiple addresses (thus
>> for dual-stack setups). Multiple "listen on" settings are allowed, the
>> default is to listen on 0.0.0.0 and :: (currently, only 0.0.0.0).
>> A single "listen on hostname" line arbitrarily supports up to 16
>> addresses.
>>
>
> Very useful, thanks!
>
>> It also tweaks the host*() functions so that addresses are used in the
>> order where they are resolved*. This affects bind addresses, but also
>> "trap receiver" addresses. So in "trap receiver hostname", if hostname
>> resolves to both an IPv4 and an IPv6 address, the address that is picked
>> up respects the order defined by the "family" keyword in resolv.conf.
>> This *could* break existing setups.
>>
>> Feedback and oks welcome,
>>
>> * httpd, relayd and ldapd might also benefit from such a change
>
> Yes, partially.
>
>>
>
> - See comments inline.
>
> - Should it handle the following case where it currently fails:
>
> listen on 127.0.0.1
> listen on 0.0.0.0
>
> In comparison, ntpd just ignores it:
> bind on 0.0.0.0 failed, skipping: Address already in use
Hmm, having ntpd fail because of a bad address could be hard to spot,
whereas snmpd is supposed to be queried from other machines. I tend to
prefer hard failures but I don't have a strong opinion. Whatever people
prefer. :)
[...]
>> + struct address *h;
>> + h = calloc(1, sizeof(*h));
>> + if (h == NULL)
>> + fatal("snmpe: %s", __func__);
>
> I usually don't complain about this style, and it is highly
> subjective, but I think this h h is just repetition and could be
>
> if ((h = calloc(1, sizeof(*h))) == NULL)
> fatal("snmpe: %s", __func__);
>
> I see that it makes sense to split the if and assignment with very
> long function calls, but you keep on doing this repetition with very
> short calls all over the diff.
ack
[...]
> Why do you need two different lists for socks and events?
No reason,
> Additionally, the name "sock" might cause namespace issues.
>
> struct listen_sock {
> int s_fd;
> struct event s_ev;
> TAILQ_ENTRY(sock) s_entry;
> };
> TAILQ_HEAD(socklist, listen_sock);
Simpler indeed, thanks. I used "entry" like in struct addresslist,
there doesn't seem to be a pattern here. We could also use "fd" and
"ev".
[...]
>> + log_info("%s: binding to address %s:%d", __func__, buf, SNMPD_TRAPPORT);
>
> you already did, I think more common is
>
> log_info("%s: listening on %s:%d", __func__, buf, SNMPD_TRAPPORT);
>
> (should be changed in snmpe_bind() as well)
Fine. While here I replaced the function name by the engine name, the
rationale being that normal messages don't need to reflect internal
details. Objections?
Here's a diff that includes your suggestions.
Index: control.c
===================================================================
RCS file: /d/cvs/src/usr.sbin/snmpd/control.c,v
retrieving revision 1.39
diff -u -p -r1.39 control.c
--- control.c 2 Sep 2016 13:28:36 -0000 1.39
+++ control.c 9 Nov 2016 23:09:28 -0000
@@ -592,7 +592,7 @@ control_dispatch_agentx(int fd, short ev
}
}
dispatch:
- snmpe_dispatchmsg(msg);
+ snmpe_dispatchmsg(msg, fd);
break;
}
Index: parse.y
===================================================================
RCS file: /d/cvs/src/usr.sbin/snmpd/parse.y,v
retrieving revision 1.40
diff -u -p -r1.40 parse.y
--- parse.y 9 Nov 2016 20:31:56 -0000 1.40
+++ parse.y 17 Nov 2016 13:07:10 -0000
@@ -198,25 +198,13 @@ yesno : STRING {
;
main : LISTEN ON STRING {
- struct addresslist al;
- struct address *h;
-
- TAILQ_INIT(&al);
- if (host($3, &al, 1, SNMPD_PORT, NULL, NULL, NULL)
- <= 0) {
+ if (host($3, &conf->sc_addresses, 16, SNMPD_PORT, NULL,
+ NULL, NULL) <= 0) {
yyerror("invalid ip address: %s", $3);
free($3);
YYERROR;
}
free($3);
- h = TAILQ_FIRST(&al);
- bcopy(&h->ss, &conf->sc_address.ss, sizeof(*h));
- conf->sc_address.port = h->port;
-
- while ((h = TAILQ_FIRST(&al)) != NULL) {
- TAILQ_REMOVE(&al, h, entry);
- free(h);
- }
}
| READONLY COMMUNITY STRING {
if (strlcpy(conf->sc_rdcommunity, $3,
@@ -989,8 +977,8 @@ parse_config(const char *filename, u_int
conf->sc_flags = flags;
conf->sc_confpath = filename;
- conf->sc_address.ss.ss_family = AF_INET;
- conf->sc_address.port = SNMPD_PORT;
+ TAILQ_INIT(&conf->sc_addresses);
+ TAILQ_INIT(&conf->sc_sockets);
conf->sc_ps.ps_csock.cs_name = SNMPD_SOCKET;
TAILQ_INIT(&conf->sc_ps.ps_rcsocks);
strlcpy(conf->sc_rdcommunity, "public", SNMPD_MAXCOMMUNITYLEN);
@@ -1011,6 +999,20 @@ parse_config(const char *filename, u_int
endservent();
+ if (TAILQ_EMPTY(&conf->sc_addresses)) {
+ struct address *h;
+ if ((h = calloc(1, sizeof(*h))) == NULL)
+ fatal("snmpe: %s", __func__);
+ h->ss.ss_family = AF_INET;
+ h->port = SNMPD_PORT;
+ TAILQ_INSERT_TAIL(&conf->sc_addresses, h, entry);
+ if ((h = calloc(1, sizeof(*h))) == NULL)
+ fatal("snmpe: %s", __func__);
+ h->ss.ss_family = AF_INET6;
+ h->port = SNMPD_PORT;
+ TAILQ_INSERT_TAIL(&conf->sc_addresses, h, entry);
+ }
+
/* Free macros and check which have not been used. */
for (sym = TAILQ_FIRST(&symhead); sym != NULL; sym = next) {
next = TAILQ_NEXT(sym, entry);
@@ -1215,7 +1217,7 @@ host_dns(const char *s, struct addressli
h->sa_srcaddr = src;
- TAILQ_INSERT_HEAD(al, h, entry);
+ TAILQ_INSERT_TAIL(al, h, entry);
cnt++;
}
if (cnt == max && res) {
@@ -1262,7 +1264,7 @@ host(const char *s, struct addresslist *
}
h->sa_srcaddr = src;
- TAILQ_INSERT_HEAD(al, h, entry);
+ TAILQ_INSERT_TAIL(al, h, entry);
return (1);
}
Index: snmpd.h
===================================================================
RCS file: /d/cvs/src/usr.sbin/snmpd/snmpd.h,v
retrieving revision 1.72
diff -u -p -r1.72 snmpd.h
--- snmpd.h 9 Nov 2016 20:31:56 -0000 1.72
+++ snmpd.h 17 Nov 2016 13:06:54 -0000
@@ -518,6 +518,13 @@ struct address {
};
TAILQ_HEAD(addresslist, address);
+struct listen_sock {
+ int s_fd;
+ struct event s_ev;
+ TAILQ_ENTRY(listen_sock) entry;
+};
+TAILQ_HEAD(socklist, listen_sock);
+
enum usmauth {
AUTH_NONE = 0,
AUTH_MD5, /* HMAC-MD5-96, RFC3414 */
@@ -556,9 +563,8 @@ struct snmpd {
#define SNMPD_F_NONAMES 0x02
const char *sc_confpath;
- struct address sc_address;
- int sc_sock;
- struct event sc_ev;
+ struct addresslist sc_addresses;
+ struct socklist sc_sockets;
struct timeval sc_starttime;
u_int32_t sc_engine_boots;
@@ -652,7 +658,7 @@ struct kif_arp *karp_getaddr(struct sock
/* snmpe.c */
void snmpe(struct privsep *, struct privsep_proc *);
void snmpe_shutdown(void);
-void snmpe_dispatchmsg(struct snmp_message *);
+void snmpe_dispatchmsg(struct snmp_message *, int);
/* trap.c */
void trap_init(void);
Index: snmpe.c
===================================================================
RCS file: /d/cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.45
diff -u -p -r1.45 snmpe.c
--- snmpe.c 9 Nov 2016 20:31:56 -0000 1.45
+++ snmpe.c 17 Nov 2016 13:08:13 -0000
@@ -61,7 +61,9 @@ static struct privsep_proc procs[] = {
void
snmpe(struct privsep *ps, struct privsep_proc *p)
{
- struct snmpd *env = ps->ps_env;
+ struct snmpd *env = ps->ps_env;
+ struct address *h;
+ struct listen_sock *so;
#ifdef DEBUG
char buf[BUFSIZ];
struct oid *oid;
@@ -74,9 +76,13 @@ snmpe(struct privsep *ps, struct privsep
}
#endif
- /* bind SNMP UDP socket */
- if ((env->sc_sock = snmpe_bind(&env->sc_address)) == -1)
- fatalx("snmpe: failed to bind SNMP UDP socket");
+ TAILQ_FOREACH(h, &env->sc_addresses, entry) {
+ if ((so = calloc(1, sizeof(*so))) == NULL)
+ fatal("snmpe: %s", __func__);
+ if ((so->s_fd = snmpe_bind(h)) == -1)
+ fatal("snmpe: failed to bind SNMP UDP socket");
+ TAILQ_INSERT_TAIL(&env->sc_sockets, so, entry);
+ }
proc_run(ps, p, procs, nitems(procs), snmpe_init, NULL);
}
@@ -85,7 +91,8 @@ snmpe(struct privsep *ps, struct privsep
void
snmpe_init(struct privsep *ps, struct privsep_proc *p, void *arg)
{
- struct snmpd *env = ps->ps_env;
+ struct snmpd *env = ps->ps_env;
+ struct listen_sock *so;
kr_init();
trap_init();
@@ -93,9 +100,11 @@ snmpe_init(struct privsep *ps, struct pr
usm_generate_keys();
/* listen for incoming SNMP UDP messages */
- event_set(&env->sc_ev, env->sc_sock, EV_READ|EV_PERSIST,
- snmpe_recvmsg, env);
- event_add(&env->sc_ev, NULL);
+ TAILQ_FOREACH(so, &env->sc_sockets, entry) {
+ event_set(&so->s_ev, so->s_fd, EV_READ|EV_PERSIST,
+ snmpe_recvmsg, env);
+ event_add(&so->s_ev, NULL);
+ }
}
void
@@ -156,7 +165,7 @@ snmpe_bind(struct address *addr)
if (print_host(&addr->ss, buf, sizeof(buf)) == NULL)
goto bad;
- log_info("snmpe_bind: binding to address %s:%d", buf, addr->port);
+ log_info("snmpe: listening on %s:%d", buf, addr->port);
return (s);
@@ -519,18 +528,18 @@ snmpe_recvmsg(int fd, short sig, void *a
}
}
- snmpe_dispatchmsg(msg);
+ snmpe_dispatchmsg(msg, fd);
}
void
-snmpe_dispatchmsg(struct snmp_message *msg)
+snmpe_dispatchmsg(struct snmp_message *msg, int sock)
{
if (snmpe_parsevarbinds(msg) == 1)
return;
/* not dispatched to subagent; respond directly */
msg->sm_context = SNMP_C_GETRESP;
- snmpe_response(snmpd_env->sc_sock, msg);
+ snmpe_response(sock, msg);
}
void
Index: traphandler.c
===================================================================
RCS file: /d/cvs/src/usr.sbin/snmpd/traphandler.c,v
retrieving revision 1.6
diff -u -p -r1.6 traphandler.c
--- traphandler.c 28 Oct 2016 09:07:08 -0000 1.6
+++ traphandler.c 17 Nov 2016 13:08:37 -0000
@@ -43,8 +43,6 @@
#include "snmpd.h"
#include "mib.h"
-int trapsock;
-struct event trapev;
char trap_path[PATH_MAX];
void traphandler_init(struct privsep *, struct privsep_proc *, void *arg);
@@ -76,10 +74,18 @@ void
traphandler(struct privsep *ps, struct privsep_proc *p)
{
struct snmpd *env = ps->ps_env;
+ struct address *h;
+ struct listen_sock *so;
- if (env->sc_traphandler &&
- (trapsock = traphandler_bind(&env->sc_address)) == -1)
- fatal("could not create trap listener socket");
+ if (env->sc_traphandler) {
+ TAILQ_FOREACH(h, &env->sc_addresses, entry) {
+ if ((so = calloc(1, sizeof(*so))) == NULL)
+ fatal("%s", __func__);
+ if ((so->s_fd = traphandler_bind(h)) == -1)
+ fatal("could not create trap listener socket");
+ TAILQ_INSERT_TAIL(&env->sc_sockets, so, entry);
+ }
+ }
proc_run(ps, p, procs, nitems(procs), traphandler_init, NULL);
}
@@ -88,20 +94,24 @@ void
traphandler_init(struct privsep *ps, struct privsep_proc *p, void *arg)
{
struct snmpd *env = ps->ps_env;
+ struct listen_sock *so;
if (!env->sc_traphandler)
return;
/* listen for SNMP trap messages */
- event_set(&trapev, trapsock, EV_READ|EV_PERSIST, traphandler_recvmsg,
- ps);
- event_add(&trapev, NULL);
+ TAILQ_FOREACH(so, &env->sc_sockets, entry) {
+ event_set(&so->s_ev, so->s_fd, EV_READ|EV_PERSIST,
+ traphandler_recvmsg, ps);
+ event_add(&so->s_ev, NULL);
+ }
}
int
traphandler_bind(struct address *addr)
{
int s;
+ char buf[512];
if ((s = snmpd_socket_af(&addr->ss, htons(SNMPD_TRAPPORT))) == -1)
return (-1);
@@ -112,6 +122,11 @@ traphandler_bind(struct address *addr)
if (bind(s, (struct sockaddr *)&addr->ss, addr->ss.ss_len) == -1)
goto bad;
+ if (print_host(&addr->ss, buf, sizeof(buf)) == NULL)
+ goto bad;
+
+ log_info("traphandler: listening on %s:%d", buf, SNMPD_TRAPPORT);
+
return (s);
bad:
close (s);
@@ -121,8 +136,12 @@ traphandler_bind(struct address *addr)
void
traphandler_shutdown(void)
{
- event_del(&trapev);
- close(trapsock);
+ struct listen_sock *so;
+
+ TAILQ_FOREACH(so, &snmpd_env->sc_sockets, entry) {
+ event_del(&so->s_ev);
+ close(so->s_fd);
+ }
}
int
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
