Re: [PATCH] column(1): -r to right justify
Hi Job, Job Snijders wrote on Wed, Jul 04, 2018 at 02:09:56PM +0200: > Following some back and forth on how disklabel output should be > formatted, I proposed to Kenneth to extend the column(1) utility. > All that was missing is the ability to right justify. I'm not totally sure we should add the feature, but maybe you are right that it's useful enough, and column(1) still remains simple enough. We have to pay attention not to turn it into a pig resembling rs(1), though. > Patch courtesy of Kenneth R Westerback. OK? No. I see at least two aspects of the patch that are clearly not yet OK, see inline. > Index: column.1 > === > RCS file: /cvs/src/usr.bin/column/column.1,v > retrieving revision 1.18 > diff -u -p -r1.18 column.1 > --- column.1 24 Oct 2016 13:53:05 - 1.18 > +++ column.1 4 Jul 2018 10:27:54 - > @@ -40,6 +40,7 @@ > .Nm column > .Op Fl tx > .Op Fl c Ar columns > +.Op Fl r Op Ar list This is unacceptable. A few programs exist that take options with optional arguments, but we certainly do not want to add new ones because their syntax and semantics is inherently ambiguous and error prone. POSIX also discourages it in general. An option can either take no argument or require an argument, so make a decision in that respect. If you opt for requiring an argument, then you can use a cut(1)-like syntax: cut -b 2-3# bytes 2 and 3 cut -b 2- # all but the first byte cut -b -2 # the first two bytes cut -b 1- # all bytes, not cutting away anything cut -b -2,4- # all but the third byte [...] > Index: column.c > === > RCS file: /cvs/src/usr.bin/column/column.c,v > retrieving revision 1.26 > diff -u -p -r1.26 column.c > --- column.c 22 Jun 2018 12:27:00 - 1.26 > +++ column.c 4 Jul 2018 10:28:00 - [...] > + rflag = 0; tflag = xflag = 0; That looks inconsistent. s/0; // [...] > @@ -207,18 +213,69 @@ print(void) > puts(table[row]->content); > } > > +int > +rightjustify(const char *rcols, const int col) > +{ > + const char *errstr; > + char c, *num, *temp; > + long long ch, rangestart; > + unsigned int i; > + > + if (rcols == NULL) > + return 1; > + > + temp = strdup(rcols); > + num = temp; > + rangestart = -1; > + > + c = 0; > + for (i = 0; i <= strlen(rcols); i++) { > + ch = temp[i]; > + if (ch == ',' || ch == '-') > + temp[i] = '\0'; > + if (temp[i] != '\0') > + continue; > + > + c = strtonum(num, 1, INT_MAX, ); This is not acceptable either. Your algorithm is of the order O(rows) * O(columns) * O(strlen(rcols)) and then calling strdup(3) on the quadratic level and strtonum(3) inside the cubic loop. Even though performance optimization is often over-valued, that is not an excuse for selecting a basic algorithm with an excessively high algorithmic order. You really have to do argument parsing once before entering the quadratic main loop rather than redoing it from scratch for each and every table cell. [...] > void > -maketbl(void) > +maketbl(const int rflag, const char *rcols) > { > struct field **row; > int col; > > for (row = table; entries--; ++row) { > - for (col = 0; (*row)[col + 1].content != NULL; ++col) [...] > + for (col = 0; (*row)[col].content != NULL; ++col) { Here, you are losing the feature that column(1) refrains from printing trailing blanks. I'm putting the audit on hold at this points because fixing the issues found so far is going to change the patch substantially. Yours, Ingo
Re: if_enc.c: "rdomain" is more descriptive than "id"
Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2018.07.08 15:26:33 +0200: > > Maybe not very important, but "id" sounds a tad too generic to me, and > it looks like we're really dealing with an rdomain here, so here's a diff. > > Thoughts? yes please, if only for making searching easier > ok? yes reads ok > > > Index: net/if_enc.c > === > RCS file: /cvs/src/sys/net/if_enc.c,v > retrieving revision 1.72 > diff -u -p -r1.72 if_enc.c > --- net/if_enc.c 8 Jul 2018 13:04:04 - 1.72 > +++ net/if_enc.c 8 Jul 2018 13:20:13 - > @@ -36,7 +36,7 @@ > #endif > > struct ifnet **enc_ifps; /* rdomain-mapped enc ifs */ > -u_int enc_max_id; > +u_int enc_max_rdomain; > struct ifnet **enc_allifps; /* unit-mapped enc ifs */ > u_int enc_max_unit; > #define ENC_MAX_UNITS 4096 /* XXX n per rdomain */ > @@ -210,7 +210,7 @@ enc_ioctl(struct ifnet *ifp, u_long cmd, > } > > struct ifnet * > -enc_getif(u_int id, u_int unit) > +enc_getif(u_int rdomain, u_int unit) > { > struct ifnet*ifp; > > @@ -221,7 +221,7 @@ enc_getif(u_int id, u_int unit) > if (unit > enc_max_unit) > return (NULL); > ifp = enc_allifps[unit]; > - if (ifp == NULL || ifp->if_rdomain != id) > + if (ifp == NULL || ifp->if_rdomain != rdomain) > return (NULL); > return (ifp); > } > @@ -229,20 +229,20 @@ enc_getif(u_int id, u_int unit) > /* Otherwise return the default enc interface for this rdomain */ > if (enc_ifps == NULL) > return (NULL); > - else if (id > RT_TABLEID_MAX) > + else if (rdomain > RT_TABLEID_MAX) > return (NULL); > - else if (id > enc_max_id) > + else if (rdomain > enc_max_rdomain) > return (NULL); > - return (enc_ifps[id]); > + return (enc_ifps[rdomain]); > } > > struct ifaddr * > -enc_getifa(u_int id, u_int unit) > +enc_getifa(u_int rdomain, u_int unit) > { > struct ifnet*ifp; > struct enc_softc*sc; > > - ifp = enc_getif(id, unit); > + ifp = enc_getif(rdomain, unit); > if (ifp == NULL) > return (NULL); > > @@ -250,7 +250,7 @@ enc_getifa(u_int id, u_int unit) > return (>sc_ifa); > } > int > -enc_setif(struct ifnet *ifp, u_int id) > +enc_setif(struct ifnet *ifp, u_int rdomain) > { > struct ifnet**new; > size_t newlen; > @@ -265,28 +265,28 @@ enc_setif(struct ifnet *ifp, u_int id) >* for this rdomain, so only the first enc interface that >* was added for this rdomain becomes the default. >*/ > - if (enc_getif(id, 0) != NULL) > + if (enc_getif(rdomain, 0) != NULL) > return (0); > > - if (id > RT_TABLEID_MAX) > + if (rdomain > RT_TABLEID_MAX) > return (EINVAL); > > - if (enc_ifps == NULL || id > enc_max_id) { > - if ((new = mallocarray(id + 1, sizeof(struct ifnet *), > + if (enc_ifps == NULL || rdomain > enc_max_rdomain) { > + if ((new = mallocarray(rdomain + 1, sizeof(struct ifnet *), > M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL) > return (ENOBUFS); > - newlen = sizeof(struct ifnet *) * (id + 1); > + newlen = sizeof(struct ifnet *) * (rdomain + 1); > > if (enc_ifps != NULL) { > memcpy(new, enc_ifps, > - sizeof(struct ifnet *) * (enc_max_id + 1)); > + sizeof(struct ifnet *) * (enc_max_rdomain + 1)); > free(enc_ifps, M_DEVBUF, 0); > } > enc_ifps = new; > - enc_max_id = id; > + enc_max_rdomain = rdomain; > } > > - enc_ifps[id] = ifp; > + enc_ifps[rdomain] = ifp; > > /* Indicate that this interface is the rdomain default */ > ifp->if_link_state = LINK_STATE_UP; > @@ -297,14 +297,14 @@ enc_setif(struct ifnet *ifp, u_int id) > void > enc_unsetif(struct ifnet *ifp) > { > - u_intid = ifp->if_rdomain, i; > + u_intrdomain = ifp->if_rdomain, i; > struct ifnet*oifp, *nifp; > > - if ((oifp = enc_getif(id, 0)) == NULL || oifp != ifp) > + if ((oifp = enc_getif(rdomain, 0)) == NULL || oifp != ifp) > return; > > /* Clear slot for this rdomain */ > - enc_ifps[id] = NULL; > + enc_ifps[rdomain] = NULL; > ifp->if_link_state = LINK_STATE_UNKNOWN; > > /* > @@ -314,10 +314,10 @@ enc_unsetif(struct ifnet *ifp) > for (i = 0; i < (enc_max_unit + 1); i++) { > nifp = enc_allifps[i]; > > - if (nifp == NULL || nifp == ifp
Re: xenodm: source Xsetup before initializing the greeter
On Sun, Jul 08 2018, Matthieu Herrb wrote: > Hi, > > prompted by a question by weerd@ who wanted to be able to rotate the > screen of his gpd win *before* the Xenodm greeter is displayed, I came > up with this (simple) patch that sources the Xsetup_0 script before > initializing the greeter widget. > > I can't think of any down sides of doing this, and it may even help > with some further pledges or privilege separation for the greeter. > > ok ? fwiw no regression in my simple setup (xsession that starts both one-shot and long-running X programs). > Index: greeter/greet.c > === > RCS file: /cvs/OpenBSD/xenocara/app/xenodm/greeter/greet.c,v > retrieving revision 1.5 > diff -u -r1.5 greet.c > --- greeter/greet.c 6 May 2018 15:25:27 - 1.5 > +++ greeter/greet.c 30 Jun 2018 08:51:44 - > @@ -301,13 +301,13 @@ > Arg arglist[2]; > Display*dpy; > > -dpy = InitGreet (d); > /* > * Run the setup script - note this usually will not work when > * the server is grabbed, so we don't even bother trying. > */ > if (!d->grabServer) > SetupDisplay (d); > +dpy = InitGreet (d); > if (!dpy) { > LogError ("Cannot reopen display %s for greet window\n", d->name); > exit (RESERVER_DISPLAY); -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Fix a kernelpanic when playing with rdomain(4) and enc(4)
On Sun, Jun 24 2018, Denis Fondras wrote: > When removing enc(4) interface from rdomain, the kernel panics randomly > (memcpy() seems to copy outside of the mallocarray() boundaries) with > something > like : > > Data modified on freelist: word -35183699295756 of object 0x8059da80 > size 0x8 previous type free (invalid addr 0x7b44962aa448c22a) > kernel: protection fault trap, code=0 > Stopped at malloc+0x4d3: movq0x8(%r14),%rbx [...] > Here is a fix : > > Index: if_enc.c > === > RCS file: /cvs/src/sys/net/if_enc.c,v > retrieving revision 1.70 > diff -u -p -r1.70 if_enc.c > --- if_enc.c 16 Oct 2017 08:22:25 - 1.70 > +++ if_enc.c 24 Jun 2018 17:15:32 - > @@ -271,7 +271,7 @@ enc_setif(struct ifnet *ifp, u_int id) > if (id > RT_TABLEID_MAX) > return (EINVAL); > > - if (id == 0 || id > enc_max_id) { > + if (enc_ifps == NULL || id > enc_max_id) { > if ((new = mallocarray(id + 1, sizeof(struct ifnet *), > M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL) > return (ENOBUFS); This pattern is also used in enc_clone_create(), even if right now we can't call this function twice for enc0 I think the code should be made consistent. ok? Index: net/if_enc.c === --- net/if_enc.c.orig +++ net/if_enc.c @@ -120,7 +120,7 @@ enc_clone_create(struct if_clone *ifc, i return (error); } - if (unit == 0 || unit > enc_max_unit) { + if (enc_ifps == NULL || unit > enc_max_unit) { if ((new = mallocarray(unit + 1, sizeof(struct ifnet *), M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL) { NET_UNLOCK(); -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: systat(1) iostat vs. iostat(1)
yes, its bytes. ok benno@ Jason McIntyre(j...@kerhand.co.uk) on 2018.07.07 17:20:19 +0100: > On Sat, Jul 07, 2018 at 08:28:52AM +0200, Marcus MERIGHI wrote: > > Hello, > > > > when I run 'iostat(1) sd1 1' I get the transfer speed in MB/s. Running > > 'systat(1) iostat 1' in parallel I get the transfer speed in bytes, as > > calculations show. Therefore adjust the manual of systat(1): > > kilobytes -> bytes. > > > > Marcus > > > > someone (obsd dev) commit this or give me an ok, please. > jmc > > > Index: systat.1 > > === > > RCS file: /cvs/src/usr.bin/systat/systat.1,v > > retrieving revision 1.106 > > diff -u -p -u -r1.106 systat.1 > > --- systat.130 May 2018 13:53:09 - 1.106 > > +++ systat.17 Jul 2018 06:20:03 - > > @@ -293,7 +293,7 @@ this is the default. > > .It Ic iostat > > Display statistics about disk throughput. > > Statistics > > -on disk throughput show, for each drive, data transferred in kilobytes, > > +on disk throughput show, for each drive, data transferred in bytes, > > number of disk transactions performed, and time spent in disk accesses > > (in fractions of a second). > > .It Ic malloc > > >
Re: Fix a kernelpanic when playing with rdomain(4) and enc(4)
On Sun, Jul 08 2018, Jeremie Courreges-Anglas wrote: > On Sun, Jun 24 2018, Denis Fondras wrote: >> When removing enc(4) interface from rdomain, the kernel panics randomly >> (memcpy() seems to copy outside of the mallocarray() boundaries) with >> something >> like : >> >> Data modified on freelist: word -35183699295756 of object 0x8059da80 >> size 0x8 previous type free (invalid addr 0x7b44962aa448c22a) >> kernel: protection fault trap, code=0 >> Stopped at malloc+0x4d3: movq0x8(%r14),%rbx > > [...] > >> Here is a fix : >> >> Index: if_enc.c >> === >> RCS file: /cvs/src/sys/net/if_enc.c,v >> retrieving revision 1.70 >> diff -u -p -r1.70 if_enc.c >> --- if_enc.c 16 Oct 2017 08:22:25 - 1.70 >> +++ if_enc.c 24 Jun 2018 17:15:32 - >> @@ -271,7 +271,7 @@ enc_setif(struct ifnet *ifp, u_int id) >> if (id > RT_TABLEID_MAX) >> return (EINVAL); >> >> -if (id == 0 || id > enc_max_id) { >> +if (enc_ifps == NULL || id > enc_max_id) { >> if ((new = mallocarray(id + 1, sizeof(struct ifnet *), >> M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL) >> return (ENOBUFS); > > This pattern is also used in enc_clone_create(), even if right now we > can't call this function twice for enc0 I think the code should be made > consistent. ok? Better send the correct diff, thanks Denis for the heads-up. Index: net/if_enc.c === --- net/if_enc.c.orig +++ net/if_enc.c @@ -120,7 +120,7 @@ enc_clone_create(struct if_clone *ifc, i return (error); } - if (unit == 0 || unit > enc_max_unit) { + if (enc_allifps == NULL || unit > enc_max_unit) { if ((new = mallocarray(unit + 1, sizeof(struct ifnet *), M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL) { NET_UNLOCK(); -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Fix a kernelpanic when playing with rdomain(4) and enc(4)
OK denis@ On Sun, Jul 08, 2018 at 02:04:56PM +0200, Jeremie Courreges-Anglas wrote: > On Sun, Jul 08 2018, Jeremie Courreges-Anglas wrote: > > On Sun, Jun 24 2018, Denis Fondras wrote: > >> When removing enc(4) interface from rdomain, the kernel panics randomly > >> (memcpy() seems to copy outside of the mallocarray() boundaries) with > >> something > >> like : > >> > >> Data modified on freelist: word -35183699295756 of object > >> 0x8059da80 size 0x8 previous type free (invalid addr > >> 0x7b44962aa448c22a) > >> kernel: protection fault trap, code=0 > >> Stopped at malloc+0x4d3: movq0x8(%r14),%rbx > > > > [...] > > > >> Here is a fix : > >> > >> Index: if_enc.c > >> === > >> RCS file: /cvs/src/sys/net/if_enc.c,v > >> retrieving revision 1.70 > >> diff -u -p -r1.70 if_enc.c > >> --- if_enc.c 16 Oct 2017 08:22:25 - 1.70 > >> +++ if_enc.c 24 Jun 2018 17:15:32 - > >> @@ -271,7 +271,7 @@ enc_setif(struct ifnet *ifp, u_int id) > >>if (id > RT_TABLEID_MAX) > >>return (EINVAL); > >> > >> - if (id == 0 || id > enc_max_id) { > >> + if (enc_ifps == NULL || id > enc_max_id) { > >>if ((new = mallocarray(id + 1, sizeof(struct ifnet *), > >>M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL) > >>return (ENOBUFS); > > > > This pattern is also used in enc_clone_create(), even if right now we > > can't call this function twice for enc0 I think the code should be made > > consistent. ok? > > Better send the correct diff, thanks Denis for the heads-up. > > > Index: net/if_enc.c > === > --- net/if_enc.c.orig > +++ net/if_enc.c > @@ -120,7 +120,7 @@ enc_clone_create(struct if_clone *ifc, i > return (error); > } > > - if (unit == 0 || unit > enc_max_unit) { > + if (enc_allifps == NULL || unit > enc_max_unit) { > if ((new = mallocarray(unit + 1, sizeof(struct ifnet *), > M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL) { > NET_UNLOCK(); > > > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE >
Re: systat(1) iostat vs. iostat(1)
On Sat, Jul 07, 2018 at 08:28:52AM +0200, Marcus MERIGHI wrote: > Hello, > > when I run 'iostat(1) sd1 1' I get the transfer speed in MB/s. Running > 'systat(1) iostat 1' in parallel I get the transfer speed in bytes, as > calculations show. Therefore adjust the manual of systat(1): > kilobytes -> bytes. > > Marcus > fixed, thanks. jmc > Index: systat.1 > === > RCS file: /cvs/src/usr.bin/systat/systat.1,v > retrieving revision 1.106 > diff -u -p -u -r1.106 systat.1 > --- systat.1 30 May 2018 13:53:09 - 1.106 > +++ systat.1 7 Jul 2018 06:20:03 - > @@ -293,7 +293,7 @@ this is the default. > .It Ic iostat > Display statistics about disk throughput. > Statistics > -on disk throughput show, for each drive, data transferred in kilobytes, > +on disk throughput show, for each drive, data transferred in bytes, > number of disk transactions performed, and time spent in disk accesses > (in fractions of a second). > .It Ic malloc >
if_enc.c: "rdomain" is more descriptive than "id"
Maybe not very important, but "id" sounds a tad too generic to me, and it looks like we're really dealing with an rdomain here, so here's a diff. Thoughts? ok? Index: net/if_enc.c === RCS file: /cvs/src/sys/net/if_enc.c,v retrieving revision 1.72 diff -u -p -r1.72 if_enc.c --- net/if_enc.c8 Jul 2018 13:04:04 - 1.72 +++ net/if_enc.c8 Jul 2018 13:20:13 - @@ -36,7 +36,7 @@ #endif struct ifnet **enc_ifps; /* rdomain-mapped enc ifs */ -u_intenc_max_id; +u_intenc_max_rdomain; struct ifnet **enc_allifps; /* unit-mapped enc ifs */ u_intenc_max_unit; #define ENC_MAX_UNITS4096 /* XXX n per rdomain */ @@ -210,7 +210,7 @@ enc_ioctl(struct ifnet *ifp, u_long cmd, } struct ifnet * -enc_getif(u_int id, u_int unit) +enc_getif(u_int rdomain, u_int unit) { struct ifnet*ifp; @@ -221,7 +221,7 @@ enc_getif(u_int id, u_int unit) if (unit > enc_max_unit) return (NULL); ifp = enc_allifps[unit]; - if (ifp == NULL || ifp->if_rdomain != id) + if (ifp == NULL || ifp->if_rdomain != rdomain) return (NULL); return (ifp); } @@ -229,20 +229,20 @@ enc_getif(u_int id, u_int unit) /* Otherwise return the default enc interface for this rdomain */ if (enc_ifps == NULL) return (NULL); - else if (id > RT_TABLEID_MAX) + else if (rdomain > RT_TABLEID_MAX) return (NULL); - else if (id > enc_max_id) + else if (rdomain > enc_max_rdomain) return (NULL); - return (enc_ifps[id]); + return (enc_ifps[rdomain]); } struct ifaddr * -enc_getifa(u_int id, u_int unit) +enc_getifa(u_int rdomain, u_int unit) { struct ifnet*ifp; struct enc_softc*sc; - ifp = enc_getif(id, unit); + ifp = enc_getif(rdomain, unit); if (ifp == NULL) return (NULL); @@ -250,7 +250,7 @@ enc_getifa(u_int id, u_int unit) return (>sc_ifa); } int -enc_setif(struct ifnet *ifp, u_int id) +enc_setif(struct ifnet *ifp, u_int rdomain) { struct ifnet**new; size_t newlen; @@ -265,28 +265,28 @@ enc_setif(struct ifnet *ifp, u_int id) * for this rdomain, so only the first enc interface that * was added for this rdomain becomes the default. */ - if (enc_getif(id, 0) != NULL) + if (enc_getif(rdomain, 0) != NULL) return (0); - if (id > RT_TABLEID_MAX) + if (rdomain > RT_TABLEID_MAX) return (EINVAL); - if (enc_ifps == NULL || id > enc_max_id) { - if ((new = mallocarray(id + 1, sizeof(struct ifnet *), + if (enc_ifps == NULL || rdomain > enc_max_rdomain) { + if ((new = mallocarray(rdomain + 1, sizeof(struct ifnet *), M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL) return (ENOBUFS); - newlen = sizeof(struct ifnet *) * (id + 1); + newlen = sizeof(struct ifnet *) * (rdomain + 1); if (enc_ifps != NULL) { memcpy(new, enc_ifps, - sizeof(struct ifnet *) * (enc_max_id + 1)); + sizeof(struct ifnet *) * (enc_max_rdomain + 1)); free(enc_ifps, M_DEVBUF, 0); } enc_ifps = new; - enc_max_id = id; + enc_max_rdomain = rdomain; } - enc_ifps[id] = ifp; + enc_ifps[rdomain] = ifp; /* Indicate that this interface is the rdomain default */ ifp->if_link_state = LINK_STATE_UP; @@ -297,14 +297,14 @@ enc_setif(struct ifnet *ifp, u_int id) void enc_unsetif(struct ifnet *ifp) { - u_intid = ifp->if_rdomain, i; + u_intrdomain = ifp->if_rdomain, i; struct ifnet*oifp, *nifp; - if ((oifp = enc_getif(id, 0)) == NULL || oifp != ifp) + if ((oifp = enc_getif(rdomain, 0)) == NULL || oifp != ifp) return; /* Clear slot for this rdomain */ - enc_ifps[id] = NULL; + enc_ifps[rdomain] = NULL; ifp->if_link_state = LINK_STATE_UNKNOWN; /* @@ -314,10 +314,10 @@ enc_unsetif(struct ifnet *ifp) for (i = 0; i < (enc_max_unit + 1); i++) { nifp = enc_allifps[i]; - if (nifp == NULL || nifp == ifp || nifp->if_rdomain != id) + if (nifp == NULL || nifp == ifp || nifp->if_rdomain != rdomain) continue; - enc_ifps[id] = nifp; + enc_ifps[rdomain] = nifp; nifp->if_link_state = LINK_STATE_UP;
xenodm: source Xsetup before initializing the greeter
Hi, prompted by a question by weerd@ who wanted to be able to rotate the screen of his gpd win *before* the Xenodm greeter is displayed, I came up with this (simple) patch that sources the Xsetup_0 script before initializing the greeter widget. I can't think of any down sides of doing this, and it may even help with some further pledges or privilege separation for the greeter. ok ? Index: greeter/greet.c === RCS file: /cvs/OpenBSD/xenocara/app/xenodm/greeter/greet.c,v retrieving revision 1.5 diff -u -r1.5 greet.c --- greeter/greet.c 6 May 2018 15:25:27 - 1.5 +++ greeter/greet.c 30 Jun 2018 08:51:44 - @@ -301,13 +301,13 @@ Argarglist[2]; Display*dpy; -dpy = InitGreet (d); /* * Run the setup script - note this usually will not work when * the server is grabbed, so we don't even bother trying. */ if (!d->grabServer) SetupDisplay (d); +dpy = InitGreet (d); if (!dpy) { LogError ("Cannot reopen display %s for greet window\n", d->name); exit (RESERVER_DISPLAY); -- Matthieu Herrb
netstart(8): check for root privileges
When running netstart as regular user, the output looks something like this: $ sh /etc/netstart iwn0 /etc/netstart[226]: /etc/soii.key: cannot open $(<) input sysctl: net.inet6.ip6.soiikey: Operation not permitted ifconfig: SIOCS80211NWID: Operation not permitted ifconfig: SIOCS80211WPAPSK: Operation not permitted etc... There was a privilege check present for a while (added by jasper in r1.170), but backed out by rpe in r1.181, as it turned out to break diskless setups, as /usr/bin/id might not be present during early boot. Diff below adds it back while checking first that id(1) is there. Index: netstart === RCS file: /var/cvs/src/etc/netstart,v retrieving revision 1.198 diff -u -p -r1.198 netstart --- netstart28 Apr 2018 22:38:32 - 1.198 +++ netstart8 Jul 2018 16:48:01 - @@ -201,6 +201,13 @@ defaultroute() { done } +# Make sure the invoking user has the right privileges. Check for presence of +# id(1) to avoid problems with diskless setups. +if [[ -x /usr/bin/id ]] && (($(id -u) != 0)); then + echo "${0##*/}: need root privileges" + exit 1 +fi + # Get network related vars from rc.conf using the parsing routine from rc.subr. FUNCS_ONLY=1 . /etc/rc.d/rc.subr _rc_parse_conf
Re: [PATCH] column(1): -r to right justify
An afterthought... Ingo Schwarze wrote on Sun, Jul 08, 2018 at 08:22:34PM +0200: > If you opt for requiring an argument, > then you can use a cut(1)-like syntax: Alternatively, you can use a syntax similar to tbl(7) "Layout" lines: rrll which allows later extensions like r|rclnl That's much more powerful, not harder to understand for the user, and probably also easier to implement. Yours, Ingo
disable seemingly unsupported MSI for AMD 17h/Raven Ridge HD Audio in azalia.c
Hi, It appears that HD Audio from AMD's generation Ryzen can't handle MSI. This leads to the bug that I reported here: https://marc.info/?l=openbsd-bugs=151648196215922=2 Disabling MSI resolves the problem on my current system which is a Raven Ridge APU. I don't have the Summit Ridge hardware anymore to test that it is also resolved there, but the line is included in the diff (PCI_PRODUCT_AMD_AMD64_17_HDA). It seems likely that this diff will also fix HD Audio on Summit Ridge. However, testing would be welcome by anyone who has a first-gen Ryzen. I was slightly confused by the fact that so far it seems I've been the only one who reported this. Even searching online for such an issue in other OS didn't yield anything. However, the issue was there between 2 different Ryzen CPUs, 3 different motherboards, and at least 2 separate OpenBSD -current installations. And it was never there on any Intel-based setup, with otherwise same hardware and OpenBSD install. While there have been several reports of people using Ryzen with OpenBSD, they may not have used audio (that's my explanation for this at the moment). This diff was collaborative work with brynet@. ok? Index: azalia.c === RCS file: /cvs/src/sys/dev/pci/azalia.c,v retrieving revision 1.244 diff -u -p -r1.244 azalia.c --- azalia.c22 Apr 2018 10:02:13 - 1.244 +++ azalia.c7 Jul 2018 18:26:20 - @@ -517,6 +517,15 @@ azalia_pci_attach(struct device *parent, azalia_pci_write(sc->pc, sc->tag, ICH_PCI_MMC, reg); } + /* disable MSI for AMD Summit Ridge/Raven Ridge HD Audio */ + if (PCI_VENDOR(sc->pciid) == PCI_VENDOR_AMD) { + switch (PCI_PRODUCT(sc->pciid)) { + case PCI_PRODUCT_AMD_AMD64_17_HDA: + case PCI_PRODUCT_AMD_RAVENRIDGE_HDA: + pa->pa_flags &= ~PCI_FLAGS_MSI_ENABLED; + } + } + /* interrupt */ if (pci_intr_map_msi(pa, ) && pci_intr_map(pa, )) { printf(": can't map interrupt\n");
Re: usbdevs & hub ports status
On 04/07/18(Wed) 17:00, Martin Pieuchot wrote: > Diff below adds support for printing USB ports status. It includes an > ABI change as we currently do not export port status/change to userland. > > I'd really like to export the current cached value to userland via the > USB_DEVICEINFO ioctl(2) to reduce the numbers of I/O. > > Note that this diff also fixes some link-status defines in usb.h > > Since it includes an ABI break I cranked devel/libusb1. > > $ usbdevs -dvv > Controller /dev/usb0: > addr 01: 8086: Intel, xHCI root hub >super speed, self powered, config 1, rev 1.00 >driver: uhub0 >port 01: .02a0 power Rx.detect >port 02: .02a0 power Rx.detect >port 03: .02a0 power Rx.detect >port 04: .0503 connect enabled recovery >port 05: .02a0 power Rx.detect > ... > addr 02: 04f2:b45d Chicony Electronics Co.,Ltd., Integrated Camera >high speed, power 500 mA, config 1, rev 0.29, iSerialNumber 0x0001 >driver: uvideo0 > > ok? Anyone? Comments? Oks? > Index: src/usr.sbin/usbdevs/usbdevs.c > === > RCS file: /cvs/src/usr.sbin/usbdevs/usbdevs.c,v > retrieving revision 1.27 > diff -u -p -r1.27 usbdevs.c > --- src/usr.sbin/usbdevs/usbdevs.c3 Jul 2018 13:21:31 - 1.27 > +++ src/usr.sbin/usbdevs/usbdevs.c4 Jul 2018 14:46:08 - > @@ -54,7 +54,7 @@ int verbose = 0; > int showdevs = 0; > > void usage(void); > -void usbdev(int f, uint8_t, int rec); > +void usbdev(int f, uint8_t); > void usbdump(int f); > void dumpone(char *name, int f, int addr); > int main(int, char **); > @@ -69,13 +69,13 @@ usage(void) > } > > char done[USB_MAX_DEVICES]; > -int indent; > > void > -usbdev(int f, uint8_t addr, int rec) > +usbdev(int f, uint8_t addr) > { > struct usb_device_info di; > - int e, p, i, s, nports; > + int e, i, port, nports; > + uint16_t change, status; > > di.udi_addr = addr; > e = ioctl(f, USB_DEVICEINFO, ); > @@ -89,6 +89,7 @@ usbdev(int f, uint8_t addr, int rec) > done[addr] = 1; > printf("%04x:%04x %s, %s", di.udi_vendorNo, di.udi_productNo, > di.udi_vendor, di.udi_product); > + > if (verbose) { > printf("\n\t "); > switch (di.udi_speed) { > @@ -122,48 +123,96 @@ usbdev(int f, uint8_t addr, int rec) > printf(", iSerialNumber %s", di.udi_serial); > } > printf("\n"); > + > if (showdevs) { > for (i = 0; i < USB_MAX_DEVNAMES; i++) > if (di.udi_devnames[i][0]) > - printf("%*s %s\n", indent, "", > - di.udi_devnames[i]); > + printf("\t driver: %s\n", di.udi_devnames[i]); > } > - if (!rec) > - return; > > - nports = MINIMUM(di.udi_nports, nitems(di.udi_ports)); > if (verbose > 1) { > - for (p = 0; p < nports; p++) { > - s = di.udi_ports[p]; > - printf("\t port %02u:", p+1); > - if (s < USB_MAX_DEVICES) > - printf(" addr %02u\n", s); > - else { > - printf(" %s\n", > - s == USB_PORT_ENABLED ? "enabled" : > - s == USB_PORT_SUSPENDED ? "suspended" : > - s == USB_PORT_POWERED ? "powered" : > - s == USB_PORT_DISABLED ? "disabled" : > - "???"); > + nports = MINIMUM(di.udi_nports, nitems(di.udi_ports)); > + for (port = 0; port < nports; port++) { > + status = di.udi_ports[port] & 0x; > + change = di.udi_ports[port] >> 16; > + printf("\t port %02u: %04x.%04x", port+1, change, > + status); > + > + if (status & UPS_CURRENT_CONNECT_STATUS) > + printf(" connect"); > + > + if (status & UPS_PORT_ENABLED) > + printf(" enabled"); > + > + if (status & UPS_SUSPEND) > + printf(" supsend"); > + > + if (status & UPS_OVERCURRENT_INDICATOR) > + printf(" overcurrent"); > + > + if (di.udi_speed < USB_SPEED_SUPER) { > + if (status & UPS_PORT_L1) > + printf(" l1"); > + > + if (status & UPS_PORT_POWER) > + printf(" power"); > + } else { > + if (status & UPS_PORT_POWER_SS) > + printf(" power"); > +
Patch for column number in doas error messages
Hey, Doas currently tells you the line but not the column for syntax errors. In the case of a missing newline at the end of a line I was confused. So I added the column number to the message as well. Also, is there any interest in relaxing the grammar so a trailing rule without a newline is ok? Let me know what you think. diff --git parse.y parse.y index fde406bcf5a..f98deb81706 100644 --- parse.y +++ parse.y @@ -195,7 +195,7 @@ yyerror(const char *fmt, ...) va_start(va, fmt); vfprintf(stderr, fmt, va); va_end(va); - fprintf(stderr, " at line %d\n", yylval.lineno + 1); + fprintf(stderr, " at line %d, column %d\n", yylval.lineno + 1, yylval.colno); parse_errors++; } -- Phil Eaton
Re: disable seemingly unsupported MSI for AMD 17h/Raven Ridge HD Audio in azalia.c
On Sun, Jul 08, 2018 at 12:15:39PM -0700, Thomas Frohwein wrote: > Hi, > > It appears that HD Audio from AMD's generation Ryzen can't handle MSI. > This leads to the bug that I reported here: > > https://marc.info/?l=openbsd-bugs=151648196215922=2 > > Disabling MSI resolves the problem on my current system which is a Raven > Ridge APU. I don't have the Summit Ridge hardware anymore to test that > it is also resolved there, but the line is included in the diff > (PCI_PRODUCT_AMD_AMD64_17_HDA). It seems likely that this diff will also > fix HD Audio on Summit Ridge. However, testing would be welcome by > anyone who has a first-gen Ryzen. > > I was slightly confused by the fact that so far it seems I've been the > only one who reported this. Even searching online for such an issue in > other OS didn't yield anything. However, the issue was there between 2 > different Ryzen CPUs, 3 different motherboards, and at least 2 separate > OpenBSD -current installations. And it was never there on any > Intel-based setup, with otherwise same hardware and OpenBSD install. > While there have been several reports of people using Ryzen with > OpenBSD, they may not have used audio (that's my explanation for this > at the moment). > > This diff was collaborative work with brynet@. > > ok? Does following the HUDSON2_HDA case and enabling pcie snooping instead change anything? > > Index: azalia.c > === > RCS file: /cvs/src/sys/dev/pci/azalia.c,v > retrieving revision 1.244 > diff -u -p -r1.244 azalia.c > --- azalia.c 22 Apr 2018 10:02:13 - 1.244 > +++ azalia.c 7 Jul 2018 18:26:20 - > @@ -517,6 +517,15 @@ azalia_pci_attach(struct device *parent, > azalia_pci_write(sc->pc, sc->tag, ICH_PCI_MMC, reg); > } > > + /* disable MSI for AMD Summit Ridge/Raven Ridge HD Audio */ > + if (PCI_VENDOR(sc->pciid) == PCI_VENDOR_AMD) { > + switch (PCI_PRODUCT(sc->pciid)) { > + case PCI_PRODUCT_AMD_AMD64_17_HDA: > + case PCI_PRODUCT_AMD_RAVENRIDGE_HDA: > + pa->pa_flags &= ~PCI_FLAGS_MSI_ENABLED; > + } > + } > + > /* interrupt */ > if (pci_intr_map_msi(pa, ) && pci_intr_map(pa, )) { > printf(": can't map interrupt\n"); >
broadcom netxtreme-c/e driver
This started out as a port of freebsd's bnxt driver, but along the way I had to rewrite the interesting bits. NetXtreme-C/E (BCM573xx and BCM574xx) is a different line of chips to NetXtreme II 10G (BCM577xx, which mje was working on), and luckily for me it's much easier to talk to. The reason these ones are interesting to me is that they're the only 10G mezzanine option for Dell's Epyc based servers. I've got it to the point where I could commit to cvs over it, so I'd like to get it into the tree. Lots of stuff is still missing - media types, multicast, jumbos, any of the vast array of offloads, and it's not particularly fast yet, only slightly over a gigabit in iperf/tcpbench type tests. Lots of work to do still. The current code is below, only I'm not including bnxtreg.h as it's fairly large (>30k lines) and it came unmodified from freebsd: https://svnweb.freebsd.org/base/head/sys/dev/bnxt/hsi_struct_def.h?revision=323233=markup ok? /* $OpenBSD$ */ /*- * Broadcom NetXtreme-C/E network driver. * * Copyright (c) 2016 Broadcom, All Rights Reserved. * The term Broadcom refers to Broadcom Limited and/or its subsidiaries * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions * are met: * 1. Redistributions of source code must retain the above copyright *notice, this list of conditions and the following disclaimer. * 2. Redistributions in binary form must reproduce the above copyright *notice, this list of conditions and the following disclaimer in the *documentation and/or other materials provided with the distribution. * * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS' * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF * THE POSSIBILITY OF SUCH DAMAGE. */ /* * Copyright (c) 2018 Jonathan Matthew * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above * copyright notice and this permission notice appear in all copies. * * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ #include "bpfilter.h" #include #include #include #include #include #include #include #include #include #include #include #include #include #define __FBSDID(x) #include #include #include #if NBPFILTER > 0 #include #endif #include #include #define BNXT_HWRM_BAR 0x10 #define BNXT_DOORBELL_BAR 0x18 #define BNXT_RX_RING_ID 0 #define BNXT_AG_RING_ID 1 #define BNXT_TX_RING_ID 3 #define BNXT_MAX_QUEUE 8 #define BNXT_MAX_MTU9000 #define BNXT_MAX_TX_SEGS32 /* a bit much? */ #define BNXT_HWRM_SHORT_REQ_LEN sizeof(struct hwrm_short_input) #define BNXT_HWRM_LOCK_INIT(_sc, _name) \ mtx_init_flags(>sc_lock, IPL_NET, _name, 0) #define BNXT_HWRM_LOCK(_sc) mtx_enter(&_sc->sc_lock) #define BNXT_HWRM_UNLOCK(_sc) mtx_leave(&_sc->sc_lock) #define BNXT_HWRM_LOCK_DESTROY(_sc) /* nothing */ #define BNXT_HWRM_LOCK_ASSERT(_sc) MUTEX_ASSERT_LOCKED(&_sc->sc_lock) #define BNXT_FLAG_VF0x0001 #define BNXT_FLAG_NPAR 0x0002 #define BNXT_FLAG_WOL_CAP 0x0004 #define BNXT_FLAG_SHORT_CMD 0x0008 #define NEXT_CP_CONS_V(_ring, _cons, _v_bit)\ do {\ if (++(_cons) == (_ring)->ring_size)\ ((_cons) = 0, (_v_bit) = !_v_bit); \ } while (0); struct bnxt_cos_queue { uint8_t id; uint8_t profile; }; struct bnxt_ring { uint64_tpaddr; uint64_tdoorbell; caddr_t vaddr; uint32_tring_size; uint16_tid; uint16_t
ber.c: simplify ber_read()
When looking at the recent changes in this file I noticed that the presence of both ber_read() and ber_readbuf() was due to an incomplete cleanup from my part. revision 1.32 date: 2018/02/08 18:02:06; author: jca; state: Exp; lines: +6 -22; commitid: YNQMco5lCS8YEifQ; Kill ber.c support for direct fd read/writes This mechanism is already unused and annotated with lots of XXX's, no need to keep it around. ok claudio@ I think the recent changes would have been easier if the code had been further simplified. Here's a shot at it: - stop looping in ber_read(), there is no fd read to handle any more so calling ber_readbuf() once is enough - amend the condition which decides whether to return -1/ECANCELED: - it makes little sense to pass a buffer length size of zero to ber_read(), but then we should probably return 0 and not -1/ECANCELED - I don't think we should perform partial reads: either the caller function has the data it asked for, or nothing. Allowing partial reads means that we're putting the burden of handling an incomplete buffer on the caller. - inline what remains of ber_readbuf() in ber_read() regress tests for ldapd and snmpd pass, it would be cool to have test reports from people who actually use those programs (ldap, ldapd, snmpd and ypldap). Looking for reviews and oks. The diff is not that long but I can split it in smaller parts if it helps. Index: usr.bin/ldap/ber.c === RCS file: /d/cvs/src/usr.bin/ldap/ber.c,v retrieving revision 1.11 diff -u -p -r1.11 ber.c --- usr.bin/ldap/ber.c 4 Jul 2018 15:21:24 - 1.11 +++ usr.bin/ldap/ber.c 6 Jul 2018 11:50:24 - @@ -31,8 +31,6 @@ #include "ber.h" -#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b)) - #define BER_TYPE_CONSTRUCTED 0x20/* otherwise primitive */ #define BER_TYPE_SINGLE_MAX30 #define BER_TAG_MASK 0x1f @@ -48,7 +46,6 @@ static ssize_tget_id(struct ber *b, uns int *cstruct); static ssize_t get_len(struct ber *b, ssize_t *len); static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm); -static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes); static ssize_t ber_getc(struct ber *b, u_char *c); static ssize_t ber_read(struct ber *ber, void *buf, size_t len); @@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct return totlen; } -static ssize_t -ber_readbuf(struct ber *b, void *buf, size_t nbytes) -{ - size_t sz, len; - - if (b->br_rbuf == NULL) - return -1; - - sz = b->br_rend - b->br_rptr; - len = MINIMUM(nbytes, sz); - if (len == 0) { - errno = ECANCELED; - return (-1);/* end of buffer and parser wants more data */ - } - - bcopy(b->br_rptr, buf, len); - b->br_rptr += len; - b->br_offs += len; - - return (len); -} - void ber_set_readbuf(struct ber *b, void *buf, size_t len) { @@ -1269,23 +1244,28 @@ ber_free(struct ber *b) static ssize_t ber_getc(struct ber *b, u_char *c) { - return ber_readbuf(b, c, 1); + return ber_read(b, c, 1); } static ssize_t ber_read(struct ber *ber, void *buf, size_t len) { - u_char *b = buf; - ssize_t r, remain = len; + size_t sz; - while (remain > 0) { - r = ber_readbuf(ber, b, remain); - if (r == -1) - return -1; - b += r; - remain -= r; + if (ber->br_rbuf == NULL) + return -1; + + sz = ber->br_rend - ber->br_rptr; + if (len > sz) { + errno = ECANCELED; + return -1; /* parser wants more data than available */ } - return (b - (u_char *)buf); + + bcopy(ber->br_rptr, buf, len); + ber->br_rptr += len; + ber->br_offs += len; + + return len; } int Index: usr.sbin/ldapd/ber.c === RCS file: /d/cvs/src/usr.sbin/ldapd/ber.c,v retrieving revision 1.21 diff -u -p -r1.21 ber.c --- usr.sbin/ldapd/ber.c4 Jul 2018 15:21:24 - 1.21 +++ usr.sbin/ldapd/ber.c6 Jul 2018 11:47:55 - @@ -31,8 +31,6 @@ #include "ber.h" -#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b)) - #define BER_TYPE_CONSTRUCTED 0x20/* otherwise primitive */ #define BER_TYPE_SINGLE_MAX30 #define BER_TAG_MASK 0x1f @@ -48,7 +46,6 @@ static ssize_tget_id(struct ber *b, uns int *cstruct); static ssize_t get_len(struct ber *b, ssize_t *len); static ssize_t ber_read_element(struct ber *ber, struct ber_element *elm); -static ssize_t ber_readbuf(struct ber *b, void *buf, size_t nbytes); static ssize_t ber_getc(struct ber *b, u_char *c); static ssize_t ber_read(struct ber *ber, void *buf, size_t len); @@ -1208,28 +1205,6 @@ ber_read_element(struct ber *ber, struct return totlen; }