As a point of discussion:
Why shouldn't we drop support for snmp:
1) The license for quagga is incompatible with the snmp licensing
(Yes I'm aware that some people/distributions don't care)
2) There hasn't been any recent developments in quagga for snmp, nor does
it look like it's really on anyone's radar.
3) snmp support in quagga is incomplete in that I'm not aware of a mib that
is even fully implemented. Leaving people who do attempt to use it with a
false impression of usability
4) new features being added to Quagga are not taking snmp into account.
5) The library is stuck with select mechanics and don't allow more than 1k
fd's. This prevents easy moving forward to implement better epoll/kpoll
semantics.
thoughts?
donald
On Mon, Jun 13, 2016 at 11:29 AM, David Lamparter <
equi...@opensourcerouting.org> wrote:
> AgentX fd/timeout handling is rather hackishly monkeyed into thread.c.
> Replace with code that uses plain thread_* functions.
>
> NB: Net-SNMP's API rivals Quagga's in terms of age and absence of
> documentation. netsnmp_check_outstanding_agent_requests() in particular
> seems to be unused and is therefore untested.
>
> The most useful documentation on this is actually the blog post Vincent
> Bernat wrote when he originally integrated this into lldpd and Quagga:
> https://vincent.bernat.im/en/blog/2012-snmp-event-loop.html
>
> Signed-off-by: David Lamparter <equi...@opensourcerouting.org>
> ---
> lib/agentx.c | 104
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> lib/thread.c | 52 ++----------------------------
> 2 files changed, 105 insertions(+), 51 deletions(-)
>
> diff --git a/lib/agentx.c b/lib/agentx.c
> index be6b432..5d7d057 100644
> --- a/lib/agentx.c
> +++ b/lib/agentx.c
> @@ -24,11 +24,108 @@
> #if defined HAVE_SNMP && defined SNMP_AGENTX
> #include <net-snmp/net-snmp-config.h>
> #include <net-snmp/net-snmp-includes.h>
> +#include <net-snmp/agent/net-snmp-agent-includes.h>
> +#include <net-snmp/agent/snmp_vars.h>
>
> #include "command.h"
> #include "smux.h"
>
> -int agentx_enabled = 0;
> +static int agentx_enabled = 0;
> +
> +static struct thread_master *agentx_tm;
> +static struct thread *timeout_thr = NULL;
> +static struct list *events = NULL;
> +
> +static void agentx_events_update(void);
> +
> +static int
> +agentx_timeout(struct thread *t)
> +{
> + timeout_thr = NULL;
> +
> + snmp_timeout ();
> + run_alarms ();
> + netsnmp_check_outstanding_agent_requests ();
> + agentx_events_update ();
> + return 0;
> +}
> +
> +static int
> +agentx_read(struct thread *t)
> +{
> + fd_set fds;
> + struct listnode *ln = THREAD_ARG (t);
> + list_delete_node (events, ln);
> +
> + FD_ZERO (&fds);
> + FD_SET (THREAD_FD (t), &fds);
> + snmp_read (&fds);
> +
> + netsnmp_check_outstanding_agent_requests ();
> + agentx_events_update ();
> + return 0;
> +}
> +
> +static void
> +agentx_events_update(void)
> +{
> + int maxfd = 0;
> + int block = 1;
> + struct timeval timeout = { .tv_sec = 0, .tv_usec = 0 };
> + fd_set fds;
> + struct listnode *ln;
> + struct thread *thr;
> + int fd, thr_fd;
> +
> + THREAD_OFF (timeout_thr);
> +
> + FD_ZERO (&fds);
> + snmp_select_info (&maxfd, &fds, &timeout, &block);
> +
> + if (!block)
> + timeout_thr = thread_add_timer_tv (agentx_tm, agentx_timeout, NULL,
> &timeout);
> +
> + ln = listhead (events);
> + thr = ln ? listgetdata (ln) : NULL;
> + thr_fd = thr ? THREAD_FD (thr) : -1;
> +
> + /* "two-pointer" / two-list simultaneous iteration
> + * ln/thr/thr_fd point to the next existing event listener to hit while
> + * fd counts to catch up */
> + for (fd = 0; fd < maxfd; fd++)
> + {
> + /* caught up */
> + if (thr_fd == fd)
> + {
> + struct listnode *nextln = listnextnode (ln);
> + if (!FD_ISSET (fd, &fds))
> + {
> + thread_cancel (thr);
> + list_delete_node (events, ln);
> + }
> + ln = nextln;
> + thr = ln ? listgetdata (ln) : NULL;
> + thr_fd = thr ? THREAD_FD (thr) : -1;
> + }
> + /* need listener, but haven't hit one where it would be */
> + else if (FD_ISSET (fd, &fds))
> + {
> + struct listnode *newln;
> + thr = thread_add_read (agentx_tm, agentx_read, NULL, fd);
> + newln = listnode_add_before (events, ln, thr);
> + thr->arg = newln;
> + }
> + }
> +
> + /* leftover event listeners at this point have fd > maxfd, delete them
> */
> + while (ln)
> + {
> + struct listnode *nextln = listnextnode (ln);
> + thread_cancel (listgetdata (ln));
> + list_delete_node (events, ln);
> + ln = nextln;
> + }
> +}
>
> /* AgentX node. */
> static struct cmd_node agentx_node =
> @@ -77,6 +174,8 @@ DEFUN (agentx_enable,
> if (!agentx_enabled)
> {
> init_snmp("quagga");
> + events = list_new();
> + agentx_events_update ();
> agentx_enabled = 1;
> return CMD_SUCCESS;
> }
> @@ -99,6 +198,8 @@ DEFUN (no_agentx,
> void
> smux_init (struct thread_master *tm)
> {
> + agentx_tm = tm;
> +
> netsnmp_enable_subagent ();
> snmp_disable_log ();
> snmp_enable_calllog ();
> @@ -207,6 +308,7 @@ smux_trap (struct variable *vp, size_t vp_len,
>
> send_v2trap (notification_vars);
> snmp_free_varbind (notification_vars);
> + agentx_events_update ();
> return 1;
> }
>
> diff --git a/lib/thread.c b/lib/thread.c
> index ea741c1..9b9e181 100644
> --- a/lib/thread.c
> +++ b/lib/thread.c
> @@ -35,15 +35,6 @@ DEFINE_MTYPE_STATIC(LIB, THREAD, "Thread")
> DEFINE_MTYPE_STATIC(LIB, THREAD_MASTER, "Thread master")
> DEFINE_MTYPE_STATIC(LIB, THREAD_STATS, "Thread stats")
>
> -#if defined HAVE_SNMP && defined SNMP_AGENTX
> -#include <net-snmp/net-snmp-config.h>
> -#include <net-snmp/net-snmp-includes.h>
> -#include <net-snmp/agent/net-snmp-agent-includes.h>
> -#include <net-snmp/agent/snmp_vars.h>
> -
> -extern int agentx_enabled;
> -#endif
> -
> #if defined(__APPLE__)
> #include <mach/mach.h>
> #include <mach/mach_time.h>
> @@ -1075,12 +1066,7 @@ thread_fetch (struct thread_master *m, struct
> thread *fetch)
> while (1)
> {
> int num = 0;
> -#if defined HAVE_SNMP && defined SNMP_AGENTX
> - struct timeval snmp_timer_wait;
> - int snmpblock = 0;
> - int fdsetsize;
> -#endif
> -
> +
> /* Signals pre-empt everything */
> quagga_sigevent_process ();
>
> @@ -1114,27 +1100,7 @@ thread_fetch (struct thread_master *m, struct
> thread *fetch)
> (!timer_wait || (timeval_cmp (*timer_wait, *timer_wait_bg)
> > 0)))
> timer_wait = timer_wait_bg;
> }
> -
> -#if defined HAVE_SNMP && defined SNMP_AGENTX
> - /* When SNMP is enabled, we may have to select() on additional
> - FD. snmp_select_info() will add them to `readfd'. The trick
> - with this function is its last argument. We need to set it to
> - 0 if timer_wait is not NULL and we need to use the provided
> - new timer only if it is still set to 0. */
> - if (agentx_enabled)
> - {
> - fdsetsize = FD_SETSIZE;
> - snmpblock = 1;
> - if (timer_wait)
> - {
> - snmpblock = 0;
> - memcpy(&snmp_timer_wait, timer_wait, sizeof(struct
> timeval));
> - }
> - snmp_select_info(&fdsetsize, &readfd, &snmp_timer_wait,
> &snmpblock);
> - if (snmpblock == 0)
> - timer_wait = &snmp_timer_wait;
> - }
> -#endif
> +
> num = select (FD_SETSIZE, &readfd, &writefd, &exceptfd, timer_wait);
>
> /* Signals should get quick treatment */
> @@ -1146,20 +1112,6 @@ thread_fetch (struct thread_master *m, struct
> thread *fetch)
> return NULL;
> }
>
> -#if defined HAVE_SNMP && defined SNMP_AGENTX
> - if (agentx_enabled)
> - {
> - if (num > 0)
> - snmp_read(&readfd);
> - else if (num == 0)
> - {
> - snmp_timeout();
> - run_alarms();
> - }
> - netsnmp_check_outstanding_agent_requests();
> - }
> -#endif
> -
> /* Check foreground timers. Historically, they have had higher
> priority than I/O threads, so let's push them onto the ready
> list in front of the I/O threads. */
> --
> 2.7.3
>
>
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev@lists.quagga.net
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>
_______________________________________________
Quagga-dev mailing list
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev
