Re: fix for spinning bgpd session engine
On Sun, Jun 30, 2019 at 12:05:15PM +0200, Sebastian Benoit wrote: > Claudio Jeker(cje...@diehard.n-r-g.com) on 2019.06.28 09:44:35 +0200: > > It is possible that a session is going down while peer->rpending (the flag > > indicating that there is more data to process) is set. If that is > > happening the session engine is spinning until the session comes back up > > or the neighbor is removed. > > > > Fix is obvious, clear the flag when the read buffer rbuf is set to NULL. > > Additionally ensure that the timeout is only set to 0 if there is > > something in the read buffer to process. > > eh, yes. > > (did you hit that?) I did not but somebody else probably hit it. At least the 6.5 SE started spinning with a 0 timeout (according to ktrace) and after inspecting the code I found this problem. > ok benno@ > > > -- > > :wq Claudio > > > > Index: session.c > > === > > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v > > retrieving revision 1.386 > > diff -u -p -r1.386 session.c > > --- session.c 22 Jun 2019 05:36:40 - 1.386 > > +++ session.c 28 Jun 2019 07:43:11 - > > @@ -422,7 +422,7 @@ session_main(int debug, int verbose) > > if (p->wbuf.queued > 0 || p->state == STATE_CONNECT) > > events |= POLLOUT; > > /* is there still work to do? */ > > - if (p->rpending) > > + if (p->rpending && p->rbuf && p->rbuf->wpos) > > timeout = 0; > > > > /* poll events */ > > @@ -888,6 +888,7 @@ change_state(struct peer *peer, enum ses > > msgbuf_clear(&peer->wbuf); > > free(peer->rbuf); > > peer->rbuf = NULL; > > + peer->rpending = 0; > > bzero(&peer->capa.peer, sizeof(peer->capa.peer)); > > if (!peer->template) > > imsg_compose(ibuf_main, IMSG_PFKEY_RELOAD, > > @@ -2959,6 +2960,7 @@ getpeerbyip(struct bgpd_config *c, struc > > newpeer->state = newpeer->prev_state = STATE_NONE; > > newpeer->reconf_action = RECONF_KEEP; > > newpeer->rbuf = NULL; > > + newpeer->rpending = 0; > > init_peer(newpeer); > > bgp_fsm(newpeer, EVNT_START); > > if (RB_INSERT(peer_head, &c->peers, newpeer) != NULL) > > > -- :wq Claudio
pf: keep src track for route-to while its state exists
Hi,
The source address tracking (sticky-address) is kept dulring there are
states which refer it. This is mentioned in pf.conf(5). This is true
for translation(nat-to, rdr-to) but it was not true for
routing(route-to).
ok?
Link the state and the source track to keep the source track while
there are states which refer it.
Index: sys/net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1081
diff -u -p -r1.1081 pf.c
--- sys/net/pf.c20 Mar 2019 20:07:28 - 1.1081
+++ sys/net/pf.c1 Jul 2019 05:15:37 -
@@ -222,7 +222,7 @@ int pf_test_state_icmp(struct pf_pdes
u_int16_t pf_calc_mss(struct pf_addr *, sa_family_t, int,
u_int16_t);
static __inline int pf_set_rt_ifp(struct pf_state *, struct pf_addr *,
- sa_family_t);
+ sa_family_t, struct pf_src_node **);
struct pf_divert *pf_get_divert(struct mbuf *);
int pf_walk_header(struct pf_pdesc *, struct ip *,
u_short *);
@@ -3410,17 +3410,16 @@ pf_calc_mss(struct pf_addr *addr, sa_fam
}
static __inline int
-pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr, sa_family_t af)
+pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr, sa_family_t af,
+struct pf_src_node **sns)
{
struct pf_rule *r = s->rule.ptr;
- struct pf_src_node *sns[PF_SN_MAX];
int rv;
s->rt_kif = NULL;
if (!r->rt)
return (0);
- memset(sns, 0, sizeof(sns));
switch (af) {
case AF_INET:
rv = pf_map_addr(AF_INET, r, saddr, &s->rt_addr, NULL, sns,
@@ -4089,6 +4088,11 @@ pf_create_state(struct pf_pdesc *pd, str
goto csfailed;
}
+ if (pf_set_rt_ifp(s, pd->src, (*skw)->af, sns) != 0) {
+ REASON_SET(&reason, PFRES_NOROUTE);
+ goto csfailed;
+ }
+
for (i = 0; i < PF_SN_MAX; i++)
if (sns[i] != NULL) {
struct pf_sn_item *sni;
@@ -4102,11 +4106,6 @@ pf_create_state(struct pf_pdesc *pd, str
SLIST_INSERT_HEAD(&s->src_nodes, sni, next);
sni->sn->states++;
}
-
- if (pf_set_rt_ifp(s, pd->src, (*skw)->af) != 0) {
- REASON_SET(&reason, PFRES_NOROUTE);
- goto csfailed;
- }
if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
pf_detach_state(s);
move to interface rx queue backpressure for if_rxr livelock detection
interface rx queue processing includes detection of when the stack
becomes too busy to process packets.
there's three stages to this mechanism. firstly, everything is fine
and the packets are simply queued for processing. the second is the
"pressure_return" stage where the interface has queued a few times,
but the stack hasn't run to process them. ifiq_input returns 1 in
this situation to notify the nic that it should start to slow down.
the last stage is the "pressure_drop" stage where the nic has
continued to queue packets and the stack still hasnt run. in this
stage it drops the packets and returns 1.
independently, the stack looks for lost clock ticks (because the stack
traditionally blocked softclock ticks) as a livelock detection
mechanism. this no longer works that well now we're in an MP worls.
firstly, the stack could be running on a different cpu to the clock and
therefore wont block it. secondly, the stack runs in a thread and doesnt
raise the spl, so it shouldnt be blocking clock interrupts even if it is
sharing a cpu now.
therefore the traditional livelock detection mechanism doesnt work and
should be moved away from. the replacement is getting nics that
implement rx ring moderation to look at the return value of the rx queue
input function and telling the rings to slow down. that is what this
diff does.
i've compiled it on amd64, which covers most of the drivers, but there's
a few in fdt that i did blind and havent tested. ive tested a couple of
the interfaces, but more testing would be appreciated.
ok?
Index: ./dev/fdt/if_dwxe.c
===
RCS file: /cvs/src/sys/dev/fdt/if_dwxe.c,v
retrieving revision 1.11
diff -u -p -r1.11 if_dwxe.c
--- ./dev/fdt/if_dwxe.c 3 Jan 2019 00:59:58 - 1.11
+++ ./dev/fdt/if_dwxe.c 1 Jul 2019 00:26:07 -
@@ -962,13 +962,14 @@ dwxe_rx_proc(struct dwxe_softc *sc)
sc->sc_rx_cons++;
}
+ if (ifiq_input(&ifp->if_rcv, &ml))
+ if_rxr_livelocked(&sc->sc_rx_ring);
+
dwxe_fill_rx_ring(sc);
bus_dmamap_sync(sc->sc_dmat, DWXE_DMA_MAP(sc->sc_rxring), 0,
DWXE_DMA_LEN(sc->sc_rxring),
BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
-
- if_input(ifp, &ml);
}
void
Index: ./dev/fdt/if_fec.c
===
RCS file: /cvs/src/sys/dev/fdt/if_fec.c,v
retrieving revision 1.8
diff -u -p -r1.8 if_fec.c
--- ./dev/fdt/if_fec.c 6 Feb 2019 22:59:06 - 1.8
+++ ./dev/fdt/if_fec.c 1 Jul 2019 00:26:07 -
@@ -1123,6 +1123,9 @@ fec_rx_proc(struct fec_softc *sc)
sc->sc_rx_cons++;
}
+ if (ifiq_input(&ifp->if_rcv, &ml))
+ if_rxr_livelocked(&sc->sc_rx_ring);
+
fec_fill_rx_ring(sc);
bus_dmamap_sync(sc->sc_dmat, ENET_DMA_MAP(sc->sc_rxring), 0,
@@ -1131,8 +1134,6 @@ fec_rx_proc(struct fec_softc *sc)
/* rx descriptors are ready */
HWRITE4(sc, ENET_RDAR, ENET_RDAR_RDAR);
-
- if_input(ifp, &ml);
}
void
Index: ./dev/fdt/if_mvneta.c
===
RCS file: /cvs/src/sys/dev/fdt/if_mvneta.c,v
retrieving revision 1.7
diff -u -p -r1.7 if_mvneta.c
--- ./dev/fdt/if_mvneta.c 30 Apr 2019 20:26:02 - 1.7
+++ ./dev/fdt/if_mvneta.c 1 Jul 2019 00:26:07 -
@@ -1369,9 +1369,10 @@ mvneta_rx_proc(struct mvneta_softc *sc)
sc->sc_rx_cons = MVNETA_RX_RING_NEXT(idx);
}
- mvneta_fill_rx_ring(sc);
+ if (ifiq_input(&ifp->if_rcv, &ml))
+ if_rxr_livelocked(&sc->sc_rx_ring);
- if_input(ifp, &ml);
+ mvneta_fill_rx_ring(sc);
}
void
Index: ./dev/pci/if_em.c
===
RCS file: /cvs/src/sys/dev/pci/if_em.c,v
retrieving revision 1.342
diff -u -p -r1.342 if_em.c
--- ./dev/pci/if_em.c 1 Mar 2019 10:02:44 - 1.342
+++ ./dev/pci/if_em.c 1 Jul 2019 00:26:07 -
@@ -2922,7 +2922,8 @@ em_rxeof(struct em_softc *sc)
sc->sc_rx_desc_tail = i;
- if_input(ifp, &ml);
+ if (ifiq_input(&ifp->if_rcv, &ml))
+ if_rxr_livelocked(&sc->sc_rx_ring);
return (rv);
}
Index: ./dev/pci/if_bge.c
===
RCS file: /cvs/src/sys/dev/pci/if_bge.c,v
retrieving revision 1.388
diff -u -p -r1.388 if_bge.c
--- ./dev/pci/if_bge.c 9 Nov 2018 14:14:31 - 1.388
+++ ./dev/pci/if_bge.c 1 Jul 2019 00:26:07 -
@@ -3561,6 +3561,13 @@ bge_rxeof(struct bge_softc *sc)
ml_enqueue(&ml, m);
}
+ if (ifiq_input(&ifp->if_rcv, &ml)) {
+ if (stdcnt)
+ if_rxr_livelocked(&sc->bge_std_ring);
+ if (jumbocnt)
+ if_rxr_livelocked(&sc->bge_jumbo_ring);
+ }
+
sc->bge_rx_saved_considx = rx_cons;
bge_writembx(sc,
let sysctl tweak interface rx queue backpressure cutoffs
this lets read and write the backpressure variables in the interface rx
queue (ifiq) handling:
dlg@ix ~$ sysctl net.link.ifrxq
net.link.ifrxq.pressure_return=6
net.link.ifrxq.pressure_drop=8
ideally this would be temporary, ie, id remove it from the tree once
everyone's happy with these numbers. 6 and 8 are the least worst
values ive come up with so far, but it is handy to move them around
for testing purposes.
Index: sys/net/ifq.c
===
RCS file: /cvs/src/sys/net/ifq.c,v
retrieving revision 1.32
diff -u -p -r1.32 ifq.c
--- sys/net/ifq.c 1 Jul 2019 00:44:29 - 1.32
+++ sys/net/ifq.c 1 Jul 2019 00:48:30 -
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
@@ -605,6 +606,43 @@ ifiq_process(void *arg)
mtx_leave(&ifiq->ifiq_mtx);
if_input_process(ifiq->ifiq_if, &ml);
+}
+
+int
+net_ifiq_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp,
+void *newp, size_t newlen)
+{
+ int val;
+ int error;
+
+ if (namelen != 1)
+ return (EISDIR);
+
+ switch (name[0]) {
+ case NET_LINK_IFRXQ_PRESSURE_RETURN:
+ val = ifiq_pressure_return;
+ error = sysctl_int(oldp, oldlenp, newp, newlen, &val);
+ if (error != 0)
+ return (error);
+ if (val < 1 || val > ifiq_pressure_drop)
+ return (EINVAL);
+ ifiq_pressure_return = val;
+ break;
+ case NET_LINK_IFRXQ_PRESSURE_DROP:
+ val = ifiq_pressure_drop;
+ error = sysctl_int(oldp, oldlenp, newp, newlen, &val);
+ if (error != 0)
+ return (error);
+ if (ifiq_pressure_return > val)
+ return (EINVAL);
+ ifiq_pressure_drop = val;
+ break;
+ default:
+ error = EOPNOTSUPP;
+ break;
+ }
+
+ return (error);
}
/*
Index: sbin/sysctl/sysctl.c
===
RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.241
diff -u -p -r1.241 sysctl.c
--- sbin/sysctl/sysctl.c21 Feb 2019 16:37:13 - 1.241
+++ sbin/sysctl/sysctl.c1 Jul 2019 00:48:30 -
@@ -192,6 +192,7 @@ void usage(void);
int findname(char *, char *, char **, struct list *);
int sysctl_inet(char *, char **, int *, int, int *);
int sysctl_inet6(char *, char **, int *, int, int *);
+int sysctl_link(char *, char **, int *, int, int *);
int sysctl_bpf(char *, char **, int *, int, int *);
int sysctl_mpls(char *, char **, int *, int, int *);
int sysctl_pipex(char *, char **, int *, int, int *);
@@ -647,6 +648,12 @@ parse(char *string, int flags)
}
break;
}
+ if (mib[1] == PF_LINK) {
+ len = sysctl_link(string, &bufp, mib, flags, &type);
+ if (len < 0)
+ return;
+ break;
+ }
if (mib[1] == PF_BPF) {
len = sysctl_bpf(string, &bufp, mib, flags, &type);
if (len < 0)
@@ -2230,6 +2237,46 @@ sysctl_inet6(char *string, char **bufpp,
*typep = lp->list[tindx].ctl_type;
return(5);
}
+ return (4);
+}
+
+/* handle net.link requests */
+struct ctlname netlinkname[] = CTL_NET_LINK_NAMES;
+struct ctlname ifrxqname[] = CTL_NET_LINK_IFRXQ_NAMES;
+struct list netlinklist = { netlinkname, NET_LINK_MAXID };
+struct list netlinkvars[] = {
+ [NET_LINK_IFRXQ] = { ifrxqname, NET_LINK_IFRXQ_MAXID },
+};
+
+int
+sysctl_link(char *string, char **bufpp, int mib[], int flags, int *typep)
+{
+ struct list *lp;
+ int indx;
+
+ if (*bufpp == NULL) {
+ listall(string, &netlinklist);
+ return (-1);
+ }
+ if ((indx = findname(string, "third", bufpp, &netlinklist)) == -1)
+ return (-1);
+ mib[2] = indx;
+ if (indx < NET_LINK_MAXID && netlinkvars[indx].list != NULL)
+ lp = &netlinkvars[indx];
+ else if (!flags)
+ return (-1);
+ else {
+ warnx("%s: no variables defined for this protocol", string);
+ return (-1);
+ }
+ if (*bufpp == NULL) {
+ listall(string, lp);
+ return (-1);
+ }
+ if ((indx = findname(string, "fourth", bufpp, lp)) == -1)
+ return (-1);
+ mib[3] = indx;
+ *typep = lp->list[indx].ctl_type;
return (4);
}
Index: sys/sys/sysctl.h
===
RCS file: /cvs/src/sys/sys/sysctl.h,v
retrieving revision 1.188
diff -u -p -r1.188 sysctl.h
--- sys/sys/sysctl.h1 Jun 2019 14:11:18 -
Re: sleep(1): simplify argument parsing
Hi Scott,
Scott Cheloha wrote on Sun, Jun 30, 2019 at 06:49:28AM -0500:
> This is cleaner, shorter.
>
> - Remove the intermediate variables and just build the timespec
>directly.
>
> - Use for-loops to consolidate initialization/incrementation of cp
>into one spot in each loop.
>
> - Use one loop to parse fractions of a second: less duplicate code
>keeps the fraction path shorter. Expand the comment to detail why
>this is fine. We then explain the magic multiplier at the point
>of use instead of making you read the next loop to find out what
>happened in the prior loop.
I like that.
It is easier to understand and survived my code review and testing.
> ok?
OK schwarze@
Ingo
> Index: sleep.c
> ===
> RCS file: /cvs/src/bin/sleep/sleep.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 sleep.c
> --- sleep.c 10 Jan 2019 16:41:10 - 1.27
> +++ sleep.c 30 Jun 2019 11:22:58 -
> @@ -49,9 +49,8 @@ int
> main(int argc, char *argv[])
> {
> int ch;
> - time_t secs = 0, t;
> + time_t t;
> char *cp;
> - long nsecs = 0;
> struct timespec rqtp;
> int i;
>
> @@ -71,40 +70,32 @@ main(int argc, char *argv[])
> if (argc != 1)
> usage();
>
> - cp = *argv;
> - while ((*cp != '\0') && (*cp != '.')) {
> + timespecclear(&rqtp);
> +
> + /* Handle whole seconds. */
> + for (cp = *argv; *cp != '\0' && *cp != '.'; cp++) {
> if (!isdigit((unsigned char)*cp))
> errx(1, "seconds is invalid: %s", *argv);
> - t = (secs * 10) + (*cp++ - '0');
> - if (t / 10 != secs) /* oflow */
> + t = (rqtp.tv_sec * 10) + (*cp - '0');
> + if (t / 10 != rqtp.tv_sec) /* overflow */
> errx(1, "seconds is too large: %s", *argv);
> - secs = t;
> + rqtp.tv_sec = t;
> }
>
> - /* Handle fractions of a second */
> + /*
> + * Handle fractions of a second. The multiplier divides to zero
> + * after nine digits so anything more precise than a nanosecond is
> + * validated but not used.
> + */
> if (*cp == '.') {
> - cp++;
> - for (i = 1; i > 0; i /= 10) {
> - if (*cp == '\0')
> - break;
> + i = 1;
> + for (cp++; *cp != '\0'; cp++) {
> if (!isdigit((unsigned char)*cp))
> errx(1, "seconds is invalid: %s", *argv);
> - nsecs += (*cp++ - '0') * i;
> - }
> -
> - /*
> - * We parse all the way down to nanoseconds
> - * in the above for loop. Be pedantic about
> - * checking the rest of the argument.
> - */
> - while (*cp != '\0') {
> - if (!isdigit((unsigned char)*cp++))
> - errx(1, "seconds is invalid: %s", *argv);
> + rqtp.tv_nsec += (*cp - '0') * i;
> + i /= 10;
> }
> }
> -
> - rqtp.tv_sec = secs;
> - rqtp.tv_nsec = nsecs;
>
> if (timespecisset(&rqtp)) {
> if (nanosleep(&rqtp, NULL) == -1)
smtpd fix proc filter chaining with proceed
Found by Mischa Peters by running some experimental code of mine.
When chaining two proc-based filters together the second proc will not
get its parameter correctly, since lka_filter_process_response won't get
a parameter from the proceed keyword, resulting in a "...|(null)"
The solution seems to be to save the parameter in the filter_session
struct.
While here remove some useless if(NULL) checks and fix two memory leaks
(fs->helo and fs->mail_from).
Mischa has been running with this diff for several days now without
issue.
OK?
martijn@
Index: lka_filter.c
===
RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
retrieving revision 1.36
diff -u -p -r1.36 lka_filter.c
--- lka_filter.c2 May 2019 11:39:45 - 1.36
+++ lka_filter.c30 Jun 2019 14:50:58 -
@@ -41,7 +41,7 @@ struct filter;
struct filter_session;
static voidfilter_protocol_internal(struct filter_session *, uint64_t *,
uint64_t, enum filter_phase, const char *);
static voidfilter_protocol(uint64_t, enum filter_phase, const char *);
-static voidfilter_protocol_next(uint64_t, uint64_t, enum filter_phase,
const char *);
+static voidfilter_protocol_next(uint64_t, uint64_t, enum filter_phase);
static voidfilter_protocol_query(struct filter *, uint64_t, uint64_t,
const char *, const char *);
static voidfilter_data_internal(struct filter_session *, uint64_t,
uint64_t, const char *);
@@ -68,6 +68,8 @@ struct filter_session {
uint64_tid;
struct io *io;
+ char *lastparam;
+
char *filter_name;
struct sockaddr_storage ss_src;
struct sockaddr_storage ss_dest;
@@ -337,6 +339,9 @@ lka_filter_end(uint64_t reqid)
fs = tree_xpop(&sessions, reqid);
free(fs->rdns);
+ free(fs->helo);
+ free(fs->mail_from);
+ free(fs->lastparam);
free(fs);
log_trace(TRACE_FILTERS, "%016"PRIx64" filters session-end", reqid);
}
@@ -504,7 +509,7 @@ lka_filter_process_response(const char *
return 1;
}
- filter_protocol_next(token, reqid, 0, parameter);
+ filter_protocol_next(token, reqid, 0);
return 1;
}
@@ -658,14 +663,11 @@ filter_protocol(uint64_t reqid, enum fil
switch (phase) {
case FILTER_HELO:
case FILTER_EHLO:
- if (fs->helo)
- free(fs->helo);
+ free(fs->helo);
fs->helo = xstrdup(param);
break;
case FILTER_MAIL_FROM:
- if (fs->mail_from)
- free(fs->mail_from);
-
+ free(fs->mail_from);
fs->mail_from = xstrdup(param + 1);
*strchr(fs->mail_from, '>') = '\0';
param = fs->mail_from;
@@ -683,13 +685,17 @@ filter_protocol(uint64_t reqid, enum fil
default:
break;
}
+
+ free(fs->lastparam);
+ fs->lastparam = xstrdup(param);
+
filter_protocol_internal(fs, &token, reqid, phase, param);
if (nparam)
free(nparam);
}
static void
-filter_protocol_next(uint64_t token, uint64_t reqid, enum filter_phase phase,
const char *param)
+filter_protocol_next(uint64_t token, uint64_t reqid, enum filter_phase phase)
{
struct filter_session *fs;
@@ -697,7 +703,7 @@ filter_protocol_next(uint64_t token, uin
if ((fs = tree_get(&sessions, reqid)) == NULL)
return;
- filter_protocol_internal(fs, &token, reqid, phase, param);
+ filter_protocol_internal(fs, &token, reqid, phase, fs->lastparam);
}
static void
Re: Thermal zone support for arm64
On Saturday, 29 June 2019 18:08, Mark Kettenis wrote: > Many of the cheap arm64 (and armv7) boards will overheat if you run > the CPU cores at full throttle for a while. Adding a heatsink may > help a little bit, but not enough. Some boards have a microcontroller > that monitors the temperature and throttles the CPUs if necessary. > Other boards don't and will eventually hit a critical temperature > where it will either do an emergency powerdown or will start to become > unreliable. Hi Mark, Great. With this diff SoC performance and temperature are subjected to the logic that highest prio is stay at <70C and second prio is subject to first prio being satified, operate at full/fullest possible performance, right? > the temperature gets too high. There are device tree bindings for > so-called thermal zones that link together temperature sensors and > cooling devices and define trip points that define the temperatures at > which we have to start cooling. Most boards use passive cooling Are the trip points default-config info stored in the hardware? > + * If the current tenperature is above the trip temperature: > + * If the current temperature is below the trip tenmperature: >+ * - decreate the cooling level if the temperature is falling Small typo should te*ure->temperature & decreate -> decrease. Thanks! Joseph
Re: ldomctl: improve usage
Hi Klemens, Klemens Nanni wrote on Sun, Jun 30, 2019 at 01:04:17PM +0200: > On Sat, Jun 22, 2019 at 11:20:10AM -0600, Theo de Raadt wrote: >> The problem is when I'm on screens that don't have scroll-back, those 9 >> lines have scrolled other information off the top, and then I've had to >> repeat the operations, or if not possible, been more frustrated. > Should we change those programs and refer to manual page or show a > synopsis one-liner instead? Yes, i think programs emitting excessive amounts of text (when called with a typo, and even more so when doing that always on startup, like gdb(1)) should be fixed to shut the hell up. This is Unix. > I get the scroll-back argument, but I dare say it's a weak one given > that this is something users can fix most of the time, e.g. by using > tmux. I find it offensive being told to use tmux(1) merely because some programs like to chew my ears off. I get it that many developers find tmux(1) useful for many purposes, but the availability of add-on tools allowing workarounds is no excuse for programs being annoying in my face. Also, with tmux(1), i might get scrollback. But having the immediate command history remain on screen is still much better than being able to scroll around and having to look for the relevant lines buried among long, irrelevant output. Yes, long usage is mostly irrelvant: a typo doesn't mean i need to re-read a long list of commands provided. > I'd really like to have a valuable usage in ldomctl and haven't > come up with a better and still consistent way of doing so. There is consensus two parts of documentation are useful: * A short usage message shown after typos, typically one or two lines. * The manual page, containing complete and concise reference documentation. Personally, i wouldn't object to exceptionally complicated programs (like openrsync(1) or ldomctl(1)) providing command summaries (typically one line per command) *when the user explicitly asks for it* with an option like -h or --help. But i do see Theo's argument that likely we don't really need three different levels of detail, adding additional maintenance effort and danger of getting outdated. > ssh-keygen is another good example of long usage that is most probably > not going to change back to a simpler one; I just stumbled over it due > to wrong usage and was therefore reminded of this mail thread. > > What do other (ldomctl) users say? This isn't really a question for ldomctl users at this point. Usage should be very short as a matter consistency of the operating system; in that sense, i do consider ssh-keygen(1) an offender. *If* we decide that it would be valuable for selected programs to provide an intermediate -h or --help, only then would it become a question for ldomctl users whether ldomctl(8) is one of them. But so far, i don't see consensus that we want to allow adding such -h or --help options to base programs. I'm mildly in favour if in favour at all, and i have heard strong opposition, with the reasonable argument that it adds maintenance effort for little gain because it's not really that hard to find the information quickly in a real manual page. Yours, Ingo
Re: ldomctl: improve usage
As an ldomctl user, I would be happy for usage to be reasonably terse, provided help gives a fuller description - provided the usage mentions the help option. If your screen does not have scroll back, the solution is a screen program that does. It is not 1978 any more. (Incidentally, do others have a very bad experience with the default terminal when using Sparc[64] consoles? Would the default of TERM=ansi help? Or is there a better solution?) On Sun, 30 Jun 2019 at 12:20, Klemens Nanni wrote: > On Sat, Jun 22, 2019 at 11:20:10AM -0600, Theo de Raadt wrote: > > The problem is when I'm on screens that don't have scroll-back, those 9 > > lines have scrolled other information off the top, and then I've had to > > repeat the operations, or if not possible, been more frustrated. > Should we change those programs and refer to manual page or show a > synopsis one-liner instead? > > I get the scroll-back argument, but I dare say it's a weak one given > that this is something users can fix most of the time, e.g. by using > tmux. > > I'd really like to have a valuable usage in ldomctl and haven't come up > with a better and still consistent way of doing so. > > ssh-keygen is another good example of long usage that is most probably > not going to change back to a simpler one; I just stumbled over it due > to wrong usage and was therefore reminded of this mail thread. > > What do other (ldomctl) users say? > >
sleep(1): simplify argument parsing
This is cleaner, shorter.
- Remove the intermediate variables and just build the timespec
directly.
- Use for-loops to consolidate initialization/incrementation of cp
into one spot in each loop.
- Use one loop to parse fractions of a second: less duplicate code
keeps the fraction path shorter. Expand the comment to detail why
this is fine. We then explain the magic multiplier at the point
of use instead of making you read the next loop to find out what
happened in the prior loop.
ok?
Index: sleep.c
===
RCS file: /cvs/src/bin/sleep/sleep.c,v
retrieving revision 1.27
diff -u -p -r1.27 sleep.c
--- sleep.c 10 Jan 2019 16:41:10 - 1.27
+++ sleep.c 30 Jun 2019 11:22:58 -
@@ -49,9 +49,8 @@ int
main(int argc, char *argv[])
{
int ch;
- time_t secs = 0, t;
+ time_t t;
char *cp;
- long nsecs = 0;
struct timespec rqtp;
int i;
@@ -71,40 +70,32 @@ main(int argc, char *argv[])
if (argc != 1)
usage();
- cp = *argv;
- while ((*cp != '\0') && (*cp != '.')) {
+ timespecclear(&rqtp);
+
+ /* Handle whole seconds. */
+ for (cp = *argv; *cp != '\0' && *cp != '.'; cp++) {
if (!isdigit((unsigned char)*cp))
errx(1, "seconds is invalid: %s", *argv);
- t = (secs * 10) + (*cp++ - '0');
- if (t / 10 != secs) /* oflow */
+ t = (rqtp.tv_sec * 10) + (*cp - '0');
+ if (t / 10 != rqtp.tv_sec) /* overflow */
errx(1, "seconds is too large: %s", *argv);
- secs = t;
+ rqtp.tv_sec = t;
}
- /* Handle fractions of a second */
+ /*
+* Handle fractions of a second. The multiplier divides to zero
+* after nine digits so anything more precise than a nanosecond is
+* validated but not used.
+*/
if (*cp == '.') {
- cp++;
- for (i = 1; i > 0; i /= 10) {
- if (*cp == '\0')
- break;
+ i = 1;
+ for (cp++; *cp != '\0'; cp++) {
if (!isdigit((unsigned char)*cp))
errx(1, "seconds is invalid: %s", *argv);
- nsecs += (*cp++ - '0') * i;
- }
-
- /*
-* We parse all the way down to nanoseconds
-* in the above for loop. Be pedantic about
-* checking the rest of the argument.
-*/
- while (*cp != '\0') {
- if (!isdigit((unsigned char)*cp++))
- errx(1, "seconds is invalid: %s", *argv);
+ rqtp.tv_nsec += (*cp - '0') * i;
+ i /= 10;
}
}
-
- rqtp.tv_sec = secs;
- rqtp.tv_nsec = nsecs;
if (timespecisset(&rqtp)) {
if (nanosleep(&rqtp, NULL) == -1)
Re: ldomctl: improve usage
On Sat, Jun 22, 2019 at 11:20:10AM -0600, Theo de Raadt wrote: > The problem is when I'm on screens that don't have scroll-back, those 9 > lines have scrolled other information off the top, and then I've had to > repeat the operations, or if not possible, been more frustrated. Should we change those programs and refer to manual page or show a synopsis one-liner instead? I get the scroll-back argument, but I dare say it's a weak one given that this is something users can fix most of the time, e.g. by using tmux. I'd really like to have a valuable usage in ldomctl and haven't come up with a better and still consistent way of doing so. ssh-keygen is another good example of long usage that is most probably not going to change back to a simpler one; I just stumbled over it due to wrong usage and was therefore reminded of this mail thread. What do other (ldomctl) users say?
Re: bgpd, unify ref counting code
Claudio Jeker(cje...@diehard.n-r-g.com) on 2019.06.25 11:13:55 +0200:
> In bgpd there are a few objects that use reference counts to keep track on
> how many things point to them. Those are struct pt_entry, rde_aspath,
> rde_communities, and nexthop. The way this reference counting is done and
> especially the cleanup is a bit different everywhere. This diff is making
> them more similar by calling all functions _ref and _unref also the _unref
> function will remove the element once the last reference is dropped.
> There are still some internal differences but all in all for the callers
> this makes the code look a lot more similar.
>
> OK?
ok
> --
> :wq Claudio
>
> Index: rde.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.474
> diff -u -p -r1.474 rde.c
> --- rde.c 25 Jun 2019 09:04:42 - 1.474
> +++ rde.c 25 Jun 2019 09:05:27 -
> @@ -1353,7 +1353,7 @@ rde_update_dispatch(struct imsg *imsg)
> }
>
> /* unlock the previously locked nexthop, it is no longer used */
> - (void)nexthop_put(state.nexthop);
> + nexthop_unref(state.nexthop);
> state.nexthop = NULL;
> if ((pos = rde_get_mp_nexthop(mpp, mplen, aid, &state)) == -1) {
> log_peer_warnx(&peer->conf, "bad nlri nexthop");
> @@ -1653,7 +1653,7 @@ bad_flags:
> op, len);
> return (-1);
> }
> - nexthop_put(state->nexthop);/* just to be sure */
> + nexthop_unref(state->nexthop); /* just to be sure */
> state->nexthop = nexthop_get(&nexthop);
> break;
> case ATTR_MED:
> @@ -2015,7 +2015,7 @@ rde_get_mp_nexthop(u_char *data, u_int16
> return (-1);
> }
>
> - nexthop_put(state->nexthop);/* just to be sure */
> + nexthop_unref(state->nexthop); /* just to be sure */
> state->nexthop = nexthop_get(&nexthop);
>
> /* ignore reserved (old SNPA) field as per RFC4760 */
> Index: rde.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.218
> diff -u -p -r1.218 rde.h
> --- rde.h 24 Jun 2019 06:39:49 - 1.218
> +++ rde.h 25 Jun 2019 09:05:27 -
> @@ -423,7 +423,7 @@ void communities_copy(struct rde_commun
> void communities_clean(struct rde_community *);
>
> static inline struct rde_community *
> -communities_get(struct rde_community *comm)
> +communities_ref(struct rde_community *comm)
> {
> if (comm->refcnt == 0)
> fatalx("%s: not-referenced community", __func__);
> @@ -433,12 +433,12 @@ communities_get(struct rde_community *co
> }
>
> static inline void
> -communities_put(struct rde_community *comm)
> +communities_unref(struct rde_community *comm)
> {
> if (comm == NULL)
> return;
> rdemem.comm_refs--;
> - if (--comm->refcnt == 1)
> + if (--comm->refcnt == 1)/* last ref is hold internally */
> communities_unlink(comm);
> }
>
> @@ -460,11 +460,15 @@ int rde_filter_equal(struct filter_hea
> void rde_filter_calc_skip_steps(struct filter_head *);
>
> /* rde_prefix.c */
> -static inline int
> -pt_empty(struct pt_entry *pt)
> -{
> - return (pt->refcnt == 0);
> -}
> +void pt_init(void);
> +void pt_shutdown(void);
> +void pt_getaddr(struct pt_entry *, struct bgpd_addr *);
> +struct pt_entry *pt_fill(struct bgpd_addr *, int);
> +struct pt_entry *pt_get(struct bgpd_addr *, int);
> +struct pt_entry *pt_add(struct bgpd_addr *, int);
> +void pt_remove(struct pt_entry *);
> +struct pt_entry *pt_lookup(struct bgpd_addr *);
> +int pt_prefix_cmp(const struct pt_entry *, const struct pt_entry *);
>
> static inline struct pt_entry *
> pt_ref(struct pt_entry *pt)
> @@ -475,25 +479,15 @@ pt_ref(struct pt_entry *pt)
> return pt;
> }
>
> -static inline int
> +static inline void
> pt_unref(struct pt_entry *pt)
> {
> if (pt->refcnt == 0)
> fatalx("pt_unref: underflow");
> - --pt->refcnt;
> - return pt_empty(pt);
> + if (--pt->refcnt == 0)
> + pt_remove(pt);
> }
>
> -void pt_init(void);
> -void pt_shutdown(void);
> -void pt_getaddr(struct pt_entry *, struct bgpd_addr *);
> -struct pt_entry *pt_fill(struct bgpd_addr *, int);
> -struct pt_entry *pt_get(struct bgpd_addr *, int);
> -struct pt_entry *pt_add(struct bgpd_addr *, int);
> -void pt_remove(struct pt_entry *);
> -struct pt_entry *pt_lookup(struct bgpd_addr *);
> -int pt_prefix_cmp(const struct pt_entry *, const struct pt_entry *);
> -
> /* rde_rib.c */
> extern u_int16_t rib_size;
> extern struct rib_desc *ribs;
> @@ -608,7 +602,7 @@ void nexthop_unlink(struct prefix *);
> void nexthop_upda
Re: fix for spinning bgpd session engine
Claudio Jeker(cje...@diehard.n-r-g.com) on 2019.06.28 09:44:35 +0200: > It is possible that a session is going down while peer->rpending (the flag > indicating that there is more data to process) is set. If that is > happening the session engine is spinning until the session comes back up > or the neighbor is removed. > > Fix is obvious, clear the flag when the read buffer rbuf is set to NULL. > Additionally ensure that the timeout is only set to 0 if there is > something in the read buffer to process. eh, yes. (did you hit that?) ok benno@ > -- > :wq Claudio > > Index: session.c > === > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v > retrieving revision 1.386 > diff -u -p -r1.386 session.c > --- session.c 22 Jun 2019 05:36:40 - 1.386 > +++ session.c 28 Jun 2019 07:43:11 - > @@ -422,7 +422,7 @@ session_main(int debug, int verbose) > if (p->wbuf.queued > 0 || p->state == STATE_CONNECT) > events |= POLLOUT; > /* is there still work to do? */ > - if (p->rpending) > + if (p->rpending && p->rbuf && p->rbuf->wpos) > timeout = 0; > > /* poll events */ > @@ -888,6 +888,7 @@ change_state(struct peer *peer, enum ses > msgbuf_clear(&peer->wbuf); > free(peer->rbuf); > peer->rbuf = NULL; > + peer->rpending = 0; > bzero(&peer->capa.peer, sizeof(peer->capa.peer)); > if (!peer->template) > imsg_compose(ibuf_main, IMSG_PFKEY_RELOAD, > @@ -2959,6 +2960,7 @@ getpeerbyip(struct bgpd_config *c, struc > newpeer->state = newpeer->prev_state = STATE_NONE; > newpeer->reconf_action = RECONF_KEEP; > newpeer->rbuf = NULL; > + newpeer->rpending = 0; > init_peer(newpeer); > bgp_fsm(newpeer, EVNT_START); > if (RB_INSERT(peer_head, &c->peers, newpeer) != NULL) >
