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
Reyk
>
> 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 9 Nov 2016 23:31:13 -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,7 @@ 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);
> 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 +998,22 @@ parse_config(const char *filename, u_int
>
> endservent();
>
> + if (TAILQ_EMPTY(&conf->sc_addresses)) {
> + 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.
> + h->ss.ss_family = AF_INET;
> + h->port = SNMPD_PORT;
> + TAILQ_INSERT_TAIL(&conf->sc_addresses, h, entry);
> + h = calloc(1, sizeof(*h));
> + if (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 +1218,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 +1265,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 9 Nov 2016 23:09:28 -0000
> @@ -518,6 +518,18 @@ struct address {
> };
> TAILQ_HEAD(addresslist, address);
>
> +struct sock {
> + int fd;
> + TAILQ_ENTRY(sock) entry;
> +};
> +TAILQ_HEAD(socklist, sock);
> +
> +struct evnode {
> + struct event event;
> + TAILQ_ENTRY(evnode) entry;
> +};
> +TAILQ_HEAD(eventlist, evnode);
> +
Why do you need two different lists for socks and events?
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);
> enum usmauth {
> AUTH_NONE = 0,
> AUTH_MD5, /* HMAC-MD5-96, RFC3414 */
> @@ -556,9 +568,7 @@ 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;
It should also have a socklist here, instead of adding more globals below.
> struct timeval sc_starttime;
> u_int32_t sc_engine_boots;
>
> @@ -652,7 +662,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 9 Nov 2016 23:09:28 -0000
> @@ -53,6 +53,8 @@ int snmpe_encode(struct snmp_message *)
> void snmp_msgfree(struct snmp_message *);
>
> struct imsgev *iev_parent;
> +struct socklist snmpesocks;
> +struct eventlist snmpeevents;
>
> static struct privsep_proc procs[] = {
> { "parent", PROC_PARENT, snmpe_dispatch_parent }
> @@ -62,6 +64,8 @@ void
> snmpe(struct privsep *ps, struct privsep_proc *p)
> {
> struct snmpd *env = ps->ps_env;
> + struct address *h;
> + struct sock *so;
> #ifdef DEBUG
> char buf[BUFSIZ];
> struct oid *oid;
> @@ -74,9 +78,18 @@ 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_INIT(&snmpesocks);
> +
> + /* bind SNMP UDP sockets */
> + TAILQ_FOREACH(h, &env->sc_addresses, entry) {
> + so = calloc(1, sizeof(*so));
> + if (so == NULL)
> + fatal("snmpe: %s", __func__);
> + so->fd = snmpe_bind(h);
> + if (so->fd == -1)
> + fatal("snmpe: failed to bind SNMP UDP socket");
> + TAILQ_INSERT_TAIL(&snmpesocks, so, entry);
> + }
>
> proc_run(ps, p, procs, nitems(procs), snmpe_init, NULL);
> }
> @@ -86,16 +99,26 @@ void
> snmpe_init(struct privsep *ps, struct privsep_proc *p, void *arg)
> {
> struct snmpd *env = ps->ps_env;
> + struct sock *so;
> + struct evnode *ev;
>
> kr_init();
> trap_init();
> timer_init();
> usm_generate_keys();
>
> + TAILQ_INIT(&snmpeevents);
> +
> /* 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, &snmpesocks, entry) {
> + ev = calloc(1, sizeof(*ev));
> + if (ev == NULL)
> + fatal("%s", __func__);
> + event_set(&ev->event, so->fd, EV_READ|EV_PERSIST,
> + snmpe_recvmsg, env);
> + event_add(&ev->event, NULL);
> + TAILQ_INSERT_TAIL(&snmpeevents, ev, entry);
> + }
> }
>
> void
> @@ -519,18 +542,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 9 Nov 2016 23:11:55 -0000
> @@ -43,9 +43,9 @@
> #include "snmpd.h"
> #include "mib.h"
>
> -int trapsock;
> -struct event trapev;
> -char trap_path[PATH_MAX];
> +struct socklist trapsocks;
> +struct eventlist trapevents;
> +char trap_path[PATH_MAX];
>
See above. I think trapsocks and trapevents can be merged into a
single "socklist" in struct snmpd, instead of adding more globals
here.
> void traphandler_init(struct privsep *, struct privsep_proc *, void *arg);
> int traphandler_dispatch_parent(int, struct privsep_proc *, struct imsg *);
> @@ -76,10 +76,20 @@ void
> traphandler(struct privsep *ps, struct privsep_proc *p)
> {
> struct snmpd *env = ps->ps_env;
> + struct address *h;
> + struct sock *so;
>
> - if (env->sc_traphandler &&
> - (trapsock = traphandler_bind(&env->sc_address)) == -1)
> - fatal("could not create trap listener socket");
> + TAILQ_INIT(&trapsocks);
> +
> + if (env->sc_traphandler) {
> + TAILQ_FOREACH(h, &env->sc_addresses, entry) {
> + so = calloc(1, sizeof(*so));
> + so->fd = traphandler_bind(h);
> + if (so->fd == -1)
> + fatal("could not create trap listener socket");
> + TAILQ_INSERT_TAIL(&trapsocks, so, entry);
> + }
> + }
>
> proc_run(ps, p, procs, nitems(procs), traphandler_init, NULL);
> }
> @@ -88,20 +98,31 @@ void
> traphandler_init(struct privsep *ps, struct privsep_proc *p, void *arg)
> {
> struct snmpd *env = ps->ps_env;
> + struct sock *so;
> + struct evnode *ev;
> +
> + TAILQ_INIT(&trapevents);
>
> 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, &trapsocks, entry) {
> + ev = calloc(1, sizeof (*ev));
> + if (ev == NULL)
> + fatal("%s", __func__);
> + event_set(&ev->event, so->fd, EV_READ|EV_PERSIST,
> + traphandler_recvmsg, ps);
> + event_add(&ev->event, NULL);
> + TAILQ_INSERT_TAIL(&trapevents, ev, entry);
> + }
> }
>
> 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 +133,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("%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)
> +
> return (s);
> bad:
> close (s);
> @@ -121,8 +147,14 @@ traphandler_bind(struct address *addr)
> void
> traphandler_shutdown(void)
> {
> - event_del(&trapev);
> - close(trapsock);
> + struct evnode *ev;
> + struct sock *so;
> +
> + TAILQ_FOREACH(ev, &trapevents, entry)
> + event_del(&ev->event);
> +
> + TAILQ_FOREACH(so, &trapsocks, entry)
> + close(so->fd);
could be one list ;)
Reyk
> }
>
> int
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
--
