Re: show advertised MTU in slaacctl
On Mon, Jul 23, 2018 at 08:59:37PM +0200, Björn Ketelaars wrote: > On Mon 23/07/2018 17:38, Florian Obser wrote: > > On Sun, Jul 22, 2018 at 10:32:31AM +0200, Björn Ketelaars wrote: > > > On Sun 22/07/2018 07:27, Björn Ketelaars wrote: > > > > Now that rad(8) is able to advertise a MTU I think it would be nice to > > > > have slaacctl(8) show this advertisement. The patch below touches both > > > > sbin/slaacd and usr.sbin/slaacctl. The addition to sbin/slaacd/engine.c > > > > makes sure that MTU RA messages are parsed, and that the result is made > > > > available. BTW running slaacd in the foreground already enables one to > > > > see the advertised MTU, see debug_log_ra(). > > > > The addition to usr.sbin/slaacctl/slaacctl.c enables one to view the > > > > advertised MTU. > > > > > > New diff as tb@ found a mistake in the first one affecting > > > ND_OPT_REDIRECTED_HEADER, ND_OPT_SOURCE_LINKADDR and > > > ND_OPT_TARGET_LINKADDR as well. > > > > > > > Do you intend to set the mtu on the interface? If not I'm a bit > > reluctand to parse and show it. I know that we are showing the > > nameservers and not do anything with them. When I wrote that code > > there was every intention to do something with the nameservers. For > > various reasons that didn't happen (yet). Maybe we shoud not parse & > > show the nameservers, either (until if and when we do something with > > them). > > > On some hosts I use advertised DNS information to put together > resolv.conf (quick-and-dirty hack using slaacctl and grep). Intention is > to do the same for setting MTU on an interface. Why not set the MTU in slaacd? We used to do it in the kernel since the beginning of time. (Until I ripped it out because it was getting in the way). It should be perfectly simple to do it in slaacd though. resolv.conf on the other hand is a can of worms and I'm not going to suggest that it can be done in an evening :) > > I'm not sure if a problem is solved by not parsing and showing > nameservers from slaacctl. It would be helpful to keep this feature > around. What I was trying to say is, if we show an option in slaacctl the assumption is that we do something with it. As a user I run slaactl, it shows me mtu 1280, I look at the interface and it still running at 1500, I would find that very confusing. Because of historical reasons nameserver information is different. "But we already show nameservers without doing anything with them, why not do the same with the mtu?" is not a valid argument. "I don't have time" or "I don't know how" are valid reasons ;) So if you can't do it (for whatever reason) tell me and I'll implement it (eventually). > I'm guessing that parsing/showing MTU would not be missed by anyone > (except me). On the other hand, it nicely complements rad(8), and could > help with testing. No need to justify the mtu as being useful to you, I'm sold on the general idea. -- I'm not entirely sure you are real.
net/bpf: +23year old gross hack within ifdef _KERNEL.
On Wed, Jul 18, 2018 at 07:57:57AM +0300, Artturi Alm wrote:
> Hi,
>
> rather random "i want to clear those XXXs"-moment with morning coffee,
> did seem like this was screaming for __packed from sys/cdefs.h,
> and less MD in sys/net/, if nothing else.
>
> With a bit of googling i also ran into a different solution,
> which is as ugly as what i'm replacing, imo.., that is:
> #define SIZEOF_BPF_HDR (sizeof(struct bpf_hdr) <= 20 ? 18 :
> sizeof(struct bpf_hdr)) /* from some apple bpf.h */
>
> compile-tested diff from was-current-before-hackathon-src-tree.
>
> -Artturi
>
now i suppose this doesn't work on some arch i can't test with, but
would anyone care to tell me which arch(s) and/or why not?
the diff i was thinking, without comment/define changes below.
-Artturi
diff --git sys/net/bpf.h sys/net/bpf.h
index 604bdcbab55..095ee283c03 100644
--- sys/net/bpf.h
+++ sys/net/bpf.h
@@ -140,7 +140,7 @@ struct bpf_hdr {
u_int32_t bh_datalen; /* original length of packet */
u_int16_t bh_hdrlen; /* length of bpf header (this struct
plus alignment padding) */
-};
+} __packed;
/*
* Because the structure above is not a multiple of 4 bytes, some compilers
* will insist on inserting padding; hence, sizeof(struct bpf_hdr) won't work.
@@ -156,6 +156,7 @@ struct bpf_hdr {
#else
#define SIZEOF_BPF_HDR sizeof(struct bpf_hdr)
#endif
+CTASSERT(sizeof(struct bpf_hdr) == 18); /* prove XXXs above !true w/packed */
#endif
/*
>
> diff --git a/sys/net/bpf.c b/sys/net/bpf.c
> index a6bcf31d471..151a34e19f2 100644
> --- a/sys/net/bpf.c
> +++ b/sys/net/bpf.c
> @@ -1600,11 +1600,12 @@ bpfsattach(caddr_t *bpfp, const char *name, u_int
> dlt, u_int hdrlen)
>
> /*
>* Compute the length of the bpf header. This is not necessarily
> - * equal to SIZEOF_BPF_HDR because we want to insert spacing such
> - * that the network layer header begins on a longword boundary (for
> - * performance reasons and to alleviate alignment restrictions).
> + * equal to sizeof(struct bpf_hdr), because we want to insert spacing
> + * such that the network layer header begins on a longword boundary
> + * (for performance reasons and to alleviate alignment restrictions).
>*/
> - bp->bif_hdrlen = BPF_WORDALIGN(hdrlen + SIZEOF_BPF_HDR) - hdrlen;
> + bp->bif_hdrlen =
> + BPF_WORDALIGN(hdrlen + sizeof(struct bpf_hdr)) - hdrlen;
>
> return (bp);
> }
> diff --git a/sys/net/bpf.h b/sys/net/bpf.h
> index 604bdcbab55..80444bc17ad 100644
> --- a/sys/net/bpf.h
> +++ b/sys/net/bpf.h
> @@ -140,22 +140,14 @@ struct bpf_hdr {
> u_int32_t bh_datalen; /* original length of packet */
> u_int16_t bh_hdrlen; /* length of bpf header (this struct
> plus alignment padding) */
> -};
> +} __packed;
> +#ifdef _KERNEL
> /*
> * Because the structure above is not a multiple of 4 bytes, some compilers
> * will insist on inserting padding; hence, sizeof(struct bpf_hdr) won't
> work.
> * Only the kernel needs to know about it; applications use bh_hdrlen.
> - * XXX To save a few bytes on 32-bit machines, we avoid end-of-struct
> - * XXX padding by using the size of the header data elements. This is
> - * XXX fail-safe: on new machines, we just use the 'safe' sizeof.
> */
> -#ifdef _KERNEL
> -#if defined(__arm__) || defined(__i386__) || defined(__mips__) || \
> -defined(__sparc64__)
> -#define SIZEOF_BPF_HDR 18
> -#else
> -#define SIZEOF_BPF_HDR sizeof(struct bpf_hdr)
> -#endif
> +CTASSERT(sizeof(struct bpf_hdr) == 18);
> #endif
>
> /*
Re: pfctl: use strtonum in host()
I am not sure either is right.
The string may begin with an arbitrary amount of whitespace (as
determined by isspace(3)) followed by a single optional `+' or `-' sign.
What about the '+' sign case?
Klemens Nanni wrote:
> strtonum(3) is simpler than checking three cases for `q' and gives nicer
> error messages. While here, use `v6mask' as maximum netmask instead of
> hardcoding it.
>
> OK?
>
> Index: pfctl_parser.c
> ===
> RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v
> retrieving revision 1.321
> diff -u -p -r1.321 pfctl_parser.c
> --- pfctl_parser.c10 Jul 2018 09:30:49 - 1.321
> +++ pfctl_parser.c21 Jul 2018 18:44:57 -
> @@ -1635,7 +1635,8 @@ host(const char *s, int opts)
> {
> struct node_host*h = NULL, *n;
> int mask = -1, v4mask = 32, v6mask = 128, cont = 1;
> - char*p, *q, *r, *ps, *if_name;
> + char*p, *r, *ps, *if_name;
> + const char *errstr;
>
> if ((ps = strdup(s)) == NULL)
> err(1, "host: strdup");
> @@ -1648,9 +1649,9 @@ host(const char *s, int opts)
> if ((p = strrchr(ps, '/')) != NULL) {
> if ((r = strdup(ps)) == NULL)
> err(1, "host: strdup");
> - mask = strtol(p+1, &q, 0);
> - if (!q || *q || mask > 128 || q == (p+1)) {
> - fprintf(stderr, "invalid netmask '%s'\n", p);
> + mask = strtonum(p+1, 0, v6mask, &errstr);
> + if (errstr) {
> + fprintf(stderr, "netmask is %s: %s\n", errstr, p);
> free(r);
> free(ps);
> return (NULL);
> Index: pfail40.ok
> ===
> RCS file: /cvs/src/regress/sbin/pfctl/pfail40.ok,v
> retrieving revision 1.3
> diff -u -p -r1.3 pfail40.ok
> --- pfail40.ok1 Oct 2004 04:33:27 - 1.3
> +++ pfail40.ok21 Jul 2018 18:45:45 -
> @@ -1,4 +1,4 @@
> -invalid netmask '/161'
> +netmask is too large: /161
> stdin:2: could not parse host specification
> -invalid netmask '/161'
> +netmask is too large: /161
> stdin:3: could not parse host specification
>
Re: cleanup defunct prototype in snmpe.c
On Mon, Jul 23, 2018 at 04:17:07PM -0400, Rob Pierce wrote: > It looks like some code was shuffled around in revision 1.34 in which > snmpe_application was renamed to smi_application and this prototype was > missed. > > ok? OK claudio@ > Index: snmpe.c > === > RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v > retrieving revision 1.52 > diff -u -p -r1.52 snmpe.c > --- snmpe.c 15 Apr 2018 11:57:29 - 1.52 > +++ snmpe.c 23 Jul 2018 19:32:40 - > @@ -45,8 +45,6 @@ int snmpe_parse(struct snmp_message *); > void snmpe_tryparse(int, struct snmp_message *); > int snmpe_parsevarbinds(struct snmp_message *); > void snmpe_response(struct snmp_message *); > -unsigned long > - snmpe_application(struct ber_element *); > void snmpe_sig_handler(int sig, short, void *); > int snmpe_dispatch_parent(int, struct privsep_proc *, struct imsg *); > int snmpe_bind(struct address *); > -- :wq Claudio
[PATCH] faq/current.html fix typo and add commas
I noticed a discrepancy in the instructions to enable rad(8). If you were already using em1 for rtadvd(8) then rad(8) should also use em1 and not em0. Also add two commas for better grammar. Bryan Index: faq/current.html === RCS file: /cvs/www/faq/current.html,v retrieving revision 1.931 diff -u -p -r1.931 current.html --- faq/current.html23 Jul 2018 12:30:57 - 1.931 +++ faq/current.html23 Jul 2018 20:52:01 - @@ -519,13 +519,13 @@ commands: # groupdel _rtadvd # rm /etc/rc.d/rtadvd /usr/sbin/rtadvd /usr/share/man/man5/rtadvd.conf.5 /usr/share/man/man8/rtadvd.8 -If you are running rtadvd(8) for IPv6 router advertisements please switch to +If you are running rtadvd(8) for IPv6 router advertisements, please switch to https://man.openbsd.org/rad.8";>rad(8). First create a /etc/rad.conf configuration file. -For example when you had rtadvd_flags=em1 in +For example, when you had rtadvd_flags=em1 in /etc/rc.conf.local, /etc/rad.conf would be: -interface em0 +interface em1 For more advanced configurations consult https://man.openbsd.org/rad.conf.5";>rad.conf(5).
cleanup defunct prototype in snmpe.c
It looks like some code was shuffled around in revision 1.34 in which snmpe_application was renamed to smi_application and this prototype was missed. ok? Index: snmpe.c === RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v retrieving revision 1.52 diff -u -p -r1.52 snmpe.c --- snmpe.c 15 Apr 2018 11:57:29 - 1.52 +++ snmpe.c 23 Jul 2018 19:32:40 - @@ -45,8 +45,6 @@ intsnmpe_parse(struct snmp_message *); voidsnmpe_tryparse(int, struct snmp_message *); int snmpe_parsevarbinds(struct snmp_message *); voidsnmpe_response(struct snmp_message *); -unsigned long -snmpe_application(struct ber_element *); voidsnmpe_sig_handler(int sig, short, void *); int snmpe_dispatch_parent(int, struct privsep_proc *, struct imsg *); int snmpe_bind(struct address *);
Re: show advertised MTU in slaacctl
On Mon 23/07/2018 17:38, Florian Obser wrote: > On Sun, Jul 22, 2018 at 10:32:31AM +0200, Björn Ketelaars wrote: > > On Sun 22/07/2018 07:27, Björn Ketelaars wrote: > > > Now that rad(8) is able to advertise a MTU I think it would be nice to > > > have slaacctl(8) show this advertisement. The patch below touches both > > > sbin/slaacd and usr.sbin/slaacctl. The addition to sbin/slaacd/engine.c > > > makes sure that MTU RA messages are parsed, and that the result is made > > > available. BTW running slaacd in the foreground already enables one to > > > see the advertised MTU, see debug_log_ra(). > > > The addition to usr.sbin/slaacctl/slaacctl.c enables one to view the > > > advertised MTU. > > > > New diff as tb@ found a mistake in the first one affecting > > ND_OPT_REDIRECTED_HEADER, ND_OPT_SOURCE_LINKADDR and > > ND_OPT_TARGET_LINKADDR as well. > > > > Do you intend to set the mtu on the interface? If not I'm a bit > reluctand to parse and show it. I know that we are showing the > nameservers and not do anything with them. When I wrote that code > there was every intention to do something with the nameservers. For > various reasons that didn't happen (yet). Maybe we shoud not parse & > show the nameservers, either (until if and when we do something with > them). On some hosts I use advertised DNS information to put together resolv.conf (quick-and-dirty hack using slaacctl and grep). Intention is to do the same for setting MTU on an interface. I'm not sure if a problem is solved by not parsing and showing nameservers from slaacctl. It would be helpful to keep this feature around. I'm guessing that parsing/showing MTU would not be missed by anyone (except me). On the other hand, it nicely complements rad(8), and could help with testing.
inteldrm: always use probed screen size for fb
On the 2015 MacBook Pro and the 12" MacBook, the firmware reports a
framebuffer size of 2880x1800 but the screens are 2560x1600 and
2304x1440. Our console ends up drawing text off screen and the
latest few lines can't be read.
Once X loads, it probes the outputs again and uses the proper
resolution.
For the console, the outputs are already being probed but the BIOS
resolution is only discarded if it's too small. I'd like to change
it to discard it if it's too large as well, so it uses the actual
resolution probed.
On a machine like a server where inteldrm loads but has nothing
connected, intel_fb is null, so this code doesn't run.
Index: sys/dev/pci/drm/i915/intel_fbdev.c
===
RCS file: /cvs/src/sys/dev/pci/drm/i915/intel_fbdev.c,v
retrieving revision 1.3
diff -u -p -u -p -r1.3 intel_fbdev.c
--- sys/dev/pci/drm/i915/intel_fbdev.c 1 Jul 2017 16:14:10 - 1.3
+++ sys/dev/pci/drm/i915/intel_fbdev.c 23 Jul 2018 17:58:03 -
@@ -202,10 +202,9 @@ static int intelfb_create(struct drm_fb_
mutex_lock(&dev->struct_mutex);
if (intel_fb &&
- (sizes->fb_width > intel_fb->base.width ||
-sizes->fb_height > intel_fb->base.height)) {
- DRM_DEBUG_KMS("BIOS fb too small (%dx%d), we require (%dx%d),"
- " releasing it\n",
+ (sizes->fb_width != intel_fb->base.width ||
+sizes->fb_height != intel_fb->base.height)) {
+ DRM_DEBUG_KMS("BIOS fb %dx%d != %dx%d, releasing it\n",
intel_fb->base.width, intel_fb->base.height,
sizes->fb_width, sizes->fb_height);
drm_framebuffer_unreference(&intel_fb->base);
Re: show advertised MTU in slaacctl
On Mon, Jul 23, 2018 at 05:53:09PM +0200, Klemens Nanni wrote: > On Mon, Jul 23, 2018 at 05:38:58PM +0200, Florian Obser wrote: > > Do you intend to set the mtu on the interface? If not I'm a bit > > reluctand to parse and show it. I know that we are showing the > > nameservers and not do anything with them. When I wrote that code > > there was every intention to do something with the nameservers. For > > various reasons that didn't happen (yet). Maybe we shoud not parse & > > show the nameservers, either (until if and when we do something with > > them). > Dropping it leaves no tool in base except for tcpdump to obtain DNS > information in IPv6 only networks. > > I appreciate that slaacctl shows `rdns' and `dnssl'; since it's already > in, I don't see a good reason to remove such functionality. > Reducing attack surface. The ND_OPT_RDNSS case worries me a bit but I can't find anything wrong with it. ND_OPT_DNSSL flat out scares me. -- I'm not entirely sure you are real.
Re: show advertised MTU in slaacctl
On Mon, Jul 23, 2018 at 05:38:58PM +0200, Florian Obser wrote: > Do you intend to set the mtu on the interface? If not I'm a bit > reluctand to parse and show it. I know that we are showing the > nameservers and not do anything with them. When I wrote that code > there was every intention to do something with the nameservers. For > various reasons that didn't happen (yet). Maybe we shoud not parse & > show the nameservers, either (until if and when we do something with > them). Dropping it leaves no tool in base except for tcpdump to obtain DNS information in IPv6 only networks. I appreciate that slaacctl shows `rdns' and `dnssl'; since it's already in, I don't see a good reason to remove such functionality.
Re: show advertised MTU in slaacctl
On Sun, Jul 22, 2018 at 10:32:31AM +0200, Björn Ketelaars wrote:
> On Sun 22/07/2018 07:27, Björn Ketelaars wrote:
> > Now that rad(8) is able to advertise a MTU I think it would be nice to
> > have slaacctl(8) show this advertisement. The patch below touches both
> > sbin/slaacd and usr.sbin/slaacctl. The addition to sbin/slaacd/engine.c
> > makes sure that MTU RA messages are parsed, and that the result is made
> > available. BTW running slaacd in the foreground already enables one to
> > see the advertised MTU, see debug_log_ra().
> > The addition to usr.sbin/slaacctl/slaacctl.c enables one to view the
> > advertised MTU.
>
> New diff as tb@ found a mistake in the first one affecting
> ND_OPT_REDIRECTED_HEADER, ND_OPT_SOURCE_LINKADDR and
> ND_OPT_TARGET_LINKADDR as well.
>
Do you intend to set the mtu on the interface? If not I'm a bit
reluctand to parse and show it. I know that we are showing the
nameservers and not do anything with them. When I wrote that code
there was every intention to do something with the nameservers. For
various reasons that didn't happen (yet). Maybe we shoud not parse &
show the nameservers, either (until if and when we do something with
them).
>
> diff --git sbin/slaacd/engine.c sbin/slaacd/engine.c
> index 961e1b115b6..2e114cdaf0b 100644
> --- sbin/slaacd/engine.c
> +++ sbin/slaacd/engine.c
> @@ -156,6 +156,7 @@ struct radv {
> uint16_t router_lifetime; /* in seconds */
> uint32_t reachable_time; /* in milliseconds */
> uint32_t retrans_time; /* in milliseconds */
> + uint32_t mtu;
> LIST_HEAD(, radv_prefix) prefixes;
> uint32_t rdns_lifetime;
> LIST_HEAD(, radv_rdns) rdns_servers;
> @@ -832,6 +833,7 @@ send_interface_info(struct slaacd_iface *iface, pid_t pid)
> cei_ra.router_lifetime = ra->router_lifetime;
> cei_ra.reachable_time = ra->reachable_time;
> cei_ra.retrans_time = ra->retrans_time;
> + cei_ra.mtu = ra->mtu;
> engine_imsg_compose_frontend(IMSG_CTL_SHOW_INTERFACE_INFO_RA,
> pid, &cei_ra, sizeof(cei_ra));
>
> @@ -1125,6 +1127,7 @@ parse_ra(struct slaacd_iface *iface, struct imsg_ra *ra)
> while ((size_t)len >= sizeof(struct nd_opt_hdr)) {
> struct nd_opt_hdr *nd_opt_hdr = (struct nd_opt_hdr *)p;
> struct nd_opt_prefix_info *prf;
> + struct nd_opt_mtu *mtu;
> struct nd_opt_rdnss *rdnss;
> struct nd_opt_dnssl *dnssl;
> struct in6_addr *in6;
> @@ -1227,11 +1230,18 @@ parse_ra(struct slaacd_iface *iface, struct imsg_ra
> *ra)
>
> LIST_INSERT_HEAD(&radv->dnssls, ra_dnssl, entries);
>
> + break;
> + case ND_OPT_MTU:
> + if (nd_opt_hdr->nd_opt_len != 1) {
> + log_warnx("invalid ND_OPT_MTU: len != 1");
> + goto err;
> + }
> + mtu = (struct nd_opt_mtu*) nd_opt_hdr;
> + radv->mtu = ntohl(mtu->nd_opt_mtu_mtu);
> break;
> case ND_OPT_REDIRECTED_HEADER:
> case ND_OPT_SOURCE_LINKADDR:
> case ND_OPT_TARGET_LINKADDR:
> - case ND_OPT_MTU:
> case ND_OPT_ROUTE_INFO:
> #if 0
> log_debug("\tOption: %u (len: %u) not implemented",
> diff --git sbin/slaacd/slaacd.h sbin/slaacd/slaacd.h
> index 910be91e687..33e85909ce9 100644
> --- sbin/slaacd/slaacd.h
> +++ sbin/slaacd/slaacd.h
> @@ -109,6 +109,7 @@ struct ctl_engine_info_ra {
> uint16_t router_lifetime; /* in seconds */
> uint32_t reachable_time;/* in milliseconds */
> uint32_t retrans_time; /* in milliseconds */
> + uint32_t mtu;
> };
>
> struct ctl_engine_info_ra_prefix {
> diff --git usr.sbin/slaacctl/slaacctl.c usr.sbin/slaacctl/slaacctl.c
> index 5b2a22f12e6..97b460aee08 100644
> --- usr.sbin/slaacctl/slaacctl.c
> +++ usr.sbin/slaacctl/slaacctl.c
> @@ -228,6 +228,8 @@ show_interface_msg(struct imsg *imsg)
> printf("\t\tDefault Router Preference: %s\n", cei_ra->rpref);
> printf("\t\tReachable Time: %9ums, Retrans Timer: %9ums\n",
> cei_ra->reachable_time, cei_ra->retrans_time);
> + if (cei_ra->mtu)
> + printf("\t\tMTU: %u bytes\n", cei_ra->mtu);
> break;
> case IMSG_CTL_SHOW_INTERFACE_INFO_RA_PREFIX:
> cei_ra_prefix = imsg->data;
>
--
I'm not entirely sure you are real.
PCI_FLAGS_MSI_ENABLED for arm64
This diff allows drivers to disable MSI for certain hardware variants by clearing the flag just like we support on other hardware platforms. I deliberately didn't include rkpcie(4) in this diff since we don't have MSI support for the interrupt controller on the RK3399 yet. ok? Index: dev/fdt/dwpcie.c === RCS file: /cvs/src/sys/dev/fdt/dwpcie.c,v retrieving revision 1.6 diff -u -p -r1.6 dwpcie.c --- dev/fdt/dwpcie.c1 Jul 2018 18:58:06 - 1.6 +++ dev/fdt/dwpcie.c23 Jul 2018 15:24:16 - @@ -324,6 +324,7 @@ dwpcie_attach(struct device *parent, str pba.pba_ioex = sc->sc_ioex; pba.pba_domain = pci_ndomains++; pba.pba_bus = sc->sc_bus; + pba.pba_flags |= PCI_FLAGS_MSI_ENABLED; config_found(self, &pba, NULL); } @@ -534,7 +535,8 @@ dwpcie_intr_map_msi(struct pci_attach_ar pcitag_t tag = pa->pa_tag; struct dwpcie_intr_handle *ih; - if (pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0) + if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 || + pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0) return -1; ih = malloc(sizeof(struct dwpcie_intr_handle), M_DEVBUF, M_WAITOK); Index: arch/arm64/dev/acpipci.c === RCS file: /cvs/src/sys/arch/arm64/dev/acpipci.c,v retrieving revision 1.2 diff -u -p -r1.2 acpipci.c --- arch/arm64/dev/acpipci.c10 Jul 2018 17:11:42 - 1.2 +++ arch/arm64/dev/acpipci.c23 Jul 2018 15:24:16 - @@ -200,6 +200,7 @@ acpipci_attach(struct device *parent, st pba.pba_memex = sc->sc_memex; pba.pba_domain = pci_ndomains++; pba.pba_bus = sc->sc_bus; + pba.pba_flags |= PCI_FLAGS_MSI_ENABLED; config_found(self, &pba, NULL); } @@ -397,7 +398,8 @@ acpipci_intr_map_msi(struct pci_attach_a pcitag_t tag = pa->pa_tag; struct acpipci_intr_handle *ih; - if (pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0) + if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 || + pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0) return -1; ih = malloc(sizeof(struct acpipci_intr_handle), M_DEVBUF, M_WAITOK); Index: arch/arm64/dev/pciecam.c === RCS file: /cvs/src/sys/arch/arm64/dev/pciecam.c,v retrieving revision 1.3 diff -u -p -r1.3 pciecam.c --- arch/arm64/dev/pciecam.c9 Apr 2018 18:35:13 - 1.3 +++ arch/arm64/dev/pciecam.c23 Jul 2018 15:24:16 - @@ -241,6 +241,7 @@ pciecam_attach(struct device *parent, st pba.pba_pc = &sc->sc_pc; pba.pba_domain = pci_ndomains++; pba.pba_bus = 0; + pba.pba_flags |= PCI_FLAGS_MSI_ENABLED; config_found(self, &pba, NULL); } @@ -334,7 +335,8 @@ pciecam_intr_map_msi(struct pci_attach_a pcitag_t tag = pa->pa_tag; struct pciecam_intr_handle *ih; - if (pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0) + if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 || + pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0) return 1; ih = malloc(sizeof(struct pciecam_intr_handle), M_DEVBUF, M_WAITOK);
Re: pfctl: use strtonum in host()
On Mon, Jul 23, 2018 at 02:43:35PM +0200, Florian Obser wrote: > I for one welcome our new inet6 maintainer! Wie darf ich das verstehen?
pfctl: introduce print_addr_str() helper
Here's a start to avoid duplicate routines that moves the inet_ntop
wiggle from four locations into a small new helper called
`print_addr_str()'.
Feedback? OK?
Index: pf_print_state.c
===
RCS file: /cvs/src/sbin/pfctl/pf_print_state.c,v
retrieving revision 1.64
diff -u -p -r1.64 pf_print_state.c
--- pf_print_state.c21 Jan 2015 21:50:33 - 1.64
+++ pf_print_state.c23 Jul 2018 10:59:53 -
@@ -81,31 +81,17 @@ print_addr(struct pf_addr_wrap *addr, sa
printf("<%s>", addr->v.tblname);
return;
case PF_ADDR_RANGE: {
- char buf[48];
-
- if (inet_ntop(af, &addr->v.a.addr, buf, sizeof(buf)) == NULL)
- printf("?");
- else
- printf("%s", buf);
- if (inet_ntop(af, &addr->v.a.mask, buf, sizeof(buf)) == NULL)
- printf(" - ?");
- else
- printf(" - %s", buf);
+ print_addr_str(af, &addr->v.a.addr);
+ printf(" - ");
+ print_addr_str(af, &addr->v.a.mask);
break;
}
case PF_ADDR_ADDRMASK:
if (PF_AZERO(&addr->v.a.addr, AF_INET6) &&
PF_AZERO(&addr->v.a.mask, AF_INET6))
printf("any");
- else {
- char buf[48];
-
- if (inet_ntop(af, &addr->v.a.addr, buf,
- sizeof(buf)) == NULL)
- printf("?");
- else
- printf("%s", buf);
- }
+ else
+ print_addr_str(af, &addr->v.a.addr);
break;
case PF_ADDR_NOROUTE:
printf("no-route");
@@ -130,6 +116,17 @@ print_addr(struct pf_addr_wrap *addr, sa
if (bits < (af == AF_INET ? 32 : 128))
printf("/%d", bits);
}
+}
+
+void
+print_addr_str(sa_family_t af, struct pf_addr *addr)
+{
+ static char buf[48];
+
+ if (inet_ntop(af, addr, buf, sizeof(buf)) == NULL)
+ printf("?");
+ else
+ printf("%s", buf);
}
void
Index: pfctl.h
===
RCS file: /cvs/src/sbin/pfctl/pfctl.h,v
retrieving revision 1.55
diff -u -p -r1.55 pfctl.h
--- pfctl.h 11 Aug 2017 22:30:38 - 1.55
+++ pfctl.h 22 Jul 2018 11:35:05 -
@@ -92,6 +92,7 @@ struct segment {
};
voidprint_addr(struct pf_addr_wrap *, sa_family_t, int);
+voidprint_addr_str(sa_family_t, struct pf_addr *);
voidprint_host(struct pf_addr *, u_int16_t p, sa_family_t, u_int16_t,
const char *, int);
voidprint_seq(struct pfsync_state_peer *);
voidprint_state(struct pfsync_state *, int);
Index: pfctl_parser.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v
retrieving revision 1.321
diff -u -p -r1.321 pfctl_parser.c
--- pfctl_parser.c 10 Jul 2018 09:30:49 - 1.321
+++ pfctl_parser.c 22 Jul 2018 11:35:06 -
@@ -1092,14 +1092,8 @@ print_rule(struct pf_rule *r, const char
case PF_DIVERT_NONE:
break;
case PF_DIVERT_TO: {
- /* XXX cut&paste from print_addr */
- char buf[48];
-
printf(" divert-to ");
- if (inet_ntop(r->af, &r->divert.addr, buf, sizeof(buf)) == NULL)
- printf("?");
- else
- printf("%s", buf);
+ print_addr_str(r->af, &r->divert.addr);
printf(" port %u", ntohs(r->divert.port));
break;
}
===
Stats: --- 26 lines 559 chars
Stats: +++ 18 lines 417 chars
Stats: -8 lines
Stats: -142 chars
Re: pfctl: use strtonum in host()
On Mon, Jul 23, 2018 at 11:22:56AM +0200, Otto Moerbeek wrote:
> On Mon, Jul 23, 2018 at 11:16:16AM +0200, Klemens Nanni wrote:
>
> > strtonum(3) is simpler than checking three cases for `q' and gives nicer
> > error messages. While here, use `v6mask' as maximum netmask instead of
> > hardcoding it.
>
> Isn't the thing called mask here actually a prefix length?
I for one welcome our new inet6 maintainer!
>
> -Otto
> >
> > OK?
> >
> > Index: pfctl_parser.c
> > ===
> > RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v
> > retrieving revision 1.321
> > diff -u -p -r1.321 pfctl_parser.c
> > --- pfctl_parser.c 10 Jul 2018 09:30:49 - 1.321
> > +++ pfctl_parser.c 21 Jul 2018 18:44:57 -
> > @@ -1635,7 +1635,8 @@ host(const char *s, int opts)
> > {
> > struct node_host*h = NULL, *n;
> > int mask = -1, v4mask = 32, v6mask = 128, cont = 1;
> > - char*p, *q, *r, *ps, *if_name;
> > + char*p, *r, *ps, *if_name;
> > + const char *errstr;
> >
> > if ((ps = strdup(s)) == NULL)
> > err(1, "host: strdup");
> > @@ -1648,9 +1649,9 @@ host(const char *s, int opts)
> > if ((p = strrchr(ps, '/')) != NULL) {
> > if ((r = strdup(ps)) == NULL)
> > err(1, "host: strdup");
> > - mask = strtol(p+1, &q, 0);
> > - if (!q || *q || mask > 128 || q == (p+1)) {
> > - fprintf(stderr, "invalid netmask '%s'\n", p);
> > + mask = strtonum(p+1, 0, v6mask, &errstr);
> > + if (errstr) {
> > + fprintf(stderr, "netmask is %s: %s\n", errstr, p);
> > free(r);
> > free(ps);
> > return (NULL);
> > Index: pfail40.ok
> > ===
> > RCS file: /cvs/src/regress/sbin/pfctl/pfail40.ok,v
> > retrieving revision 1.3
> > diff -u -p -r1.3 pfail40.ok
> > --- pfail40.ok 1 Oct 2004 04:33:27 - 1.3
> > +++ pfail40.ok 21 Jul 2018 18:45:45 -
> > @@ -1,4 +1,4 @@
> > -invalid netmask '/161'
> > +netmask is too large: /161
> > stdin:2: could not parse host specification
> > -invalid netmask '/161'
> > +netmask is too large: /161
> > stdin:3: could not parse host specification
>
--
I'm not entirely sure you are real.
pfctl: use mask parameter in host_v4() to avoid duplicate strchr call
Coming from the only caller `host()', the `mask' parameter is -1 iff
`*s' does not contain "/".
Let's not discard that information and avoid the redundant search; this
makes the `mask' parameter actually being used.
OK?
Index: pfctl_parser.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v
retrieving revision 1.321
diff -u -p -r1.321 pfctl_parser.c
--- pfctl_parser.c 10 Jul 2018 09:30:49 - 1.321
+++ pfctl_parser.c 22 Jul 2018 10:47:47 -
@@ -1750,7 +1750,7 @@ host_v4(const char *s, int mask)
int bits = 32;
memset(&ina, 0, sizeof(struct in_addr));
- if (strrchr(s, '/') != NULL) {
+ if (mask > -1) {
if ((bits = inet_net_pton(AF_INET, s, &ina, sizeof(ina))) == -1)
return (NULL);
} else {
Re: pfctl: simplify getaddrinfo() error handling in host_dns()
On Mon, Jul 23, 2018 at 11:19:50AM +0200, Klemens Nanni wrote: > We're not using `error' anyway so drop the variable and jump to the end. > > OK? OK sashan
Re: pfctl: use strtonum in host()
On Mon, Jul 23, 2018 at 11:22:56AM +0200, Otto Moerbeek wrote: > Isn't the thing called mask here actually a prefix length? Yes, but `mask' is used for both IPv4 and IPv6 here so it's both a netmask and prefixlength until `host_v4()' and `host_v6()' tell us more.
Re: pfctl: use strtonum in host()
Hello,
On Mon, Jul 23, 2018 at 11:16:16AM +0200, Klemens Nanni wrote:
> strtonum(3) is simpler than checking three cases for `q' and gives nicer
> error messages. While here, use `v6mask' as maximum netmask instead of
> hardcoding it.
>
> OK?
I'm OK with your change. I have just one small note, which is
more matter of my personal 'coding taste/style' than anything else.
>
> Index: pfctl_parser.c
> ===
> RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v
> retrieving revision 1.321
> diff -u -p -r1.321 pfctl_parser.c
> --- pfctl_parser.c10 Jul 2018 09:30:49 - 1.321
> +++ pfctl_parser.c21 Jul 2018 18:44:57 -
> @@ -1635,7 +1635,8 @@ host(const char *s, int opts)
> {
> struct node_host*h = NULL, *n;
> int mask = -1, v4mask = 32, v6mask = 128, cont = 1;
> - char*p, *q, *r, *ps, *if_name;
> + char*p, *r, *ps, *if_name;
> + const char *errstr;
>
> if ((ps = strdup(s)) == NULL)
> err(1, "host: strdup");
> @@ -1648,9 +1649,9 @@ host(const char *s, int opts)
> if ((p = strrchr(ps, '/')) != NULL) {
> if ((r = strdup(ps)) == NULL)
> err(1, "host: strdup");
> - mask = strtol(p+1, &q, 0);
> - if (!q || *q || mask > 128 || q == (p+1)) {
> - fprintf(stderr, "invalid netmask '%s'\n", p);
> + mask = strtonum(p+1, 0, v6mask, &errstr);
IMO using a constant (being it 128 or V6MASK or ...) is better
option than using v6mask variable here. But as I've said earlier
it's a nit.
regards
sashan
Re: systat.1 bar graph - add lockspin "@"
On Mon, Jul 23, 2018 at 10:07:03AM +0200, Marcus MERIGHI wrote: > I've noticed that systat(1), in the CPU bar graph, shows "@"s, which > aren't explained in systat(1). I went looking and found: This commit is more relevant as I fixed the output afterwards. src/usr.bin/systat/vmstat.c revision 1.85 date: 2018/05/19 13:24:10; author: bluhm; state: Exp; lines: +6 -4; commitid: 4ynbHJn9ppGCJ82S; Do not ignore nice time in systat(1). The sum would not be 100%, a busy machine would look idle. As %Nic does not fit in the columns, add it to %Usr. Introduce @ for spinning time to keep the characters people are used to. Put %Spn between %Int and %Sys like in top. OK visa@ mpi@ Unfortunately I forgot the man page. top(1) is also incomplete. ok? bluhm Index: usr.bin/systat/systat.1 === RCS file: /data/mirror/openbsd/cvs/src/usr.bin/systat/systat.1,v retrieving revision 1.109 diff -u -p -r1.109 systat.1 --- usr.bin/systat/systat.1 8 Jul 2018 13:23:57 - 1.109 +++ usr.bin/systat/systat.1 23 Jul 2018 09:27:45 - @@ -460,12 +460,12 @@ Below the queue length listing is a nume a bar graph showing the amount of interrupt (shown as .Ql | ) , +spinning (shown as +.Ql @ ) , system (shown as .Ql = ) , -user (shown as +user plus nice (shown as .Ql > ) , -nice (shown as -.Ql - ) , and idle time (shown as .Ql \ \& ) . .Pp Index: usr.bin/top/top.1 === RCS file: /data/mirror/openbsd/cvs/src/usr.bin/top/top.1,v retrieving revision 1.68 diff -u -p -r1.68 top.1 --- usr.bin/top/top.1 8 Sep 2016 16:47:47 - 1.68 +++ usr.bin/top/top.1 23 Jul 2018 09:25:32 - @@ -388,7 +388,7 @@ the number of existing processes, the number of processes in each state (starting, running, idle, stopped, zombie, dead, and on processor), and a percentage of time spent in each of the processor states -(user, nice, system, interrupt, and idle). +(user, nice, system, spinning, interrupt, and idle). It also includes information about physical and virtual memory allocation. The load average numbers give the number of jobs in the run queue averaged over 1, 5, and 15 minutes.
Re: pfctl: use strtonum in host()
On Mon, Jul 23, 2018 at 11:16:16AM +0200, Klemens Nanni wrote:
> strtonum(3) is simpler than checking three cases for `q' and gives nicer
> error messages. While here, use `v6mask' as maximum netmask instead of
> hardcoding it.
Isn't the thing called mask here actually a prefix length?
-Otto
>
> OK?
>
> Index: pfctl_parser.c
> ===
> RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v
> retrieving revision 1.321
> diff -u -p -r1.321 pfctl_parser.c
> --- pfctl_parser.c10 Jul 2018 09:30:49 - 1.321
> +++ pfctl_parser.c21 Jul 2018 18:44:57 -
> @@ -1635,7 +1635,8 @@ host(const char *s, int opts)
> {
> struct node_host*h = NULL, *n;
> int mask = -1, v4mask = 32, v6mask = 128, cont = 1;
> - char*p, *q, *r, *ps, *if_name;
> + char*p, *r, *ps, *if_name;
> + const char *errstr;
>
> if ((ps = strdup(s)) == NULL)
> err(1, "host: strdup");
> @@ -1648,9 +1649,9 @@ host(const char *s, int opts)
> if ((p = strrchr(ps, '/')) != NULL) {
> if ((r = strdup(ps)) == NULL)
> err(1, "host: strdup");
> - mask = strtol(p+1, &q, 0);
> - if (!q || *q || mask > 128 || q == (p+1)) {
> - fprintf(stderr, "invalid netmask '%s'\n", p);
> + mask = strtonum(p+1, 0, v6mask, &errstr);
> + if (errstr) {
> + fprintf(stderr, "netmask is %s: %s\n", errstr, p);
> free(r);
> free(ps);
> return (NULL);
> Index: pfail40.ok
> ===
> RCS file: /cvs/src/regress/sbin/pfctl/pfail40.ok,v
> retrieving revision 1.3
> diff -u -p -r1.3 pfail40.ok
> --- pfail40.ok1 Oct 2004 04:33:27 - 1.3
> +++ pfail40.ok21 Jul 2018 18:45:45 -
> @@ -1,4 +1,4 @@
> -invalid netmask '/161'
> +netmask is too large: /161
> stdin:2: could not parse host specification
> -invalid netmask '/161'
> +netmask is too large: /161
> stdin:3: could not parse host specification
pfctl: simplify getaddrinfo() error handling in host_dns()
We're not using `error' anyway so drop the variable and jump to the end.
OK?
Index: pfctl_parser.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v
retrieving revision 1.321
diff -u -p -r1.321 pfctl_parser.c
--- pfctl_parser.c 10 Jul 2018 09:30:49 - 1.321
+++ pfctl_parser.c 22 Jul 2018 10:47:47 -
@@ -1806,8 +1806,7 @@ host_dns(const char *s, int v4mask, int
{
struct addrinfo hints, *res0, *res;
struct node_host*n, *h = NULL;
- int error, noalias = 0;
- int got4 = 0, got6 = 0;
+ int noalias = 0, got4 = 0, got6 = 0;
char*p, *ps;
if ((ps = strdup(s)) == NULL)
@@ -1821,11 +1820,8 @@ host_dns(const char *s, int v4mask, int
hints.ai_socktype = SOCK_STREAM; /* DUMMY */
if (numeric)
hints.ai_flags = AI_NUMERICHOST;
- error = getaddrinfo(ps, NULL, &hints, &res0);
- if (error) {
- free(ps);
- return (h);
- }
+ if (getaddrinfo(ps, NULL, &hints, &res0) != 0)
+ goto error;
for (res = res0; res; res = res->ai_next) {
if (res->ai_family != AF_INET &&
@@ -1873,6 +1869,7 @@ host_dns(const char *s, int v4mask, int
}
}
freeaddrinfo(res0);
+error:
free(ps);
return (h);
pfctl: use strtonum in host()
strtonum(3) is simpler than checking three cases for `q' and gives nicer
error messages. While here, use `v6mask' as maximum netmask instead of
hardcoding it.
OK?
Index: pfctl_parser.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v
retrieving revision 1.321
diff -u -p -r1.321 pfctl_parser.c
--- pfctl_parser.c 10 Jul 2018 09:30:49 - 1.321
+++ pfctl_parser.c 21 Jul 2018 18:44:57 -
@@ -1635,7 +1635,8 @@ host(const char *s, int opts)
{
struct node_host*h = NULL, *n;
int mask = -1, v4mask = 32, v6mask = 128, cont = 1;
- char*p, *q, *r, *ps, *if_name;
+ char*p, *r, *ps, *if_name;
+ const char *errstr;
if ((ps = strdup(s)) == NULL)
err(1, "host: strdup");
@@ -1648,9 +1649,9 @@ host(const char *s, int opts)
if ((p = strrchr(ps, '/')) != NULL) {
if ((r = strdup(ps)) == NULL)
err(1, "host: strdup");
- mask = strtol(p+1, &q, 0);
- if (!q || *q || mask > 128 || q == (p+1)) {
- fprintf(stderr, "invalid netmask '%s'\n", p);
+ mask = strtonum(p+1, 0, v6mask, &errstr);
+ if (errstr) {
+ fprintf(stderr, "netmask is %s: %s\n", errstr, p);
free(r);
free(ps);
return (NULL);
Index: pfail40.ok
===
RCS file: /cvs/src/regress/sbin/pfctl/pfail40.ok,v
retrieving revision 1.3
diff -u -p -r1.3 pfail40.ok
--- pfail40.ok 1 Oct 2004 04:33:27 - 1.3
+++ pfail40.ok 21 Jul 2018 18:45:45 -
@@ -1,4 +1,4 @@
-invalid netmask '/161'
+netmask is too large: /161
stdin:2: could not parse host specification
-invalid netmask '/161'
+netmask is too large: /161
stdin:3: could not parse host specification
systat.1 bar graph - add lockspin "@"
I've noticed that systat(1), in the CPU bar graph, shows "@"s, which aren't explained in systat(1). I went looking and found: src/sys/sys/sched.h revision 1.45 date: 2018/05/14 12:31:21; author: mpi; state: Exp; lines: +9 -12; commitid: NO9HXG6NFNz3ttlB; Stopping counting and reporting CPU time spent spinning on a lock as system time. Introduce a new CP_SPIN "scheduler state" and modify userland tools to display the % of timer a CPU spents spinning. Based on a diff from jmatthew@, ok pirofti@, bluhm@, visa@, deraadt@ I've just tried to apply mpi@'s words, see below. Marcus Index: systat.1 === RCS file: /cvs/src/usr.bin/systat/systat.1,v retrieving revision 1.109 diff -u -p -u -r1.109 systat.1 --- systat.18 Jul 2018 13:23:57 - 1.109 +++ systat.123 Jul 2018 08:00:34 - @@ -460,6 +460,8 @@ Below the queue length listing is a nume a bar graph showing the amount of interrupt (shown as .Ql | ) , +lockspin (shown as +.Ql @ ) , system (shown as .Ql = ) , user (shown as
