Re: tcp keep-alives sent without timestamps
On Tue, Apr 14 2015 20:40:58 +0200, Mike Belopuhov wrote: According to 3.2 in RFC 7323: Once TSopt has been successfully negotiated, that is both SYN and SYN,ACK contain TSopt, the TSopt MUST be sent in every non-RST segment for the duration of the connection, and SHOULD be sent in an RST segment (see Section 5.2 for details). The TCP SHOULD remember this state by setting a flag, referred to as Snd.TS.OK, to one. If a non-RST segment is received without a TSopt, a TCP SHOULD silently drop the segment. A TCP MUST NOT abort a TCP connection because any segment lacks an expected TSopt. Thank you, I somehow missed the existence of this RFC. I had a stab at adding timestamp support to tcp_respond but couldn't test yet. If you feel like giving it a try, please be my guest. With your patch, I confirm that timestamps are present on keep-alive messages. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: paste(1): default to stdin if no files given
On Sat, Dec 30 2017 19:41:24 +0100, Klemens Nanni wrote:
> On Fri, Dec 29, 2017 at 03:55:10PM +0200, Lauri Tirkkonen wrote:
> > Currently paste(1) silently does nothing if it's given no file
> > arguments:
> >
> > % printf 'hello\nworld\n'|paste
> > %
> That's a bug, FWIW, FreeBSD has it fixed.
>
> > I often do things like 'ps -p $(pgrep sh | paste -sd,)' and forget the
> > "-" argument. Some other systems (gnu coreutils, illumos, perhaps more)
> > default to reading from stdin if no files are given; I think that makes
> > sense, so diff to do that below.
> This would invalidate the STANDARDS section as it breaks POSIX.1-2008
> compliance:
>
> STDIN
> The standard input shall be used only if one or more file
> operands is '-'. See the INPUT FILES section.
>
> INPUT FILES
> The input files shall be text files, except that line
> lengths shall be unlimited.
Perhaps. I did read what POSIX said before submitting the patch, but I
feel it's less useful to error out than to use stdin here. But I'm ok
with either, the real problem is silent failure.
> While your diff works, it's missing the respective usage() and manual
> bits.
Ok, here you go.
Index: paste.1
===
RCS file: /cvs/src/usr.bin/paste/paste.1,v
retrieving revision 1.15
diff -u -p -r1.15 paste.1
--- paste.1 28 Jun 2017 14:49:26 - 1.15
+++ paste.1 30 Dec 2017 20:22:29 -
@@ -43,7 +43,7 @@
.Nm paste
.Op Fl s
.Op Fl d Ar list
-.Ar
+.Op Ar
.Sh DESCRIPTION
The
.Nm paste
@@ -107,6 +107,8 @@ is specified for one or more of the inpu
input is used; standard input is read one line at a time, circularly,
for each instance of
.Dq - .
+.Pp
+If no files are specified, only the standard input is used.
.Sh EXIT STATUS
.Ex -std paste
.Sh EXAMPLES
Index: paste.c
===
RCS file: /cvs/src/usr.bin/paste/paste.c,v
retrieving revision 1.22
diff -u -p -r1.22 paste.c
--- paste.c 9 Dec 2015 19:39:10 - 1.22
+++ paste.c 30 Dec 2017 20:22:29 -
@@ -56,6 +56,7 @@ main(int argc, char *argv[])
extern char *optarg;
extern int optind;
int ch, seq;
+ char **files = (char *[]){ "-", NULL };
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
@@ -77,15 +78,18 @@ main(int argc, char *argv[])
argc -= optind;
argv += optind;
+ if (argc)
+ files = argv;
+
if (!delim) {
delimcnt = 1;
delim = "\t";
}
if (seq)
- sequential(argv);
+ sequential(files);
else
- parallel(argv);
+ parallel(files);
exit(0);
}
@@ -251,7 +255,7 @@ void
usage(void)
{
extern char *__progname;
- (void)fprintf(stderr, "usage: %s [-s] [-d list] file ...\n",
+ (void)fprintf(stderr, "usage: %s [-s] [-d list] [file ...]\n",
__progname);
exit(1);
}
--
Lauri Tirkkonen | lotheac @ IRCnet
paste(1): default to stdin if no files given
Currently paste(1) silently does nothing if it's given no file
arguments:
% printf 'hello\nworld\n'|paste
%
I often do things like 'ps -p $(pgrep sh | paste -sd,)' and forget the
"-" argument. Some other systems (gnu coreutils, illumos, perhaps more)
default to reading from stdin if no files are given; I think that makes
sense, so diff to do that below.
Index: paste.c
===
RCS file: /cvs/src/usr.bin/paste/paste.c,v
retrieving revision 1.22
diff -u -p -r1.22 paste.c
--- paste.c 9 Dec 2015 19:39:10 - 1.22
+++ paste.c 29 Dec 2017 13:47:50 -
@@ -56,6 +56,7 @@ main(int argc, char *argv[])
extern char *optarg;
extern int optind;
int ch, seq;
+ char **files = (char *[]){ "-", NULL };
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
@@ -77,15 +78,18 @@ main(int argc, char *argv[])
argc -= optind;
argv += optind;
+ if (argc)
+ files = argv;
+
if (!delim) {
delimcnt = 1;
delim = "\t";
}
if (seq)
- sequential(argv);
+ sequential(files);
else
- parallel(argv);
+ parallel(files);
exit(0);
}
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: update Mesa to 17.2.6
eldrm0: 1366x768, 32bpp wsdisplay0 at inteldrm0 mux 1: console (std, vt100 emulation) wsdisplay0: screen 1-5 added (std, vt100 emulation) "Intel 6 Series MEI" rev 0x04 at pci0 dev 22 function 0 not configured em0 at pci0 dev 25 function 0 "Intel 82579LM" rev 0x04: msi, address f0:de:f1:bc:2d:42 ehci0 at pci0 dev 26 function 0 "Intel 6 Series USB" rev 0x04: apic 2 int 16 usb0 at ehci0: USB revision 2.0 uhub0 at usb0 configuration 1 interface 0 "Intel EHCI root hub" rev 2.00/1.00 addr 1 azalia0 at pci0 dev 27 function 0 "Intel 6 Series HD Audio" rev 0x04: msi azalia0: codecs: Conexant CX20590, Intel/0x2805, using Conexant CX20590 audio0 at azalia0 ppb0 at pci0 dev 28 function 0 "Intel 6 Series PCIE" rev 0xb4: msi pci1 at ppb0 bus 2 ppb1 at pci0 dev 28 function 1 "Intel 6 Series PCIE" rev 0xb4: msi pci2 at ppb1 bus 3 iwn0 at pci2 dev 0 function 0 "Intel Centrino Advanced-N 6205" rev 0x34: msi, MIMO 2T2R, MoW, address 10:0b:a9:49:23:68 ppb2 at pci0 dev 28 function 4 "Intel 6 Series PCIE" rev 0xb4: msi pci3 at ppb2 bus 13 sdhc0 at pci3 dev 0 function 0 "Ricoh 5U822 SD/MMC" rev 0x07: apic 2 int 16 sdhc0: SDHC 3.0, 50 MHz base clock sdmmc0 at sdhc0: 4-bit, sd high-speed, mmc high-speed, dma ehci1 at pci0 dev 29 function 0 "Intel 6 Series USB" rev 0x04: apic 2 int 23 usb1 at ehci1: USB revision 2.0 uhub1 at usb1 configuration 1 interface 0 "Intel EHCI root hub" rev 2.00/1.00 addr 1 pcib0 at pci0 dev 31 function 0 "Intel QM67 LPC" rev 0x04 ahci0 at pci0 dev 31 function 2 "Intel 6 Series AHCI" rev 0x04: msi, AHCI 1.3 ahci0: port 2: 3.0Gb/s scsibus1 at ahci0: 32 targets sd0 at scsibus1 targ 2 lun 0: <ATA, INTEL SSDMCEAC03, LLLi> SCSI3 0/direct fixed naa.55cd2e404bc780d4 sd0: 28626MB, 512 bytes/sector, 58626288 sectors, thin ichiic0 at pci0 dev 31 function 3 "Intel 6 Series SMBus" rev 0x04: apic 2 int 18 iic0 at ichiic0 spdmem0 at iic0 addr 0x50: 4GB DDR3 SDRAM PC3-10600 SO-DIMM spdmem1 at iic0 addr 0x51: 4GB DDR3 SDRAM PC3-10600 SO-DIMM isa0 at pcib0 isadma0 at isa0 pckbc0 at isa0 port 0x60/5 irq 1 irq 12 pckbd0 at pckbc0 (kbd slot) wskbd0 at pckbd0: console keyboard, using wsdisplay0 pms0 at pckbc0 (aux slot) wsmouse0 at pms0 mux 0 wsmouse1 at pms0 mux 0 pms0: Synaptics clickpad, firmware 8.0, 0x1e2b1 0x940300 pcppi0 at isa0 port 0x61 spkr0 at pcppi0 aps0 at isa0 port 0x1600/31 vmm0 at mainbus0: VMX/EPT uhub2 at uhub0 port 1 configuration 1 interface 0 "Intel Rate Matching Hub" rev 2.00/0.00 addr 2 uhub3 at uhub1 port 1 configuration 1 interface 0 "Intel Rate Matching Hub" rev 2.00/0.00 addr 2 vscsi0 at root scsibus2 at vscsi0: 256 targets softraid0 at root scsibus3 at softraid0: 256 targets sd1 at scsibus3 targ 1 lun 0: <OPENBSD, SR CRYPTO, 006> SCSI2 0/direct fixed sd1: 28623MB, 512 bytes/sector, 58620593 sectors root on sd1a (1e3f2d2fd0a15e70.a) swap on sd1b dump on sd1b -- Lauri Tirkkonen | lotheac @ IRCnet
Re: update Mesa to 17.2.6
Hi, On Fri, Jan 05 2018 10:09:08 +1100, Jonathan Gray wrote: > Does switching to the intel driver with xorg.conf or the below > diff change anything? Yes, with the intel driver the corruption is gone. However, I updated my work Haswell laptop (x240; cpu0: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 1995.72 MHz; inteldrm0 at pci0 dev 2 function 0 "Intel HD Graphics" rev 0x0b) and it does exhibit similar corruption; it's more difficult to notice as it seems to clear away faster but it's there, at least if you know to look for it (I can provide a shaky-cam video if you like). It too goes away when switching to intel driver, so perhaps your diff isn't quite sufficient (although granted, with Haswell the effect is so much less noticeable, it may not be a problem). -- Lauri Tirkkonen | lotheac @ IRCnet
Re: [bugfix] xterm(1) needs "cpath" pledge(2)
On Sun, Jul 29 2018 07:28:19 +0200, Sebastien Marie wrote: > personally, I like to know that xterm is unable to create a file. At least this feature will create files (accessible through the context menu through ctrl-left click): Print-All Immediately (resource print-immediate) Invokes the print-immediate action, sending the text of the current window directly to a file, as specified by the printFileImmediate, printModeImmediate and printOptsImmediate resources. and possibly the other "Commands for capturing output" listed in the manpage. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: md5: convert from fgetln(3) to getline(3)
if (!found)
warnx("%s: no properly formatted checksum lines found", file);
- free(lbuf);
if (sel_found != NULL) {
/*
* Mark found files by setting them to NULL so that we can
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: md5: convert from fgetln(3) to getline(3)
On Tue, Aug 14 2018 14:29:30 -0500, Scott Cheloha wrote:
> On Tue, Aug 14, 2018 at 08:55:14PM +0300, Lauri Tirkkonen wrote:
> > Hi,
> >
> > On Tue, Aug 14 2018 12:08:54 -0500, Scott Cheloha wrote:
> > > > + len = (size_t)linelen;
> > > > + if (line[len - 1] == '\n')
> > > > + line[len - 1] = '\0';
> > >
> > > You can just use linelen directly and skip the assignment.
> >
> > I did it this way because this bit later
> >
> > if (base64)
> > cmp = strncmp(checksum, digest, len);
> >
> > could have been reached without assigning len (if the "could be GNU form"
> > branch is taken). It looks like checksum and digest will always be null
> > temrinated, but 'len' may be reduced to something less than linelen
> > (eg. in the case that the line starts with whitespace). Maybe there's
> > some reason to not use strcmp here?
>
> base64 is only set in the BSD form case, and in that case len is set to
> the length of checksum before the comparison.
Thanks, I neglected to check when base64 is set. Your logic makes sense
in that case, so I applied your suggestion below.
> > But shouldn't we check *before* calling free to ensure errno is not
> > clobbered? (although, getline.3's example code certainly does err(1,
> > "getline"); after free()...)
>
> free(3) doesn't set errno. At least, not portably. I definitely wouldn't
> worry about the possibility of there being a malloc implementation that
> clobbers errno.
>
> The ordering (check or free first) doesn't matter in this case because you
> aren't returning from the function early if you have an error on listfp,
> but I think the intent of the example in the manpage is to prevent leaks
> in such cases, e.g.:
>
> if (ferror(listfp)) {
> warn("getline");
> return 1;
> }
> free(line); /* leak */
>
> I would just stick to the manpage example in this case and keep the free(3)
> as close as possible to the allocation (getline).
Thanks for the explanation. Maybe I'm too used to not trusting the
platform to do things that make sense (in the past I remember thinking
that even a pathological ferror could modify errno; its manpage says it
won't though so I could trust that).
> > + while ((linelen = getline(, , listfp)) != -1) {
> > + char *tmpline = line;
>
> You're already modifying the stack variable declarations up above, so
> declare tmpline alongside the other char pointers.
fixed.
> > + if (ferror(listfp))
> > + warn("%s: getline", file);
> > + free(line);
>
> Set error = 1 in the ferror case, too.
fixed.
diff --git a/bin/md5/md5.c b/bin/md5/md5.c
index 84adbcf0989..1abf28cfa16 100644
--- a/bin/md5/md5.c
+++ b/bin/md5/md5.c
@@ -549,11 +549,11 @@ digest_filelist(const char *file, struct hash_function
*defhash, int selcount,
int found, base64, error, cmp, i;
size_t algorithm_max, algorithm_min;
const char *algorithm;
- char *filename, *checksum, *buf, *p;
+ char *filename, *checksum, *line, *p, *tmpline;
char digest[MAX_DIGEST_LEN + 1];
- char *lbuf = NULL;
+ ssize_t linelen;
FILE *listfp, *fp;
- size_t len, nread;
+ size_t len, linesize, nread;
int *sel_found = NULL;
u_char data[32 * 1024];
union ANY_CTX context;
@@ -580,20 +580,15 @@ digest_filelist(const char *file, struct hash_function
*defhash, int selcount,
}
error = found = 0;
- while ((buf = fgetln(listfp, ))) {
+ line = NULL;
+ linesize = 0;
+ while ((linelen = getline(, , listfp)) != -1) {
+ tmpline = line;
base64 = 0;
- if (buf[len - 1] == '\n')
- buf[len - 1] = '\0';
- else {
- if ((lbuf = malloc(len + 1)) == NULL)
- err(1, NULL);
-
- (void)memcpy(lbuf, buf, len);
- lbuf[len] = '\0';
- buf = lbuf;
- }
- while (isspace((unsigned char)*buf))
- buf++;
+ if (line[linelen - 1] == '\n')
+ line[linelen - 1] = '\0';
+ while (isspace((unsigned char)*tmpline))
+ tmpline++;
/*
* Crack the line into an algorithm, filename, and checksum.
@@ -603,11 +598,11 @@ digest_filelist(const char *file, struct hash_function
*defhash, int selcount,
* Fallback on GNU form:
* CHECKSUM FILENAME
*/
- p = strchr(buf,
spamd.conf.5: document https
Hi, spamd.conf supports method=https, but this is not documented. So document it and also modify the example config to default to it. diff --git a/etc/mail/spamd.conf b/etc/mail/spamd.conf index b080300d952..1361e864742 100644 --- a/etc/mail/spamd.conf +++ b/etc/mail/spamd.conf @@ -27,7 +27,7 @@ nixspam:\ :black:\ :msg="Your address %A is in the nixspam list\n\ See http://www.heise.de/ix/nixspam/dnsbl_en/ for details":\ - :method=http:\ + :method=https:\ :file=www.openbsd.org/spamd/nixspam.gz # An example of a list containing addresses which should not talk to spamd. diff --git a/share/man/man5/spamd.conf.5 b/share/man/man5/spamd.conf.5 index 27899d50a80..5dd651db3a4 100644 --- a/share/man/man5/spamd.conf.5 +++ b/share/man/man5/spamd.conf.5 @@ -57,7 +57,7 @@ nixspam:\e :black:\e :msg="Your address %A is in the nixspam list\en\e See http://www.heise.de/ix/nixspam/dnsbl_en/ for details":\e - :method=http:\e + :method=https:\e :file=www.openbsd.org/spamd/nixspam.gz override:\e @@ -123,12 +123,14 @@ capability entries. specifies the method by which to retrieve a file containing a list of addresses and may be one of .Ar http , +.Ar https , .Ar ftp , .Ar file , or .Ar exec . The .Ar http , +.Ar https , .Ar ftp , and .Ar file -- Lauri Tirkkonen | lotheac @ IRCnet
paste(1): use getline instead of fgetln
In the same vein as my previous diff for join(1), make paste(1) use
getline instead of fgetln.
diff --git a/usr.bin/paste/paste.c b/usr.bin/paste/paste.c
index fd51bfa0a4c..185fb6d1535 100644
--- a/usr.bin/paste/paste.c
+++ b/usr.bin/paste/paste.c
@@ -107,8 +107,8 @@ parallel(char **argv)
int cnt;
char ch, *p;
int opencnt, output;
- char *buf, *lbuf;
- size_t len;
+ char *line;
+ size_t len, linesize;
for (cnt = 0; (p = *argv); ++argv, ++cnt) {
if (!(lp = malloc(sizeof(struct list
@@ -123,17 +123,21 @@ parallel(char **argv)
SIMPLEQ_INSERT_TAIL(, lp, entries);
}
+ line = NULL;
+ linesize = 0;
+
for (opencnt = cnt; opencnt;) {
output = 0;
SIMPLEQ_FOREACH(lp, , entries) {
- lbuf = NULL;
if (!lp->fp) {
if (output && lp->cnt &&
(ch = delim[(lp->cnt - 1) % delimcnt]))
putchar(ch);
continue;
}
- if (!(buf = fgetln(lp->fp, ))) {
+ if ((len = getline(, , lp->fp)) == -1) {
+ if (ferror(lp->fp))
+ err(1, "getline");
if (!--opencnt)
break;
if (lp->fp != stdin)
@@ -144,15 +148,8 @@ parallel(char **argv)
putchar(ch);
continue;
}
- if (buf[len - 1] == '\n')
- buf[len - 1] = '\0';
- else {
- if ((lbuf = malloc(len + 1)) == NULL)
- err(1, "malloc");
- memcpy(lbuf, buf, len);
- lbuf[len] = '\0';
- buf = lbuf;
- }
+ if (line[len - 1] == '\n')
+ line[len - 1] = '\0';
/*
* make sure that we don't print any delimiters
* unless there's a non-empty file.
@@ -164,13 +161,12 @@ parallel(char **argv)
putchar(ch);
} else if ((ch = delim[(lp->cnt - 1) % delimcnt]))
putchar(ch);
- (void)printf("%s", buf);
- if (lbuf)
- free(lbuf);
+ (void)printf("%s", line);
}
if (output)
putchar('\n');
}
+ free(line);
}
void
@@ -179,30 +175,27 @@ sequential(char **argv)
FILE *fp;
int cnt;
char ch, *p, *dp;
- char *buf, *lbuf;
- size_t len;
+ char *line;
+ size_t len, linesize;
+ line = NULL;
+ linesize = 0;
for (; (p = *argv); ++argv) {
- lbuf = NULL;
if (p[0] == '-' && !p[1])
fp = stdin;
else if (!(fp = fopen(p, "r"))) {
warn("%s", p);
continue;
}
- if ((buf = fgetln(fp, ))) {
+ len = getline(, , fp);
+ if (len == -1 && ferror(fp))
+ err(1, "getline");
+ else if (len != -1) {
for (cnt = 0, dp = delim;;) {
- if (buf[len - 1] == '\n')
- buf[len - 1] = '\0';
- else {
- if ((lbuf = malloc(len + 1)) == NULL)
- err(1, "malloc");
- memcpy(lbuf, buf, len);
- lbuf[len] = '\0';
- buf = lbuf;
- }
- (void)printf("%s", buf);
- if (!(buf = fgetln(fp, )))
+ if (line[len - 1] == '\n')
+ line[len - 1] = '\0';
+ (void)printf("%s", line);
+ if ((len = getline(, , fp)) == -1)
break;
if ((ch = *dp++))
putchar(ch);
@@ -215,8 +208,8 @@ sequential(char **argv)
}
join(1): use getline instead of fgetln
Hi,
while porting join(1) to Unleashed OS (which does not have fgetln(3)) I
came up with the following. Since the fgetln man page advises against
using it, I thought OpenBSD might want this diff too.
diff --git a/usr.bin/join/join.c b/usr.bin/join/join.c
index 3049a423196..ac62cf83cd1 100644
--- a/usr.bin/join/join.c
+++ b/usr.bin/join/join.c
@@ -298,9 +298,9 @@ void
slurpit(INPUT *F)
{
LINE *lp, *lastlp, tmp;
- size_t len;
+ size_t len, linesize;
u_long cnt;
- char *bp, *fieldp;
+ char *bp, *fieldp, *line;
long fpos;
/*
* Read all of the lines from an input file that have the same
@@ -308,6 +308,8 @@ slurpit(INPUT *F)
*/
F->setcnt = 0;
+ line = NULL;
+ linesize = 0;
for (lastlp = NULL; ; ++F->setcnt, lastlp = lp) {
/*
* If we're out of space to hold line structures, allocate
@@ -343,8 +345,10 @@ slurpit(INPUT *F)
F->pushbool = 0;
continue;
}
- if ((bp = fgetln(F->fp, )) == NULL)
+ if ((len = getline(, , F->fp)) == -1) {
+ free(line);
return;
+ }
/*
* we depend on knowing on what field we are, one safe way is
* the file position.
@@ -360,17 +364,15 @@ slurpit(INPUT *F)
lp->linealloc = newsize;
}
F->setusedc++;
- memmove(lp->line, bp, len);
+ memmove(lp->line, line, len);
lp->fpos = fpos;
/* Replace trailing newline, if it exists. */
- if (bp[len - 1] == '\n')
+ if (lp->line[len - 1] == '\n')
lp->line[len - 1] = '\0';
- else
- lp->line[len] = '\0';
- bp = lp->line;
/* Split the line into fields, allocate space as necessary. */
lp->fieldcnt = 0;
+ bp = lp->line;
while ((fieldp = strsep(, tabchar)) != NULL) {
if (spans && *fieldp == '\0')
continue;
@@ -393,6 +395,7 @@ slurpit(INPUT *F)
break;
}
}
+ free(line);
}
int
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: join(1): use getline instead of fgetln
On Tue, Jul 17 2018 13:21:31 -0600, Todd C. Miller wrote:
> > @@ -360,17 +364,15 @@ slurpit(INPUT *F)
> > lp->linealloc = newsize;
> > }
> > F->setusedc++;
> > - memmove(lp->line, bp, len);
> > + memmove(lp->line, line, len);
> > lp->fpos = fpos;
> > /* Replace trailing newline, if it exists. */
> > - if (bp[len - 1] == '\n')
> > + if (lp->line[len - 1] == '\n')
> > lp->line[len - 1] = '\0';
> > - else
> > - lp->line[len] = '\0';
> > - bp = lp->line;
>
> It probably makes more sense to do the newline check (and decrement
> len if one is present) before newsize is computed. Then you would
> need to unconditionally NUL-terminate lp->line.
Like below you mean? I'm unsure if I like it -- I wonder if it'd be
possible to use strlcpy instead of memmove here to keep things strings
and not have to deal with termination at all. But that goes beyond what
I set out to do with this diff :)
Anyway, updated as you requested so feel free to commit, or not.
diff --git a/usr.bin/join/join.c b/usr.bin/join/join.c
index 3049a423196..0ac19e5be2b 100644
--- a/usr.bin/join/join.c
+++ b/usr.bin/join/join.c
@@ -298,9 +298,9 @@ void
slurpit(INPUT *F)
{
LINE *lp, *lastlp, tmp;
- size_t len;
+ size_t len, linesize;
u_long cnt;
- char *bp, *fieldp;
+ char *bp, *fieldp, *line;
long fpos;
/*
* Read all of the lines from an input file that have the same
@@ -308,6 +308,8 @@ slurpit(INPUT *F)
*/
F->setcnt = 0;
+ line = NULL;
+ linesize = 0;
for (lastlp = NULL; ; ++F->setcnt, lastlp = lp) {
/*
* If we're out of space to hold line structures, allocate
@@ -343,8 +345,12 @@ slurpit(INPUT *F)
F->pushbool = 0;
continue;
}
- if ((bp = fgetln(F->fp, )) == NULL)
+ if ((len = getline(, , F->fp)) == -1) {
+ free(line);
return;
+ }
+ if (line[len - 1] == '\n')
+ len--;
/*
* we depend on knowing on what field we are, one safe way is
* the file position.
@@ -360,17 +366,13 @@ slurpit(INPUT *F)
lp->linealloc = newsize;
}
F->setusedc++;
- memmove(lp->line, bp, len);
+ memmove(lp->line, line, len);
lp->fpos = fpos;
- /* Replace trailing newline, if it exists. */
- if (bp[len - 1] == '\n')
- lp->line[len - 1] = '\0';
- else
- lp->line[len] = '\0';
- bp = lp->line;
+ lp->line[len - 1] = '\0';
/* Split the line into fields, allocate space as necessary. */
lp->fieldcnt = 0;
+ bp = lp->line;
while ((fieldp = strsep(, tabchar)) != NULL) {
if (spans && *fieldp == '\0')
continue;
@@ -393,6 +395,7 @@ slurpit(INPUT *F)
break;
}
}
+ free(line);
}
int
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: join(1): use getline instead of fgetln
On Tue, Jul 17 2018 13:38:14 -0600, Todd C. Miller wrote: > On Tue, 17 Jul 2018 13:21:31 -0600, "Todd C. Miller" wrote: > > > It probably makes more sense to do the newline check (and decrement > > len if one is present) before newsize is computed. Then you would > > need to unconditionally NUL-terminate lp->line. > > Perhaps something like this. you beat me to it, and yours is better (I decremented len before calculating fpos without thinking) :) -- Lauri Tirkkonen | lotheac @ IRCnet
Re: [patch] smtpd: missing space for "from local" with -T rules
On Wed, Jun 20 2018 12:19:43 +0300, Lauri Tirkkonen wrote:
> "from local" rules are missing a space when printed out with -T rules.
>
> diff --git a/usr.sbin/smtpd/to.c b/usr.sbin/smtpd/to.c
> index e1d399d74f2..27d5321408f 100644
> --- a/usr.sbin/smtpd/to.c
> +++ b/usr.sbin/smtpd/to.c
> @@ -461,7 +461,7 @@ rule_to_text(struct rule *r)
> if (strcmp(r->table_from, "") == 0)
> (void)strlcat(buf, "from any ", sizeof buf);
> else if (strcmp(r->table_from, "") == 0)
> - (void)strlcat(buf, "from local", sizeof buf);
> + (void)strlcat(buf, "from local ", sizeof buf);
> else {
> (void)strlcat(buf, "from src ", sizeof buf);
> (void)strlcat(buf, r->table_from, sizeof buf);
ping?
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: paste(1): use getline instead of fgetln
On Tue, Jul 17 2018 21:14:29 +0300, Lauri Tirkkonen wrote: > In the same vein as my previous diff for join(1), make paste(1) use > getline instead of fgetln. ping. the join(1) fix was committed but this one still needs attention. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: md5: convert from fgetln(3) to getline(3)
On Thu, Aug 23 2018 11:24:54 -0500, Scott Cheloha wrote: > On Tue, Aug 14, 2018 at 03:11:47PM -0500, Scott Cheloha wrote: > > This patch is ok cheloha@ and I can commit if someone else > > is ok, too. > > 1 week bump. Anyone else ok? no takers? -- Lauri Tirkkonen | lotheac @ IRCnet
Re: Minor cal(1) man diff
On Tue, Sep 04 2018 14:08:23 +0200, Martijn van Duren wrote: > Month is only taken into account if year is also specified. > This is also how POSIX notates the format.[0] that synopsis doesn't match this usage though: Alternatively, a single parameter may be given specifying the name or abbreviated name of a month: in that case a calendar is displayed for that month of the current year. -- Lauri Tirkkonen | lotheac @ IRCnet
[patch] smtpd: missing space for "from local" with -T rules
"from local" rules are missing a space when printed out with -T rules.
diff --git a/usr.sbin/smtpd/to.c b/usr.sbin/smtpd/to.c
index e1d399d74f2..27d5321408f 100644
--- a/usr.sbin/smtpd/to.c
+++ b/usr.sbin/smtpd/to.c
@@ -461,7 +461,7 @@ rule_to_text(struct rule *r)
if (strcmp(r->table_from, "") == 0)
(void)strlcat(buf, "from any ", sizeof buf);
else if (strcmp(r->table_from, "") == 0)
- (void)strlcat(buf, "from local", sizeof buf);
+ (void)strlcat(buf, "from local ", sizeof buf);
else {
(void)strlcat(buf, "from src ", sizeof buf);
(void)strlcat(buf, r->table_from, sizeof buf);
--
Lauri Tirkkonen | lotheac @ IRCnet
grep: convert fgetln to getline
Hi, another simple diff converting fgetln usage to getline.
I also considered converting grep_fgetln to grep_getline, but that would
mean that for FILE_MMAP we'd have to copy the string. So this diff keeps
the grep_fgetln interface as is, but avoids using fgetln from libc (and
adds error handling for FILE_STDIO).
diff --git a/usr.bin/grep/file.c b/usr.bin/grep/file.c
index 4b3c689e4ab..87c49dd5cc0 100644
--- a/usr.bin/grep/file.c
+++ b/usr.bin/grep/file.c
@@ -34,10 +34,8 @@
#include "grep.h"
static char fname[PATH_MAX];
-#ifndef NOZ
static char*lnbuf;
-static size_t lnbuflen;
-#endif
+static size_t lnbufsize;
#define FILE_STDIO 0
#define FILE_MMAP 1
@@ -73,9 +71,9 @@ gzfgetln(gzFile *f, size_t *len)
else
errx(2, "%s: %s", fname, gzerrstr);
}
- if (n >= lnbuflen) {
- lnbuflen *= 2;
- lnbuf = grep_realloc(lnbuf, ++lnbuflen);
+ if (n >= lnbufsize) {
+ lnbufsize *= 2;
+ lnbuf = grep_realloc(lnbuf, ++lnbufsize);
}
if (c == '\n')
break;
@@ -182,7 +180,13 @@ grep_fgetln(file_t *f, size_t *l)
{
switch (f->type) {
case FILE_STDIO:
- return fgetln(f->f, l);
+ if ((*l = getline(, , f->f)) == -1) {
+ if (ferror(f->f))
+ err(2, "%s: getline", fname);
+ else
+ return NULL;
+ }
+ return lnbuf;
#ifndef SMALL
case FILE_MMAP:
return mmfgetln(f->mmf, l);
diff --git a/usr.bin/grep/grep.c b/usr.bin/grep/grep.c
index 913cc97a0f3..cfac24b12aa 100644
--- a/usr.bin/grep/grep.c
+++ b/usr.bin/grep/grep.c
@@ -224,15 +224,19 @@ read_patterns(const char *fn)
{
FILE *f;
char *line;
- size_t len;
+ ssize_t len;
+ size_t linesize;
if ((f = fopen(fn, "r")) == NULL)
err(2, "%s", fn);
- while ((line = fgetln(f, )) != NULL)
+ line = NULL;
+ linesize = 0;
+ while ((len = getline(, , f)) != -1)
add_pattern(line, *line == '\n' ? 0 : len);
if (ferror(f))
err(2, "%s", fn);
fclose(f);
+ free(line);
}
int
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: net80211: don't drop unencrypted data frames with no data
On Mon, Jan 14 2019 14:23:44 +0100, Stefan Sperling wrote:
> Thank you for tracking this problem down.
>
> Your diff is not correct. This part introduces a memory leak because
> the mbuf is not going to be freed anymore:
>
> > @@ -411,6 +412,12 @@ ieee80211_input(struct ifnet *ifp, struct mbuf *m,
> > struct ieee80211_node *ni,
> > /* protection is on for Rx */
> > if (!(rxi->rxi_flags & IEEE80211_RXI_HWDEC)) {
> > if (!(wh->i_fc[1] & IEEE80211_FC1_PROTECTED)) {
> > + /*
> > +* 9.2.4.1.9 frames without data are
> > +* not protected
> > +*/
> > + if (!hasdata)
> > + return;
>
> This should say 'goto out' instead of 'return'.
thanks. This is why I placed the disclaimer in my mail, I didn't fully
understand what I was doing :) I just thought that since normally we
just return without an explicit free after decryption & decapsulation, I
should do the same thing; I assumed the caller would handle it. But I
guess not :)
> I'd like to propose a more general solution:
>
> The diff below improves naming of so far unused frame subtype constants
> and makes it more obvious which subtypes do not carry data, it attributes
> "no data" frames to a more suitable stat counter, and it drops them early.
>
> Index: ieee80211.h
> ===
> RCS file: /cvs/src/sys/net80211/ieee80211.h,v
> retrieving revision 1.60
> diff -u -p -r1.60 ieee80211.h
> --- ieee80211.h 2 Jul 2017 14:48:19 - 1.60
> +++ ieee80211.h 14 Jan 2019 13:05:08 -
> @@ -140,13 +140,13 @@ struct ieee80211_htframe_addr4 {/* 11n
> #define IEEE80211_FC0_SUBTYPE_CF_END_ACK0xf0
> /* for TYPE_DATA (bit combination) */
> #define IEEE80211_FC0_SUBTYPE_DATA 0x00
> -#define IEEE80211_FC0_SUBTYPE_CF_ACK0x10
> -#define IEEE80211_FC0_SUBTYPE_CF_POLL 0x20
> -#define IEEE80211_FC0_SUBTYPE_CF_ACPL 0x30
> +#define IEEE80211_FC0_SUBTYPE_DATA_CF_ACK 0x10
> +#define IEEE80211_FC0_SUBTYPE_DATA_CF_POLL 0x20
> +#define IEEE80211_FC0_SUBTYPE_CF_ACK_POLL 0x30
> #define IEEE80211_FC0_SUBTYPE_NODATA0x40
> -#define IEEE80211_FC0_SUBTYPE_CFACK 0x50
> -#define IEEE80211_FC0_SUBTYPE_CFPOLL0x60
> -#define IEEE80211_FC0_SUBTYPE_CF_ACK_CF_ACK 0x70
> +#define IEEE80211_FC0_SUBTYPE_CF_ACK_NODATA 0x50
> +#define IEEE80211_FC0_SUBTYPE_CF_POLL_NODATA0x60
> +#define IEEE80211_FC0_SUBTYPE_CF_ACK_CF_POLL0x70
why doesn't SUBTYPE_CF_ACK_CF_POLL have NODATA in the name? it has the
NODATA bit set (ie. & 0x40), and "QoS CF-Ack + CF_Poll (no data)" is
explicitly listed in 9.2.4.1.9.
> #define IEEE80211_FC0_SUBTYPE_QOS 0x80
>
> #define IEEE80211_FC1_DIR_MASK 0x03
> Index: ieee80211_input.c
> ===
> RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
> retrieving revision 1.202
> diff -u -p -r1.202 ieee80211_input.c
> --- ieee80211_input.c 7 Aug 2018 18:13:14 - 1.202
> +++ ieee80211_input.c 14 Jan 2019 13:19:30 -
> @@ -202,6 +202,19 @@ ieee80211_input(struct ifnet *ifp, struc
> goto err;
> }
> }
> +
> + /*
> + * "no data" frames are used for various MAC coordination functions,
> + * particularly in the context of QoS. We do not implement related
> + * features yet so do not process "no data" frames any further.
> + */
> + if (subtype & (IEEE80211_FC0_SUBTYPE_NODATA |
> + IEEE80211_FC0_SUBTYPE_CF_POLL_NODATA |
> + IEEE80211_FC0_SUBTYPE_CF_ACK_NODATA)) {
1) shouldn't we first check that type is data here?
2) isn't this a false positive for subtype ==
IEEE80211_FC0_SUBTYPE_DATA_CF_ACK and subtype ==
IEEE80211_FC0_SUBTYPE_DATA_FC_POLL, since the _NODATA versions are just
the _DATA_ bits ORed with FC0_SUBTYPE_NODATA? I think we should either
check (subtype & IEEE80211_FC0_SUBTYPE_NODATA), or test subtype's
equality to each of the possible NODATA macros.
3) where is IEEE80211_FC0_SUBTYPE_CF_ACK_CF_POLL?
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: net80211: don't drop unencrypted data frames with no data
On Mon, Jan 14 2019 15:34:22 +0100, Stefan Sperling wrote:
> > why doesn't SUBTYPE_CF_ACK_CF_POLL have NODATA in the name? it has the
> > NODATA bit set (ie. & 0x40), and "QoS CF-Ack + CF_Poll (no data)" is
> > explicitly listed in 9.2.4.1.9.
>
> Yes, that was a mistake. See this follow-up patch:
> https://marc.info/?l=openbsd-tech=154747407319373=2
yeah, I saw it right after hitting send. sorry for the noise on that
part, and thanks :)
> > > +
> > > + /*
> > > + * "no data" frames are used for various MAC coordination functions,
> > > + * particularly in the context of QoS. We do not implement related
> > > + * features yet so do not process "no data" frames any further.
> > > + */
> > > + if (subtype & (IEEE80211_FC0_SUBTYPE_NODATA |
> > > + IEEE80211_FC0_SUBTYPE_CF_POLL_NODATA |
> > > + IEEE80211_FC0_SUBTYPE_CF_ACK_NODATA)) {
> >
> > 1) shouldn't we first check that type is data here?
> > 2) isn't this a false positive for subtype ==
> > IEEE80211_FC0_SUBTYPE_DATA_CF_ACK and subtype ==
> > IEEE80211_FC0_SUBTYPE_DATA_FC_POLL, since the _NODATA versions are just
> > the _DATA_ bits ORed with FC0_SUBTYPE_NODATA? I think we should either
> > check (subtype & IEEE80211_FC0_SUBTYPE_NODATA), or test subtype's
> > equality to each of the possible NODATA macros.
> > 3) where is IEEE80211_FC0_SUBTYPE_CF_ACK_CF_POLL?
>
> Indeed, my diff was bad as well. Thanks for spotting these issues.
> I hadn't run this diff yet cause I was still building a new snapshot
> to test it. Could you also test this new version please?
I'm not currently physically near my AP, but the diff looks good and I
can test it later today.
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: grep: convert fgetln to getline
On Mon, Jan 07 2019 23:01:47 +0200, Lauri Tirkkonen wrote: > On Mon, Jan 07 2019 15:41:53 -0500, Ted Unangst wrote: > > Lauri Tirkkonen wrote: > > > On Sun, Jan 06 2019 14:02:16 -0500, Ted Unangst wrote: > > > > Lauri Tirkkonen wrote: > > > > > Hi, another simple diff converting fgetln usage to getline. > > > > > > > > > > I also considered converting grep_fgetln to grep_getline, but that > > > > > would > > > > > mean that for FILE_MMAP we'd have to copy the string. So this diff > > > > > keeps > > > > > the grep_fgetln interface as is, but avoids using fgetln from libc > > > > > (and > > > > > adds error handling for FILE_STDIO). > > > > > > > > this looks good and seems to work. thanks. > > > > > > actually, it doesn't work :) the added error handling catches that > > > directory fd's are being passed into grep_fgetln, causing an error exit > > > with grep -r (or just grep foo /etc/*). I'm working on a second diff to > > > refactor the file handling a bit. > > > > oh, interesting. that's sloppy. can you please fix that first, separately? > > Sure, here it is. ping. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: net80211: don't drop unencrypted data frames with no data
On Mon, Jan 14 2019 16:41:13 +0200, Lauri Tirkkonen wrote: > > Indeed, my diff was bad as well. Thanks for spotting these issues. > > I hadn't run this diff yet cause I was still building a new snapshot > > to test it. Could you also test this new version please? > > I'm not currently physically near my AP, but the diff looks good and I > can test it later today. I've had a chance to test your diff, and it seems to work well, but I have one behavior change I can't explain (haven't looked at packet captures at this point): without this diff applied, or with my own previous diff applied, I can run iperf3 -s on my Android phone and connect to it with UDP iperf3 -c from a wired host connected to the AP, and measure packet loss. With this diff applied I'm unable to connect to the phone's iperf server, and in fact am getting huge packet loss even pinging the phone (and it seems that the times that it responds to pings are correlated with times the phone was actually transmitting, eg. when I hit refresh in the phone browser). Maybe it could be a good thing, perhaps related to aggressive power management on the phone, but I don't know what it is with your diff that could cause this behavior change. Maybe I need to do some packet captures later. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: install(1) could fail due to race
On Wed, Jan 16 2019 11:00:04 +0100, Ingo Schwarze wrote: > Lauri Tirkkonen wrote on Mon, Jan 07, 2019 at 08:13:09PM +0200: > > > Hi, it seems install(1) has a race condition: in create_newfile, it > > first unlinks the target file and then tries to open it with > > O_CREAT|O_EXCL. > > > > Normally this would not be a problem, > > A race condition is almost always a problem and can almost always > cause incorrect behaviour. In this case, *anything* might create > the file in the time window, causing spurious failures, in unusual > scenarios maybe even facilitating DOS attacks. The fact that some > build systems shoot themselves in the foot the way you describe > merely exacerbates the bug. Yes, you're right. > > The below diff essentially removes the -S option > > We clearly cannot delete -S from the user interface. That would break > all kinds of build systems for no benefit. It is even used in base, > for example in bsd.lib.mk. I don't doubt it's used in ports, too, > and having to fix it there would be even more annoying than in base. The second diff I sent removes the documentation, but keeps accepting the option as a no-op just for that reason. I don't see much point in documenting it since you cannot turn it off, but it's your call, of course. > > and makes install always use temp files (ie. -S is always on), > > eliminating the race since rename(2) cannot fail like this. > > I think that is the right thing to do. Even if it happens to > increase resource consumption with softupdates, that is of little > relevance because we advise against using softupdates in the first > place, for reasons of reliability. Also, bugs in one subsystem > must not prevent bugfixing in another. Right, that's why I said I think softupdates is orthogonal. Besides the default mk files already use -S, so there should be no behavior change for building base. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: net80211: don't drop unencrypted data frames with no data
On Tue, Jan 15 2019 10:30:47 +0100, Stefan Sperling wrote: > Turns out we need to allow "no data" frames to pass a bit further into > ieee80211_input() because they carry sequence numbers and because they > are used to toggle power-saving state via IEEE80211_FC1_PWR_MGT in the > frame header. > > I've checked what Linux mac80211 does -- it ignores "no data" frames > for A-MPDU reordering like we already did (good!), but it drops them > right before decryption. So your original diff was actually correct, > apart from the memory leak ;-) oh, heh :) > Does this diff work? You should now be able to see 'no data' frames > on your AP with 'tcpdump -n -i athn0 -y IEEE802_11_RADIO' as well. indeed it does - iperf connection works again and pings aren't lost. and nodata frames are visible with tcpdump. I notice this diff doesn't increment a counter for the nodata frames anymore, but that seems reasonable since we are indeed using them for something, not ignoring them completely. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: grep: convert fgetln to getline
On Mon, Jan 07 2019 23:01:47 +0200, Lauri Tirkkonen wrote: > On Mon, Jan 07 2019 15:41:53 -0500, Ted Unangst wrote: > > Lauri Tirkkonen wrote: > > > On Sun, Jan 06 2019 14:02:16 -0500, Ted Unangst wrote: > > > > Lauri Tirkkonen wrote: > > > > > Hi, another simple diff converting fgetln usage to getline. > > > > > > > > > > I also considered converting grep_fgetln to grep_getline, but that > > > > > would > > > > > mean that for FILE_MMAP we'd have to copy the string. So this diff > > > > > keeps > > > > > the grep_fgetln interface as is, but avoids using fgetln from libc > > > > > (and > > > > > adds error handling for FILE_STDIO). > > > > > > > > this looks good and seems to work. thanks. > > > > > > actually, it doesn't work :) the added error handling catches that > > > directory fd's are being passed into grep_fgetln, causing an error exit > > > with grep -r (or just grep foo /etc/*). I'm working on a second diff to > > > refactor the file handling a bit. > > > > oh, interesting. that's sloppy. can you please fix that first, separately? > > Sure, here it is. Could you please take a look at it? It's been a couple weeks. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: running Xorg without root
On Wed, Dec 12 2018 11:09:18 +0100, Mark Kettenis wrote: > > Date: Wed, 12 Dec 2018 01:27:24 +0200 > > From: Lauri Tirkkonen > > > > Hi, > > > > since the Xorg setuid bit was removed, I looked a little bit into what > > it would take to run it without root privs. I have a proof of concept > > put together, and things seem to work (on an X220 amd64 + modesetting > > driver, inteldrm; login on ttyC0 and running xinit). Testing the waters > > here to see if anyone else is interested :) > > I have a somewhat similar set of diffs that still need some polishing. > I have not been able to push that forward the last couple of weeks > because of lack of time. But I hope that'll change after next > weekend. ah, nice. let me know if I can help in some way :) -- Lauri Tirkkonen | lotheac @ IRCnet
running Xorg without root
Hi,
since the Xorg setuid bit was removed, I looked a little bit into what
it would take to run it without root privs. I have a proof of concept
put together, and things seem to work (on an X220 amd64 + modesetting
driver, inteldrm; login on ttyC0 and running xinit). Testing the waters
here to see if anyone else is interested :)
First, we need Xorg to attempt xf86OpenConsole even when we're not uid
0. That will call xf86OpenWScons (through xf86ConsTab), which opens the
console device and does wscons ioctls on it -- succeeding if the user
running X has logged in on that console.
diff --git a/xserver/hw/xfree86/common/xf86Init.c
b/xserver/hw/xfree86/common/xf86Init.c
index 2a04da045..b814eb412 100644
--- a/xserver/hw/xfree86/common/xf86Init.c
+++ b/xserver/hw/xfree86/common/xf86Init.c
@@ -967,9 +967,13 @@ OsVendorInit(void)
#endif
#endif
#if defined(X_PRIVSEP)
- if (!beenHere && !xf86KeepPriv && geteuid() == 0) {
- xf86PrivilegedInit();
- xf86DropPriv();
+ if (!beenHere) {
+ if(!xf86KeepPriv && geteuid() == 0) {
+ xf86PrivilegedInit();
+ xf86DropPriv();
+ } else {
+ xf86OpenConsole();
+ }
}
#endif
Next, we need the user logging in on ttyC0 to be able to access
/dev/pci0 and /dev/ttyC4:
diff --git a/etc/etc.amd64/fbtab b/etc/etc.amd64/fbtab
index 79cfb535c9f..b746b7684f7 100644
--- a/etc/etc.amd64/fbtab
+++ b/etc/etc.amd64/fbtab
@@ -1 +1 @@
-/dev/ttyC0 0600
/dev/console:/dev/wskbd:/dev/wskbd0:/dev/wsmouse:/dev/wsmouse0:/dev/ttyCcfg:/dev/drm0
+/dev/ttyC0 0600
/dev/console:/dev/wskbd:/dev/wskbd0:/dev/wsmouse:/dev/wsmouse0:/dev/ttyCcfg:/dev/drm0:/dev/pci0:/dev/ttyC4
Finally, there's a check in drm_drv.c that only allows superuser to
become a master on /dev/drm0 and fails the open for other users. I
removed the superuser check; filesystem permissions should prevent
anyone except the user logging in on ttyC0 from accessing this device
anyway. I haven't studied what exactly being the master allows here and
if there's possible privilege escalation hiding there; my reading of
drm_do_ioctl is that ioctls marked DRM_ROOT_ONLY will still fail, so I
admit I don't really know what the check was there for...
Still, this allows running X without any user processes as root (and
unbreaks xinit/startx) - is there any potential here? :)
diff --git a/sys/dev/pci/drm/drm_drv.c b/sys/dev/pci/drm/drm_drv.c
index e09380e3257..be9773b0671 100644
--- a/sys/dev/pci/drm/drm_drv.c
+++ b/sys/dev/pci/drm/drm_drv.c
@@ -745,14 +745,10 @@ drmopen(dev_t kdev, int flags, int fmt, struct proc *p)
}
mutex_lock(>struct_mutex);
- /* first opener automatically becomes master if root */
- if (SPLAY_EMPTY(>files) && !DRM_SUSER(p)) {
- mutex_unlock(>struct_mutex);
- ret = EPERM;
- goto free_priv;
- }
-
+ /* first opener automatically becomes master */
file_priv->is_master = SPLAY_EMPTY(>files);
+ if (!file_priv->authenticated)
+ file_priv->authenticated = file_priv->is_master;
SPLAY_INSERT(drm_file_tree, >files, file_priv);
mutex_unlock(>struct_mutex);
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: running Xorg without root
argh: On Wed, Dec 12 2018 10:26:42 +0200, Lauri Tirkkonen wrote: > > What was the issue that prompted you to make this change ? > > The console was not opened otherwise if euid==0 euid != 0, I meant. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: running Xorg without root
On Wed, Dec 12 2018 09:18:45 +0100, Matthieu Herrb wrote:
> > diff --git a/xserver/hw/xfree86/common/xf86Init.c
> > b/xserver/hw/xfree86/common/xf86Init.c
> > index 2a04da045..b814eb412 100644
> > --- a/xserver/hw/xfree86/common/xf86Init.c
> > +++ b/xserver/hw/xfree86/common/xf86Init.c
> > @@ -967,9 +967,13 @@ OsVendorInit(void)
> > #endif
> > #endif
> > #if defined(X_PRIVSEP)
> > - if (!beenHere && !xf86KeepPriv && geteuid() == 0) {
> > - xf86PrivilegedInit();
> > - xf86DropPriv();
> > + if (!beenHere) {
> > + if(!xf86KeepPriv && geteuid() == 0) {
> > + xf86PrivilegedInit();
> > + xf86DropPriv();
> > + } else {
> > + xf86OpenConsole();
> > + }
> >}
> > #endif
>
> This shouldn't be needed (And wasn't when I did my own tests with
> running X without root). With startx you can re-use the virtual
> console from which X was started, there won't be any other process
> (like getty(8) fighting with the X server for input.
>
> What was the issue that prompted you to make this change ?
The console was not opened otherwise if euid==0 (xf86OpenConsole is only
called from xf86PrivilegedInit), and I saw some ioctl's on fd -1 as a
result in ktrace.
> > Finally, there's a check in drm_drv.c that only allows superuser to
> > become a master on /dev/drm0 and fails the open for other users. I
> > removed the superuser check; filesystem permissions should prevent
> > anyone except the user logging in on ttyC0 from accessing this device
> > anyway. I haven't studied what exactly being the master allows here and
> > if there's possible privilege escalation hiding there; my reading of
> > drm_do_ioctl is that ioctls marked DRM_ROOT_ONLY will still fail, so I
> > admit I don't really know what the check was there for...
> >
> > Still, this allows running X without any user processes as root (and
> > unbreaks xinit/startx) - is there any potential here? :)
>
> Yes. We dont wan't to do that. Mark has some plans to implement the
> proper solution here.
ok, thanks. It was worth a shot anyway :)
> Also note that this will only work for the modesetting
> driver. Unfortnatly, even the other KMS based driver (intel and
> radeon) still need more work to run without some root privileges.
true, I did not test other drivers.
> And older drivers (non AMD/intel cards) will stop working completly.
I don't see how that follows. This change *relaxes* the requirements for
becoming master on /dev/drm0 (they no longer have to be root), how would
that cause those older drivers to stop working?
--
Lauri Tirkkonen | lotheac @ IRCnet
install(1) could fail due to race
* Unlink now... avoid ETXTBSY errors later. Try and turn
-* off the append/immutable bits -- if we fail, go ahead,
-* it might work.
-*/
- if (sbp->st_flags & (NOCHANGEBITS))
- (void)chflags(path, sbp->st_flags & ~(NOCHANGEBITS));
-
- if (dobackup) {
- (void)snprintf(backup, PATH_MAX, "%s%s", path, suffix);
- /* It is ok for the target file not to exist. */
- if (rename(path, backup) < 0 && errno != ENOENT)
- err(1, "rename: %s to %s (errno %d)", path, backup,
errno);
- } else {
- if (unlink(path) < 0 && errno != ENOENT)
- err(1, "%s", path);
- }
-
- return(open(path, O_CREAT | O_RDWR | O_EXCL, S_IRUSR | S_IWUSR));
-}
-
/*
* file_write()
* Write/copy a file (during copy or archive extract). This routine knows
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: install(1) could fail due to race
On Mon, Jan 07 2019 20:13:09 +0200, Lauri Tirkkonen wrote:
> @@ -128,9 +127,6 @@ main(int argc, char *argv[])
> case 'p':
> docompare = dopreserve = 1;
> break;
> - case 'S':
> - safecopy = 1;
> - break;
> case 's':
> dostrip = 1;
> break;
I realized -S is used in many places throughout src (including in
xinstall/Makefile itself, and in bsd.lib.mk, bsd.prog.mk), so -S should
probably be accepted for compatibility until those -S's can be removed.
diff revised as such.
diff --git a/usr.bin/xinstall/install.1 b/usr.bin/xinstall/install.1
index fd5db0abc37..f7d8707ce6f 100644
--- a/usr.bin/xinstall/install.1
+++ b/usr.bin/xinstall/install.1
@@ -38,7 +38,7 @@
.Nd install binaries
.Sh SYNOPSIS
.Nm install
-.Op Fl bCcDdFpSs
+.Op Fl bCcDdFps
.Op Fl B Ar suffix
.Op Fl f Ar flags
.Op Fl g Ar group
@@ -139,17 +139,6 @@ Copy the file, as if the
(compare and copy) option is specified,
except if the target file doesn't already exist or is different,
then preserve the modification time of the file.
-.It Fl S
-Safe copy.
-Normally,
-.Nm
-unlinks an existing target before installing the new file.
-With the
-.Fl S
-flag a temporary file is used and then renamed to be
-the target.
-The reason this is safer is that if the copy or
-rename fails, the existing target is left untouched.
.It Fl s
.Nm
exec's the command
@@ -186,18 +175,8 @@ Default is
.Sh FILES
.Bl -tag -width INS@XX -compact
.It Pa INS@XX
-If either
-.Fl S
-option is specified, or the
-.Fl C
-or
-.Fl p
-option is used in conjunction with the
-.Fl s
-option, temporary files named INS@XX,
-where XX is decided by
-.Xr mkstemp 3 ,
-are created in the target directory.
+Temporary files created in the target directory by
+.Xr mkstemp 3 .
.El
.Sh EXIT STATUS
.Ex -std install
diff --git a/usr.bin/xinstall/xinstall.c b/usr.bin/xinstall/xinstall.c
index c08d82eeed4..be7311b354a 100644
--- a/usr.bin/xinstall/xinstall.c
+++ b/usr.bin/xinstall/xinstall.c
@@ -60,7 +60,7 @@
#define NOCHANGEBITS (UF_IMMUTABLE | UF_APPEND | SF_IMMUTABLE | SF_APPEND)
#define BACKUP_SUFFIX ".old"
-int dobackup, docompare, dodest, dodir, dopreserve, dostrip, safecopy;
+int dobackup, docompare, dodest, dodir, dopreserve, dostrip;
int mode = S_IRWXU|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH;
char pathbuf[PATH_MAX], tempfile[PATH_MAX];
char *suffix = BACKUP_SUFFIX;
@@ -73,7 +73,6 @@ void install(char *, char *, u_long, u_int);
void install_dir(char *, int);
void strip(char *);
void usage(void);
-intcreate_newfile(char *, struct stat *);
intcreate_tempfile(char *, char *, size_t);
intfile_write(int, char *, size_t, int *, int *, int);
void file_flush(int, int);
@@ -129,7 +128,6 @@ main(int argc, char *argv[])
docompare = dopreserve = 1;
break;
case 'S':
- safecopy = 1;
break;
case 's':
dostrip = 1;
@@ -148,17 +146,13 @@ main(int argc, char *argv[])
argv += optind;
/* some options make no sense when creating directories */
- if ((safecopy || docompare || dostrip) && dodir)
+ if ((docompare || dostrip) && dodir)
usage();
/* must have at least two arguments, except when creating directories */
if (argc < 2 && !dodir)
usage();
- /* need to make a temp copy so we can compare stripped version */
- if (docompare && dostrip)
- safecopy = 1;
-
/* get group and owner id's */
if (group != NULL && gid_from_group(group, ) == -1) {
gid = strtonum(group, 0, GID_MAX, );
@@ -265,54 +259,30 @@ install(char *from_name, char *to_name, u_long fset,
u_int flags)
err(1, "%s", from_name);
}
- if (safecopy) {
- to_fd = create_tempfile(to_name, tempfile, sizeof(tempfile));
- if (to_fd < 0)
- err(1, "%s", tempfile);
- } else if (docompare && !dostrip) {
- if ((to_fd = open(to_name, O_RDONLY, 0)) < 0)
- err(1, "%s", to_name);
- } else {
- if ((to_fd = create_newfile(to_name, _sb)) < 0)
- err(1, "%s", to_name);
- }
+ to_fd = create_tempfile(to_name, tempfile, sizeof(tempfile));
+ if (to_fd < 0)
+ err(1, "%s", tempfile);
- if (!devnull) {
- if (docompare && !safecopy) {
- files_match = !(compare(from_fd, from_name,
- from_sb.st_size, to_fd,
-
Re: grep: convert fgetln to getline
On Sun, Jan 06 2019 14:02:16 -0500, Ted Unangst wrote: > Lauri Tirkkonen wrote: > > Hi, another simple diff converting fgetln usage to getline. > > > > I also considered converting grep_fgetln to grep_getline, but that would > > mean that for FILE_MMAP we'd have to copy the string. So this diff keeps > > the grep_fgetln interface as is, but avoids using fgetln from libc (and > > adds error handling for FILE_STDIO). > > this looks good and seems to work. thanks. actually, it doesn't work :) the added error handling catches that directory fd's are being passed into grep_fgetln, causing an error exit with grep -r (or just grep foo /etc/*). I'm working on a second diff to refactor the file handling a bit. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: grep: convert fgetln to getline
On Mon, Jan 07 2019 15:41:53 -0500, Ted Unangst wrote:
> Lauri Tirkkonen wrote:
> > On Sun, Jan 06 2019 14:02:16 -0500, Ted Unangst wrote:
> > > Lauri Tirkkonen wrote:
> > > > Hi, another simple diff converting fgetln usage to getline.
> > > >
> > > > I also considered converting grep_fgetln to grep_getline, but that would
> > > > mean that for FILE_MMAP we'd have to copy the string. So this diff keeps
> > > > the grep_fgetln interface as is, but avoids using fgetln from libc (and
> > > > adds error handling for FILE_STDIO).
> > >
> > > this looks good and seems to work. thanks.
> >
> > actually, it doesn't work :) the added error handling catches that
> > directory fd's are being passed into grep_fgetln, causing an error exit
> > with grep -r (or just grep foo /etc/*). I'm working on a second diff to
> > refactor the file handling a bit.
>
> oh, interesting. that's sloppy. can you please fix that first, separately?
Sure, here it is. The idea is to use grep_fdopen for everything, making
grep_open just open the file and pass the fd to grep_fdopen, do fstat()
in grep_fdopen and modify mmopen() accordingly (it no longer needs to
open the file or stat it).
grep_tree is modified to not call procfile() on FTS_D (FTS_DP was
already there). In addition grep_fdopen returns NULL and sets errno ==
EISDIR if it encounters a directory; this can happen if a directory
argument is given on command line, or for recursive grep, if the file
being processed is a symlink to a directory, which fts doesn't notice.
procfile() handles the EISDIR return silently, matching current
behavior.
I also removed the useless mode argument from grep_open and grep_fdopen;
we always open files read-only.
diff --git a/usr.bin/grep/file.c b/usr.bin/grep/file.c
index 4b3c689e4ab..2befc4304c1 100644
--- a/usr.bin/grep/file.c
+++ b/usr.bin/grep/file.c
@@ -26,7 +26,10 @@
* SUCH DAMAGE.
*/
+#include
#include
+#include
+#include
#include
#include
#include
@@ -90,68 +93,64 @@ gzfgetln(gzFile *f, size_t *len)
#endif
file_t *
-grep_fdopen(int fd, char *mode)
+grep_fdopen(int fd)
{
file_t *f;
+ struct stat sb;
if (fd == STDIN_FILENO)
snprintf(fname, sizeof fname, "(standard input)");
- else
+ else if (fname[0] == '\0')
snprintf(fname, sizeof fname, "(fd %d)", fd);
+ if (fstat(fd, ) == -1)
+ return NULL;
+ if (S_ISDIR(sb.st_mode)) {
+ errno = EISDIR;
+ return NULL;
+ }
+
f = grep_malloc(sizeof *f);
#ifndef NOZ
if (Zflag) {
f->type = FILE_GZIP;
f->noseek = lseek(fd, 0L, SEEK_SET) == -1;
- if ((f->gzf = gzdopen(fd, mode)) != NULL)
+ if ((f->gzf = gzdopen(fd, "r")) != NULL)
return f;
- } else
+ }
#endif
- {
- f->type = FILE_STDIO;
- f->noseek = isatty(fd);
- if ((f->f = fdopen(fd, mode)) != NULL)
- return f;
+ f->noseek = isatty(fd);
+#ifndef SMALL
+ /* try mmap first; if it fails, try stdio */
+ if (!f->noseek && (f->mmf = mmopen(fd, )) != NULL) {
+ f->type = FILE_MMAP;
+ return f;
}
+#endif
+ f->type = FILE_STDIO;
+ if ((f->f = fdopen(fd, "r")) != NULL)
+ return f;
free(f);
return NULL;
}
file_t *
-grep_open(char *path, char *mode)
+grep_open(char *path)
{
file_t *f;
+ int fd;
snprintf(fname, sizeof fname, "%s", path);
- f = grep_malloc(sizeof *f);
- f->noseek = 0;
-
-#ifndef NOZ
- if (Zflag) {
- f->type = FILE_GZIP;
- if ((f->gzf = gzopen(fname, mode)) != NULL)
- return f;
- } else
-#endif
- {
-#ifndef SMALL
- /* try mmap first; if it fails, try stdio */
- if ((f->mmf = mmopen(fname, mode)) != NULL) {
- f->type = FILE_MMAP;
- return f;
- }
-#endif
- f->type = FILE_STDIO;
- if ((f->f = fopen(path, mode)) != NULL)
- return f;
- }
+ if ((fd = open(fname, O_RDONLY)) == -1)
+ return NULL;
- free(f);
- return NULL;
+ f = grep_fdopen(fd);
+ if (f == NULL)
+ close(fd);
+ return f;
}
int
diff --git a/usr.bin/grep/grep.c b/usr.bin/grep/grep.c
index 913cc97a0f3..e5e48fa163a 100644
--- a/usr.bin/grep/grep.c
+++ b/usr.bin/grep/grep.c
@@ -63,9 +63,7 @@ intFflag; /* -F: interpret pattern as list of
fixed strings */
int Hflag;
Re: install(1) could fail due to race
On Mon, Jan 07 2019 16:32:31 -0500, Ted Unangst wrote: > > > This doubles the number of synchronous > > > file operations. > > > > Does it? Without safecopy, the operations performed are: > > > > unlink(targetfile); > > open(targetfile, O_CREAT|O_EXCL); > > write(); > > fchmod(); > > close(); > > > > with safecopy, they are: > > > > open(tempfile, O_CREAT|O_EXCL); > > write(); > > fchmod(); > > close(); > > rename(tempfile, targetfile); > > > > which to me seems identical in the number of file syscalls made. > > oh, I think I forgot to count the unlink(). rename() within a directory is > about the same cost as unlink(), so the two cases do seem equal. great, we're on the same page :) here's an additional manpage adjustment on top, that I somehow missed the first time around. diff --git a/usr.bin/xinstall/install.1 b/usr.bin/xinstall/install.1 index f7d8707ce6f..64f99504752 100644 --- a/usr.bin/xinstall/install.1 +++ b/usr.bin/xinstall/install.1 @@ -101,7 +101,7 @@ Create directories. Missing parent directories are created as required. This option cannot be used with the .Fl B , b , C , c , -.Fl f , p , S , +.Fl f , p , or .Fl s options. @@ -198,9 +198,8 @@ The .Fl C , .Fl D , .Fl F , -.Fl p , and -.Fl S +.Fl p flags are non-standard and should not be relied upon for portability. .Pp Temporary files may be left in the target directory if -- Lauri Tirkkonen | lotheac @ IRCnet
Re: install(1) could fail due to race
On Mon, Jan 07 2019 15:48:25 -0500, Ted Unangst wrote: > Lauri Tirkkonen wrote: > > Hi, it seems install(1) has a race condition: in create_newfile, it > > first unlinks the target file and then tries to open it with > > O_CREAT|O_EXCL. > > > The below diff essentially removes the -S option and makes install > > always use temp files (ie. -S is always on), eliminating the race since > > rename(2) cannot fail like this. > > I don't know. Presumably if there weren't any downside to safecopy, it would > already have been made the default. I had this thought, but could not think of any real downsides... > This doubles the number of synchronous > file operations. Does it? Without safecopy, the operations performed are: unlink(targetfile); open(targetfile, O_CREAT|O_EXCL); write(); fchmod(); close(); with safecopy, they are: open(tempfile, O_CREAT|O_EXCL); write(); fchmod(); close(); rename(tempfile, targetfile); which to me seems identical in the number of file syscalls made. -- Lauri Tirkkonen | lotheac @ IRCnet
typo in getgrent.3
Hi, trivial typo fix for getgrent.3. diff --git a/lib/libc/gen/getgrent.3 b/lib/libc/gen/getgrent.3 index d3c12bad042..f26ae625997 100644 --- a/lib/libc/gen/getgrent.3 +++ b/lib/libc/gen/getgrent.3 @@ -193,7 +193,7 @@ group database file .El .Sh ERRORS The -.Fn getgruid_r +.Fn getgrgid_r and .Fn getgrnam_r functions may fail if: -- Lauri Tirkkonen | lotheac @ IRCnet
Re: install(1) could fail due to race
On Mon, Jan 07 2019 14:27:29 -0700, Todd C. Miller wrote: > The main difference with -S is that you end up with two copies of > the target file on disk for a short period of time. That was a > bigger deal back when -S was added. Today, it probably doesn't > matter. so, how about this patch then? I think the discussion about softupdates is a little orthogonal. -- Lauri Tirkkonen | lotheac @ IRCnet
net80211: don't drop unencrypted data frames with no data
Hi, (disclaimer: I know basically nothing about 802.11)
I noticed on my AP a high counter on netstat -W "input unencrypted
packets with wep/wpa config discarded", aka is_rx_unencrypted. After
investigation it looked like all of these were frames with type Data,
but with the "No data" bit set in FC0. Per IEEE's 80211-2016.pdf
9.2.4.1.9 (page 644) the Protected bit is set to 0 for these frames, so
don't insist on them being encrypted. (See also 9.2.4.1.3, p. 640, about
bit 6 (ie. FC0_SUBTYPE_NODATA) implying no Frame Body).
diff --git a/sys/net80211/ieee80211_input.c b/sys/net80211/ieee80211_input.c
index a614a67cc59..1d1720268f4 100644
--- a/sys/net80211/ieee80211_input.c
+++ b/sys/net80211/ieee80211_input.c
@@ -164,7 +164,7 @@ ieee80211_input(struct ifnet *ifp, struct mbuf *m, struct
ieee80211_node *ni,
struct ieee80211_frame *wh;
u_int16_t *orxseq, nrxseq, qos;
u_int8_t dir, type, subtype, tid;
- int hdrlen, hasqos;
+ int hdrlen, hasqos, hasdata;
KASSERT(ni != NULL);
@@ -209,9 +209,10 @@ ieee80211_input(struct ifnet *ifp, struct mbuf *m, struct
ieee80211_node *ni,
qos = 0;
tid = 0;
}
+ hasdata = (type == IEEE80211_FC0_TYPE_DATA &&
+ (subtype & IEEE80211_FC0_SUBTYPE_NODATA) == 0);
- if (type == IEEE80211_FC0_TYPE_DATA && hasqos &&
- (subtype & IEEE80211_FC0_SUBTYPE_NODATA) == 0 &&
+ if (hasdata && hasqos &&
!(rxi->rxi_flags & IEEE80211_RXI_AMPDU_DONE)) {
int ba_state = ni->ni_rx_ba[tid].ba_state;
@@ -411,6 +412,12 @@ ieee80211_input(struct ifnet *ifp, struct mbuf *m, struct
ieee80211_node *ni,
/* protection is on for Rx */
if (!(rxi->rxi_flags & IEEE80211_RXI_HWDEC)) {
if (!(wh->i_fc[1] & IEEE80211_FC1_PROTECTED)) {
+ /*
+* 9.2.4.1.9 frames without data are
+* not protected
+*/
+ if (!hasdata)
+ return;
/* drop unencrypted */
ic->ic_stats.is_rx_unencrypted++;
goto err;
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: grep: convert fgetln to getline
On Thu, Jan 24 2019 18:08:25 -0600, Scott Cheloha wrote: > not to be standoffish, I'm just travelling so I can't really look into it. yeah, it's not like I have infinite time either. I'll try to do something with this over the weekend but no promises. > if you came forward with malloc numbers before/after with sufficient > N to get to a standard CI, that would help your case and be useful > in the future. It's been a while since I did any statistical analysis. To be honest, this seems super overkill for measuring a code path that is not actually taken most of the time; the test would have to compile both of the greps under test with -DSMALL to avoid the mmap path. But fine, since it seems so important to you I'll try to get *something*... -- Lauri Tirkkonen | lotheac @ IRCnet
Re: grep: convert fgetln to getline
On Thu, Jan 24 2019 17:03:57 -0700, Theo de Raadt wrote:
> Scott Cheloha wrote:
>
> > > On Jan 24, 2019, at 06:19, Lauri Tirkkonen wrote:
> > >
> > > [...]
> > >
> > > I haven't done any actual measurements though, so it's possible my
> > > reading is wrong.
> >
> > Is there a "grepbench" or canonical corpus we can use to get a 95%
> > CI on this and future changes? People talk about grep(1) perf. like
> > CPU time and memory use, there ought to be something better than
> > "let's search the Linux kernel source for a string that isn't there."
>
> A study of call patterns with ltrace might help
Ok, so let's do some measurements. First, I compiled four copies of
grep:
/usr/bin/grep # no modification; uses fgetln
/usr/bin/grep.small # as above, with -DSMALL
/usr/bin/grep.getline # uses getline through my diff
/usr/bin/grep.small.getline # as above, with -DSMALL
First let's look at a somewhat pathological case: a ~2GB file where each
line is longer than the previous line, by one byte.
cd /tmp
python3 -c 'for i in range(65536): print("a"*i)' > foo
ltrace -u libc -f fgetln.out grep b foo
ltrace -u libc -f getline.out grep.getline b foo
ltrace -u libc -f fgetln.small.out grep.small b foo
ltrace -u libc -f getline.small.out grep.small.getline b foo
side note: my shell reported the following times for the above commands.
these are not conclusive but it is very interesting that if we disable
the mmap path (which supposedly is an optimisation), the process
finishes quite a bit faster, with a lot less user time spent than the
non-SMALL version, regardless of whether we use fgetln or getline...
ltrace -u libc -f fgetln.out grep b foo 12.24s user 5.26s system 62% cpu
27.797 total
ltrace -u libc -f getline.out grep.getline b foo 13.45s user 3.67s system 64%
cpu 26.361 total
ltrace -u libc -f fgetln.small.out grep.small b foo 3.62s user 5.00s system
42% cpu 20.418 total
ltrace -u libc -f getline.small.out grep.small.getline b foo 3.79s user 3.25s
system 35% cpu 19.726 total
Since both /usr/bin/grep and /usr/bin/grep.getline are able to use
mmap() on this file, I expect no difference in the number of allocations
they make. ltrace agrees:
rush /tmp % kdump -f getline.out|egrep -c
'"(malloc|calloc|realloc|recalloc)'
9
rush /tmp % kdump -f fgetln.out|egrep -c
'"(malloc|calloc|realloc|recalloc)'
9
So how about the -DSMALL versions? These should be more allocations.
rush /tmp % kdump -f fgetln.small.out|egrep -c
'"(malloc|calloc|realloc|recalloc)'
12
rush /tmp % kdump -f getline.small.out|egrep -c
'"(malloc|calloc|realloc|recalloc)'
12
Well, that's actually quite surprising - far fewer allocations than I
expected. Maybe I'm measuring the wrong thing somehow (I'm not really
familiar with ltrace). Let's take a look at what ktrace has to say about
these as a sanity check.
ktrace -tc -f kt.fgetln.out grep b foo
ktrace -tc -f kt.getline.out grep.getline b foo
ktrace -tc -f kt.fgetln.small.out grep.small b foo
ktrace -tc -f kt.getline.small.out grep.small.getline b foo
rush /tmp % kdump -f kt.fgetln.out|grep -c 'CALL *mmap.*MAP_ANON'
27886
rush /tmp % kdump -f kt.getline.out|grep -c 'CALL *mmap.*MAP_ANON'
52
rush /tmp % kdump -f kt.fgetln.small.out|grep -c 'CALL *mmap.*MAP_ANON'
27883
rush /tmp % kdump -f kt.getline.small.out|grep -c 'CALL *mmap.*MAP_ANON'
51
So, yes, the fgetln case for this file is indeed hitting MAP_ANON mmaps
way more often. But now I'm also a bit confused - why does the non-SMALL
case also call MAP_ANON mmap so many times? It's supposed to just
memory-map the file.
41564 grep CALL
mmap(0,0x80008000,0x1,0x2,0,0)
41564 grep RET mmap -1 errno 22 Invalid argument
wait - fd 0? It turns out that mmfile.c passes mmf->fd to mmap, but does
not actually initialize it. That's my fault, sorry - I apparently broke
it in the previous commit to grep (although I don't know why I wouldn't
get a maybe-uninitialized warning from this; perhaps it's because of the
grep_malloc wrapper?). Let's fix it:
diff --git a/usr.bin/grep/mmfile.c b/usr.bin/grep/mmfile.c
index fa5a0ea7752..d866dd3977b 100644
--- a/usr.bin/grep/mmfile.c
+++ b/usr.bin/grep/mmfile.c
@@ -49,6 +49,7 @@ mmopen(int fd, struct stat *st)
if (st->st_size > SIZE_MAX) /* too big to mmap */
goto ouch;
mmf->len = (size_t)st->st_size;
+ mmf->fd = fd;
mmf->base = mmap(NULL, mmf->len, PROT_READ, MAP_PRIVATE, mmf->fd,
(off_t)0);
if (mmf->base == MAP_FAILED)
goto ouch;
and then rerun t
bsd.{prog,lib}.mk: drop -S for install
now that install -S is always enabled, no need to specify it in mk
files.
diff --git a/share/mk/bsd.lib.mk b/share/mk/bsd.lib.mk
index a7689506a58..baca99ae45b 100644
--- a/share/mk/bsd.lib.mk
+++ b/share/mk/bsd.lib.mk
@@ -274,7 +274,7 @@ beforeinstall:
realinstall:
# ranlib lib${LIB}.a
- ${INSTALL} ${INSTALL_COPY} -S -o ${LIBOWN} -g ${LIBGRP} -m 600
lib${LIB}.a \
+ ${INSTALL} ${INSTALL_COPY} -o ${LIBOWN} -g ${LIBGRP} -m 600 lib${LIB}.a
\
${DESTDIR}${LIBDIR}/lib${LIB}.a
.if (${INSTALL_COPY} != "-p")
${RANLIB} -t ${DESTDIR}${LIBDIR}/lib${LIB}.a
@@ -282,7 +282,7 @@ realinstall:
chmod ${LIBMODE} ${DESTDIR}${LIBDIR}/lib${LIB}.a
.if !defined(NOPROFILE)
# ranlib lib${LIB}_p.a
- ${INSTALL} ${INSTALL_COPY} -S -o ${LIBOWN} -g ${LIBGRP} -m 600 \
+ ${INSTALL} ${INSTALL_COPY} -o ${LIBOWN} -g ${LIBGRP} -m 600 \
lib${LIB}_p.a ${DESTDIR}${LIBDIR}
.if (${INSTALL_COPY} != "-p")
${RANLIB} -t ${DESTDIR}${LIBDIR}/lib${LIB}_p.a
@@ -290,7 +290,7 @@ realinstall:
chmod ${LIBMODE} ${DESTDIR}${LIBDIR}/lib${LIB}_p.a
.endif
.if !defined(NOPIC) && defined(SHLIB_MAJOR) && defined(SHLIB_MINOR)
- ${INSTALL} ${INSTALL_COPY} -S -o ${LIBOWN} -g ${LIBGRP} -m ${LIBMODE} \
+ ${INSTALL} ${INSTALL_COPY} -o ${LIBOWN} -g ${LIBGRP} -m ${LIBMODE} \
${FULLSHLIBNAME} ${DESTDIR}${LIBDIR}
.if defined(LIBREBUILD)
.if !defined(DESTDIR)
@@ -299,7 +299,7 @@ realinstall:
.endif
${INSTALL} -d -o ${LIBOWN} -g ${LIBGRP} -m 755 \
${DESTDIR}/usr/share/relink/${LIBDIR}
- ${INSTALL} ${INSTALL_COPY} -S -o ${LIBOWN} -g ${LIBGRP} -m ${LIBMODE} \
+ ${INSTALL} ${INSTALL_COPY} -o ${LIBOWN} -g ${LIBGRP} -m ${LIBMODE} \
${FULLSHLIBNAME}.a ${DESTDIR}/usr/share/relink/${LIBDIR}
.endif
.endif
diff --git a/share/mk/bsd.prog.mk b/share/mk/bsd.prog.mk
index 87bc4a72c05..13361ea3777 100644
--- a/share/mk/bsd.prog.mk
+++ b/share/mk/bsd.prog.mk
@@ -148,13 +148,13 @@ afterinstall:
.if !target(realinstall)
realinstall:
. if defined(PROG)
- ${INSTALL} ${INSTALL_COPY} -S ${INSTALL_STRIP} \
+ ${INSTALL} ${INSTALL_COPY} ${INSTALL_STRIP} \
-o ${BINOWN} -g ${BINGRP} \
-m ${BINMODE} ${PROG} ${DESTDIR}${BINDIR}/${PROG}
. endif
. if defined(PROGS)
.for p in ${PROGS}
- ${INSTALL} ${INSTALL_COPY} -S ${INSTALL_STRIP} \
+ ${INSTALL} ${INSTALL_COPY} ${INSTALL_STRIP} \
-o ${BINOWN} -g ${BINGRP} \
-m ${BINMODE} $p ${DESTDIR}${BINDIR}/$p
.endfor
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: bsd.{prog,lib}.mk: drop -S for install
} -g ${LIBGRP} -m 600
lib${LIB}.a \
+ ${INSTALL} ${INSTALL_COPY} -o ${LIBOWN} -g ${LIBGRP} -m 600 lib${LIB}.a
\
${DESTDIR}${LIBDIR}/lib${LIB}.a
.if (${INSTALL_COPY} != "-p")
${RANLIB} -t ${DESTDIR}${LIBDIR}/lib${LIB}.a
@@ -282,7 +282,7 @@ realinstall:
chmod ${LIBMODE} ${DESTDIR}${LIBDIR}/lib${LIB}.a
.if !defined(NOPROFILE)
# ranlib lib${LIB}_p.a
- ${INSTALL} ${INSTALL_COPY} -S -o ${LIBOWN} -g ${LIBGRP} -m 600 \
+ ${INSTALL} ${INSTALL_COPY} -o ${LIBOWN} -g ${LIBGRP} -m 600 \
lib${LIB}_p.a ${DESTDIR}${LIBDIR}
.if (${INSTALL_COPY} != "-p")
${RANLIB} -t ${DESTDIR}${LIBDIR}/lib${LIB}_p.a
@@ -290,7 +290,7 @@ realinstall:
chmod ${LIBMODE} ${DESTDIR}${LIBDIR}/lib${LIB}_p.a
.endif
.if !defined(NOPIC) && defined(SHLIB_MAJOR) && defined(SHLIB_MINOR)
- ${INSTALL} ${INSTALL_COPY} -S -o ${LIBOWN} -g ${LIBGRP} -m ${LIBMODE} \
+ ${INSTALL} ${INSTALL_COPY} -o ${LIBOWN} -g ${LIBGRP} -m ${LIBMODE} \
${FULLSHLIBNAME} ${DESTDIR}${LIBDIR}
.if defined(LIBREBUILD)
.if !defined(DESTDIR)
@@ -299,7 +299,7 @@ realinstall:
.endif
${INSTALL} -d -o ${LIBOWN} -g ${LIBGRP} -m 755 \
${DESTDIR}/usr/share/relink/${LIBDIR}
- ${INSTALL} ${INSTALL_COPY} -S -o ${LIBOWN} -g ${LIBGRP} -m ${LIBMODE} \
+ ${INSTALL} ${INSTALL_COPY} -o ${LIBOWN} -g ${LIBGRP} -m ${LIBMODE} \
${FULLSHLIBNAME}.a ${DESTDIR}/usr/share/relink/${LIBDIR}
.endif
.endif
diff --git a/share/mk/bsd.prog.mk b/share/mk/bsd.prog.mk
index 87bc4a72c05..13361ea3777 100644
--- a/share/mk/bsd.prog.mk
+++ b/share/mk/bsd.prog.mk
@@ -148,13 +148,13 @@ afterinstall:
.if !target(realinstall)
realinstall:
. if defined(PROG)
- ${INSTALL} ${INSTALL_COPY} -S ${INSTALL_STRIP} \
+ ${INSTALL} ${INSTALL_COPY} ${INSTALL_STRIP} \
-o ${BINOWN} -g ${BINGRP} \
-m ${BINMODE} ${PROG} ${DESTDIR}${BINDIR}/${PROG}
. endif
. if defined(PROGS)
.for p in ${PROGS}
- ${INSTALL} ${INSTALL_COPY} -S ${INSTALL_STRIP} \
+ ${INSTALL} ${INSTALL_COPY} ${INSTALL_STRIP} \
-o ${BINOWN} -g ${BINGRP} \
-m ${BINMODE} $p ${DESTDIR}${BINDIR}/$p
.endfor
diff --git a/sys/arch/alpha/stand/netboot/Makefile
b/sys/arch/alpha/stand/netboot/Makefile
index e871ef819da..4f88c7ddda5 100644
--- a/sys/arch/alpha/stand/netboot/Makefile
+++ b/sys/arch/alpha/stand/netboot/Makefile
@@ -41,10 +41,10 @@ ${PROG} ${PROG}.mop: ${PROG}.nosym
mopa.out ${PROG}.nosym ${PROG}.mop
realinstall:
- ${INSTALL} ${INSTALL_COPY} -S ${INSTALL_STRIP} \
+ ${INSTALL} ${INSTALL_COPY} ${INSTALL_STRIP} \
-o ${BINOWN} -g ${BINGRP} \
-m ${BINMODE} ${PROG} ${DESTDIR}${BINDIR}/${PROG}
- ${INSTALL} ${INSTALL_COPY} -S ${INSTALL_STRIP} \
+ ${INSTALL} ${INSTALL_COPY} ${INSTALL_STRIP} \
-o ${BINOWN} -g ${BINGRP} \
-m ${BINMODE} ${PROG}.mop ${DESTDIR}${BINDIR}/${PROG}.mop
diff --git a/usr.bin/lorder/Makefile b/usr.bin/lorder/Makefile
index 21755a9db30..51b524557cd 100644
--- a/usr.bin/lorder/Makefile
+++ b/usr.bin/lorder/Makefile
@@ -5,7 +5,7 @@ MAN=lorder.1
SCRIPT=lorder.sh
realinstall:
- ${INSTALL} ${INSTALL_COPY} -S -o ${BINOWN} -g ${BINGRP} -m ${BINMODE} \
+ ${INSTALL} ${INSTALL_COPY} -o ${BINOWN} -g ${BINGRP} -m ${BINMODE} \
${.CURDIR}/${SCRIPT} ${DESTDIR}${BINDIR}/lorder
.include
diff --git a/usr.bin/mandoc/Makefile b/usr.bin/mandoc/Makefile
index eb665fd863d..389fa011228 100644
--- a/usr.bin/mandoc/Makefile
+++ b/usr.bin/mandoc/Makefile
@@ -69,7 +69,7 @@ man.cgi: ${CGI_OBJS}
installcgi: man.cgi
${INSTALL} -d -o root -g wheel -m 755 ${DESTDIR}/var/www/cgi-bin
- ${INSTALL} ${INSTALL_COPY} -S ${INSTALL_STRIP} \
+ ${INSTALL} ${INSTALL_COPY} ${INSTALL_STRIP} \
-o ${BINOWN} -g ${BINGRP} -m ${BINMODE} \
man.cgi ${DESTDIR}/var/www/cgi-bin/man.cgi
${INSTALL} ${INSTALL_COPY} -o root -g wheel -m 644 \
diff --git a/usr.bin/xinstall/Makefile b/usr.bin/xinstall/Makefile
index 865cfbb6ddc..71eb74a9294 100644
--- a/usr.bin/xinstall/Makefile
+++ b/usr.bin/xinstall/Makefile
@@ -4,7 +4,7 @@ PROG= xinstall
MAN= install.1
realinstall:
- ${INSTALL} ${INSTALL_COPY} -S ${INSTALL_STRIP} \
+ ${INSTALL} ${INSTALL_COPY} ${INSTALL_STRIP} \
-o ${BINOWN} -g ${BINGRP} \
-m ${BINMODE} ${PROG} ${DESTDIR}${BINDIR}/install
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: update ctype data to unicode 10
On Thu, Feb 21 2019 20:22:16 -0700, Andrew Hewus Fresh wrote: > > I'm only including the diff because it took quite a long time to run the > > script (177m08.01s real). > > There are a lot of unicode symbols. Someday if I get super bored I'll > write something to do it in parallel :-) True, there's a lot of them, but it does also seem to be spending quite a bit of time per symbol. In FreeBSD and also my hobby OS Unleashed, data is taken from Unicode's CLDR, which I think *might* be faster (it's been a long time since I did anything with that), but on the other hand there are so many reasons to dislike that approach. I quite like the simplicity of your script - it doesn't really matter much if it takes a long time to run since you don't need to run it very often. -- Lauri Tirkkonen | lotheac @ IRCnet
mail(1): whitespace fixes
spotted by git diff --check.
bin/mail/aux.c:115: trailing whitespace.
+ ;
bin/mail/head.c:258: space before tab in indent.
+ while ((c = (unsigned char)*wp++) && c != '"')
bin/mail/head.c:259: space before tab in indent.
+ *wbuf++ = c;
bin/mail/head.c:260: space before tab in indent.
+ if (c == '"')
bin/mail/head.c:261: space before tab in indent.
+ *wbuf++ = c;
bin/mail/head.c:264: space before tab in indent.
+ }
bin/mail/list.c:701: trailing whitespace.
+
bin/mail/names.c:539: trailing whitespace.
+
bin/mail/send.c:372: trailing whitespace.
+
diff --git a/usr.bin/mail/aux.c b/usr.bin/mail/aux.c
index a7bb8d67314..ff60406fc9e 100644
--- a/usr.bin/mail/aux.c
+++ b/usr.bin/mail/aux.c
@@ -112,7 +112,7 @@ argcount(char **argv)
char **ap;
for (ap = argv; *ap++ != NULL;)
- ;
+ ;
return(ap - argv - 1);
}
diff --git a/usr.bin/mail/head.c b/usr.bin/mail/head.c
index 3043e5c36db..3b1268a5cdd 100644
--- a/usr.bin/mail/head.c
+++ b/usr.bin/mail/head.c
@@ -255,13 +255,13 @@ nextword(char *wp, char *wbuf)
while ((c = (unsigned char)*wp++) && c != ' ' && c != '\t') {
*wbuf++ = c;
if (c == '"') {
- while ((c = (unsigned char)*wp++) && c != '"')
- *wbuf++ = c;
- if (c == '"')
- *wbuf++ = c;
+ while ((c = (unsigned char)*wp++) && c != '"')
+ *wbuf++ = c;
+ if (c == '"')
+ *wbuf++ = c;
else
wp--;
- }
+ }
}
*wbuf = '\0';
for (; c == ' ' || c == '\t'; c = (unsigned char)*wp++)
diff --git a/usr.bin/mail/list.c b/usr.bin/mail/list.c
index c870b9be9e7..b5d1b532c4f 100644
--- a/usr.bin/mail/list.c
+++ b/usr.bin/mail/list.c
@@ -698,7 +698,7 @@ matchsubj(char *str, int mesg)
else
strlcpy(lastscan, str, sizeof(lastscan));
mp = [mesg-1];
-
+
/*
* Now look, ignoring case, for the word in the string.
*/
diff --git a/usr.bin/mail/names.c b/usr.bin/mail/names.c
index 5b07dc6817c..b8196bf8405 100644
--- a/usr.bin/mail/names.c
+++ b/usr.bin/mail/names.c
@@ -536,7 +536,7 @@ elide(struct name *names)
np = np->n_flink;
continue;
}
-
+
/*
* Now t points to the last entry with the same name
* as np. Make np point beyond t.
diff --git a/usr.bin/mail/send.c b/usr.bin/mail/send.c
index 8f127ac837f..2ae8f233549 100644
--- a/usr.bin/mail/send.c
+++ b/usr.bin/mail/send.c
@@ -369,7 +369,7 @@ mail1(struct header *hp, int printheaders)
}
if ((cp = value("record")) != NULL)
(void)savemail(expand(cp), mtf);
-
+
/* Setup sendmail arguments. */
*ap++ = "send-mail";
*ap++ = "-i";
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: net80211 hostap HT protection fixes
On Wed, Feb 27 2019 15:06:49 +0100, Stefan Sperling wrote: > Otherwise this diff affects 11n access points only, i.e. athn(4). > It makes my athn(4) AP run circles (up to 15 Mbps) on a 2 GHz channel > with overlapping networks. > > Any more testers? Based on cursory testing, seems to work fine on my athn(4) 11n 5GHz AP. But I didn't observe any improvements in bandwidth, using an Android phone station as traffic sink (~9MBit/s maximum both before and after). -- Lauri Tirkkonen | lotheac @ IRCnet
mail(1): use "sendmail" as argv[0] for sendmail
For some reason mail(1) is using "send-mail" as argv[0] for sendmail.
/etc/mailer.conf and smtpctl handle this identically to "sendmail", so
it seems a bit redundant. This diff makes mail(1) use "sendmail" as
argv[0], possibly allowing that duplication to be removed later from
mailer.conf and smtpctl.
Noticed while porting mail(1) to Unleashed (where we also have
mailwrapper, but no default configuration for "send-mail").
diff --git a/usr.bin/mail/send.c b/usr.bin/mail/send.c
index 8f127ac837f..723f9da0a53 100644
--- a/usr.bin/mail/send.c
+++ b/usr.bin/mail/send.c
@@ -371,7 +371,7 @@ mail1(struct header *hp, int printheaders)
(void)savemail(expand(cp), mtf);
/* Setup sendmail arguments. */
-*ap++ = "send-mail";
+*ap++ = "sendmail";
*ap++ = "-i";
*ap++ = "-t";
cp = hp->h_from ? hp->h_from : value("from");
--
Lauri Tirkkonen | lotheac @ IRCnet
update ctype data to unicode 10
1f6e0 - 0x1f6ec 0x1f6f0 - 0x1f6f8 +PUNCT 0x1f680 - 0x1f6d4 0x1f6e0 - 0x1f6ec 0x1f6f0 - 0x1f6f8 +PRINT 0x1f680 - 0x1f6d4 0x1f6e0 - 0x1f6ec 0x1f6f0 - 0x1f6f8 +SWIDTH1 0x1f6c6 - 0x1f6cb 0x1f6cd - 0x1f6cf 0x1f6d3 - 0x1f6d4 +SWIDTH1 0x1f6e0 - 0x1f6ea 0x1f6f0 - 0x1f6f3 +SWIDTH2 0x1f680 - 0x1f6c5 0x1f6cc 0x1f6d0 - 0x1f6d2 0x1f6eb - 0x1f6ec +SWIDTH2 0x1f6f4 - 0x1f6f8 /* @@ -5503,10 +5802,15 @@ SWIDTH1 0x1f860 - 0x1f887 0x1f890 - 0x1f8ad * U+1F900 - U+1F9FF : Supplemental Symbols and Pictographs */ -GRAPH 0x1f910 - 0x1f918 0x1f980 - 0x1f984 0x1f9c0 -PUNCT 0x1f910 - 0x1f918 0x1f980 - 0x1f984 0x1f9c0 -PRINT 0x1f910 - 0x1f918 0x1f980 - 0x1f984 0x1f9c0 -SWIDTH1 0x1f910 - 0x1f918 0x1f980 - 0x1f984 0x1f9c0 +GRAPH 0x1f900 - 0x1f90b 0x1f910 - 0x1f93e 0x1f940 - 0x1f94c +GRAPH 0x1f950 - 0x1f96b 0x1f980 - 0x1f997 0x1f9c0 0x1f9d0 - 0x1f9e6 +PUNCT 0x1f900 - 0x1f90b 0x1f910 - 0x1f93e 0x1f940 - 0x1f94c +PUNCT 0x1f950 - 0x1f96b 0x1f980 - 0x1f997 0x1f9c0 0x1f9d0 - 0x1f9e6 +PRINT 0x1f900 - 0x1f90b 0x1f910 - 0x1f93e 0x1f940 - 0x1f94c +PRINT 0x1f950 - 0x1f96b 0x1f980 - 0x1f997 0x1f9c0 0x1f9d0 - 0x1f9e6 +SWIDTH1 0x1f900 - 0x1f90b +SWIDTH2 0x1f910 - 0x1f93e 0x1f940 - 0x1f94c 0x1f950 - 0x1f96b +SWIDTH2 0x1f980 - 0x1f997 0x1f9c0 0x1f9d0 - 0x1f9e6 /* @@ -5579,7 +5883,17 @@ SWIDTH2 0x2b820 - 0x2cea1 /* - * U+2CEB0 - U+2F7FF : No_Block + * U+2CEB0 - U+2EBEF : CJK Unified Ideographs Extension F + */ + +ALPHA 0x2ceb0 - 0x2ebe0 +GRAPH 0x2ceb0 - 0x2ebe0 +PRINT 0x2ceb0 - 0x2ebe0 +SWIDTH2 0x2ceb0 - 0x2ebe0 + + +/* + * U+2EBF0 - U+2F7FF : No_Block */ @@ -5603,9 +5917,10 @@ TODIGIT < 0x2f890 9 > * U+E - U+E007F : Tags */ -CONTROL 0xe0001 0xe0020 - 0xe007f +CONTROL 0xe0001 GRAPH 0xe0001 0xe0020 - 0xe007f PRINT 0xe0001 0xe0020 - 0xe007f +SPECIAL 0xe0020 - 0xe007f SWIDTH0 0xe0001 0xe0020 - 0xe007f -- Lauri Tirkkonen | lotheac @ IRCnet
smtpctl, mailer.conf: drop send-mail (was: mail(1): use "sendmail" as argv[0] for sendmail)
On Tue, Mar 19 2019 07:26:41 -0600, Todd C. Miller wrote:
> On Tue, 19 Mar 2019 10:33:07 +0200, Lauri Tirkkonen wrote:
>
> > ping - doesn't look removed yet :)
>
> Committed.
thanks. that means no need for smtpctl and mailer.conf to check for it
any more.
diff --git a/etc/mailer.conf b/etc/mailer.conf
index fff9e7adbd1..9c752022261 100644
--- a/etc/mailer.conf
+++ b/etc/mailer.conf
@@ -3,7 +3,6 @@
# Execute the "real" sendmail program, which is now smtpd by default
#
sendmail /usr/sbin/smtpctl
-send-mail /usr/sbin/smtpctl
mailq /usr/sbin/smtpctl
makemap/usr/sbin/smtpctl
newaliases /usr/sbin/smtpctl
diff --git a/usr.sbin/mailwrapper/mailer.conf.5
b/usr.sbin/mailwrapper/mailer.conf.5
index 288ff4f4c2e..2c303dedbdc 100644
--- a/usr.sbin/mailwrapper/mailer.conf.5
+++ b/usr.sbin/mailwrapper/mailer.conf.5
@@ -67,7 +67,6 @@ MTA suite:
.Bd -literal
# Emulate sendmail using smtpd
sendmail /usr/sbin/smtpctl
-send-mail /usr/sbin/smtpctl
mailq /usr/sbin/smtpctl
makemap/usr/sbin/smtpctl
newaliases /usr/sbin/smtpctl
@@ -80,7 +79,6 @@ MTA suite in place of
.Bd -literal
# Execute the "real" sendmail program
sendmail /usr/local/libexec/sendmail/sendmail
-send-mail /usr/local/libexec/sendmail/sendmail
mailq /usr/local/libexec/sendmail/sendmail
makemap/usr/local/libexec/sendmail/makemap
newaliases /usr/local/libexec/sendmail/sendmail
diff --git a/usr.sbin/smtpd/smtpctl.c b/usr.sbin/smtpd/smtpctl.c
index 4b44d9dd30a..9ebef45f159 100644
--- a/usr.sbin/smtpd/smtpctl.c
+++ b/usr.sbin/smtpd/smtpctl.c
@@ -1116,8 +1116,7 @@ sendmail_compat(int argc, char **argv)
gid_tgid;
int i, r;
- if (strcmp(__progname, "sendmail") == 0 ||
- strcmp(__progname, "send-mail") == 0) {
+ if (strcmp(__progname, "sendmail") == 0) {
/*
* determine whether we are called with flags
* that should invoke makemap/newaliases.
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: smtpctl, mailer.conf: drop send-mail (was: mail(1): use "sendmail" as argv[0] for sendmail)
On Tue, Mar 19 2019 14:18:06 -0600, Theo de Raadt wrote: > Have you verified there are *no other programs* in the entire greater > ecosystem > using that name? I have not. But eg. FreeBSD and NetBSD don't support these names in mailer.conf either: https://github.com/freebsd/freebsd/blob/master/etc/mail/mailer.conf https://github.com/NetBSD/src/blob/trunk/etc/mailer.conf so I'm not really worried about that. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: smtpctl, mailer.conf: drop send-mail (was: mail(1): use "sendmail" as argv[0] for sendmail)
On Tue, Mar 19 2019 15:16:10 -0600, Todd C. Miller wrote: > On Tue, 19 Mar 2019 22:15:17 +0200, Lauri Tirkkonen wrote: > > > thanks. that means no need for smtpctl and mailer.conf to check for it > > any more. > > Not so fast. There are still things in ports that use send-mail. > I haven't grepped the ports tree but the nmh port at least uses > "send-mail" for argv[0] in its spost utility. I might be blind, but 'grep -r send-mail /usr/ports/pobj/nmh-1.7.1' comes up empty for me after 'make extract' in /usr/ports/mail/nmh? -- Lauri Tirkkonen | lotheac @ IRCnet
Re: mail(1): use "sendmail" as argv[0] for sendmail
On Mon, Mar 04 2019 09:01:40 -0700, Todd C. Miller wrote: > On Mon, 04 Mar 2019 16:38:37 +0100, Gilles Chehade wrote: > > > I wish we had an historian who could enlighten us as to why both exist. > > That code actually predates sendmail and was in the original revision > when delivermail was still in use. Sendmail itself never checked > its argv[0] for "send-mail" as far as I know. > > At this point I consider it a historical oddity that is safe to > remove. ping - doesn't look removed yet :) -- Lauri Tirkkonen | lotheac @ IRCnet
xterm and wcwidth()
Hi,
it appears xterm tests the system's wcwidth() function on startup,
comparing the results between it and the wcwidth implementation it ships
itself (mk_wcwidth()), for each wchar_t value between 0 and 0x. This
is done in systemWcwidthOk(), called from decode_wcwidth().
I turned some TRACE()'s into printf()'s in util.c (by hand, since
OPT_TRACE didn't compile), and it turns out that xterm deems the system
wcwidth() inadequate, both before the unicode10 update:
% /usr/xenocara/app/xterm/obj/xterm
systemWcwidthOk: 656/55533 mismatches, allowed 655
using MK wcwidth() function
and after it:
% /usr/xenocara/app/xterm/obj/xterm
systemWcwidthOk: 656/55516 mismatches, allowed 655
using MK wcwidth() function
in other words, xterm is (and was) using its own idea of how many
columns characters take up. The way this manifested itself to me was
that I received some email that contained emoji characters in the
subject, and they look fine in mutt when I use another terminal (st on
OpenBSD, or termux on my Android phone), but on 'xterm -fa "DejaVu Sans
Mono"' on OpenBSD, only the left half of these characters is rendered,
and mutt's index background color indicating the selection doesn't reach
the right edge of the screen: libc wcwidth() returns 2, but xterm's idea
seems to be 1 column.
Another way this problem manifests is typing one of these
mismatched-width characters in bash or zsh, and erasing it with
backspace: the character is rendered in one column, but on backspace the
cursor goes backward two columns, erasing part of the prompt. ksh
doesn't seem to have this problem.
I feel like xterm should just use the system wcwidth() to avoid these
mismatches, so rudimentary diff to do that below. Using the default
font, this makes emojis render as double-wide boxes; using '-fa "DejaVu
Sans Mono"', they actually render correctly. In both cases the width
mismatches in mutt and bash/zsh are fixed.
diff --git a/app/xterm/util.c b/app/xterm/util.c
index a3de4a005..9b6443683 100644
--- a/app/xterm/util.c
+++ b/app/xterm/util.c
@@ -4980,61 +4980,6 @@ decode_keyboard_type(XtermWidget xw, XTERM_RESOURCE * rp)
}
#if OPT_WIDE_CHARS
-#if defined(HAVE_WCHAR_H) && defined(HAVE_WCWIDTH)
-/*
- * If xterm is running in a UTF-8 locale, it is still possible to encounter
- * old runtime configurations which yield incomplete or inaccurate data.
- */
-static Bool
-systemWcwidthOk(int samplesize, int samplepass)
-{
-wchar_t n;
-int oops = 0;
-
-for (n = 21; n <= 25; ++n) {
- wchar_t code = (wchar_t) dec2ucs(NULL, (unsigned) n);
- int system_code = wcwidth(code);
- int intern_code = mk_wcwidth(code);
-
- /*
-* Solaris 10 wcwidth() returns "2" for all of the line-drawing (page
-* 0x2500) and most of the geometric shapes (a few are excluded, just
-* to make it more difficult to use). Do a sanity check to avoid using
-* it.
-*/
- if ((system_code < 0 && intern_code >= 1)
- || (system_code >= 0 && intern_code != system_code)) {
- TRACE(("systemWcwidthOk: broken system line-drawing wcwidth\n"));
- oops += (samplepass + 1);
- break;
- }
-}
-
-for (n = 0; n < (wchar_t) samplesize; ++n) {
- int system_code = wcwidth(n);
- int intern_code = mk_wcwidth(n);
-
- /*
-* When this check was originally implemented, there were few if any
-* libraries with full Unicode coverage. Time passes, and it is
-* possible to make a full comparison of the BMP. There are some
-* differences: mk_wcwidth() marks some codes as combining and some
-* as single-width, differing from GNU libc.
-*/
- if ((system_code < 0 && intern_code >= 1)
- || (system_code >= 0 && intern_code != system_code)) {
- TRACE((".. width(U+%04X) = %d, expected %d\n",
- (unsigned) n, system_code, intern_code));
- if (++oops > samplepass)
- break;
- }
-}
-TRACE(("systemWcwidthOk: %d/%d mismatches, allowed %d\n",
- oops, (int) n, samplepass));
-return (oops <= samplepass);
-}
-#endif /* HAVE_WCWIDTH */
-
void
decode_wcwidth(XtermWidget xw)
{
@@ -5045,8 +4990,7 @@ decode_wcwidth(XtermWidget xw)
switch (mode) {
default:
#if defined(HAVE_WCHAR_H) && defined(HAVE_WCWIDTH)
- if (xtermEnvUTF8() &&
- systemWcwidthOk(xw->misc.mk_samplesize, xw->misc.mk_samplepass)) {
+ if (xtermEnvUTF8()) {
my_wcwidth = wcwidth;
TRACE(("using system wcwidth() function\n"));
break;
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: smtpctl, mailer.conf: drop send-mail (was: mail(1): use "sendmail" as argv[0] for sendmail)
On Tue, Mar 19 2019 15:28:48 -0600, Todd C. Miller wrote: > On Tue, 19 Mar 2019 23:24:24 +0200, Lauri Tirkkonen wrote: > > > I might be blind, but 'grep -r send-mail /usr/ports/pobj/nmh-1.7.1' > > comes up empty for me after 'make extract' in /usr/ports/mail/nmh? > > It looks like spost.c is historical code that is not actually packaged > in the distribution (though it is in the nmh repo). I did 'make fetch' in /usr/ports, extracted the results by hand (I avoided 'make extract' because that would try to compile and install dependencies), and did grep -rw send-mail on the directory where I extracted the distfiles. As far as I can tell there is nothing in ports that uses "send-mail" as argv[0] to anything. The false positives are: - 364 occurrences in emacs-26.1/; these are references to 'send-mail-function', 'send-mail-with-sendmail', 'message-send-mail', 'message-send-mail-hook', etc. - 362 occurrences in xemacs-packages/; similar to above. - 11 occurrences in gettext-0.19.8.1/; references to 'po-send-mail' in po-mode.el - 11 occurrences in mgetty-1.1.37/; options called 'success-send-mail' and 'failure-send-mail', in faxrunq/faxrunqd - 1 occurrence in git-2.21.0/Documentation/RelNotes/1.6.0.txt; typo of 'git-send-mail', when it means to say 'git-send-email'. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: install(1) could fail due to race
On Sun, Jan 27 2019 10:37:52 -0500, Ted Unangst wrote: > Ingo Schwarze wrote: > > If people here agree with the general direction of making -S the > > default and removing the fragile non-S mode (see the patch below), > > i'll run a full make build and make release and then ask for OKs. > > Just checking we didn't forget about this. Seems the right thing to do. weekly ping. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: grep: convert fgetln to getline
On Wed, Jan 23 2019 18:01:24 -0500, Ted Unangst wrote:
> Lauri Tirkkonen wrote:
> > > > oh, interesting. that's sloppy. can you please fix that first,
> > > > separately?
> > >
> > > Sure, here it is.
> >
> > Could you please take a look at it? It's been a couple weeks.
>
> yup, sorry, slipped by. committed.
Thanks. What about the fgetln->getline diff (same as I sent previously
applies cleanly; reattached below)? I know you said you're concerned
about the extra copy, but as I pointed out the getline code path will
only be taken if mmopen() fails, the file is not seekable, or if grep
was compiled -DSMALL.
diff --git a/usr.bin/grep/file.c b/usr.bin/grep/file.c
index 4b3c689e4ab..87c49dd5cc0 100644
--- a/usr.bin/grep/file.c
+++ b/usr.bin/grep/file.c
@@ -34,10 +34,8 @@
#include "grep.h"
static char fname[PATH_MAX];
-#ifndef NOZ
static char*lnbuf;
-static size_t lnbuflen;
-#endif
+static size_t lnbufsize;
#define FILE_STDIO 0
#define FILE_MMAP 1
@@ -73,9 +71,9 @@ gzfgetln(gzFile *f, size_t *len)
else
errx(2, "%s: %s", fname, gzerrstr);
}
- if (n >= lnbuflen) {
- lnbuflen *= 2;
- lnbuf = grep_realloc(lnbuf, ++lnbuflen);
+ if (n >= lnbufsize) {
+ lnbufsize *= 2;
+ lnbuf = grep_realloc(lnbuf, ++lnbufsize);
}
if (c == '\n')
break;
@@ -182,7 +180,13 @@ grep_fgetln(file_t *f, size_t *l)
{
switch (f->type) {
case FILE_STDIO:
- return fgetln(f->f, l);
+ if ((*l = getline(, , f->f)) == -1) {
+ if (ferror(f->f))
+ err(2, "%s: getline", fname);
+ else
+ return NULL;
+ }
+ return lnbuf;
#ifndef SMALL
case FILE_MMAP:
return mmfgetln(f->mmf, l);
diff --git a/usr.bin/grep/grep.c b/usr.bin/grep/grep.c
index 913cc97a0f3..cfac24b12aa 100644
--- a/usr.bin/grep/grep.c
+++ b/usr.bin/grep/grep.c
@@ -224,15 +224,19 @@ read_patterns(const char *fn)
{
FILE *f;
char *line;
- size_t len;
+ ssize_t len;
+ size_t linesize;
if ((f = fopen(fn, "r")) == NULL)
err(2, "%s", fn);
- while ((line = fgetln(f, )) != NULL)
+ line = NULL;
+ linesize = 0;
+ while ((len = getline(, , f)) != -1)
add_pattern(line, *line == '\n' ? 0 : len);
if (ferror(f))
err(2, "%s", fn);
fclose(f);
+ free(line);
}
int
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: grep: convert fgetln to getline
On Thu, Jan 24 2019 04:22:20 -0700, Theo de Raadt wrote: > I would like to know if this does more malloc. I worry it is an additional > level of malloc per line. It does do more malloc than plain fgetln since fgetln does no copying, but nowhere near every line. The same buffer lnbuf is used for each line, and libc getline() reallocates it if it is not large enough. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: grep: convert fgetln to getline
On Thu, Jan 24 2019 04:40:08 -0700, Theo de Raadt wrote: > >On Thu, Jan 24 2019 04:22:20 -0700, Theo de Raadt wrote: > >> I would like to know if this does more malloc. I worry it is an additional > >> level of malloc per line. > > > >It does do more malloc than plain fgetln since fgetln does no copying, > >but nowhere near every line. The same buffer lnbuf is used for each > >line, and libc getline() reallocates it if it is not large enough. > > If there is more allocation, it will be more expensive. Our malloc > has many checks and also fresh allocations trigger expensive code paths > in mmap intentionally -- this makes our runtime somewhat slower, but finds > and fixes bugs in the same software running on other operating systems > who lack our attitude towards rigor over performance). > > question is, are you fixing a stylistic nit, or making it fundamentally > better. I do think I am making it fundamentally better, since fgetln is a broken interface (it returns lines that are not null terminated). The actual reason I started doing this was porting grep to an OS without fgetln(). But I don't think it's a bad thing to reduce fgetln usage in OpenBSD either. > A bunch of us run a whole ton of grep during our work... > > that's my basic question. Sure, I understand this and I do agree grep performance matters. I just want to reiterate that this diff only affects performance if the mmap code path is not taken (ie. either stream is not seekable (determined with isatty), or if grep was built -DSMALL (bsd.rd grep I think), or if mmap fails). And even then I think the cost is negligible: getline grows the buffer in powers of 2, so only lines that happen to be twice as long as any previously encountered line pay the price. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: grep: convert fgetln to getline
On Thu, Jan 24 2019 13:58:04 +0200, Lauri Tirkkonen wrote: > And even then I think the cost is negligible: getline grows > the buffer in powers of 2, so only lines that happen to be twice as long > as any previously encountered line pay the price. Actually, I went to skim the fgetln() implementation and it might very well be that fgetln() does *more* reallocations than getline. The storage that fgetln() uses is hidden from the user, but obviously it has to store its buffer somewhere. It's a member of struct FILE: 125 /* separate buffer for fgetln() when line crosses buffer boundary */ 126 struct __sbuf _lb; /* buffer for fgetln() */ and it is expanded by __slbexpand() in stdio/fgetln.c, which just does recallocarray(). To me it looks like the buffer is either incremented by OPTIMISTIC (#define'd to be 80 bytes), or again by exactly as much as is required by the current line if that wasn't enough. I haven't done any actual measurements though, so it's possible my reading is wrong. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: grep: convert fgetln to getline
On Wed, Jan 30 2019 20:32:50 -0500, Ted Unangst wrote: > Thanks for digging into this. I went ahead and committed your diff. Thanks for committing it. You know, having seen fgetln's allocation strategy and its usage of the stdio internals, I couldn't help but wonder if it's something that could be eventually removed entirely. It's used in a bunch of places in-tree for sure, but not too many to convert, I think; and I've already done a few... It's not exactly a serious suggestion at this point; I realize there probably is third-party software too that uses this function (Linuxes provide compat for it in libbsd; there must be a reason for that). I'm speaking as someone who's been removing a bunch of crap from that other OS I mentioned, so that's my reason for this line of thinking slash pipe-dreaming ;) -- Lauri Tirkkonen | lotheac @ IRCnet
Re: net80211: fix ifconfig mode command
On Mon, Apr 15 2019 16:56:52 +0200, Stefan Sperling wrote: > The ifconfig mode command is broken; It is supposed to force a wireless > interface into 11a/b/g/n media mode. This stopped working some time ago, > probably during my work on background scanning. Problem spotted by mlarkin@ > who noticed that interfaces were using 11g mode while forced to 11b mode. > > The diff below allows me to force media modes again on iwm(4), urtwn(4), > and athn(4), with e.g. 'ifconfig iwm0 mode 11b' More tests welcome. applied both your patches and it works properly on iwn(4) here. before applying 'mode 11b' or 'mode 11g' on my laptop would not actually disconnect from my 5GHz-only AP, and the ifconfig output was something like "mode 11b (HT-MCS0 mode 11n)" - after, it works as expected. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: ssh_config.5: refer consistently to HostName, not Hostname
On Tue, Jun 11 2019 14:40:03 +0100, Jason McIntyre wrote:
> On Tue, Jun 11, 2019 at 03:11:03PM +0300, Lauri Tirkkonen wrote:
> > Hi, trivial manpage diff.
> >
>
> fixed, thanks.
> jmc
I noticed the followup commit that changes the canonical option name to
'Hostname'. I also like that capitalization better, but your diff seems
to have been a bit incomplete; in particular the option should be called
oHostname in code also, and a couple manpage mentions of HostName still
remain. X11UseLocalhost is used consistently in comparison.
diff --git a/usr.bin/ssh/clientloop.c b/usr.bin/ssh/clientloop.c
index fdcca0e25ed..fd43ee4cbb4 100644
--- a/usr.bin/ssh/clientloop.c
+++ b/usr.bin/ssh/clientloop.c
@@ -121,7 +121,7 @@ extern int muxserver_sock; /* XXX use mux_client_cleanup()
instead */
/*
* Name of the host we are connecting to. This is the name given on the
- * command line, or the HostName specified for the user-supplied name in a
+ * command line, or the Hostname specified for the user-supplied name in a
* configuration file.
*/
extern char *host;
diff --git a/usr.bin/ssh/readconf.c b/usr.bin/ssh/readconf.c
index 27638663b78..5b9c97b7c8d 100644
--- a/usr.bin/ssh/readconf.c
+++ b/usr.bin/ssh/readconf.c
@@ -71,7 +71,7 @@
User foo
Host fake.com
- HostName another.host.name.real.org
+ Hostname another.host.name.real.org
User blaah
Port 34289
ForwardX11 no
@@ -133,7 +133,7 @@ typedef enum {
oGatewayPorts, oExitOnForwardFailure,
oPasswordAuthentication, oRSAAuthentication,
oChallengeResponseAuthentication, oXAuthLocation,
- oIdentityFile, oHostName, oPort, oCipher, oRemoteForward, oLocalForward,
+ oIdentityFile, oHostname, oPort, oCipher, oRemoteForward, oLocalForward,
oCertificateFile, oAddKeysToAgent, oIdentityAgent,
oUser, oEscapeChar, oRhostsRSAAuthentication, oProxyCommand,
oGlobalKnownHostsFile, oUserKnownHostsFile, oConnectionAttempts,
@@ -225,7 +225,7 @@ static struct {
{ "certificatefile", oCertificateFile },
{ "addkeystoagent", oAddKeysToAgent },
{ "identityagent", oIdentityAgent },
- { "hostname", oHostName },
+ { "hostname", oHostname },
{ "hostkeyalias", oHostKeyAlias },
{ "proxycommand", oProxyCommand },
{ "port", oPort },
@@ -1102,7 +1102,7 @@ parse_char_array:
max_entries = SSH_MAX_HOSTS_FILES;
goto parse_char_array;
- case oHostName:
+ case oHostname:
charptr = >hostname;
goto parse_string;
@@ -2576,7 +2576,7 @@ dump_client_config(Options *o, const char *host)
/* Most interesting options first: user, host, port */
dump_cfg_string(oUser, o->user);
- dump_cfg_string(oHostName, host);
+ dump_cfg_string(oHostname, host);
dump_cfg_int(oPort, o->port);
/* Flag options */
diff --git a/usr.bin/ssh/scp.1 b/usr.bin/ssh/scp.1
index a2833dab041..29e97808ed1 100644
--- a/usr.bin/ssh/scp.1
+++ b/usr.bin/ssh/scp.1
@@ -164,7 +164,7 @@ For full details of the options listed below, and their
possible values, see
.It HostbasedKeyTypes
.It HostKeyAlgorithms
.It HostKeyAlias
-.It HostName
+.It Hostname
.It IdentitiesOnly
.It IdentityAgent
.It IdentityFile
diff --git a/usr.bin/ssh/sftp.1 b/usr.bin/ssh/sftp.1
index 259095885a7..a6757705941 100644
--- a/usr.bin/ssh/sftp.1
+++ b/usr.bin/ssh/sftp.1
@@ -241,7 +241,7 @@ For full details of the options listed below, and their
possible values, see
.It HostbasedKeyTypes
.It HostKeyAlgorithms
.It HostKeyAlias
-.It HostName
+.It Hostname
.It IdentitiesOnly
.It IdentityAgent
.It IdentityFile
diff --git a/usr.bin/ssh/ssh.1 b/usr.bin/ssh/ssh.1
index 9480eba8d3e..4c2c101d765 100644
--- a/usr.bin/ssh/ssh.1
+++ b/usr.bin/ssh/ssh.1
@@ -504,7 +504,7 @@ For full details of the options listed below, and their
possible values, see
.It HostbasedKeyTypes
.It HostKeyAlgorithms
.It HostKeyAlias
-.It HostName
+.It Hostname
.It IdentitiesOnly
.It IdentityAgent
.It IdentityFile
diff --git a/usr.bin/ssh/ssh.c b/usr.bin/ssh/ssh.c
index defa3c1c337..aeefbe4f91c 100644
--- a/usr.bin/ssh/ssh.c
+++ b/usr.bin/ssh/ssh.c
@@ -146,7 +146,7 @@ char *config = NULL;
/*
* Name of the host we are connecting to. This is the name given on the
- * command line, or the HostName specified for the user-supplied name in a
+ * command line, or the Hostname specified for the user-supplied name in a
* configuration file.
*/
char *host;
diff --git a/usr.bin/ssh/ssh_config.5 b/usr.bin/ssh/ssh_config.5
index 7bb6cde7ae1..719288b03a5 100644
--- a/usr.bin/ssh/ssh_config.5
+++ b/usr.bin/ssh/ssh_config.5
@@ -1223,8 +1223,8 @@ server running on some machine, or execute
.Ic sshd -i
somewhere.
Host key management will be done using the
-HostName of the host being connected (defaulting to t
ssh_config.5: refer consistently to HostName, not Hostname
Hi, trivial manpage diff. diff --git a/usr.bin/ssh/ssh_config.5 b/usr.bin/ssh/ssh_config.5 index b8ba4022a56..1f2094cb981 100644 --- a/usr.bin/ssh/ssh_config.5 +++ b/usr.bin/ssh/ssh_config.5 @@ -203,7 +203,7 @@ The criteria for the .Cm host keyword are matched against the target hostname, after any substitution by the -.Cm Hostname +.Cm HostName or .Cm CanonicalizeHostname options. -- Lauri Tirkkonen | lotheac @ IRCnet
ftplist.cgi & ftpinstall.cgi source code
Hi tech@, I was doing an install into a fresh VM, and the installer was able to guess the (non-public) mirror I want to use correctly; I got curious and ran into ftplist.cgi in install.sub. That in itself made me more curious, but I was unable to find the source code for these CGI scripts anywhere. Is it available somewhere? -- Lauri Tirkkonen | lotheac @ IRCnet
Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior
rently built serially, which has quite a large impact on how many jobs are running at any one time. An alternative could be to use unix sockets with paths instead and pass the path through the environment, but I'm not convinced I like that any better. > The first step would definitely be to check how your patch works in > real conditions (at least a full make release, and maybe a significant > subset of ports). > > Assuming this goes well, for starters, I'd like the change to be way > more clearly separated, to not included gratuitous changes to existing code, > to look like OpenBSD code, and to have decent error paths. Thanks; I take it this means there is at least a little interest in this proof of concept :) I'll resume work on this to polish it more, fix the remaining bugs and try to address your concerns. I did try to make it look like OpenBSD code, but obviously failed on some counts; please bear with me with any future mistakes I might make on that path. And I suppose I'll need some help on the "compiles on all architectures" part eventually; I just have amd64/arm64 stuff. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior
On Fri, Nov 15 2019 14:16:44 +0100, Martin Pieuchot wrote: > > Yes, that was my point exactly. Less jobs didn't fare any better (well, > > it had less spin time, but took around the same real time), so the > > conclusion I arrived at was that something in my setup was eventually > > contending on a small number of locks. My guess is that it's either the > > filesystem, the IDE driver, something Hyper-V specific, or a combination > > of the above. > > What does it bring to guess? Why don't you look deeper where the > contention is? > > > This change is all about utilizing CPUs better in parallelizing existing > > workloads, so I wouldn't expect a very large change in user time (but it > > should happen over a smaller amout of real time). > > Is this change about better parallelizing? Do we see that? Or is it a > guess? If we want OpenBSD to do a better job at parallelizing maybe we > should look at where the contention is and then how to get rid of it? > > > > You can also write Makefiles that expose less the limitation of the > > > system. ktrace(1) is your friend for that. > > > > The idea was to test real-world workloads, ie. actual OpenBSD builds. I > > do have enough memory on this thing to place objs in mfs; maybe I'll try > > that next time around. > > I'd suggest you to spend your time understanding where is the bottleneck > instead of randomly trying to change stuff :) Your points are valid and I agree with them completely. There are clearly problems with lock contention, and there are problems with resource utilisation due to make not starting enough jobs. While they are related, I kind of have to pick my battles one by one not to descend too far into yak shaving territory. Given infinite time, let's fix everything, but since that is not afforded to me, some educated guesses have to be made to be able to test the right thing in the meantime. ;) -- Lauri Tirkkonen | lotheac @ IRCnet
Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior
Hi Martin, On Thu, Nov 14 2019 17:57:20 +0100, Martin Pieuchot wrote: > On 14/11/19(Thu) 17:30, Lauri Tirkkonen wrote: > > [...] > > The first test I made was on a beefy virtual machine under Hyper-V, > > with 32 vCPUs on a Threadripper 2950X. With and without this patch (and > > the followup for share/mk), I did make -j32 -C /usr/src/usr.bin. The > > results were very disappointing: with my patches the build was *slower* > > even though it used way more CPU. I think 'top' said it best: > > > > 32 CPUs: 0.1% user, 0.0% nice, 3.9% sys, 95.4% spin, 0.0% intr, 0.7% > > idle > > ^^ > > I'd suggest considering the %user time when working on a userland tool. > A high %spin time indicates that syscalls and page faults are spending > a lot of time busy waiting on the KERNEL_LOCK(). > > In other words you're exposing the non-scalability of OpenBSD on such > machine. I'd suggest you use less jobs, I'd try 8 or 12 as a first step. Yes, that was my point exactly. Less jobs didn't fare any better (well, it had less spin time, but took around the same real time), so the conclusion I arrived at was that something in my setup was eventually contending on a small number of locks. My guess is that it's either the filesystem, the IDE driver, something Hyper-V specific, or a combination of the above. SATA on bare metal didn't spin nearly as badly, which is why I suspected IDE. But this is a bit of a digression :) This change is all about utilizing CPUs better in parallelizing existing workloads, so I wouldn't expect a very large change in user time (but it should happen over a smaller amout of real time). > You can also write Makefiles that expose less the limitation of the > system. ktrace(1) is your friend for that. The idea was to test real-world workloads, ie. actual OpenBSD builds. I do have enough memory on this thing to place objs in mfs; maybe I'll try that next time around. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior
On Fri, Nov 15 2019 15:24:57 +0200, Lauri Tirkkonen wrote: > Your points are valid and I agree with them completely. There are > clearly problems with lock contention, and I should mention here that I would probably not have observed the magnitude of these problems had I not made make start more jobs at once in the first place. So while the make/mk diffs need more work, we've already identified more areas of improvement as a result of trying them out. But one thing at a time. :) -- Lauri Tirkkonen | lotheac @ IRCnet
Re: make 'ifconfig scan' trigger a background scan
Hi, a small, hopefully not too bikesheddy note from a user perspective: On Thu, Nov 07 2019 08:59:54 +, Stuart Henderson wrote: > On 2019/11/06 19:17, Stefan Sperling wrote: > > This diff allows the root user to trigger a background scan with: > > > > ifconfig iwm0 scan > > > > It supports two use cases which are currently not supported: > > > > 1) It will force an attempt at finding a better AP, even if the > > current AP is above the signal level threshold which will usually > > trigger a background scan. > > > > 2) It will update the list of cached APs. The updated list will be > > shown by subsequent invocations of 'ifconfig scan'. This allows the > > root user to view an up-to-date list of available networks without > > disassociating from the current AP. Currently, the list is updated only > > if a background scan is triggered via the signal strength threshold. > > > > Suggested by sthen@ > > > > ok? > > I'm finding this very helpful, if I'm on a low signal but working AP and > suspect that another one may be better this allows me to easily rescan > and (fairly seamlessly) move to a better one. Latency is a little higher > for a few seconds while the scan completes but packets continue flowing. > > Currently the only way to trigger a scan on a bgscan interface (unless > moving to a low signal area) is ifconfig down+up which interrupts > connectivity. > > The main thing I'm unsure about is whether to restrict this to root (as > this diff does) or not, there are arguments both ways - some will think > that it's also useful for an unprivileged user to be able to trigger a > search for a better AP, some will think that it's more useful to prevent > an unprivileged [possibly remote] user from being able to potentially > trigger a move to a different AP. Anyway this is an improvement and we > could remove the restriction later if wanted, so I'm OK with this as-is. I think it might be a little confusing to make this operation implicit in another operation (ie. list APs), and for root only. How does a user find out about this feature? Wouldn't it be better to have a separate command to trigger the scan, eg. 'rescan'? That could be documented and restricted to root. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: make 'ifconfig scan' trigger a background scan
On Thu, Nov 07 2019 09:22:40 +, Stuart Henderson wrote: > > Wouldn't it be better to have a separate > > command to trigger the scan, eg. 'rescan'? That could be documented and > > restricted to root. > > But the command is not "ifconfig list_aps", it's "ifconfig scan" and > it's really expected that this would trigger a scan. To me the current > behaviour (where "ifconfig iwm0 scan" might give you a list from 300km > while you were travelling connected to your phone hotspot) with no > way to update it short of forcing a disconnection is more confusing. > > It's not ideal that "ifconfig scan" prints an old list at all, but that > is the current status and this diff doesn't change that. Still it's an > improvement. Could it be made better still? Yes, but I don't see that as > a reason to hold off on making *this* change. Yeah, I totally agree. -- Lauri Tirkkonen | lotheac @ IRCnet
[PATCH] mk: build subdirs in parallel
Followup diff to mk that builds subdirs in parallel instead of a serial
shell loop. Like the parent diff, this is just a lightly tested proof of
concept. In particular the '===> subdir' printouts are not quite so
useful if commands are output out of order.
---
share/mk/bsd.subdir.mk | 92 +++---
1 file changed, 41 insertions(+), 51 deletions(-)
diff --git a/share/mk/bsd.subdir.mk b/share/mk/bsd.subdir.mk
index 062defef7e1..8a4106e5b3e 100644
--- a/share/mk/bsd.subdir.mk
+++ b/share/mk/bsd.subdir.mk
@@ -10,56 +10,53 @@
SKIPDIR?=
_SUBDIRUSE: .USE
-.if defined(SUBDIR)
- @for entry in ${SUBDIR}; do \
- set -e; if test -d ${.CURDIR}/$${entry}.${MACHINE}; then \
- _newdir_="$${entry}.${MACHINE}"; \
- else \
- _newdir_="$${entry}"; \
- fi; \
- if test X"${_THISDIR_}" = X""; then \
- _nextdir_="$${_newdir_}"; \
- else \
- _nextdir_="$${_THISDIR_}/$${_newdir_}"; \
- fi; \
- _makefile_spec_=""; \
- if [ -e ${.CURDIR}/$${_newdir_}/Makefile.bsd-wrapper ]; then \
- _makefile_spec_="-f Makefile.bsd-wrapper"; \
- fi; \
- subskipdir=''; \
- for skipdir in ${SKIPDIR}; do \
- subentry=$${skipdir#$${entry}}; \
- if [ X$${subentry} != X$${skipdir} ]; then \
- if [ X$${subentry} = X ]; then \
- echo "($${_nextdir_} skipped)"; \
- break; \
- fi; \
- subskipdir="$${subskipdir} $${subentry#/}"; \
- fi; \
- done; \
- if [ X$${skipdir} = X -o X$${subentry} != X ]; then \
- echo "===> $${_nextdir_}"; \
- ${MAKE} -C ${.CURDIR}/$${_newdir_} \
- SKIPDIR="$${subskipdir}" \
- $${_makefile_spec_} _THISDIR_="$${_nextdir_}" \
- ${MAKE_FLAGS} \
- ${.TARGET:S/^real//}; \
- fi; \
- done
-${SUBDIR}::
- @set -e; if test -d ${.CURDIR}/${.TARGET}.${MACHINE}; then \
- _newdir_=${.TARGET}.${MACHINE}; \
+.if defined(SUBDIR)
+_SUBDIRUSE: ${SUBDIR}
+. for t in all cleandir includes depend obj tags manlint
+. if make($t)
+. for dir in ${SUBDIR}
+${dir}::
+ @entry=${dir}; \
+ set -e; if test -d ${.CURDIR}/$${entry}.${MACHINE}; then \
+ _newdir_="$${entry}.${MACHINE}"; \
+ else \
+ _newdir_="$${entry}"; \
+ fi; \
+ if test X"${_THISDIR_}" = X""; then \
+ _nextdir_="$${_newdir_}"; \
else \
- _newdir_=${.TARGET}; \
+ _nextdir_="$${_THISDIR_}/$${_newdir_}"; \
fi; \
_makefile_spec_=""; \
- if [ -f ${.CURDIR}/$${_newdir_}/Makefile.bsd-wrapper ]; then \
+ if [ -e ${.CURDIR}/$${_newdir_}/Makefile.bsd-wrapper ]; then \
_makefile_spec_="-f Makefile.bsd-wrapper"; \
fi; \
- echo "===> $${_newdir_}"; \
- exec ${MAKE} -C ${.CURDIR}/$${_newdir_} ${MAKE_FLAGS} \
- $${_makefile_spec_} _THISDIR_="$${_newdir_}" all
+ subskipdir=''; \
+ for skipdir in ${SKIPDIR}; do \
+ subentry=$${skipdir#$${entry}}; \
+ if [ X$${subentry} != X$${skipdir} ]; then \
+ if [ X$${subentry} = X ]; then \
+ echo "($${_nextdir_} skipped)"; \
+ break; \
+ fi; \
+ subskipdir="$${subskipdir} $${subentry#/}"; \
+ fi; \
+ done; \
+ if [ X$${skipdir} = X -o X$${subentry} != X ]; then \
+ echo "===> $${_nextdir_}"; \
+ ${MAKE} -C ${.CURDIR}/$${_newdir_} \
+ SKIPDIR="$${subskipdir}" \
+ $${_makefile_spec_} _THISDIR_="$${_nextdir_}" \
+ ${MAKE_FLAGS} \
+ ${t:S/^real//}; \
+ fi
+. if !target($t)
+$t: _SUBDIRUSE
+. endif
+. endfor
+. endif
+. endfor
.endif
.if !target(install)
@@ -74,13 +71,6 @@ maninstall: afterinstall
afterinstall: realinstall
realinstall: beforeinstall _SUBDIRUSE
.endif
-
-
-.for t in all cleandir includes depend obj tags manlint
-. if !target($t)
-$t: _SUBDIRUSE
-. endif
-.endfor
.if !target(regress) && empty(.TARGETS:Mall)
regress: _SUBDIRUSE
.endif
--
2.23.0
--
Lauri Tirkkonen | lotheac @ IRCnet
[PATCH] make: implement jobserver and use it to avoid exponential behavior
87,7 +187,7 @@ MainParseArgs(int argc, char **argv)
{
int c, optend;
-#define OPTFLAGS "BC:D:I:SV:d:ef:ij:km:npqrst"
+#define OPTFLAGS "BC:D:I:J:SV:d:ef:ij:km:npqrst"
#define OPTLETTERS "BSiknpqrst"
if (pledge("stdio rpath wpath cpath fattr proc exec", NULL) == -1)
@@ -218,6 +218,19 @@ MainParseArgs(int argc, char **argv)
Parse_AddIncludeDir(optarg);
record_option(c, optarg);
break;
+ case 'J': {
+ const char *errstr;
+
+ jobserverfd = strtonum(optarg, STDERR_FILENO + 1,
+ INT_MAX, );
+ if (errstr != NULL)
+ errx(1, "illegal argument -J%s: "
+ "fd number is %s", optarg, errstr);
+ if (fcntl(jobserverfd, F_GETFD, 0) < 0)
+ err(1, "illegal argument -J%d", jobserverfd);
+ record_option(c, optarg);
+ break;
+ }
case 'V':
Lst_AtEnd(, optarg);
record_option(c, optarg);
@@ -723,6 +736,17 @@ main(int argc, char **argv)
if (!forceJobs)
compatMake = true;
+ Job_Init(maxJobs, jobserverfd);
+ if (jobserverfd == -1) {
+ char optstr[16];
+
+ /* We are jobserver master; add -J for children */
+ if (snprintf(optstr, sizeof(optstr), "%d",
+ jobserver_get_slave_fd()) < 0)
+ err(1, "could not create -J option string");
+ record_option('J', optstr);
+ }
+
/* And set up everything for sub-makes */
Var_AddCmdline(MAKEFLAGS);
@@ -796,7 +820,6 @@ main(int argc, char **argv)
else
Targ_FindList(, create);
- Job_Init(maxJobs);
/* If the user has defined a .BEGIN target, execute the commands
* attached to it. */
if (!queryFlag)
diff --git a/usr.bin/make/main.h b/usr.bin/make/main.h
index 469487ee058..41a74af9d0d 100644
--- a/usr.bin/make/main.h
+++ b/usr.bin/make/main.h
@@ -29,6 +29,9 @@
/* main
* User interface to make.
*/
+
+#define MAKEFLAGS ".MAKEFLAGS"
+
/* Main_ParseArgLine(string);
* Parse string as a line of arguments, and treats them as if they
* were given at make's invocation. Used to implement .MFLAGS. */
--
2.23.0
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior
On Wed, Nov 27 2019 11:49:00 +0100, Marc Espie wrote:
> On Wed, Nov 27, 2019 at 12:42:48PM +0200, Lauri Tirkkonen wrote:
> > In a lesson to always proof-read *before* sending your message:
> >
> > On Wed, Nov 27 2019 12:31:51 +0200, Lauri Tirkkonen wrote:
> > > - a full release build with the jobserver enabled passed after
> > >disabling gnu/usr.bin/binutils
> >
> > this should, in fact, read "gnu/usr.bin/binutils-2.17". binutils builds
> > fine.
>
> We would need to investigate this.
>
> Instead of disabling them, send logs
Sure; I just considered these separate problems that happened to be
exposed by the jobserver usage ('expensive' avoids these races since it
does not run more than one sub-make at a time).
The problem I encountered with binutils-2.17 seems to be that configure is run
in 'intl' more than once at the same time. pstree output seems to
support my hypothesis:
| | |-+= 94071 lotheac make -f Makefile.bsd-wrapper -j16 -x
| | | \-+- 26584 lotheac /bin/sh -ec SUBDIRS='opcodes bfd binutils ld gas'
CONFIGURE_HOST_MODULES='configure-
| | | \-+- 71033 lotheac make CC=cc CFLAGS=-O2 -pipe -DPIE_DEFAULT=1
LDFLAGS= scriptdir=/usr/libdata toold
| | | |-+- 08037 lotheac /bin/sh -ec r=`${PWDCMD-pwd}`; export r;
s=`cd /usr/src/gnu/usr.bin/binutils-2.1
| | | | \-+- 57596 lotheac make DESTDIR= RPATH_ENVVAR=LD_LIBRARY_PATH
TARGET_SUBDIR=amd64-unknown-openbsd6
| | | | \-+- 46889 lotheac /bin/sh -ec r=`${PWDCMD-pwd}`; export r;
s=`cd /usr/src/gnu/usr.bin/binutils
| | | | \-+- 07550 lotheac /bin/sh
/usr/src/gnu/usr.bin/binutils-2.17/intl/configure --cache-file=./co
| | | | \--- 50218 lotheac (grep)
| | | \-+- 47358 lotheac /bin/sh -ec r=`${PWDCMD-pwd}`; export r;
s=`cd /usr/src/gnu/usr.bin/binutils-2.1
| | | \-+- 26216 lotheac make DESTDIR= RPATH_ENVVAR=LD_LIBRARY_PATH
TARGET_SUBDIR=amd64-unknown-openbsd6
| | | \-+- 51511 lotheac /bin/sh -ec r=`${PWDCMD-pwd}`; export r;
s=`cd /usr/src/gnu/usr.bin/binutils
| | | \-+- 22785 lotheac /bin/sh
/usr/src/gnu/usr.bin/binutils-2.17/intl/configure --cache-file=./co
| | | \-+- 25979 lotheac /bin/sh
/usr/src/gnu/usr.bin/binutils-2.17/intl/configure --cache-file=./
| | | \--- 51834 lotheac (cc)
Partial log of the failure below. I have a full log of the make
invocation as well but I think the relevant parts are included here.
[ ... ]
cd /usr/src/gnu/usr.bin/binutils-2.17/obj/ld && make ld.1
cd /usr/src/gnu/usr.bin/binutils-2.17/obj/gas/doc && make as.1
`as.1' is up to date.
`ld.1' is up to date.
mv ld/ld.1 ld/ld.bfd.1
SUBDIRS='opcodes bfd binutils ld gas'
CONFIGURE_HOST_MODULES='configure-opcodes configure-bfd configure-binutils
configure-ld configure-gas' make CC="cc" CFLAGS="-O2 -pipe -DPIE_DEFAULT=1 "
LDFLAGS="" scriptdir=/usr/libdata tooldir=/usr MAKEINFO='makeinfo
--no-split' MAKEINFOFLAGS='' BSDSRCDIR=/usr/src ALL_MODULES="all-opcodes
all-bfd all-binutils all-ld all-gas" ALL_HOST_MODULES='all-opcodes all-bfd
all-binutils all-ld all-gas' INFO_HOST_MODULES='info-opcodes info-bfd
info-binutils info-ld info-gas' all info
Configuring in ./intl
Configuring in ./intl
Doing info in opcodes
Doing info in bfd
Doing info in binutils
Doing info in gas
Doing info in ld
Making info in po
Making info in doc
touch ld.1
Making info in doc
perl /usr/src/gnu/usr.bin/binutils-2.17/ld/../etc/texi2pod.pl -I
/usr/src/gnu/usr.bin/binutils-2.17/ld -I
/usr/src/gnu/usr.bin/binutils-2.17/ld/../bfd/doc -I
/usr/src/gnu/usr.bin/binutils-2.17/ld/../../../lib/libiberty/src -Dman <
/usr/src/gnu/usr.bin/binutils-2.17/ld/ld.texinfo > ld.pod
Making info in doc
rm -f /usr/src/gnu/usr.bin/binutils-2.17/bfd/doc/bfd.info
/usr/src/gnu/usr.bin/binutils-2.17/bfd/doc/bfd.info-[0-9]
/usr/src/gnu/usr.bin/binutils-2.17/bfd/doc/bfd.info-[0-9][0-9]
if test -f cxxfilt.man; then man=cxxfilt.man; else
man=/usr/src/gnu/usr.bin/binutils-2.17/binutils/doc/cxxfilt.man; fi; sed -e
's/@PROGRAM@/c++filt/' -e 's/cxxfilt/c++filt/' < $man > c++filt.1
makeinfo --no-split -I /usr/src/gnu/usr.bin/binutils-2.17/bfd/doc
/usr/src/gnu/usr.bin/binutils-2.17/bfd/doc/bfd.texinfo
Making info in po
makeinfo --no-split -I "/usr/src/gnu/usr.bin/binutils-2.17/binutils/doc" -I
"/usr/src/gnu/usr.bin/binutils-2.17/binutils/../../../lib/libiberty/src" -I
/usr/src/gnu/usr.bin/binutils-2.17/binutils/doc
/usr/src/gnu/usr.bin/binutils-2.17/binutils/doc/binutils.texi
Making info in po
loading cache ./config.cache
Making info in po
loading cache ./config.cache
checking for a BSD compatible install... /usr/bin/install -c
checking how to run the C preprocessor... (pod2man --center="GNU Development
Tools" --release="binutils-2.17" --section=1
Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior
On Wed, Nov 27 2019 17:09:09 +0100, Marc Espie wrote: > I did experiment with something similar a while back: > > reorganizing a large part of usr.bin or usr.sbin to just be one > single variation of bsd.prog.mk with multiple progs and multiple object > files... works just fine for, say 95% of the binaries in those directories > > (considering there are lots of directories with one single C file or two > C files, this sounds like a gain for make -jN, right ?) yep, exactly my thinking as well. > End result: no measurable gain. That on 4 cpu boxes at the time. > There are SMP issues to solve before this actually yields any > useful result... I did see (and report) a measurable gain previously in this thread in one of the environments I was testing this on, exactly in usr.bin; just under two minutes of improvement with 4 CPUs. https://marc.info/?l=openbsd-tech=157374553407474=2 -- see the note about my test on Intel Atom C2550. Now, I don't really know why I saw that improvement there, but you did not in your tests and I did not in the Hyper-V guest that I tested my most recent diff on; that merits further investigation. Solving the actual SMP issues involved here would be great; I think the potential benefits of work in that area are pretty big. All that said I do understand if there is reluctance to merge the jobserver stuff since it doesn't actually help the current situation in most cases. Nevertheless it has been personally beneficial to me in identifying areas of improvement, even if those are nothing new to OpenBSD developers. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior
On Wed, Nov 27 2019 16:14:44 +0100, Marc Espie wrote: > On Fri, Nov 15, 2019 at 03:29:29PM +0200, Lauri Tirkkonen wrote: > > On Fri, Nov 15 2019 15:24:57 +0200, Lauri Tirkkonen wrote: > > > Your points are valid and I agree with them completely. There are > > > clearly problems with lock contention, > > > > and I should mention here that I would probably not have observed the > > magnitude of these problems had I not made make start more jobs at once > > in the first place. So while the make/mk diffs need more work, we've > > already identified more areas of improvement as a result of trying them > > out. But one thing at a time. :) > > Seriously, as much as what you're doing is interesting, you've not identified > anything new in THAT specific area. We've run enough build configurations > to know about those scalability issues for years. Fair enough, I was being unfair in assuming otherwise. I guess I wanted to offer some validation for my efforts or something ;) > It's cool that you found new races that are hidden by expensive jobs, though, > that much I fully agree with. I'll give you some more background that I maybe should have given earlier already: in my hobby OS Unleashed, we use bmake and earlier did some (slightly hacky) modifications to subdir.mk to enable paralellizing jobs in multiple subdirs at the same time. I don't remember the exact numbers since it's been quite some time, but I was able to cut the base build time by *a lot* with just that change -- during the build there were previously a lot of idle CPUs. Those results served as motivation for me to start looking into doing similar things in OpenBSD, and having a jobserver is sort of the first step there. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior
In a lesson to always proof-read *before* sending your message: On Wed, Nov 27 2019 12:31:51 +0200, Lauri Tirkkonen wrote: > - a full release build with the jobserver enabled passed after >disabling gnu/usr.bin/binutils this should, in fact, read "gnu/usr.bin/binutils-2.17". binutils builds fine. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: [PATCH] make: implement jobserver and use it to avoid exponential behavior
add0f55f 100644
--- a/usr.bin/make/main.c
+++ b/usr.bin/make/main.c
@@ -41,6 +41,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -67,8 +68,6 @@
#include "make.h"
#include "dump.h"
-#define MAKEFLAGS ".MAKEFLAGS"
-
static LISTto_create; /* Targets to be made */
Lst create = _create;
bool allPrecious;/* .PRECIOUS given on line by itself */
@@ -77,6 +76,7 @@ static bool noBuiltins; /* -r flag */
static LISTmakefiles; /* ordered list of makefiles to read */
static LISTvarstoprint;/* list of variables to print */
intmaxJobs;/* -j argument */
+static int jobserverfd = -1; /* -J argument */
bool compatMake; /* -B argument */
static boolforceJobs = false;
intdebug; /* -d flag */
@@ -87,6 +87,7 @@ bool touchFlag; /* -t flag */
bool ignoreErrors; /* -i flag */
bool beSilent; /* -s flag */
bool dumpData; /* -p flag */
+bool usejobserver = false; /* -x flag */
struct dirs {
char *current;
@@ -187,7 +188,7 @@ MainParseArgs(int argc, char **argv)
{
int c, optend;
-#define OPTFLAGS "BC:D:I:SV:d:ef:ij:km:npqrst"
+#define OPTFLAGS "BC:D:I:J:SV:d:ef:ij:km:npqrstx"
#define OPTLETTERS "BSiknpqrst"
if (pledge("stdio rpath wpath cpath fattr proc exec", NULL) == -1)
@@ -218,6 +219,18 @@ MainParseArgs(int argc, char **argv)
Parse_AddIncludeDir(optarg);
record_option(c, optarg);
break;
+ case 'J': {
+ const char *errstr;
+
+ usejobserver = true;
+ jobserverfd = strtonum(optarg, STDERR_FILENO + 1,
+ INT_MAX, );
+ if (errstr != NULL)
+ errx(1, "illegal argument -J%s: "
+ "fd number is %s", optarg, errstr);
+ record_option(c, optarg);
+ break;
+ }
case 'V':
Lst_AtEnd(, optarg);
record_option(c, optarg);
@@ -265,7 +278,7 @@ MainParseArgs(int argc, char **argv)
debug |= DEBUG_JOB | DEBUG_KILL;
break;
case 'J':
- /* ignore */
+ debug |= DEBUG_JOBSERVER;
break;
case 'k':
debug |= DEBUG_KILL;
@@ -327,6 +340,10 @@ MainParseArgs(int argc, char **argv)
Dir_AddDir(systemIncludePath, optarg);
record_option(c, optarg);
break;
+ case 'x':
+ usejobserver = true;
+ /* don't record_option; -J implies this for children */
+ break;
case -1:
/* Check for variable assignments and targets. */
if (argv[optind] != NULL &&
@@ -723,6 +740,17 @@ main(int argc, char **argv)
if (!forceJobs)
compatMake = true;
+ Job_Init(maxJobs, jobserverfd);
+ if (usejobserver && jobserverfd == -1) {
+ char optstr[16];
+
+ /* We are jobserver master; add -J for children */
+ if (snprintf(optstr, sizeof(optstr), "%d",
+ jobserver_get_slave_fd()) < 0)
+ err(1, "could not create -J option string");
+ record_option('J', optstr);
+ }
+
/* And set up everything for sub-makes */
Var_AddCmdline(MAKEFLAGS);
@@ -796,7 +824,6 @@ main(int argc, char **argv)
else
Targ_FindList(, create);
- Job_Init(maxJobs);
/* If the user has defined a .BEGIN target, execute the commands
* attached to it. */
if (!queryFlag)
diff --git a/usr.bin/make/main.h b/usr.bin/make/main.h
index 469487ee058..41a74af9d0d 100644
--- a/usr.bin/make/main.h
+++ b/usr.bin/make/main.h
@@ -29,6 +29,9 @@
/* main
* User interface to make.
*/
+
+#define MAKEFLAGS ".MAKEFLAGS"
+
/* Main_ParseArgLine(string);
* Parse string as a line of arguments, and treats them as if they
* were given at make's invocation. Used to implement .MFLAGS. */
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: librthread sem_t opaqueness, storage & unnamed semaphore sharing
On Sun, Feb 23 2020 14:48:36 +0200, Lauri Tirkkonen wrote:
> So, diff below makes struct __sem non-opaque and removes the indirect
> allocations, so that the application is required to provide storage and
> can therefore control where it's stored (which could be eg. shm).
followup diff that makes sem_init() ignore pshared arg -- it doesn't
need to care, if the application puts the semaphore in shm. I tested an
unnamed shm_init() semaphore in shm_mkstemp() -created shm with this,
passing the fd to a child process and mmap()ing in the child process;
sharing a semaphore this way does seem to work.
diff --git a/lib/librthread/rthread_sem.c b/lib/librthread/rthread_sem.c
index 984e5fb0047..441843d87ca 100644
--- a/lib/librthread/rthread_sem.c
+++ b/lib/librthread/rthread_sem.c
@@ -105,41 +105,6 @@ sem_init(sem_t *sem, int pshared, unsigned int value)
return (-1);
}
- if (pshared) {
- errno = EPERM;
- return (-1);
-#ifdef notyet
- char name[SEM_RANDOM_NAME_LEN];
- sem_t *sempshared;
- int i;
-
- for (;;) {
- for (i = 0; i < SEM_RANDOM_NAME_LEN - 1; i++)
- name[i] = arc4random_uniform(255) + 1;
- name[SEM_RANDOM_NAME_LEN - 1] = '\0';
- sempshared = sem_open(name, O_CREAT | O_EXCL, 0, value);
- if (sempshared != SEM_FAILED)
- break;
- if (errno == EEXIST)
- continue;
- if (errno != EPERM)
- errno = ENOSPC;
- return (-1);
- }
-
- /* unnamed semaphore should not be opened twice */
- if (sem_unlink(name) == -1) {
- sem_close(sempshared);
- errno = ENOSPC;
- return (-1);
- }
-
- *semp = *sempshared;
- free(sempshared);
- return (0);
-#endif
- }
-
bzero(sem, sizeof(*sem));
sem->value = value;
diff --git a/lib/librthread/rthread_sem_compat.c
b/lib/librthread/rthread_sem_compat.c
index 91c888cd49b..8dc863a04d3 100644
--- a/lib/librthread/rthread_sem_compat.c
+++ b/lib/librthread/rthread_sem_compat.c
@@ -116,41 +116,6 @@ sem_init(sem_t *sem, int pshared, unsigned int value)
return (-1);
}
- if (pshared) {
- errno = EPERM;
- return (-1);
-#ifdef notyet
- char name[SEM_RANDOM_NAME_LEN];
- sem_t *sempshared;
- int i;
-
- for (;;) {
- for (i = 0; i < SEM_RANDOM_NAME_LEN - 1; i++)
- name[i] = arc4random_uniform(255) + 1;
- name[SEM_RANDOM_NAME_LEN - 1] = '\0';
- sempshared = sem_open(name, O_CREAT | O_EXCL, 0, value);
- if (sempshared != SEM_FAILED)
- break;
- if (errno == EEXIST)
- continue;
- if (errno != EPERM)
- errno = ENOSPC;
- return (-1);
- }
-
- /* unnamed semaphore should not be opened twice */
- if (sem_unlink(name) == -1) {
- sem_close(sempshared);
- errno = ENOSPC;
- return (-1);
- }
-
- *semp = *sempshared;
- free(sempshared);
- return (0);
-#endif
- }
-
bzero(sem, sizeof(*sem));
sem->lock = _SPINLOCK_UNLOCKED;
sem->value = value;
--
Lauri Tirkkonen | lotheac @ IRCnet
librthread sem_t opaqueness, storage & unnamed semaphore sharing
_rthread_init();
- if (!semp || !(sem = *semp)) {
+ if (!semp) {
errno = EINVAL;
return (-1);
}
@@ -186,21 +177,18 @@ sem_destroy(sem_t *semp)
return (-1);
}
- *semp = NULL;
if (sem->shared)
munmap(sem, SEM_MMAP_SIZE);
else
- free(sem);
+ bzero(sem, sizeof(*sem));
return (0);
}
int
-sem_getvalue(sem_t *semp, int *sval)
+sem_getvalue(sem_t *sem, int *sval)
{
- sem_t sem;
-
- if (!semp || !(sem = *semp)) {
+ if (!sem) {
errno = EINVAL;
return (-1);
}
@@ -213,11 +201,9 @@ sem_getvalue(sem_t *semp, int *sval)
}
int
-sem_post(sem_t *semp)
+sem_post(sem_t *sem)
{
- sem_t sem;
-
- if (!semp || !(sem = *semp)) {
+ if (!sem) {
errno = EINVAL;
return (-1);
}
@@ -228,11 +214,10 @@ sem_post(sem_t *semp)
}
int
-sem_wait(sem_t *semp)
+sem_wait(sem_t *sem)
{
struct tib *tib = TIB_GET();
pthread_t self;
- sem_t sem;
int r;
PREP_CANCEL_POINT(tib);
@@ -258,15 +243,14 @@ sem_wait(sem_t *semp)
}
int
-sem_timedwait(sem_t *semp, const struct timespec *abstime)
+sem_timedwait(sem_t *sem, const struct timespec *abstime)
{
struct tib *tib = TIB_GET();
pthread_t self;
- sem_t sem;
int r;
PREP_CANCEL_POINT(tib);
- if (!semp || !(sem = *semp) || abstime == NULL ||
+ if (!sem || abstime == NULL ||
abstime->tv_nsec < 0 || abstime->tv_nsec >= 10) {
errno = EINVAL;
return (-1);
@@ -289,12 +273,11 @@ sem_timedwait(sem_t *semp, const struct timespec *abstime)
}
int
-sem_trywait(sem_t *semp)
+sem_trywait(sem_t *sem)
{
- sem_t sem;
int r;
- if (!semp || !(sem = *semp)) {
+ if (!sem) {
errno = EINVAL;
return (-1);
}
@@ -330,7 +313,7 @@ sem_open(const char *name, int oflag, ...)
{
char sempath[SEM_PATH_SIZE];
struct stat sb;
- sem_t sem, *semp;
+ sem_t *sem;
unsigned int value = 0;
int created = 0, fd;
@@ -396,35 +379,24 @@ sem_open(const char *name, int oflag, ...)
errno = EINVAL;
return (SEM_FAILED);
}
- semp = malloc(sizeof(*semp));
- if (!semp) {
- munmap(sem, SEM_MMAP_SIZE);
- errno = ENOSPC;
- return (SEM_FAILED);
- }
if (created) {
sem->lock = _SPINLOCK_UNLOCKED;
sem->value = value;
sem->shared = 1;
}
- *semp = sem;
- return (semp);
+ return (sem);
}
int
-sem_close(sem_t *semp)
+sem_close(sem_t *sem)
{
- sem_t sem;
-
- if (!semp || !(sem = *semp) || !sem->shared) {
+ if (!semp || !sem->shared) {
errno = EINVAL;
return (-1);
}
- *semp = NULL;
munmap(sem, SEM_MMAP_SIZE);
- free(semp);
return (0);
}
diff --git a/lib/librthread/shlib_version b/lib/librthread/shlib_version
index 72168dfd16a..54ef0c4cc0c 100644
--- a/lib/librthread/shlib_version
+++ b/lib/librthread/shlib_version
@@ -1,2 +1,2 @@
-major=26
-minor=1
+major=27
+minor=0
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: librthread sem_t opaqueness, storage & unnamed semaphore sharing
On Mon, Feb 24 2020 10:24:53 +0100, Martin Pieuchot wrote: > On 23/02/20(Sun) 14:48, Lauri Tirkkonen wrote: > > I was working on a make jobserver implementation that uses POSIX > > semaphores as job tokens instead of a complicated socket-based approach. > > Initially I used named semaphores, which work fine, except if child > > processes with less privileges need to also open the named semaphore > > (eg. 'make build' as root executing 'su build -c make'). For that reason > > I wanted to use an unnamed semaphore (sem_init()) which is stored in shm > > -- that way I could leave the shm fd open and pass it to children. > > > > But unfortunately, sem_t is currently just a pointer to the opaque > > struct __sem, and sem_int() always calloc()s the storage for the struct. > > That's by design. Ok - could you elaborate what the design is? > > This means the application cannot control where unnamed semaphores are > > stored, so I can't put it in shm. > > Are you trying to use semaphore shared between process? Did you called > sem_init() with pshared=1? Have you seen that the current implementation > doesn't support them? Yes, that's what I'm trying to do. Yes, I've seen the current implementation -- that's why I started this thread, in an attempt to make them supported. :) See the followup patch -- sharing the semaphore between processes does work with it. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: librthread sem_t opaqueness, storage & unnamed semaphore sharing
On Mon, Feb 24 2020 10:42:22 +0100, Martin Pieuchot wrote: > On 24/02/20(Mon) 11:29, Lauri Tirkkonen wrote: > > On Mon, Feb 24 2020 10:24:53 +0100, Martin Pieuchot wrote: > > > On 23/02/20(Sun) 14:48, Lauri Tirkkonen wrote: > > > > I was working on a make jobserver implementation that uses POSIX > > > > semaphores as job tokens instead of a complicated socket-based approach. > > > > Initially I used named semaphores, which work fine, except if child > > > > processes with less privileges need to also open the named semaphore > > > > (eg. 'make build' as root executing 'su build -c make'). For that reason > > > > I wanted to use an unnamed semaphore (sem_init()) which is stored in shm > > > > -- that way I could leave the shm fd open and pass it to children. > > > > > > > > But unfortunately, sem_t is currently just a pointer to the opaque > > > > struct __sem, and sem_int() always calloc()s the storage for the struct. > > > > > > That's by design. > > > > Ok - could you elaborate what the design is? > > If the size of a descriptor change, because some fields are added and/or > removed, it doesn't matter for the application because it only manipulates > pointers. That means we can change the data types without creating an ABI > break. I understand this, that's why I bumped the major and build-tested it. What I don't understand is how an application could ever share a semaphore with another process without sharing the userspace structure that contains the important bits. As I mentioned, NetBSD works around that in the pshared case by stuffing a kernel semaphore identifier into the sem_t* returned by sem_init -- essentially, that identifier becomes the only important bit and no storage is allocated, but from the application point of view, sem_t must still be shared with another process through shm otherwise (only now it's only a pointer that is used to hold an integer value). > > > > This means the application cannot control where unnamed semaphores are > > > > stored, so I can't put it in shm. > > > > > > Are you trying to use semaphore shared between process? Did you called > > > sem_init() with pshared=1? Have you seen that the current implementation > > > doesn't support them? > > > > Yes, that's what I'm trying to do. Yes, I've seen the current > > implementation -- that's why I started this thread, in an attempt to > > make them supported. :) > > > > See the followup patch -- sharing the semaphore between processes does > > work with it. > > Well ignoring the `pshared' argument is questionable. Why don't you > remove the "#if notyet" and start playing with the existing code and > try to figure out if something is missing for your use case? I did read the existing code behind the #ifdef notyet, and even found a commit from 2013 disabling the code (by adding the #ifdef) "until it really works". It seems to me that time has not come. I don't understand how the pshared case under #ifdef notyet was even supposed to work. It seems to just turn an unnamed semaphore into a randomly-named semaphore, but provides no way for the application to share it with another process; if the application puts sem_t in shm, only a pointer is now in shm -- the struct __sem is not. Named semaphores at least can be opened by other processes if they know the name, but randomly-named semaphores that are unlinked after creation can certainly not. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: librthread sem_t opaqueness, storage & unnamed semaphore sharing
On Mon, Feb 24 2020 10:42:22 +0100, Martin Pieuchot wrote: > > Yes, that's what I'm trying to do. Yes, I've seen the current > > implementation -- that's why I started this thread, in an attempt to > > make them supported. :) > > > > See the followup patch -- sharing the semaphore between processes does > > work with it. > > Well ignoring the `pshared' argument is questionable. It does feel strange, but a fully userspace implementation of semaphores (ie. using only mmap MAP_SHARED files for communication with other thread/processes) doesn't really have a meaningful distinction whether sharing happens between threads or processes. The other semaphore implementations I glanced at all had some kind of 'kernel semaphore'. I suppose a userspace implementation like this could store the pid of the process allowed to access the semaphore in struct __sem if pshared=0, and check that on every access, but I'm honestly not sure what value it brings for a userspace library to essentially prevent access to userspace memory if the application went through the effort to share that memory between processes anyway. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: librthread sem_t opaqueness, storage & unnamed semaphore sharing
On Mon, Feb 24 2020 15:33:35 -0500, Ted Unangst wrote: > Martin Pieuchot wrote: > > On 24/02/20(Mon) 11:29, Lauri Tirkkonen wrote: > > > On Mon, Feb 24 2020 10:24:53 +0100, Martin Pieuchot wrote: > > > > On 23/02/20(Sun) 14:48, Lauri Tirkkonen wrote: > > > > > I was working on a make jobserver implementation that uses POSIX > > > > > semaphores as job tokens instead of a complicated socket-based > > > > > approach. > > > > > Initially I used named semaphores, which work fine, except if child > > > > > processes with less privileges need to also open the named semaphore > > > > > (eg. 'make build' as root executing 'su build -c make'). For that > > > > > reason > > > > > I wanted to use an unnamed semaphore (sem_init()) which is stored in > > > > > shm > > > > > -- that way I could leave the shm fd open and pass it to children. > > > > > > > > > > But unfortunately, sem_t is currently just a pointer to the opaque > > > > > struct __sem, and sem_int() always calloc()s the storage for the > > > > > struct. > > > > > > > > That's by design. > > > > > > Ok - could you elaborate what the design is? > > > > If the size of a descriptor change, because some fields are added and/or > > removed, it doesn't matter for the application because it only manipulates > > pointers. That means we can change the data types without creating an ABI > > break. > > I think we are approaching the point where we can settle on fixed sized types > now. If we want to be cautious, we can add a reserved padding field, too. But > there are some edge cases which I think can be removed by eliminating the > dynamic allocation paths. > > > > See the followup patch -- sharing the semaphore between processes does > > > work with it. > > > > Well ignoring the `pshared' argument is questionable. Why don't you > > remove the "#if notyet" and start playing with the existing code and > > try to figure out if something is missing for your use case? > > I'm not sure the code in notyet will work. It was based on a misunderstanding > I had of the requirements. Returning control of the sem_t placement to the > application is the right approach. Thanks for the input, and ping - is there still something about this diff that I should fix? -- Lauri Tirkkonen | lotheac @ IRCnet
Re: Teach du(1) the -m flag, disk usage in megabytes
On Wed, Jan 29 2020 12:25:34 +0100, Sebastian Benoit wrote: > Lauri Tirkkonen(la...@hacktheplanet.fi) on 2020.01.29 01:31:56 +0200: > > On Tue, Jan 28 2020 18:03:19 +0100, Florian Obser wrote: > > > On Tue, Jan 28, 2020 at 09:58:40AM -0700, Todd C. Miller wrote: > > > > On Mon, 27 Jan 2020 18:29:39 -0500, Daniel Jakots wrote: > > > > > > > > > Can't you achieve what you want with `du -sh * | sort -h`? du(1)'s -h > > > > > options will automatically select the best suffix and sort(1)'s -h > > > > > will sort first using the suffix then the numerical value. > > > > > > > > Yes, I forgot about "sort -h". Old habits die hard :-) > > > > > > ... which is not in posix, netbsd nor illumos. > > > > So, do you think that 'du -m' will be in all those then? POSIX doesn't > > have it [0]. > > > > The way I see it, the entire conversation in this thread is about doing > > things that might be useful to people. IMO, arguing about where > > extensions are or aren't implemented isn't productive. > > Yes it is. We try to avoid adding options whereever we can, because > > * every option letter we grab thats not used for the same purpose elsewhere > can crate problems down the road > * more options means more bugs Yeah, I agree with this completely -- it was in fact kind of my point: why add -m to du, if the usecase it helps with is already handled by sort -h? I think it's a bit of a moot point whether sort -h is supported elsewhere or not. I concede that I was not speaking clearly enough; there might have been some beer that was doing the talking. Apologies for that and for shouting from the gallery :) -- Lauri Tirkkonen | lotheac @ IRCnet
Re: Teach du(1) the -m flag, disk usage in megabytes
On Tue, Jan 28 2020 18:03:19 +0100, Florian Obser wrote: > On Tue, Jan 28, 2020 at 09:58:40AM -0700, Todd C. Miller wrote: > > On Mon, 27 Jan 2020 18:29:39 -0500, Daniel Jakots wrote: > > > > > Can't you achieve what you want with `du -sh * | sort -h`? du(1)'s -h > > > options will automatically select the best suffix and sort(1)'s -h > > > will sort first using the suffix then the numerical value. > > > > Yes, I forgot about "sort -h". Old habits die hard :-) > > ... which is not in posix, netbsd nor illumos. So, do you think that 'du -m' will be in all those then? POSIX doesn't have it [0]. The way I see it, the entire conversation in this thread is about doing things that might be useful to people. IMO, arguing about where extensions are or aren't implemented isn't productive. [0]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/du.html -- Lauri Tirkkonen | lotheac @ IRCnet
Re: pshared semaphores
On Wed, Jul 08 2020 19:19:38 +0200, Mark Kettenis wrote: > This will require a libpthread major bump, and those are really > painful! So I'm not sure we should do this just for pshared > semaphores which hardly anybody uses. FWIW, I have an implementation of a jobserver for make(1) that uses pshared semaphores that I intend to submit if this goes through (my initial attempts used sockets, but that was tricky to get right). But yes, there isn't a lot of software that uses them. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: make btrace interval event to reciprocal of ticks
On Thu, Jun 25 2020 11:36:48 +0200, Martin Pieuchot wrote: > On 23/06/20(Tue) 12:04, Yuichiro NAITO wrote: > > Current btrace has `interval:hz:1` probe that makes periodically events. > > `interval:hz:1` looks to make events once per second (= 1Hz), > > but current implementation makes once per tick (= 100Hz on amd64). > > I think the interval should be counted by reciprocal of ticks so that fit > > to Hz. > > Indeed the current implementations assumes it's a number of ticks. How > does other tracing tool behave? Is the behavior you suggest similar to > bpftrace(8) or dtrace(1)? In DTrace, profile- probes fire at a frequency of hertz, but apparently also support suffixes. http://dtrace.org/guide/chp-profile.html -- Lauri Tirkkonen | lotheac @ IRCnet
[PATCH] make pthread_mutex_t non-opaque
int
-pthread_mutex_getprioceiling(pthread_mutex_t *mutexp, int *prioceiling)
+pthread_mutex_getprioceiling(pthread_mutex_t *mutex, int *prioceiling)
{
- pthread_mutex_t mutex = *mutexp;
-
if (mutex->prioceiling == -1)
return (EINVAL);
*prioceiling = mutex->prioceiling;
@@ -34,20 +32,19 @@ pthread_mutex_getprioceiling(pthread_mutex_t *mutexp, int
*prioceiling)
}
int
-pthread_mutex_setprioceiling(pthread_mutex_t *mutexp, int prioceiling,
+pthread_mutex_setprioceiling(pthread_mutex_t *mutex, int prioceiling,
int *old_ceiling)
{
- pthread_mutex_t mutex = *mutexp;
int ret;
if (mutex->prioceiling == -1 ||
prioceiling < PTHREAD_MIN_PRIORITY ||
prioceiling > PTHREAD_MAX_PRIORITY) {
ret = EINVAL;
- } else if ((ret = pthread_mutex_lock(mutexp)) == 0) {
+ } else if ((ret = pthread_mutex_lock(mutex)) == 0) {
*old_ceiling = mutex->prioceiling;
mutex->prioceiling = prioceiling;
- pthread_mutex_unlock(mutexp);
+ pthread_mutex_unlock(mutex);
}
return (ret);
diff --git a/lib/librthread/shlib_version b/lib/librthread/shlib_version
index 72168dfd16a..54ef0c4cc0c 100644
--- a/lib/librthread/shlib_version
+++ b/lib/librthread/shlib_version
@@ -1,2 +1,2 @@
-major=26
-minor=1
+major=27
+minor=0
--
2.31.1
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: smtpd: use libtls
On Wed, Jan 27 2021 09:36:31 +0100, Eric Faurot wrote: > There has been a plan for some time now to make smtpd use libtls > instead of openssl. Recent changes in libtls allow to move forward > with this. Here is a diff to start the switch. I've tried to keep > it as small as possible, sticking to the necessary changes. There is > still a lot of code that can be removed but that will be done in a > second time. I'm all for this, and sorry for screaming from the gallery, but I want to ask - is there a plan relating to libtls for portable OpenSMTPD? As it stands, OpenSSL-based systems are largely unable to use libtls (which in itself is a shame) - how would this change make it to portable? -- Lauri Tirkkonen | lotheac @ IRCnet
Re: athn(4): switch Tx rate control to RA
On Tue, Mar 23 2021 18:01:27 +0100, Stefan Sperling wrote: > This switches athn(4) to the new RA Tx rate adaptation module. > Tests on athn(4) PCI devices are welcome. > USB devices don't need to be tested in this case Tx rate adaptation > is taken care of by firmware. > > I could only test on AR9285 so far, but the result looks promising. Some numbers from quick tests on: athn0 at pci1 dev 0 function 0 "Atheros AR9281" rev 0x01: apic 5 int 0 athn0: AR9280 rev 2 (2T2R), ROM rev 16, address in hostap mode (11n), sending data to a laptop on iwn(4). tcpbench (tcp), 10sec: - before: Mbps varied between 0.677 and 15.187, avg 4.647 - after: Mbps varied between 10.345 and 10.623, avg 10.588 tcpbench (udp), 10sec: - before: Rx PPS between 1444 and 1461, about 17 Mbps athn(4) sent: 495668032 bytes sent over 10.999 seconds - after: Rx PPS between 1021 and 1056, about 12 Mbps athn(4) sent: 462994048 bytes sent over 10.999 seconds (I failed to record bytes received on iwn(4) for this test, sorry) ping -f -c 1 from athn(4) hostap to iwn(4): before: 1 packets transmitted, 9982 packets received, 0.2% packet loss round-trip min/avg/max/std-dev = 0.397/1.029/100.599/1.400 ms after: 1 packets transmitted, 1 packets received, 0.0% packet loss round-trip min/avg/max/std-dev = 0.402/1.191/10.510/0.463 ms Subjectively, the reduced loss & more predictable latency feels much better with eg. interactive ssh sessions. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: wcwidth of soft hyphen
Hi Ingo, On Mon, Apr 05 2021 20:30:39 +0200, Ingo Schwarze wrote: > Whether all control chars are always width 0 can maybe also be > disputed. Again, the stronger argument seems to me that they are. > If they weren't, they would not be control characters but alphanumeric, > punctuation, spaces, or special printable characters, none of which > they are. I say width 1 and 2 require standalone glyphs that are > normally used for the character. Besides, no operating system > correctly identifies this as a control character and yet gives it > width 1. I agree with your assessments about iswcntrl and iswprint. My original patch proposed to keep those return values as is, and only change the wcwidth() from 0 to 1. > I insist that the discussion should remain very strictly formal, > about the properties and classification in the Unicode data files > and nothing else. If people start arguing about what makes sense > for any particular character, that's already an argument going > astray. In general I agree, but I contend that SHY is, unfortunately, a little bit special. This confusion about its printability and/or column width is definitely not unique to OpenBSD. > > So going by this phrase the character should not be printed > > When formatting a document, for example for printing on paper or > the online equivalent like PostScript or PDF, i agree. But i > strongly prefer the terminal to always display this character because > the terminal's usual purpose is not nice text formatting for visual > consumption. It should usually show the full content of strings > or files, be it for inspection or for editing. Omitting characters > in such contexts sets nasty traps for the person working with the > terminal. I agree with this completely - you said it better than I could have. This is another reason why I think it makes sense for this character to have wcwidth() of 1 - applications that are "SHY-aware" can print (or not) the soft hyphen however they wish, but terminal software seems to almost always ask wcwidth() to figure out the column width. Indeed, terminal software is where I ran into the problem of SHY sometimes being invisible. > So i say nothing should be changed at all in OpenBSD. > > Yes, that means column counting is wrong on the terminal, but that's > a very minor problem, if it's a problem at all, compared to the havoc > that could result from not showing the character on the terminal at > all, and it cannot be fixed without causing worse problems in situations > that matter more. Right, I am not at all advocating to hide the SHY on the terminal - quite the contrary, I want to make its width consistent. The current situation, with SHY having a wcwidth() of 0 causes, for example, the following discrepancy between xterm(1) (on the left) and tmux(1) in xterm(1) (on the right): https://hacktheplanet.fi/shytmux.png -- in tmux, the SHY is not visible. The other issues I observed with discrepancies between OpenBSD and other systems I already outlined in my initial mail. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: wcwidth of soft hyphen
On Tue, Apr 06 2021 13:09:11 +0200, Martijn van Duren wrote: > On Thu, 2021-04-01 at 10:39 +0300, Lauri Tirkkonen wrote: > > On Thu, Apr 01 2021 09:30:36 +0200, Martijn van Duren wrote: > > > However, based on the description by the Unicode Consortium I think > > > OpenBSD does the right thing and xterm and others should be fixed, > > > > practically, I doubt this will happen. I don't think the glibc people will > > be > > convinced to break compatibility to their older versions, for example. I > > explicitly mentioned I don't wish to engage in a discussion about which way > > is > > _correct_ - I am interested in interoperability with real, existing systems. > > > I´m not convinced that you´ve shown that it´s actually an > interoperability issue. In your last mail you state that it´s a simple > display difference between tmux and raw xterm on OpenBSD. To me that´s > similar to most linux distro´s having grep being an alias for > grep --color=auto by default and stating that we should do the same > because you like pretty colours. What applications fail to operate or > operate in an severely erroneous way because of this discrepency? I'll try again to describe the problem, and show an example. TUI applications often care, for layout purposes, how long a particular string or line will be on the output device. A not insignificant number of those applications use wcwidth() to figure out how much column space will be taken by a certain character or string. If the application performing the width calculations is running on a different machine than the terminal, say, through ssh, it is important that the application's idea of width matches what the terminal will eventually render; if it doesn't, then the application could print the string over some other TUI element, for example. This is difficult and messy for many reasons already discussed, especially when different operating systems disagree about the width or printability of a character. Nevertheles , in 2021, wcwidth implementations mostly agree and even things like emojis get a wcwidth of 2 everywhere I've observed (in contrast to some -1 wcwidths of printable characters I observed on other OSes in the past). But SHY seems to still be something that causes issues in terminals, at least for me. As the example, I ran the command "/exec cat longshy.txt /etc/motd" inside of irssi, in a 80x24 terminal window, with a few different terminal/OS-running-terminal/OS-running-irssi configurations. 'longshy.txt' is available at https://hacktheplanet.fi/shy/longshy.txt Let's start with st(1), since it's simple and uses wcwidth() directly to decide how wide a character should be printed: st on OpenBSD, local irssi https://hacktheplanet.fi/shy/st-openbsd-local.png st on Debian, local irssi https://hacktheplanet.fi/shy/st-debian-local.png Here we can see two key things: 1) on Debian, st is rendering the SHY characters - on OpenBSD it is not 2) on Debian, irssi considers the line long enough that it splits it and prints the remainder on the next line, indented So, let's introduce ssh into the mix: st on OpenBSD, irssi on Debian https://hacktheplanet.fi/shy/st-openbsd-ssh-debian.png st on Debian, irssi on OpenBSD https://hacktheplanet.fi/shy/st-debian-ssh-openbsd.png We begin to see differences that stem from wcwidth(SHY). These problems aren't very big, since in both cases the output is still readable and no information is lost. Now, let's try xterm(1). It has been observed in this thread that xterm always prints SHY. xterm on OpenBSD, local irssi https://hacktheplanet.fi/shy/xterm-openbsd-local.png xterm on Debian, local irssi https://hacktheplanet.fi/shy/xterm-debian-local.png On OpenBSD, irssi thinks that the entire line fits into the 80 columns available. But because xterm prints SHYs, the line overflows onto the next and is promptly overwritten by the next line that irssi puts there (the motd). And finally ssh with xterm: xterm on OpenBSD, irssi on Debian https://hacktheplanet.fi/shy/xterm-openbsd-ssh-debian.png xterm on Debian, irssi on OpenBSD https://hacktheplanet.fi/shy/xterm-debian-ssh-openbsd.png This isn't the best example: there are many different problems that can arise from the width calculation discrepancy - some of them can be more spectacular I think, but I could only come up with this one on demand. Despite the bad example, I do consider cases where text messes up in ways the application did not intend (in the worst case, overwriting other text) on the same terminal on different operating systems interoperability bugs. In this case the outputs are different due to interactions between systems that use wcwidth(SHY) = 1 (such as, apparently, xterm even locally) and OpenBSD. I might not say it is "operating in a severely erroneous way", but then I don't consider "severely erroneous" as a requi
Re: wcwidth of soft hyphen
On Tue, Apr 06 2021 11:27:21 +0100, Stuart Henderson wrote: > Some terminal emulators are using iso-8859-1 semantics of soft hyphen, > unicode did things differently but those terminals haven't changed. > > xterm printed as hyphen > mltermprinted as hyphen > putty printed as hyphen > urxvt overprinted on previous character > stnot printed, no space > kitty not printed, no space > cool-retro-term not printed, no space > sakuraprinted as space st actually relies on wcwidth(), so on Debian (for example) it prints the SHY as a hyphen. > Pragmatically the simplest fix for the original problem might be if > irssi filtered out soft-hyphen characters like mutt does in its > "is_display_corrupting_utf8()" function: > > https://gitlab.com/muttmua/mutt/-/blob/master/mbyte.c#L528 Thanks, it's news to me that mutt does that. It speaks to something when an application is explicitly hardcoding codepoints not to print. I don't particularly like the 'solution' of every TUI application having to ship their own fixes for stuff like this though. -- Lauri Tirkkonen | lotheac @ IRCnet
wcwidth of soft hyphen
When using terminal software on non-OpenBSD to connect to my OpenBSD IRC
machine, I noticed that sometimes the local terminal disagrees with the remote
tmux and application (in this case, irssi) about the character width of some
lines, causing different kinds of breakage. Those lines happened to contain soft
hyphens (U+00AD), which behave as follows across a few different operating
systems:
OpenBSD-CURRENT:iswprint(SHY) = 1 iswcntrl(SHY) = 1 wcwidth(SHY) = 0
NetBSD 9.1: iswprint(SHY) = 1 iswcntrl(SHY) = 0 wcwidth(SHY) = 1
FreeBSD 12.2: iswprint(SHY) = 0 iswcntrl(SHY) = 1 wcwidth(SHY) = -1
glibc (Debian sid): iswprint(SHY) = 1 iswcntrl(SHY) = 0 wcwidth(SHY) = 1
musl (Alpine 3.13.3): iswprint(SHY) = 1 iswcntrl(SHY) = 0 wcwidth(SHY) = 1
On Windows, PowerShell, PuTTY and MinTTY (shipped with the default install of
git from git-scm.com as part of MSYS2) render the soft hyphen as a visible
character with a width of 1 column.
The OpenBSD wcwidth(SHY) of 0 is what the problem comes down to (FreeBSD's
return values are also strange, but this is an OpenBSD list). There is a lot of
background discussion about whether or not Unicode intends the SHY to be
printable or not, and whether it should have width of 0 or 1, in eg. [0] and
[1], but for better or worse, it seems most other systems decided that SHY has a
width of 1 and should be a visible character (at least in terminal contexts).
Therefore, in the interest of interoperability, I propose the following diff to
special-case SHY into having a width of 1. I don't intend to go down the rabbit
hole of a discussion regarding what the 'correct' width is, but the discrepancy
with other systems causes real problems, and I think those other systems made
their decisions years ago (see eg. [0] for glibc).
Diff below only for gen_ctype_utf8.pl; I am not including the resulting
en_US.UTF-8.src diff, because it seems there is a Unicode 12.1.0 to 13.0.0
update that happens on regeneration of that file, and that is orthogonal to this
change (essentially: [2], which has not been committed yet)
[0]: https://sourceware.org/bugzilla/show_bug.cgi?id=22073
[1]: https://jkorpela.fi/shy.html
[2]: https://marc.info/?l=openbsd-tech=161534047428793=2
diff --git a/share/locale/ctype/gen_ctype_utf8.pl
b/share/locale/ctype/gen_ctype_utf8.pl
index e23472efb2c..c593dc628ee 100755
--- a/share/locale/ctype/gen_ctype_utf8.pl
+++ b/share/locale/ctype/gen_ctype_utf8.pl
@@ -404,6 +404,9 @@ sub codepoint_columns
# Several fonts provide glyphs in this range
return 1 if $code >= 0xe000 and $code <= 0xf8ff;
+ # Soft hyphen (SHY) is in category Cf, which implies width 0, but since
+ # it is width 1 in nearly every other environment, set it here.
+ return 1 if $code == 0x00ad;
return 0 if $charinfo->{category} eq 'Mn';
return 0 if $charinfo->{category} eq 'Me';
--
Lauri Tirkkonen | lotheac @ IRCnet
Re: wcwidth of soft hyphen
On Thu, Apr 01 2021 09:30:36 +0200, Martijn van Duren wrote: > However, based on the description by the Unicode Consortium I think > OpenBSD does the right thing and xterm and others should be fixed, practically, I doubt this will happen. I don't think the glibc people will be convinced to break compatibility to their older versions, for example. I explicitly mentioned I don't wish to engage in a discussion about which way is _correct_ - I am interested in interoperability with real, existing systems. -- Lauri Tirkkonen | lotheac @ IRCnet
Re: wcwidth of soft hyphen
Since the discussion seems to have died out, I take my patch will not be accepted. The decision appears to be that OpenBSD is right and everyone else is wrong in this matter. Given that, and the calls to change the behavior of other OSes and terminal emulators around SHY: are you going to at least patch xterm in-tree so that it does not render SHY? Or must it remain broken? -- Lauri Tirkkonen | lotheac @ IRCnet
