> Am 17.11.2016 um 14:28 schrieb Jeremie Courreges-Anglas <[email protected]>:
>
> 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. :)
>
you're right
> [...]
>
>>> + 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".
>
I prefer prefixes in structs because it's easier to grep and read, especially
when using pointers all over the place. non-prefixed struct members are
artifacts here.
> [...]
>
>>> + 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?
>
right, __func__ is for log_debug and sometimes fatal.
> Here's a diff that includes your suggestions.
>
looks good. OK.
>
> 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
