Re: iked regression test with obj dir
I will continue to argue that testing out-of-the-tree is wrong, and
instead the regression tests should test the installed binaries.
Less than 10 regression tests are going this way. I think it is
a problematic fad.
Moritz Buhl wrote:
> Hi,
> I noticed that the iked regress test fails if I have an obj directory.
> The following patch adresses this.
>
> mbuhl
>
> Index: regress/sbin/iked/parser/Makefile
> ===
> RCS file: /mount/openbsd/cvs//src/regress/sbin/iked/parser/Makefile,v
> retrieving revision 1.2
> diff -u -p -r1.2 Makefile
> --- regress/sbin/iked/parser/Makefile 30 May 2017 15:36:13 - 1.2
> +++ regress/sbin/iked/parser/Makefile 21 Jun 2019 02:06:02 -
> @@ -17,7 +17,7 @@ test_parser: ${IKEOBJS}
>
> ${IKEOBJS}:
> cd ${.CURDIR}/../../../../sbin/iked && make $@
> - ln -sf ${.OBJDIR}/../../../../sbin/iked/$@ .
> + ln -sf ${.CURDIR}/../../../../sbin/iked/$@ .
>
> LDADD+=-L${.OBJDIR} -ltest_helper
> DPADD+=libtest_helper.a
>
iked regression test with obj dir
Hi,
I noticed that the iked regress test fails if I have an obj directory.
The following patch adresses this.
mbuhl
Index: regress/sbin/iked/parser/Makefile
===
RCS file: /mount/openbsd/cvs//src/regress/sbin/iked/parser/Makefile,v
retrieving revision 1.2
diff -u -p -r1.2 Makefile
--- regress/sbin/iked/parser/Makefile 30 May 2017 15:36:13 - 1.2
+++ regress/sbin/iked/parser/Makefile 21 Jun 2019 02:06:02 -
@@ -17,7 +17,7 @@ test_parser: ${IKEOBJS}
${IKEOBJS}:
cd ${.CURDIR}/../../../../sbin/iked && make $@
- ln -sf ${.OBJDIR}/../../../../sbin/iked/$@ .
+ ln -sf ${.CURDIR}/../../../../sbin/iked/$@ .
LDADD+=-L${.OBJDIR} -ltest_helper
DPADD+=libtest_helper.a
Re: ldomctl: improve usage
On Sat, Jun 22, 2019 at 11:16:59AM -0600, Theo de Raadt wrote: > I can give a few crazy examples: ld, cc, ksh. I'll say again, there > surely are cases where it is pointless making usage be complete, because > the compleness can be harmful. Is this one? Maybe... Fair point, although these tools are used differently; compressing all possible usages into one synopsis does lack information about which options are mutually exclusive, but you can still available one which is often what I'm looking for: ls [-1AaCcdFfgHhikLlmnopqRrSsTtux] [file ...] Same goes for ldomctl (and vmctl), except doing something like ldomctl start|stop|init-system|... [domain|file|...] is hardly possible or even useful.
Re: ldomctl: improve usage
On Sat, Jun 22, 2019 at 11:05:00AM -0600, Theo de Raadt wrote: > I'm not happy with usage messages which fill half a screen. There has > to be some threshold where we include very little information, and force > people to the manual page instead. The many *ctl programs stradle this > threshold and while I understand the desire for a better usage message, > I think this is heading too far in the wrong direction. vmctl and ldomctl seem to be the only two control programs with such extensive usage; I appreciate this it does indeed helps me faster and more effectively than reading the manual. gpioctl is on the same path with five lines, the rest of *ctl I glanced over either shows one (long) synopsis a la pfctl or uses an entirely different approach as seen with bgpctl. Neither of those make sense here I think. While I agree with you that a certain threshold of complexity requires reading the manual, but I do not think this go with limiting terse information in the usage.
Re: ldomctl: improve usage
Theo de Raadt wrote: > I can give a few crazy examples: ld, cc, ksh. I'll say again, there > surely are cases where it is pointless making usage be complete, because > the compleness can be harmful. Is this one? Maybe... I've been burned a few times by bgpctl having such a long usage. # bgpctl missing argument: valid commands/args: reload show fib neighbor network irrfilter log 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.
Re: ldomctl: improve usage
Klemens Nanni wrote: > On Sat, Jun 22, 2019 at 07:00:53PM +0200, Mark Kettenis wrote: > > Not sure such a long list is actually useful, but sure. > I'd argue it is complete and consistent. Both `man -h ldomctl' and > `ldomctl [-h]' would lack information, so there was no way of getting > a quick overview without reading the entire manual. Sure there is a quick overview! Obviously, the existing usage hints that things are complicated, and a manu page consultation is required. Many many commands have incomplete usage, thereby leading you to the manual page. On the other hand, spamming 1/4 of a screen during incorrect usage risks damaging the user's understanding of context. I can give a few crazy examples: ld, cc, ksh. I'll say again, there surely are cases where it is pointless making usage be complete, because the compleness can be harmful. Is this one? Maybe...
Re: ldomctl: improve usage
On Sat, Jun 22, 2019 at 07:00:53PM +0200, Mark Kettenis wrote: > Not sure such a long list is actually useful, but sure. I'd argue it is complete and consistent. Both `man -h ldomctl' and `ldomctl [-h]' would lack information, so there was no way of getting a quick overview without reading the entire manual.
Re: ldomctl: improve usage
Klemens Nanni wrote: > ldomctl(8) describes much more commands than the poor usage: > > $ ldomctl > usage: ldomctl start|stop|panic domain > ldomctl status [domain] > > Doing as vmctl already does, this diff turns it into > > usage: ldomctl command [argument ...] > ldomctl delete configuration > ldomctl download directory > ldomctl dump > ldomctl init-system file > ldomctl list > ldomctl panic domain > ldomctl select configuration > ldomctl start domain > ldomctl status [domain] > ldomctl stop domain > > The order of this output is lexicographically sorted exactly as in the > manual page. I'm not happy with usage messages which fill half a screen. There has to be some threshold where we include very little information, and force people to the manual page instead. The many *ctl programs stradle this threshold and while I understand the desire for a better usage message, I think this is heading too far in the wrong direction.
Re: ldomctl: improve usage
> Date: Sat, 22 Jun 2019 18:35:27 +0200
> From: Klemens Nanni
>
> ldomctl(8) describes much more commands than the poor usage:
>
> $ ldomctl
> usage: ldomctl start|stop|panic domain
> ldomctl status [domain]
>
> Doing as vmctl already does, this diff turns it into
>
> usage: ldomctl command [argument ...]
> ldomctl delete configuration
> ldomctl download directory
> ldomctl dump
> ldomctl init-system file
> ldomctl list
> ldomctl panic domain
> ldomctl select configuration
> ldomctl start domain
> ldomctl status [domain]
> ldomctl stop domain
>
> The order of this output is lexicographically sorted exactly as in the
> manual page.
>
> OK?
Not sure such a long list is actually useful, but sure.
> Index: usr.sbin/ldomctl/ldomctl.c
> ===
> RCS file: /cvs/src/usr.sbin/ldomctl/ldomctl.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 ldomctl.c
> --- usr.sbin/ldomctl/ldomctl.c15 Sep 2018 13:20:16 - 1.21
> +++ usr.sbin/ldomctl/ldomctl.c22 Jun 2019 16:10:40 -
> @@ -37,6 +37,7 @@ extern struct ds_service pri_service;
> struct command {
> const char *cmd_name;
> void (*cmd_func)(int, char **);
> + const char *usage;
> };
>
> __dead void usage(void);
> @@ -59,17 +60,17 @@ void guest_status(int argc, char **argv)
> void init_system(int argc, char **argv);
>
> struct command commands[] = {
> - { "download", download },
> - { "dump", dump },
> - { "list", list },
> - { "select", xselect },
> - { "delete", delete },
> - { "start", guest_start },
> - { "stop", guest_stop },
> - { "panic", guest_panic },
> - { "status", guest_status },
> - { "init-system", init_system },
> - { NULL, NULL }
> + { "delete", delete, "configuration" },
> + { "download", download, "directory" },
> + { "dump", dump, "" },
> + { "init-system", init_system, "file" },
> + { "list", list, "" },
> + { "panic", guest_panic,"domain" },
> + { "select", xselect,"configuration" },
> + { "start", guest_start,"domain" },
> + { "status", guest_status, "[domain]" },
> + { "stop", guest_stop, "domain" },
> + { NULL }
> };
>
> void hv_open(void);
> @@ -158,9 +159,13 @@ void
> usage(void)
> {
> extern char *__progname;
> + int i;
>
> - fprintf(stderr, "usage: %s start|stop|panic domain\n", __progname);
> - fprintf(stderr, " %s status [domain]\n", __progname);
> + fprintf(stderr, "usage:\t%s command [argument ...]\n", __progname);
> + for (i = 0; commands[i].cmd_name != NULL; i++) {
> + fprintf(stderr, "\t%s %s %s\n", __progname,
> + commands[i].cmd_name, commands[i].usage);
> + }
> exit(EXIT_FAILURE);
> }
>
>
>
ldomctl: improve usage
ldomctl(8) describes much more commands than the poor usage:
$ ldomctl
usage: ldomctl start|stop|panic domain
ldomctl status [domain]
Doing as vmctl already does, this diff turns it into
usage: ldomctl command [argument ...]
ldomctl delete configuration
ldomctl download directory
ldomctl dump
ldomctl init-system file
ldomctl list
ldomctl panic domain
ldomctl select configuration
ldomctl start domain
ldomctl status [domain]
ldomctl stop domain
The order of this output is lexicographically sorted exactly as in the
manual page.
OK?
Index: usr.sbin/ldomctl/ldomctl.c
===
RCS file: /cvs/src/usr.sbin/ldomctl/ldomctl.c,v
retrieving revision 1.21
diff -u -p -r1.21 ldomctl.c
--- usr.sbin/ldomctl/ldomctl.c 15 Sep 2018 13:20:16 - 1.21
+++ usr.sbin/ldomctl/ldomctl.c 22 Jun 2019 16:10:40 -
@@ -37,6 +37,7 @@ extern struct ds_service pri_service;
struct command {
const char *cmd_name;
void (*cmd_func)(int, char **);
+ const char *usage;
};
__dead void usage(void);
@@ -59,17 +60,17 @@ void guest_status(int argc, char **argv)
void init_system(int argc, char **argv);
struct command commands[] = {
- { "download", download },
- { "dump", dump },
- { "list", list },
- { "select", xselect },
- { "delete", delete },
- { "start", guest_start },
- { "stop", guest_stop },
- { "panic", guest_panic },
- { "status", guest_status },
- { "init-system", init_system },
- { NULL, NULL }
+ { "delete", delete, "configuration" },
+ { "download", download, "directory" },
+ { "dump", dump, "" },
+ { "init-system", init_system, "file" },
+ { "list", list, "" },
+ { "panic", guest_panic,"domain" },
+ { "select", xselect,"configuration" },
+ { "start", guest_start,"domain" },
+ { "status", guest_status, "[domain]" },
+ { "stop", guest_stop, "domain" },
+ { NULL }
};
void hv_open(void);
@@ -158,9 +159,13 @@ void
usage(void)
{
extern char *__progname;
+ int i;
- fprintf(stderr, "usage: %s start|stop|panic domain\n", __progname);
- fprintf(stderr, " %s status [domain]\n", __progname);
+ fprintf(stderr, "usage:\t%s command [argument ...]\n", __progname);
+ for (i = 0; commands[i].cmd_name != NULL; i++) {
+ fprintf(stderr, "\t%s %s %s\n", __progname,
+ commands[i].cmd_name, commands[i].usage);
+ }
exit(EXIT_FAILURE);
}
sppp: more obvious re-challenge timeout computation
Reading the code to understand it's usage of timers, I think we can do better here. Instead of masking the difference between lower and upper bound to yield a random summand that fits, instruct the API to limit their result accordingly. 0x01fe = 510 = 810 - 300. arc4random_uniform(upper_bound) returns `upper_bound - 1' as maximum, so add one to make 810 a possible value for `i'. Feedback? OK? Index: net/if_spppsubr.c === RCS file: /cvs/src/sys/net/if_spppsubr.c,v retrieving revision 1.175 diff -u -p -r1.175 if_spppsubr.c --- net/if_spppsubr.c 21 Jun 2019 17:11:42 - 1.175 +++ net/if_spppsubr.c 22 Jun 2019 14:53:44 - @@ -3580,7 +3580,7 @@ sppp_chap_tlu(struct sppp *sp) * Compute the re-challenge timeout. This will yield * a number between 300 and 810 seconds. */ - i = 300 + (arc4random() & 0x01fe); + i = 300 + arc4random_uniform(1 + 810 - 300); timeout_add_sec(&sp->ch[IDX_CHAP], i); }
Re: sppp: remove duplicate initialisation
On Sat, Jun 22, 2019 at 11:29:42AM +0200, Klemens Nanni wrote: > OK? OK semarie@ > Index: net/if_spppsubr.c > === > RCS file: /cvs/src/sys/net/if_spppsubr.c,v > retrieving revision 1.174 > diff -u -p -r1.174 if_spppsubr.c > --- net/if_spppsubr.c 19 Feb 2018 08:59:52 - 1.174 > +++ net/if_spppsubr.c 22 Jun 2019 09:28:17 - > @@ -3566,7 +3562,6 @@ sppp_chap_tlu(struct sppp *sp) > STDDCL; > int i = 0, x; > > - i = 0; > sp->rst_counter[IDX_CHAP] = sp->lcp.max_configure; > > /* > -- Sebastien Marie
sppp: remove duplicate initialisation
OK? Index: net/if_spppsubr.c === RCS file: /cvs/src/sys/net/if_spppsubr.c,v retrieving revision 1.174 diff -u -p -r1.174 if_spppsubr.c --- net/if_spppsubr.c 19 Feb 2018 08:59:52 - 1.174 +++ net/if_spppsubr.c 22 Jun 2019 09:28:17 - @@ -3566,7 +3562,6 @@ sppp_chap_tlu(struct sppp *sp) STDDCL; int i = 0, x; - i = 0; sp->rst_counter[IDX_CHAP] = sp->lcp.max_configure; /*
bgpd fix mrt table dumps
Once again I broke mrt table dumps a bit. This time by not dumping the
community data anymore. Add this back by adding the needed code in
rde_community.c and some other minor adjustments.
With this the just commited regress test passes again :)
--
:wq Claudio
Index: mrt.c
===
RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v
retrieving revision 1.95
diff -u -p -r1.95 mrt.c
--- mrt.c 22 Jun 2019 05:44:05 - 1.95
+++ mrt.c 22 Jun 2019 06:34:50 -
@@ -34,7 +34,8 @@
#include "mrt.h"
#include "log.h"
-int mrt_attr_dump(struct ibuf *, struct rde_aspath *, struct bgpd_addr *, int);
+int mrt_attr_dump(struct ibuf *, struct rde_aspath *, struct rde_community *,
+struct bgpd_addr *, int);
int mrt_dump_entry_mp(struct mrt *, struct prefix *, u_int16_t,
struct rde_peer*);
int mrt_dump_entry(struct mrt *, struct prefix *, u_int16_t, struct rde_peer*);
@@ -143,8 +144,8 @@ fail:
}
int
-mrt_attr_dump(struct ibuf *buf, struct rde_aspath *a, struct bgpd_addr
*nexthop,
-int v2)
+mrt_attr_dump(struct ibuf *buf, struct rde_aspath *a, struct rde_community *c,
+struct bgpd_addr *nexthop, int v2)
{
struct attr *oa;
u_char *pdata;
@@ -188,6 +189,10 @@ mrt_attr_dump(struct ibuf *buf, struct r
if (attr_writebuf(buf, ATTR_WELL_KNOWN, ATTR_LOCALPREF, &tmp, 4) == -1)
return (-1);
+ /* communities */
+ if (community_writebuf(buf, c) == -1)
+ return (-1);
+
/* dump all other path attributes without modification */
for (l = 0; l < a->others_len; l++) {
if ((oa = a->others[l]) == NULL)
@@ -272,7 +277,8 @@ mrt_dump_entry_mp(struct mrt *mrt, struc
return (-1);
}
- if (mrt_attr_dump(buf, prefix_aspath(p), NULL, 0) == -1) {
+ if (mrt_attr_dump(buf, prefix_aspath(p), prefix_communities(p),
+ NULL, 0) == -1) {
log_warnx("mrt_dump_entry_mp: mrt_attr_dump error");
goto fail;
}
@@ -401,7 +407,8 @@ mrt_dump_entry(struct mrt *mrt, struct p
nh = &addr;
} else
nh = &nexthop->exit_nexthop;
- if (mrt_attr_dump(buf, prefix_aspath(p), nh, 0) == -1) {
+ if (mrt_attr_dump(buf, prefix_aspath(p), prefix_communities(p),
+ nh, 0) == -1) {
log_warnx("mrt_dump_entry: mrt_attr_dump error");
ibuf_free(buf);
return (-1);
@@ -529,7 +536,8 @@ mrt_dump_entry_v2(struct mrt *mrt, struc
log_warn("%s: ibuf_dynamic", __func__);
return (-1);
}
- if (mrt_attr_dump(tbuf, prefix_aspath(p), nh, 1) == -1) {
+ if (mrt_attr_dump(tbuf, prefix_aspath(p), prefix_communities(p),
+ nh, 1) == -1) {
log_warnx("%s: mrt_attr_dump error", __func__);
ibuf_free(buf);
return (-1);
@@ -641,7 +649,7 @@ mrt_dump_peer(struct ibuf *buf, struct r
goto fail;
}
break;
- case AID_UNSPEC: /* XXX special handling for peer_self? */
+ case AID_UNSPEC: /* XXX special handling for peerself? */
DUMP_NLONG(buf, 0);
break;
default:
Index: rde.h
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.217
diff -u -p -r1.217 rde.h
--- rde.h 22 Jun 2019 05:44:05 - 1.217
+++ rde.h 22 Jun 2019 06:34:50 -
@@ -395,20 +395,21 @@ u_char*aspath_override(struct aspath *
u_int16_t *);
int aspath_lenmatch(struct aspath *, enum aslen_spec, u_int);
-int community_match(struct rde_community *, struct community *,
+intcommunity_match(struct rde_community *, struct community *,
struct rde_peer *);
-int community_set(struct rde_community *, struct community *,
+intcommunity_set(struct rde_community *, struct community *,
struct rde_peer *);
-voidcommunity_delete(struct rde_community *, struct community *,
+void community_delete(struct rde_community *, struct community *,
struct rde_peer *);
-int community_add(struct rde_community *, int, void *, size_t);
-int community_large_add(struct rde_community *, int, void *, size_t);
-int community_ext_add(struct rde_community *, int, void *, size_t);
+intcommunity_add(struct rde_community *, int, void *, size_t);
+intcommunity_large_add(struct rde_community *, int, void *, size_t);
+intcommunity_ext_add(struct rde_community *, int, void *, size_t);
-int community_write(struct rde_community *, void *, u_int16_t);
-int community_large_write(struct rde_community *, void *, u_int16_t);
-int community_ext_write(struct rde_community *, int, void *, u_int16_t);
Re: [patch] relayd OCSP stapling for TLS server
On 22.06., Theo Buehler wrote: > On Fri, Jun 21, 2019 at 01:28:03PM +0200, Reyk Floeter wrote: > > On Thu, Jun 20, 2019 at 07:58:10PM +0200, Bruno Flueckiger wrote: > > > Hi, > > > > > > The patch below adds OCSP stapling to the TLS server in relayd(8). The > > > OCSP response is read from a binary encoded DER file that can be created > > > using ocspcheck(8). > > > > > > If a file with the same name as the certificate and private key files is > > > found, its content is loaded and OCSP stapling is active. If there is no > > > file or loading its content fails, OCSP stapling remains disabled. > > > > > > relayd(8) uses the same mechanism it uses to find the certificate file, > > > only the file name extension is different: .der instead of .pem > > > > > > > I had this diff finished more than a month ago, but it had to wait for > > the SNI diff to go in. It is suprisingly similar to your version > > except some minor difference in relay_tls_ctx_create(), the man page, > > and the fact that I've decided for using ".ocsp" instead of ".der" for > > the ending (as .der could be anything). > > > > OK? > > Reads fine. Would be nice to hear that this works for Bruno, but it is > > ok tb > I like ".ocsp" better than ".der". And I'm a bit proud that my diff turns out to be good, although late :-). It works for me. Bruno
