Re: ftp: make use of getline(3)
Christian Weisgerber:
> > Make use of getline(3) in ftp(1).
> >
> > Replace fparseln(3) with getline(3). This removes the only use
> > of libutil.a(fparseln.o) from the ramdisk.
> > Replace a complicated fgetln(3) idiom with the much simpler getline(3).
>
> OK?
ping?
I've been fetching distfiles with it, and I also built a bsd.rd and
performed a http install with it.
Index: distrib/special/ftp/Makefile
===
RCS file: /cvs/src/distrib/special/ftp/Makefile,v
retrieving revision 1.14
diff -u -p -r1.14 Makefile
--- distrib/special/ftp/Makefile16 May 2019 12:44:17 - 1.14
+++ distrib/special/ftp/Makefile29 Jan 2021 18:05:46 -
@@ -6,7 +6,4 @@ PROG= ftp
SRCS= fetch.c ftp.c main.c small.c util.c
.PATH: ${.CURDIR}/../../../usr.bin/ftp
-LDADD+=-lutil
-DPADD+=${LIBUTIL}
-
.include
Index: usr.bin/ftp/Makefile
===
RCS file: /cvs/src/usr.bin/ftp/Makefile,v
retrieving revision 1.34
diff -u -p -r1.34 Makefile
--- usr.bin/ftp/Makefile27 Jan 2021 22:27:41 - 1.34
+++ usr.bin/ftp/Makefile29 Jan 2021 17:57:31 -
@@ -8,8 +8,8 @@ PROG= ftp
SRCS= cmds.c cmdtab.c complete.c cookie.c domacro.c fetch.c ftp.c \
list.c main.c ruserpass.c small.c stringlist.c util.c
-LDADD+=-ledit -lcurses -lutil -ltls -lssl -lcrypto
-DPADD+=${LIBEDIT} ${LIBCURSES} ${LIBUTIL} ${LIBTLS} ${LIBSSL}
${LIBCRYPTO}
+LDADD+=-ledit -lcurses -ltls -lssl -lcrypto
+DPADD+=${LIBEDIT} ${LIBCURSES} ${LIBTLS} ${LIBSSL} ${LIBCRYPTO}
#COPTS+= -Wall -Wconversion -Wstrict-prototypes -Wmissing-prototypes
Index: usr.bin/ftp/cookie.c
===
RCS file: /cvs/src/usr.bin/ftp/cookie.c,v
retrieving revision 1.9
diff -u -p -r1.9 cookie.c
--- usr.bin/ftp/cookie.c16 May 2019 12:44:17 - 1.9
+++ usr.bin/ftp/cookie.c6 Feb 2021 16:23:32 -
@@ -58,10 +58,10 @@ void
cookie_load(void)
{
field_t field;
- size_t len;
time_t date;
char*line;
- char*lbuf;
+ char*lbuf = NULL;
+ size_t lbufsize = 0;
char*param;
const char *estr;
FILE*fp;
@@ -75,19 +75,9 @@ cookie_load(void)
if (fp == NULL)
err(1, "cannot open cookie file %s", cookiefile);
date = time(NULL);
- lbuf = NULL;
- while ((line = fgetln(fp, )) != NULL) {
- if (line[len - 1] == '\n') {
- line[len - 1] = '\0';
- --len;
- } else {
- if ((lbuf = malloc(len + 1)) == NULL)
- err(1, NULL);
- memcpy(lbuf, line, len);
- lbuf[len] = '\0';
- line = lbuf;
- }
- line[strcspn(line, "\r")] = '\0';
+ while (getline(, , fp) != -1) {
+ line = lbuf;
+ line[strcspn(line, "\r\n")] = '\0';
line += strspn(line, " \t");
if ((*line == '#') || (*line == '\0')) {
Index: usr.bin/ftp/fetch.c
===
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.200
diff -u -p -r1.200 fetch.c
--- usr.bin/ftp/fetch.c 2 Feb 2021 12:58:42 - 1.200
+++ usr.bin/ftp/fetch.c 2 Feb 2021 13:59:09 -
@@ -56,7 +56,6 @@
#include
#include
#include
-#include
#include
#include
@@ -76,7 +75,6 @@ static void aborthttp(int);
static charhextochar(const char *);
static char*urldecode(const char *);
static char*recode_credentials(const char *_userinfo);
-static char*ftp_readline(FILE *, size_t *);
static voidftp_close(FILE **, struct tls **, int *);
static const char *sockerror(struct tls *);
#ifdef SMALL
@@ -330,6 +328,7 @@ url_get(const char *origline, const char
off_t hashbytes;
const char *errstr;
ssize_t len, wlen;
+ size_t bufsize;
char *proxyhost = NULL;
#ifndef NOSSL
char *sslpath = NULL, *sslhost = NULL;
@@ -805,12 +804,13 @@ noslash:
free(buf);
#endif /* !NOSSL */
buf = NULL;
+ bufsize = 0;
if (fflush(fin) == EOF) {
warnx("Writing HTTP request: %s", sockerror(tls));
goto cleanup_url_get;
}
- if ((buf = ftp_readline(fin, )) == NULL) {
+ if ((len = getline(, , fin)) == -1) {
warnx("Receiving HTTP reply: %s", sockerror(tls));
goto cleanup_url_get;
}
@@ -885,11 +885,10 @@ noslash:
/*
* Read the rest of the header.
*/
- fr
Re: ftp: make use of getline(3)
Christian Weisgerber:
> Make use of getline(3) in ftp(1).
>
> Replace fparseln(3) with getline(3). This removes the only use
> of libutil.a(fparseln.o) from the ramdisk.
> Replace a complicated fgetln(3) idiom with the much simpler getline(3).
New diff that fixes a bug I introduced in cookie loading.
OK?
Index: distrib/special/ftp/Makefile
===
RCS file: /cvs/src/distrib/special/ftp/Makefile,v
retrieving revision 1.14
diff -u -p -r1.14 Makefile
--- distrib/special/ftp/Makefile16 May 2019 12:44:17 - 1.14
+++ distrib/special/ftp/Makefile29 Jan 2021 18:05:46 -
@@ -6,7 +6,4 @@ PROG= ftp
SRCS= fetch.c ftp.c main.c small.c util.c
.PATH: ${.CURDIR}/../../../usr.bin/ftp
-LDADD+=-lutil
-DPADD+=${LIBUTIL}
-
.include
Index: usr.bin/ftp/Makefile
===
RCS file: /cvs/src/usr.bin/ftp/Makefile,v
retrieving revision 1.34
diff -u -p -r1.34 Makefile
--- usr.bin/ftp/Makefile27 Jan 2021 22:27:41 - 1.34
+++ usr.bin/ftp/Makefile29 Jan 2021 17:57:31 -
@@ -8,8 +8,8 @@ PROG= ftp
SRCS= cmds.c cmdtab.c complete.c cookie.c domacro.c fetch.c ftp.c \
list.c main.c ruserpass.c small.c stringlist.c util.c
-LDADD+=-ledit -lcurses -lutil -ltls -lssl -lcrypto
-DPADD+=${LIBEDIT} ${LIBCURSES} ${LIBUTIL} ${LIBTLS} ${LIBSSL}
${LIBCRYPTO}
+LDADD+=-ledit -lcurses -ltls -lssl -lcrypto
+DPADD+=${LIBEDIT} ${LIBCURSES} ${LIBTLS} ${LIBSSL} ${LIBCRYPTO}
#COPTS+= -Wall -Wconversion -Wstrict-prototypes -Wmissing-prototypes
Index: usr.bin/ftp/cookie.c
===
RCS file: /cvs/src/usr.bin/ftp/cookie.c,v
retrieving revision 1.9
diff -u -p -r1.9 cookie.c
--- usr.bin/ftp/cookie.c16 May 2019 12:44:17 - 1.9
+++ usr.bin/ftp/cookie.c6 Feb 2021 16:23:32 -
@@ -58,10 +58,10 @@ void
cookie_load(void)
{
field_t field;
- size_t len;
time_t date;
char*line;
- char*lbuf;
+ char*lbuf = NULL;
+ size_t lbufsize = 0;
char*param;
const char *estr;
FILE*fp;
@@ -75,19 +75,9 @@ cookie_load(void)
if (fp == NULL)
err(1, "cannot open cookie file %s", cookiefile);
date = time(NULL);
- lbuf = NULL;
- while ((line = fgetln(fp, )) != NULL) {
- if (line[len - 1] == '\n') {
- line[len - 1] = '\0';
- --len;
- } else {
- if ((lbuf = malloc(len + 1)) == NULL)
- err(1, NULL);
- memcpy(lbuf, line, len);
- lbuf[len] = '\0';
- line = lbuf;
- }
- line[strcspn(line, "\r")] = '\0';
+ while (getline(, , fp) != -1) {
+ line = lbuf;
+ line[strcspn(line, "\r\n")] = '\0';
line += strspn(line, " \t");
if ((*line == '#') || (*line == '\0')) {
Index: usr.bin/ftp/fetch.c
===
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.200
diff -u -p -r1.200 fetch.c
--- usr.bin/ftp/fetch.c 2 Feb 2021 12:58:42 - 1.200
+++ usr.bin/ftp/fetch.c 2 Feb 2021 13:59:09 -
@@ -56,7 +56,6 @@
#include
#include
#include
-#include
#include
#include
@@ -76,7 +75,6 @@ static void aborthttp(int);
static charhextochar(const char *);
static char*urldecode(const char *);
static char*recode_credentials(const char *_userinfo);
-static char*ftp_readline(FILE *, size_t *);
static voidftp_close(FILE **, struct tls **, int *);
static const char *sockerror(struct tls *);
#ifdef SMALL
@@ -330,6 +328,7 @@ url_get(const char *origline, const char
off_t hashbytes;
const char *errstr;
ssize_t len, wlen;
+ size_t bufsize;
char *proxyhost = NULL;
#ifndef NOSSL
char *sslpath = NULL, *sslhost = NULL;
@@ -805,12 +804,13 @@ noslash:
free(buf);
#endif /* !NOSSL */
buf = NULL;
+ bufsize = 0;
if (fflush(fin) == EOF) {
warnx("Writing HTTP request: %s", sockerror(tls));
goto cleanup_url_get;
}
- if ((buf = ftp_readline(fin, )) == NULL) {
+ if ((len = getline(, , fin)) == -1) {
warnx("Receiving HTTP reply: %s", sockerror(tls));
goto cleanup_url_get;
}
@@ -885,11 +885,10 @@ noslash:
/*
* Read the rest of the header.
*/
- free(buf);
filesize = -1;
for (;;) {
- if ((buf = ftp_rea
disklabel: make use of getline(3)
Replace fgetln(3) with getline(3).
Since getline() returns a C string, we don't need to carry around
the length separately.
OK?
Index: sbin/disklabel/editor.c
===
RCS file: /cvs/src/sbin/disklabel/editor.c,v
retrieving revision 1.364
diff -u -p -r1.364 editor.c
--- sbin/disklabel/editor.c 31 Jan 2021 14:18:44 - 1.364
+++ sbin/disklabel/editor.c 31 Jan 2021 14:35:03 -
@@ -172,7 +172,7 @@ voidzero_partitions(struct disklabel *)
u_int64_t max_partition_size(struct disklabel *, int);
void display_edit(struct disklabel *, char);
void psize(u_int64_t sz, char unit, struct disklabel *lp);
-char *get_token(char **, size_t *);
+char *get_token(char **);
intapply_unit(double, u_char, u_int64_t *);
intparse_sizespec(const char *, double *, char **);
intparse_sizerange(char *, u_int64_t *, u_int64_t *);
@@ -2331,8 +2331,8 @@ void
parse_autotable(char *filename)
{
FILE*cfile;
- size_t len;
- char*buf, *t;
+ size_t bufsize = 0;
+ char*buf = NULL, *t;
uint idx = 0, pctsum = 0;
struct space_allocation *sa;
@@ -2342,7 +2342,7 @@ parse_autotable(char *filename)
err(1, NULL);
alloc_table_nitems = 1;
- while ((buf = fgetln(cfile, )) != NULL) {
+ while (getline(, , cfile) != -1) {
if ((alloc_table[0].table = reallocarray(alloc_table[0].table,
idx + 1, sizeof(*sa))) == NULL)
err(1, NULL);
@@ -2350,13 +2350,13 @@ parse_autotable(char *filename)
memset(sa, 0, sizeof(*sa));
idx++;
- if ((sa->mp = get_token(, )) == NULL ||
+ if ((sa->mp = get_token()) == NULL ||
(sa->mp[0] != '/' && strcmp(sa->mp, "swap")))
errx(1, "%s: parse error on line %u", filename, idx);
- if ((t = get_token(, )) == NULL ||
+ if ((t = get_token()) == NULL ||
parse_sizerange(t, >minsz, >maxsz) == -1)
errx(1, "%s: parse error on line %u", filename, idx);
- if ((t = get_token(, )) != NULL &&
+ if ((t = get_token()) != NULL &&
parse_pct(t, >rate) == -1)
errx(1, "%s: parse error on line %u", filename, idx);
if (sa->minsz > sa->maxsz)
@@ -2367,29 +2367,27 @@ parse_autotable(char *filename)
if (pctsum > 100)
errx(1, "%s: sum of extra space allocation > 100%%", filename);
alloc_table[0].sz = idx;
+ free(buf);
fclose(cfile);
}
char *
-get_token(char **s, size_t *len)
+get_token(char **s)
{
char*p, *r;
size_t tlen = 0;
p = *s;
- while (*len > 0 && !isspace((u_char)**s)) {
+ while (**s != '\0' && !isspace((u_char)**s)) {
(*s)++;
- (*len)--;
tlen++;
}
if (tlen == 0)
return (NULL);
/* eat whitespace */
- while (*len > 0 && isspace((u_char)**s)) {
+ while (isspace((u_char)**s))
(*s)++;
- (*len)--;
- }
if ((r = strndup(p, tlen)) == NULL)
err(1, NULL);
--
Christian "naddy" Weisgerber na...@mips.inka.de
fdisk: make use of getline(3)
Replace fgetln(3) with getline(3).
OK?
Index: sbin/fdisk/misc.c
===
RCS file: /cvs/src/sbin/fdisk/misc.c,v
retrieving revision 1.63
diff -u -p -r1.63 misc.c
--- sbin/fdisk/misc.c 3 Jul 2019 03:24:01 - 1.63
+++ sbin/fdisk/misc.c 30 Jan 2021 16:48:36 -
@@ -64,20 +64,18 @@ unit_lookup(char *units)
int
string_from_line(char *buf, size_t buflen)
{
- char *line;
- size_t sz;
+ static char *line;
+ static size_t sz;
+ ssize_t len;
- line = fgetln(stdin, );
- if (line == NULL)
+ len = getline(, , stdin);
+ if (len == -1)
return (1);
- if (line[sz - 1] == '\n')
- sz--;
- if (sz >= buflen)
- sz = buflen - 1;
+ if (line[len - 1] == '\n')
+ line[len - 1] = '\0';
- memcpy(buf, line, sz);
- buf[sz] = '\0';
+ strlcpy(buf, line, buflen);
return (0);
}
--
Christian "naddy" Weisgerber na...@mips.inka.de
disklabel: pointer deref fix
Fix a pointer dereference in disklabel(8).
This looks like somebody wrote *s[0] in place of (*s)[0].
Which in this case happens to be equivalent, but it still looks
wrong.
OK?
Index: sbin/disklabel/editor.c
===
RCS file: /cvs/src/sbin/disklabel/editor.c,v
retrieving revision 1.363
diff -u -p -U6 -r1.363 editor.c
--- sbin/disklabel/editor.c 19 Nov 2019 06:20:37 - 1.363
+++ sbin/disklabel/editor.c 29 Jan 2021 23:50:24 -
@@ -2374,22 +2374,22 @@ char *
get_token(char **s, size_t *len)
{
char*p, *r;
size_t tlen = 0;
p = *s;
- while (*len > 0 && !isspace((u_char)*s[0])) {
+ while (*len > 0 && !isspace((u_char)**s)) {
(*s)++;
(*len)--;
tlen++;
}
if (tlen == 0)
return (NULL);
/* eat whitespace */
- while (*len > 0 && isspace((u_char)*s[0])) {
+ while (*len > 0 && isspace((u_char)**s)) {
(*s)++;
(*len)--;
}
if ((r = strndup(p, tlen)) == NULL)
err(1, NULL);
--
Christian "naddy" Weisgerber na...@mips.inka.de
sed: make use of getline(3)
Replace fgetln(3) with getline(3) in sed.
The mf_fgets() part is from Johann Oskarsson for Illumos/FreeBSD.
Passes our sed regression tests.
OK?
Index: usr.bin/sed/main.c
===
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.41
diff -u -p -r1.41 main.c
--- usr.bin/sed/main.c 13 Oct 2020 06:07:54 - 1.41
+++ usr.bin/sed/main.c 29 Jan 2021 23:12:23 -
@@ -252,15 +252,9 @@ again:
goto again;
}
case ST_FILE:
- if ((p = fgetln(f, )) != NULL) {
+ if (getline(outbuf, outsize, f) != -1) {
+ p = *outbuf;
linenum++;
- if (len >= *outsize) {
- free(*outbuf);
- *outsize = ROUNDLEN(len + 1);
- *outbuf = xmalloc(*outsize);
- }
- memcpy(*outbuf, p, len);
- (*outbuf)[len] = '\0';
if (linenum == 1 && p[0] == '#' && p[1] == 'n')
nflag = 1;
return (*outbuf);
@@ -344,7 +338,8 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
struct stat sb;
size_t len;
char dirbuf[PATH_MAX];
- char *p;
+ static char *p;
+ static size_t psize;
int c, fd;
static int firstfile;
@@ -429,13 +424,13 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
* We are here only when infile is open and we still have something
* to read from it.
*
-* Use fgetln so that we can handle essentially infinite input data.
-* Can't use the pointer into the stdio buffer as the process space
-* because the ungetc() can cause it to move.
+* Use getline() so that we can handle essentially infinite input
+* data. The p and psize are static so each invocation gives
+* getline() the same buffer which is expanded as needed.
*/
- p = fgetln(infile, );
- if (ferror(infile))
- error(FATAL, "%s: %s", fname, strerror(errno ? errno : EIO));
+ len = getline(, , infile);
+ if ((ssize_t)len == -1)
+ error(FATAL, "%s: %s", fname, strerror(errno));
if (len != 0 && p[len - 1] == '\n') {
sp->append_newline = 1;
len--;
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: ftp: make use of getline(3)
Hiltjo Posthuma:
> > @@ -75,19 +74,8 @@ cookie_load(void)
> > if (fp == NULL)
> > err(1, "cannot open cookie file %s", cookiefile);
> > date = time(NULL);
> > - lbuf = NULL;
> > - while ((line = fgetln(fp, )) != NULL) {
> > - if (line[len - 1] == '\n') {
> > - line[len - 1] = '\0';
> > - --len;
> > - } else {
> > - if ((lbuf = malloc(len + 1)) == NULL)
> > - err(1, NULL);
> > - memcpy(lbuf, line, len);
> > - lbuf[len] = '\0';
> > - line = lbuf;
> > - }
> > - line[strcspn(line, "\r")] = '\0';
> > + while (getline(, , fp) != -1) {
> > + line[strcspn(line, "\r\n")] = '\0';
> >
>
> getline returns the number of characters read including the delimeter. This
> size could be used to '\0' terminate the string instead of a strcspn() call.
A strcspn() call is already there.
--
Christian "naddy" Weisgerber na...@mips.inka.de
ftp: make use of getline(3)
Make use of getline(3) in ftp(1).
Replace fparseln(3) with getline(3). This removes the only use
of libutil.a(fparseln.o) from the ramdisk.
Replace a complicated fgetln(3) idiom with the much simpler getline(3).
OK?
Index: distrib/special/ftp/Makefile
===
RCS file: /cvs/src/distrib/special/ftp/Makefile,v
retrieving revision 1.14
diff -u -p -r1.14 Makefile
--- distrib/special/ftp/Makefile16 May 2019 12:44:17 - 1.14
+++ distrib/special/ftp/Makefile29 Jan 2021 18:05:46 -
@@ -6,7 +6,4 @@ PROG= ftp
SRCS= fetch.c ftp.c main.c small.c util.c
.PATH: ${.CURDIR}/../../../usr.bin/ftp
-LDADD+=-lutil
-DPADD+=${LIBUTIL}
-
.include
Index: usr.bin/ftp/Makefile
===
RCS file: /cvs/src/usr.bin/ftp/Makefile,v
retrieving revision 1.34
diff -u -p -r1.34 Makefile
--- usr.bin/ftp/Makefile27 Jan 2021 22:27:41 - 1.34
+++ usr.bin/ftp/Makefile29 Jan 2021 17:57:31 -
@@ -8,8 +8,8 @@ PROG= ftp
SRCS= cmds.c cmdtab.c complete.c cookie.c domacro.c fetch.c ftp.c \
list.c main.c ruserpass.c small.c stringlist.c util.c
-LDADD+=-ledit -lcurses -lutil -ltls -lssl -lcrypto
-DPADD+=${LIBEDIT} ${LIBCURSES} ${LIBUTIL} ${LIBTLS} ${LIBSSL}
${LIBCRYPTO}
+LDADD+=-ledit -lcurses -ltls -lssl -lcrypto
+DPADD+=${LIBEDIT} ${LIBCURSES} ${LIBTLS} ${LIBSSL} ${LIBCRYPTO}
#COPTS+= -Wall -Wconversion -Wstrict-prototypes -Wmissing-prototypes
Index: usr.bin/ftp/cookie.c
===
RCS file: /cvs/src/usr.bin/ftp/cookie.c,v
retrieving revision 1.9
diff -u -p -r1.9 cookie.c
--- usr.bin/ftp/cookie.c16 May 2019 12:44:17 - 1.9
+++ usr.bin/ftp/cookie.c29 Jan 2021 16:07:56 -
@@ -58,10 +58,9 @@ void
cookie_load(void)
{
field_t field;
- size_t len;
time_t date;
- char*line;
- char*lbuf;
+ char*line = NULL;
+ size_t linesize = 0;
char*param;
const char *estr;
FILE*fp;
@@ -75,19 +74,8 @@ cookie_load(void)
if (fp == NULL)
err(1, "cannot open cookie file %s", cookiefile);
date = time(NULL);
- lbuf = NULL;
- while ((line = fgetln(fp, )) != NULL) {
- if (line[len - 1] == '\n') {
- line[len - 1] = '\0';
- --len;
- } else {
- if ((lbuf = malloc(len + 1)) == NULL)
- err(1, NULL);
- memcpy(lbuf, line, len);
- lbuf[len] = '\0';
- line = lbuf;
- }
- line[strcspn(line, "\r")] = '\0';
+ while (getline(, , fp) != -1) {
+ line[strcspn(line, "\r\n")] = '\0';
line += strspn(line, " \t");
if ((*line == '#') || (*line == '\0')) {
@@ -172,7 +160,7 @@ cookie_load(void)
} else
TAILQ_INSERT_TAIL(, ck, entry);
}
- free(lbuf);
+ free(line);
fclose(fp);
}
Index: usr.bin/ftp/fetch.c
===
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.199
diff -u -p -r1.199 fetch.c
--- usr.bin/ftp/fetch.c 1 Jan 2021 17:39:54 - 1.199
+++ usr.bin/ftp/fetch.c 29 Jan 2021 17:57:58 -
@@ -56,7 +56,6 @@
#include
#include
#include
-#include
#include
#ifndef NOSSL
@@ -75,7 +74,6 @@ static void aborthttp(int);
static charhextochar(const char *);
static char*urldecode(const char *);
static char*recode_credentials(const char *_userinfo);
-static char*ftp_readline(FILE *, size_t *);
static voidftp_close(FILE **, struct tls **, int *);
static const char *sockerror(struct tls *);
#ifdef SMALL
@@ -329,6 +327,7 @@ url_get(const char *origline, const char
off_t hashbytes;
const char *errstr;
ssize_t len, wlen;
+ size_t bufsize;
char *proxyhost = NULL;
#ifndef NOSSL
char *sslpath = NULL, *sslhost = NULL;
@@ -790,12 +789,13 @@ noslash:
free(buf);
#endif /* !NOSSL */
buf = NULL;
+ bufsize = 0;
if (fflush(fin) == EOF) {
warnx("Writing HTTP request: %s", sockerror(tls));
goto cleanup_url_get;
}
- if ((buf = ftp_readline(fin, )) == NULL) {
+ if ((len = getline(, , fin)) == -1) {
warnx("Receiving HTTP reply: %s", sockerror(tls));
goto cleanup_url_get;
}
@@ -867,11 +867,10 @@ noslash:
/*
* Read the rest of the header.
*/
- free(buf);
filesize = -1;
for (;;) {
- if ((buf =
Re: getopt.3 bugs section
Edgar Pettijohn: > In the BUGS section for the getopt(3) manual it mentions not using > single digits for options. I know spamd uses -4 and -6 there are > probably others. Should they be changed? Or is the manual mistaken? You misunderstand. The manual warns against the use of digits to pass numerical arguments. This usage exists in some historical cases, e.g. "nice -10" where <10> is the number 10. The use of digits as flags characters, like -4 or -6 to indicate IPv4 or IPv6, is perfectly fine. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: Rename SIMPLEQ_ to STAILQ_, diff 1/7
Denis Fondras: > This diff renames SIMPLEQ_* to STAILQ_* in /usr/src/sys/sys to unify with > FreeBSD and Linux. > > I added aliases at the end of queue.h to avoid breaking base too much. they > will > be removed as soon as diff 2,3,4,5,6,7 are commited. > > net/sniproxy has a patch to define STAILQ_*, it may be removed later. I applied diffs 1 and 2 and did a "make includes" to ensure that _all_ header files were switched over to STAILQ. I then ran a package bulk build on amd64. There were seven build failures. Except for sniproxy those are all programs developed on OpenBSD that use SIMPLEQ: audio/morseplayer devel/got mail/pop3d net/adsuck net/oicb net/sniproxy x11/spectrwm sniproxy breaks because the renamed s/SIMPLEQ/STAILQ/ macros lack STAILQ_REMOVE(). -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: Rename SIMPLEQ_ to STAILQ_, diff 1/7
Theo de Raadt: > What is lacking in this converstation is the justification. > Why? Providing STAILQ in OpenBSD will simplify porting to OpenBSD. (Reality check: There is one port affected by this.) Switching OpenBSD to STAILQ will simplify porting from OpenBSD. (There are three or four FreeBSD ports affected by this.) Having both SIMPLEQ and STAILQ will cause no disruption, but means providing two APIs that are identical except for their name. This appears to be a post-Berkeley divergence; 4.4BSD has neither in . -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: Rename SIMPLEQ_ to STAILQ_, diff 1/7
Denis Fondras: > This diff renames SIMPLEQ_* to STAILQ_* in /usr/src/sys/sys to unify with > FreeBSD and Linux. > > I added aliases at the end of queue.h to avoid breaking base too much. they > will > be removed as soon as diff 2,3,4,5,6,7 are commited. We'll need to run a ports bulk build without the aliases. (I can do that.) There will be some breakage. -- Christian "naddy" Weisgerber na...@mips.inka.de
libcurses: --enable-const
ncurses has a configure option that adds a few more consts to its
headers by way of the NCURSES_CONST define. Starting with version
6.0, this has become the default. OpenBSD is still on ncurses 5.7,
but FreeBSD and I guess most Linux distributions have moved on.
I suggest we also enable the additional consts. This eliminates
compiler warnings when sharing code with other platforms.
The diff below has successfully gone through a release build on
amd64, as well as a full amd64 package build.
OK?
Index: lib/libcurses/curses.h
===
RCS file: /cvs/src/lib/libcurses/curses.h,v
retrieving revision 1.61
diff -u -p -r1.61 curses.h
--- lib/libcurses/curses.h 6 Sep 2010 17:26:17 - 1.61
+++ lib/libcurses/curses.h 9 Dec 2020 22:56:31 -
@@ -101,7 +101,7 @@
* doing so makes it incompatible with other implementations of X/Open Curses.
*/
#undef NCURSES_CONST
-#define NCURSES_CONST /*nothing*/
+#define NCURSES_CONST const
#undef NCURSES_INLINE
#define NCURSES_INLINE inline
Index: lib/libcurses/term.h
===
RCS file: /cvs/src/lib/libcurses/term.h,v
retrieving revision 1.15
diff -u -p -r1.15 term.h
--- lib/libcurses/term.h14 Nov 2015 23:56:49 - 1.15
+++ lib/libcurses/term.h9 Dec 2020 23:03:46 -
@@ -68,7 +68,7 @@ extern "C" {
*/
#undef NCURSES_CONST
-#define NCURSES_CONST /*nothing*/
+#define NCURSES_CONST const
#undef NCURSES_SBOOL
#define NCURSES_SBOOL signed char
Index: lib/libcurses/termcap.h
===
RCS file: /cvs/src/lib/libcurses/termcap.h,v
retrieving revision 1.10
diff -u -p -r1.10 termcap.h
--- lib/libcurses/termcap.h 10 Dec 2013 20:33:51 - 1.10
+++ lib/libcurses/termcap.h 9 Dec 2020 23:03:54 -
@@ -62,7 +62,7 @@ extern "C"
#include
#undef NCURSES_CONST
-#define NCURSES_CONST /*nothing*/
+#define NCURSES_CONST const
#undef NCURSES_OSPEED
#define NCURSES_OSPEED int
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: rpki-client: use strndup instead of malloc + memcpy
Claudio Jeker: > In tal_parse() use strndup() to create the tal descr instead of the more > complex malloc, memcpy version. Result is the same but the strndup version > is a lot nicer. Yes, but... > --- tal.c 11 Oct 2020 12:39:25 - 1.22 > +++ tal.c 3 Dec 2020 12:00:25 - > @@ -198,10 +198,8 @@ tal_parse(const char *fn, char *buf) > dlen = strlen(d); > if (strcasecmp(d + dlen - 4, ".tal") == 0) > dlen -= 4; That looks like a potential out-of-bounds access. Are we guaranteed that dlen >= 4 here? > - if ((p->descr = malloc(dlen + 1)) == NULL) > + if ((p->descr = strndup(d, dlen)) == NULL) > err(1, NULL); > - memcpy(p->descr, d, dlen); > - p->descr[dlen] = '\0'; > > return p; > } ok -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: powerpc lld fix
Mark Kettenis: > What would the impact on ports of disabling base-gcc be on powerpc? None. $ cd /usr/ports $ make ARCH=macppc MACHINE_ARCH=powerpc show=CHOSEN_COMPILER |grep -B1 base-gcc $ -- Christian "naddy" Weisgerber na...@mips.inka.de
basename(3) should have non-const arg, says POSIX
[Picking this up again after a month:]
Our basename(3) and dirname(3) take a const argument:
char*basename(const char *);
char*dirname(const char *);
POSIX says otherwise...
char *basename(char *path);
char *dirname(char *path);
... and explicitly says the functions may modify the input string.
Our functions were const-ified in 1999 by espie@:
proper const semantics for dirname & basename.
(this follows FreeBSD and Linux. Single Unix 2 is still illogical)
Well, four years ago, FreeBSD finally switched to the POSIX prototypes
https://svnweb.freebsd.org/base/head/include/libgen.h?revision=303451=markup
and in fact now has an implementation that modifies the string.
Linux (GNU libc) has a bizarro solution where you get a const basename()
and no dirname() if you just include , but POSIX basename()
and dirname() if you instead or also include .
This is a portability trap. Code written on OpenBSD may not be
prepared for basename() or dirname() to splat a '\0' into the input
string, despite the warning in the man page. This is not hypothetical.
Both Got and OpenCVS have fallen victim to the unportable assumption.
The patch below aligns the function prototypes with POSIX. All
resulting warnings "passing 'const char *' to parameter of type
'char *' discards qualifiers" in the base system have been cleaned
up. It successfully passes "make release". For good measure I'm
also running a package bulk build with it as we speak.
OK?
Index: include/libgen.h
===
RCS file: /cvs/src/include/libgen.h,v
retrieving revision 1.9
diff -u -p -r1.9 libgen.h
--- include/libgen.h25 Jan 2019 00:19:25 - 1.9
+++ include/libgen.h11 Sep 2020 20:41:34 -
@@ -22,8 +22,8 @@
#include
__BEGIN_DECLS
-char *basename(const char *);
-char *dirname(const char *);
+char *basename(char *);
+char *dirname(char *);
__END_DECLS
#endif /* _LIBGEN_H_ */
Index: lib/libc/gen/basename.3
===
RCS file: /cvs/src/lib/libc/gen/basename.3,v
retrieving revision 1.24
diff -u -p -r1.24 basename.3
--- lib/libc/gen/basename.3 25 Jan 2019 00:19:25 - 1.24
+++ lib/libc/gen/basename.3 11 Sep 2020 20:46:30 -
@@ -23,7 +23,7 @@
.Sh SYNOPSIS
.In libgen.h
.Ft char *
-.Fn basename "const char *path"
+.Fn basename "char *path"
.Sh DESCRIPTION
The
.Fn basename
Index: lib/libc/gen/basename.c
===
RCS file: /cvs/src/lib/libc/gen/basename.c,v
retrieving revision 1.16
diff -u -p -r1.16 basename.c
--- lib/libc/gen/basename.c 25 Jan 2019 00:19:25 - 1.16
+++ lib/libc/gen/basename.c 11 Sep 2020 20:43:13 -
@@ -22,7 +22,7 @@
#include
char *
-basename(const char *path)
+basename(char *path)
{
static char bname[PATH_MAX];
size_t len;
Index: lib/libc/gen/dirname.3
===
RCS file: /cvs/src/lib/libc/gen/dirname.3,v
retrieving revision 1.23
diff -u -p -r1.23 dirname.3
--- lib/libc/gen/dirname.3 8 Mar 2019 17:33:23 - 1.23
+++ lib/libc/gen/dirname.3 11 Sep 2020 20:47:08 -
@@ -23,7 +23,7 @@
.Sh SYNOPSIS
.In libgen.h
.Ft char *
-.Fn dirname "const char *path"
+.Fn dirname "char *path"
.Sh DESCRIPTION
The
.Fn dirname
Index: lib/libc/gen/dirname.c
===
RCS file: /cvs/src/lib/libc/gen/dirname.c,v
retrieving revision 1.16
diff -u -p -r1.16 dirname.c
--- lib/libc/gen/dirname.c 25 Jan 2019 00:19:25 - 1.16
+++ lib/libc/gen/dirname.c 11 Sep 2020 20:43:34 -
@@ -24,7 +24,7 @@
/* A slightly modified copy of this file exists in libexec/ld.so */
char *
-dirname(const char *path)
+dirname(char *path)
{
static char dname[PATH_MAX];
size_t len;
--
Christian "naddy" Weisgerber na...@mips.inka.de
arm64 ddb: decode "udf" instruction
This decodes the UDF ("permanently undefined") instruction in ddb's
arm64 disassembler. The particular immediate16 format appears to
be unique to this instruction.
OK? Or don't bother?
Index: arch/arm64/arm64/disasm.c
===
RCS file: /cvs/src/sys/arch/arm64/arm64/disasm.c,v
retrieving revision 1.2
diff -u -p -r1.2 disasm.c
--- arch/arm64/arm64/disasm.c 11 Sep 2020 09:27:10 - 1.2
+++ arch/arm64/arm64/disasm.c 19 Oct 2020 16:17:55 -
@@ -3107,6 +3107,11 @@ OP4FUNC(op_tbz, b5, b40, imm14, Rt)
PRINTF("\n");
}
+OP1FUNC(op_udf, imm16)
+{
+ PRINTF("udf\t#0x%"PRIx64"\n", imm16);
+}
+
OP4FUNC(op_udiv, sf, Rm, Rn, Rd)
{
PRINTF("udiv\t%s, %s, %s\n",
@@ -3668,6 +3673,8 @@ struct insn_info {
{{ 5,16}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}}
#define FMT_IMM16_LL \
{{ 5,16}, { 0, 2}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}}
+#define FMT_IMM16_UDF \
+ {{ 0,16}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}, { 0, 0}}
#define FMT_OP0_OP1_CRN_CRM_OP2_RT \
{{19, 2}, {16, 3}, {12, 4}, { 8, 4}, { 5, 3}, { 0, 5}, { 0, 0}, { 0, 0}}
#define FMT_IMM7_RT2_RN_RT \
@@ -3786,6 +3793,7 @@ static const struct insn_info insn_table
{ 0xd800, 0xdac1, FMT_Z_M_RN_RD, op_pacia },
{ 0xcc00, 0x4e284800, FMT_M_D_RN_RD, op_simd_aes },
{ 0x8c00, 0x5e280800, FMT_OP3_RN_RD, op_simd_sha_reg2 },
+ { 0x, 0x, FMT_IMM16_UDF, op_udf },
{ 0xfff8f01f, 0xd500401f, FMT_OP1_CRM_OP2, op_msr_imm },
{ 0xfff8, 0xd508, FMT_OP1_CRN_CRM_OP2_RT, op_sys },
{ 0xfff8, 0xd528, FMT_OP1_CRN_CRM_OP2_RT, op_sysl },
--
Christian "naddy" Weisgerber na...@mips.inka.de
arm64, armv7: proper illegal instruction
Belatedly, ARM has taken a slice of the reserved opcode space and assigned it as a properly defined illegal instruction, udf #imm16. (Armv8 Architecture Reference Manual, edition F.c, section C6.2.335). Clang already knows about it. We really should use this instead of picking something ad-hoc out of the opcode space. I have verified that this builds on arm64, produces a SIGILL in userland, and drops me into ddb in the kernel. armv7 has an equivalent instruction. kettenis@ confirms it builds and SIGILLs there. OK? Index: lib/csu/aarch64/md_init.h === RCS file: /cvs/src/lib/csu/aarch64/md_init.h,v retrieving revision 1.9 diff -u -p -r1.9 md_init.h --- lib/csu/aarch64/md_init.h 15 Oct 2020 16:30:21 - 1.9 +++ lib/csu/aarch64/md_init.h 19 Oct 2020 11:57:02 - @@ -115,5 +115,5 @@ " svc #0 \n" \ " dsb nsh \n" \ " isb \n" \ - " .word 0xa000f7f0 /* illegal */ \n" \ + " udf #0 \n" \ ".previous"); Index: lib/csu/arm/md_init.h === RCS file: /cvs/src/lib/csu/arm/md_init.h,v retrieving revision 1.16 diff -u -p -r1.16 md_init.h --- lib/csu/arm/md_init.h 15 Oct 2020 16:30:23 - 1.16 +++ lib/csu/arm/md_init.h 19 Oct 2020 13:23:00 - @@ -159,5 +159,5 @@ " swi #0 \n" \ " dsb nsh \n" \ " isb \n" \ - " .word 0xa000f7f0 /* illegal */ \n" \ + " udf #0 \n" \ ".previous"); Index: lib/libc/arch/aarch64/sys/tfork_thread.S === RCS file: /cvs/src/lib/libc/arch/aarch64/sys/tfork_thread.S,v retrieving revision 1.5 diff -u -p -r1.5 tfork_thread.S --- lib/libc/arch/aarch64/sys/tfork_thread.S18 Oct 2020 14:28:16 - 1.5 +++ lib/libc/arch/aarch64/sys/tfork_thread.S19 Oct 2020 11:59:32 - @@ -43,6 +43,6 @@ ENTRY(__tfork_thread) mov x0, x3 blr x2 SYSTRAP(__threxit) - .word 0xa000f7f0 /* illegal on all cpus? */ + udf #0 .cfi_endproc END(__tfork_thread) Index: lib/libc/arch/arm/sys/tfork_thread.S === RCS file: /cvs/src/lib/libc/arch/arm/sys/tfork_thread.S,v retrieving revision 1.5 diff -u -p -r1.5 tfork_thread.S --- lib/libc/arch/arm/sys/tfork_thread.S18 Oct 2020 14:28:17 - 1.5 +++ lib/libc/arch/arm/sys/tfork_thread.S19 Oct 2020 13:23:35 - @@ -37,5 +37,5 @@ ENTRY(__tfork_thread) mov pc, r2 nop SYSTRAP(__threxit) - .word 0xa000f7f0 /* illegal on all cpus? */ + udf #0 END(__tfork_thread) Index: sys/arch/arm/arm/sigcode.S === RCS file: /cvs/src/sys/arch/arm/arm/sigcode.S,v retrieving revision 1.9 diff -u -p -r1.9 sigcode.S --- sys/arch/arm/arm/sigcode.S 13 Mar 2020 08:46:50 - 1.9 +++ sys/arch/arm/arm/sigcode.S 19 Oct 2020 13:23:55 - @@ -72,7 +72,7 @@ _C_LABEL(esigcode): .globl sigfill sigfill: - .word 0xa000f7f0 /* illegal on all cpus? */ + udf #0 esigfill: .data Index: sys/arch/arm64/arm64/locore.S === RCS file: /cvs/src/sys/arch/arm64/arm64/locore.S,v retrieving revision 1.31 diff -u -p -r1.31 locore.S --- sys/arch/arm64/arm64/locore.S 13 Mar 2020 00:14:38 - 1.31 +++ sys/arch/arm64/arm64/locore.S 19 Oct 2020 12:02:23 - @@ -366,7 +366,7 @@ _C_LABEL(esigcode): .globl sigfill sigfill: - .word 0xa000f7f0 /* FIXME: illegal on all cpus? */ + udf #0 esigfill: .data -- Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: usr.bin/cvs
Accommodate POSIX basename(3) that takes a non-const parameter and
may modify the string buffer.
There were only two compiler warnings about discarded const, but
there are numerous instances where the code assumes non-POSIX
semantics for basename() and dirname(). Given that there is at
least a FreeBSD port of OpenCVS, cleaning this up is more than
cosmetic.
This could definitely use proofreading.
I chose __func__ (otherwise not used anywhere) over breaking an overly
long "function_" "name" into parts like that.
OK?
Index: usr.bin/cvs/admin.c
===
RCS file: /cvs/src/usr.bin/cvs/admin.c,v
retrieving revision 1.68
diff -u -p -r1.68 admin.c
--- usr.bin/cvs/admin.c 1 Jun 2017 08:08:24 - 1.68
+++ usr.bin/cvs/admin.c 16 Oct 2020 21:14:05 -
@@ -246,12 +246,17 @@ cvs_admin_local(struct cvs_file *cf)
struct cvs_file *ocf;
struct rcs_access *acp;
int ofd;
- char *d, *f, fpath[PATH_MAX], repo[PATH_MAX];
+ char *d, dbuf[PATH_MAX], *f, fbuf[PATH_MAX];
+ char fpath[PATH_MAX], repo[PATH_MAX];
-
- if ((f = basename(oldfilename)) == NULL)
+ if (strlcpy(fbuf, oldfilename, sizeof(fbuf)) >= sizeof(fbuf))
+ fatal("cvs_admin_local: truncation");
+ if ((f = basename(fbuf)) == NULL)
fatal("cvs_admin_local: basename failed");
- if ((d = dirname(oldfilename)) == NULL)
+
+ if (strlcpy(dbuf, oldfilename, sizeof(dbuf)) >= sizeof(dbuf))
+ fatal("cvs_admin_local: truncation");
+ if ((d = dirname(dbuf)) == NULL)
fatal("cvs_admin_local: dirname failed");
cvs_get_repository_path(d, repo, PATH_MAX);
Index: usr.bin/cvs/checkout.c
===
RCS file: /cvs/src/usr.bin/cvs/checkout.c,v
retrieving revision 1.171
diff -u -p -r1.171 checkout.c
--- usr.bin/cvs/checkout.c 1 Jun 2017 08:08:24 - 1.171
+++ usr.bin/cvs/checkout.c 16 Oct 2020 21:46:33 -
@@ -239,7 +239,7 @@ checkout_check_repository(int argc, char
struct module_checkout *mc;
struct cvs_ignpat *ip;
struct cvs_filelist *fl, *nxt;
- char repo[PATH_MAX], fpath[PATH_MAX], *f[1];
+ char repo[PATH_MAX], fpath[PATH_MAX], path[PATH_MAX], *f[1];
build_dirs = print_stdout ? 0 : 1;
@@ -329,14 +329,25 @@ checkout_check_repository(int argc, char
cr.flags = flags;
if (!(mc->mc_flags & MODULE_ALIAS)) {
+ if (strlcpy(path, fl->file_path,
+ sizeof(path)) >= sizeof(path))
+ fatal("%s: truncation",
+ __func__);
module_repo_root =
- xstrdup(dirname(fl->file_path));
+ xstrdup(dirname(path));
d = wdir;
+ if (strlcpy(path, fl->file_path,
+ sizeof(path)) >= sizeof(path))
+ fatal("%s: truncation",
+ __func__);
(void)xsnprintf(fpath, sizeof(fpath),
- "%s/%s", d,
- basename(fl->file_path));
+ "%s/%s", d, basename(path));
} else {
- d = dirname(wdir);
+ if (strlcpy(path, wdir,
+ sizeof(path)) >= sizeof(path))
+ fatal("%s: truncation",
+ __func__);
+ d = dirname(path);
strlcpy(fpath, fl->file_path,
sizeof(fpath));
}
@@ -387,7 +398,7 @@ checkout_check_repository(int argc, char
static int
checkout_classify(const char *repo, const char *arg)
{
- char *d, *f, fpath[PATH_MAX];
+ char *d, dbuf[PATH_MAX], *f, fbuf[PATH_MAX], fpath[PATH_MAX];
struct stat sb;
if (stat(repo, ) == 0) {
@@ -395,8 +406,13 @@ checkout_classify(const char *repo, cons
return CVS_DIR;
}
- d = dirname(repo);
- f = basename(repo);
+ if (strlcpy(dbuf, repo, sizeof(dbuf)) >= sizeof(dbuf))
+ fatal("checkout_classify: truncation");
+ d =
Non-const basename: usr.bin/ftp
Accommodate POSIX basename(3) that takes a non-const parameter and
may modify the string buffer.
I've tried to follow the conventions of the existing code.
ok?
Index: usr.bin/ftp/fetch.c
===
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.197
diff -u -p -r1.197 fetch.c
--- usr.bin/ftp/fetch.c 4 Jul 2020 11:23:35 - 1.197
+++ usr.bin/ftp/fetch.c 15 Oct 2020 21:14:28 -
@@ -192,7 +192,7 @@ file_get(const char *path, const char *o
int fd, out = -1, rval = -1, save_errno;
volatile sig_t oldintr, oldinti;
const char *savefile;
- char*buf = NULL, *cp;
+ char*buf = NULL, *cp, *pathbuf = NULL;
const size_t buflen = 128 * 1024;
off_thashbytes;
ssize_t len, wlen;
@@ -215,8 +215,12 @@ file_get(const char *path, const char *o
else {
if (path[strlen(path) - 1] == '/') /* Consider no file */
savefile = NULL;/* after dir invalid. */
- else
- savefile = basename(path);
+ else {
+ pathbuf = strdup(path);
+ if (pathbuf == NULL)
+ errx(1, "Can't allocate memory for filename");
+ savefile = basename(pathbuf);
+ }
}
if (EMPTYSTRING(savefile)) {
@@ -294,6 +298,7 @@ file_get(const char *path, const char *o
cleanup_copy:
free(buf);
+ free(pathbuf);
if (out >= 0 && out != fileno(stdout))
close(out);
close(fd);
@@ -315,6 +320,7 @@ url_get(const char *origline, const char
int isunavail = 0, retryafter = -1;
struct addrinfo hints, *res0, *res;
const char *savefile;
+ char *pathbuf = NULL;
char *proxyurl = NULL;
char *credentials = NULL, *proxy_credentials = NULL;
int fd = -1, out = -1;
@@ -412,8 +418,12 @@ noslash:
else {
if (path[strlen(path) - 1] == '/') /* Consider no file */
savefile = NULL;/* after dir invalid. */
- else
- savefile = basename(path);
+ else {
+ pathbuf = strdup(path);
+ if (pathbuf == NULL)
+ errx(1, "Can't allocate memory for filename");
+ savefile = basename(pathbuf);
+ }
}
if (EMPTYSTRING(savefile)) {
@@ -1106,6 +1116,7 @@ cleanup_url_get:
if (out >= 0 && out != fileno(stdout))
close(out);
free(buf);
+ free(pathbuf);
free(proxyhost);
free(proxyurl);
free(newline);
Index: usr.bin/ftp/util.c
===
RCS file: /cvs/src/usr.bin/ftp/util.c,v
retrieving revision 1.93
diff -u -p -r1.93 util.c
--- usr.bin/ftp/util.c 6 Jul 2020 17:11:29 - 1.93
+++ usr.bin/ftp/util.c 15 Oct 2020 21:31:55 -
@@ -763,7 +763,7 @@ progressmeter(int flag, const char *file
off_t cursize, abbrevsize;
double elapsed;
int ratio, barlength, i, remaining, overhead = 30;
- char buf[512];
+ char buf[512], *filenamebuf;
if (flag == -1) {
clock_gettime(CLOCK_MONOTONIC, );
@@ -782,11 +782,13 @@ progressmeter(int flag, const char *file
ratio = MAXIMUM(ratio, 0);
ratio = MINIMUM(ratio, 100);
if (!verbose && flag == -1) {
- filename = basename(filename);
- if (filename != NULL) {
+ filenamebuf = strdup(filename);
+ filename = basename(filenamebuf);
+ if (filenamebuf != NULL && filename != NULL) {
free(title);
title = strdup(filename);
}
+ free(filenamebuf);
}
buf[0] = 0;
--
Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: usr.sbin/vmd, usr.sbin/vmctl
Accommodate POSIX basename(3) that takes a non-const parameter and
may in fact modify the string buffer.
The file is built by vmd and vmctl.
I'm uncertain if we want a truncation check here. Both in vmd and
vmctl, the path has been validated by a previous open(), but given
the code complexity, do we want to rely on this?
Index: usr.sbin/vmd/vioqcow2.c
===
RCS file: /cvs/src/usr.sbin/vmd/vioqcow2.c,v
retrieving revision 1.13
diff -u -p -r1.13 vioqcow2.c
--- usr.sbin/vmd/vioqcow2.c 10 Jan 2019 19:21:02 - 1.13
+++ usr.sbin/vmd/vioqcow2.c 14 Oct 2020 20:57:31 -
@@ -145,6 +145,7 @@ virtio_qcow2_init(struct virtio_backing
ssize_t
virtio_qcow2_get_base(int fd, char *path, size_t npath, const char *dpath)
{
+ char dpathbuf[PATH_MAX];
char expanded[PATH_MAX];
struct qcheader header;
uint64_t backingoff;
@@ -186,7 +187,8 @@ virtio_qcow2_get_base(int fd, char *path
return -1;
}
} else {
- s = dirname(dpath);
+ strlcpy(dpathbuf, dpath, sizeof(dpathbuf));
+ s = dirname(dpathbuf);
if (snprintf(expanded, sizeof(expanded),
"%s/%s", s, path) >= (int)sizeof(expanded)) {
log_warnx("path too long: %s/%s", s, path);
--
Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: usr.bin/rcs
Accommodate POSIX basename(3) that takes a non-const parameter and
may in fact modify the string buffer.
ok?
Index: usr.bin/rcs/rlog.c
===
RCS file: /cvs/src/usr.bin/rcs/rlog.c,v
retrieving revision 1.74
diff -u -p -r1.74 rlog.c
--- usr.bin/rcs/rlog.c 16 Oct 2016 13:35:51 - 1.74
+++ usr.bin/rcs/rlog.c 14 Oct 2020 20:18:55 -
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -348,7 +349,7 @@ rlog_select_daterev(RCSFILE *rcsfile, ch
static void
rlog_file(const char *fname, RCSFILE *file)
{
- char numb[RCS_REV_BUFSZ];
+ char fnamebuf[PATH_MAX], numb[RCS_REV_BUFSZ];
u_int nrev;
struct rcs_sym *sym;
struct rcs_access *acp;
@@ -364,7 +365,10 @@ rlog_file(const char *fname, RCSFILE *fi
} else
nrev = file->rf_ndelta;
- if ((workfile = basename(fname)) == NULL)
+ if (strlcpy(fnamebuf, fname, sizeof(fnamebuf)) >= sizeof(fnamebuf))
+ errx(1, "rlog_file: truncation");
+
+ if ((workfile = basename(fnamebuf)) == NULL)
err(1, "basename");
/*
--
Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: sbin/pfctl
Accommodate a basename(3) that takes a non-const parameter and may
in fact modify the string buffer.
The length of anchor has already been checked in main().
ok?
Index: sbin/pfctl/pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.382
diff -u -p -r1.382 pfctl.c
--- sbin/pfctl/pfctl.c 16 Jan 2020 01:02:20 - 1.382
+++ sbin/pfctl/pfctl.c 13 Oct 2020 20:45:16 -
@@ -2241,16 +2241,19 @@ pfctl_get_anchors(int dev, const char *a
{
struct pfioc_rulesetpr;
static struct pfr_anchors anchors;
+ char anchorbuf[PATH_MAX];
char *n;
SLIST_INIT();
memset(, 0, sizeof(pr));
if (*anchor != '\0') {
- n = dirname(anchor);
+ strlcpy(anchorbuf, anchor, sizeof(anchorbuf));
+ n = dirname(anchorbuf);
if (n[0] != '.' && n[1] != '\0')
strlcpy(pr.path, n, sizeof(pr.path));
- n = basename(anchor);
+ strlcpy(anchorbuf, anchor, sizeof(anchorbuf));
+ n = basename(anchorbuf);
if (n != NULL)
strlcpy(pr.name, n, sizeof(pr.name));
}
--
Christian "naddy" Weisgerber na...@mips.inka.de
net/pfvar.h: MAXPATHLEN -> PATH_MAX
In revision 1.407 of , deraadt@ replaced MAXPATHLEN
with PATH_MAX so userland wouldn't have to pull in .
In 1.466, sashan@ accidentally slipped one MAXPATHLEN back in,
because its use is ubiquitous on the kernel side of pf. Switch
this over to PATH_MAX again.
pfctl(8) doesn't actually use this, so it's very cosmetic.
Index: sys/net/pfvar.h
===
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.496
diff -u -p -r1.496 pfvar.h
--- sys/net/pfvar.h 24 Aug 2020 15:30:58 - 1.496
+++ sys/net/pfvar.h 13 Oct 2020 20:21:04 -
@@ -475,7 +475,7 @@ union pf_rule_ptr {
};
#definePF_ANCHOR_NAME_SIZE 64
-#definePF_ANCHOR_MAXPATH (MAXPATHLEN - PF_ANCHOR_NAME_SIZE - 1)
+#definePF_ANCHOR_MAXPATH (PATH_MAX - PF_ANCHOR_NAME_SIZE - 1)
#definePF_OPTIMIZER_TABLE_PFX "__automatic_"
struct pf_rule {
--
Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: bin/chio
As far as I understand, the basename() here is specifically intended
to skip a leading "/dev/". So how about doing this expressly?
Do we want to use _PATH_DEV or "/dev/"? There's a "/dev/rst%d" a
few lines outside of the diff context...
Index: bin/chio/parse.y
===
RCS file: /cvs/src/bin/chio/parse.y,v
retrieving revision 1.22
diff -u -p -r1.22 parse.y
--- bin/chio/parse.y13 Feb 2019 22:57:07 - 1.22
+++ bin/chio/parse.y13 Oct 2020 19:23:22 -
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -445,10 +446,12 @@ parse_tapedev(const char *filename, cons
errors = file->errors;
popfile();
+ if (strncmp(changer, _PATH_DEV, sizeof(_PATH_DEV) - 1) == 0)
+ changer += sizeof(_PATH_DEV) - 1;
TAILQ_FOREACH(p, , entry) {
- if (strcmp(basename(changer), p->name) == 0) {
+ if (strcmp(changer, p->name) == 0) {
if (drive >= 0 && drive < p->drivecnt) {
- if (asprintf(, "/dev/%s",
+ if (asprintf(, _PATH_DEV "%s",
p->drives[drive]) == -1)
errx(1, "malloc failed");
} else
--
Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: usr.sbin/hotplugd
Accommodate a basename(3) that takes a non-const parameter and may
in fact modify the string buffer.
Between access(file, ...) and execl(file, ...), no check for
truncation should be necessary.
ok?
Index: usr.sbin/hotplugd/hotplugd.c
===
RCS file: /cvs/src/usr.sbin/hotplugd/hotplugd.c,v
retrieving revision 1.15
diff -u -p -r1.15 hotplugd.c
--- usr.sbin/hotplugd/hotplugd.c30 Apr 2019 17:05:15 - 1.15
+++ usr.sbin/hotplugd/hotplugd.c13 Oct 2020 16:19:43 -
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -163,7 +164,10 @@ exec_script(const char *file, int class,
}
if (pid == 0) {
/* child process */
- execl(file, basename(file), strclass, name, (char *)NULL);
+ char filebuf[PATH_MAX];
+
+ strlcpy(filebuf, file, sizeof(filebuf));
+ execl(file, basename(filebuf), strclass, name, (char *)NULL);
syslog(LOG_ERR, "execl %s: %m", file);
_exit(1);
/* NOTREACHED */
--
Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: libkvm
Accommodate a basename(3) that takes a non-const parameter and may
in fact modify the string buffer.
Note that strlen(uf) >= PATH_MAX is already checked by the caller
in _kvm_open().
ok?
Index: lib/libkvm/kvm.c
===
RCS file: /cvs/src/lib/libkvm/kvm.c,v
retrieving revision 1.66
diff -u -p -r1.66 kvm.c
--- lib/libkvm/kvm.c28 Jun 2019 13:32:42 - 1.66
+++ lib/libkvm/kvm.c13 Oct 2020 08:55:29 -
@@ -676,12 +676,13 @@ static int
kvm_dbopen(kvm_t *kd, const char *uf)
{
char dbversion[_POSIX2_LINE_MAX], kversion[_POSIX2_LINE_MAX];
- char dbname[PATH_MAX];
+ char dbname[PATH_MAX], ufbuf[PATH_MAX];
struct nlist nitem;
size_t dbversionlen;
DBT rec;
- uf = basename(uf);
+ strlcpy(ufbuf, uf, sizeof(ufbuf));
+ uf = basename(ufbuf);
(void)snprintf(dbname, sizeof(dbname), "%skvm_%s.db", _PATH_VARDB, uf);
kd->db = dbopen(dbname, O_RDONLY, 0, DB_HASH, NULL);
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: Non-const basename: usr.bin/sed
Martijn van Duren: > Wouldn't the following diff be a little simpler? Yes, it would. Should dirbuf be static like oldfname and tmpfname? Index: main.c === RCS file: /cvs/src/usr.bin/sed/main.c,v retrieving revision 1.40 diff -u -p -r1.40 main.c --- main.c 8 Dec 2018 23:11:24 - 1.40 +++ main.c 11 Oct 2020 15:08:29 - @@ -96,6 +96,7 @@ const char *fname;/* File name. */ const char *outfname; /* Output file name */ static char oldfname[PATH_MAX];/* Old file name (for in-place editing) */ static char tmpfname[PATH_MAX];/* Temporary file name (for in-place editing) */ +static char dirbuf[PATH_MAX]; /* Temporary path name (for dirname(3)) */ char *inplace; /* Inplace edit file extension */ u_long linenum; @@ -397,8 +398,9 @@ mf_fgets(SPACE *sp, enum e_spflag spflag if (len > sizeof(oldfname)) error(FATAL, "%s: name too long", fname); } - len = snprintf(tmpfname, sizeof(tmpfname), "%s/sedXX", - dirname(fname)); + strlcpy(dirbuf, fname, sizeof(dirbuf)); + len = snprintf(tmpfname, sizeof(tmpfname), + "%s/sedXX", dirname(dirbuf)); if (len >= sizeof(tmpfname)) error(FATAL, "%s: name too long", fname); if ((fd = mkstemp(tmpfname)) == -1) -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: WANTLIB problems and possible solution: the libset design
Marc Espie: > The new design: > > The idea behind "libset" is to be able to specify a "set" of wantlib that > corresponds to our package, AND to just write WANTLIB wrt that libset for > that specific set of libraries. I'm struggling to understand whether this libset records the libraries a port depends on, the libraries the port provides, or both. Let's say--slightly simplified from reality--we have devel/gettext that provides libintl and depends on iconv from converters/libiconv. What would gettext's LIBSET entry look like? (1) LIBSET = iconv (2) LIBSET = intl (3) LIBSET = intl iconv -- Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: usr.bin/compress
Accommodate POSIX basename(3) that takes a non-const parameter and
may in fact modify the string buffer.
Following martijn@'s comment for the sed diff, we don't need to
check for truncation because the "in" path has already been validated
by a preceding open(2).
ok?
Index: usr.bin/compress/main.c
===
RCS file: /cvs/src/usr.bin/compress/main.c,v
retrieving revision 1.96
diff -u -p -r1.96 main.c
--- usr.bin/compress/main.c 28 Jun 2019 13:35:00 - 1.96
+++ usr.bin/compress/main.c 10 Oct 2020 20:10:44 -
@@ -481,6 +481,7 @@ docompress(const char *in, char *out, co
{
#ifndef SMALL
u_char buf[Z_BUFSIZE];
+ char namebuf[PATH_MAX];
char *name;
int error, ifd, ofd, oreg;
void *cookie;
@@ -534,7 +535,8 @@ docompress(const char *in, char *out, co
}
if (!pipin && storename) {
- name = basename(in);
+ strlcpy(namebuf, in, sizeof(namebuf));
+ name = basename(namebuf);
mtime = (u_int32_t)sb->st_mtime;
}
if ((cookie = method->wopen(ofd, name, bits, mtime)) == NULL) {
--
Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: usr.bin/sed
Changing basename(3) and dirname(3) to the POSIX-mandated non-const
parameters produces this warnings when compiling sed(1):
/usr/src/usr.bin/sed/main.c:401:16: warning: passing 'const char *' to parameter
of type 'char *' discards qualifiers [-Wincompatible-pointer-types-discards-qua
lifiers]
Here's a fix to accommodate a basename(3) that takes a non-const
parameter and may in fact modify the string buffer. Based on FreeBSD
like the surrounding in-place editing code.
OK?
Index: main.c
===
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.40
diff -u -p -r1.40 main.c
--- main.c 8 Dec 2018 23:11:24 - 1.40
+++ main.c 10 Oct 2020 15:16:12 -
@@ -343,6 +343,7 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
{
struct stat sb;
size_t len;
+ char *dirbuf;
char *p;
int c, fd;
static int firstfile;
@@ -397,8 +398,11 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
if (len > sizeof(oldfname))
error(FATAL, "%s: name too long",
fname);
}
+ if ((dirbuf = strdup(fname)) == NULL)
+ error(FATAL, "%s", strerror(errno));
len = snprintf(tmpfname, sizeof(tmpfname),
"%s/sedXX",
- dirname(fname));
+ dirname(dirbuf));
+ free(dirbuf);
if (len >= sizeof(tmpfname))
error(FATAL, "%s: name too long", fname);
if ((fd = mkstemp(tmpfname)) == -1)
--
Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: usr.bin/patch
Changing basename(3) and dirname(3) to the POSIX-mandated non-const
parameters produces these warnings when compiling patch(1):
/usr/src/usr.bin/patch/backupfile.c:61:34: warning: passing 'const char *' to pa
rameter of type 'char *' discards qualifiers [-Wincompatible-pointer-types-disca
rds-qualifiers]
/usr/src/usr.bin/patch/backupfile.c:64:16: warning: passing 'const char *' to pa
rameter of type 'char *' discards qualifiers [-Wincompatible-pointer-types-disca
rds-qualifiers]
Here's a fix to accommodate a basename(3) that takes a non-const
parameter and may in fact modify the string buffer. This is
originally from Joerg Sonnenberger for DragonFly BSD and has since
spread to the other BSDs.
https://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/41871674d0079dec70d55eb824f39d07dc7b3310
(The corresponding change to inp.c is no longer applicable as that
code has been removed.)
OK?
Index: usr.bin/patch/backupfile.c
===
RCS file: /cvs/src/usr.bin/patch/backupfile.c,v
retrieving revision 1.21
diff -u -p -r1.21 backupfile.c
--- usr.bin/patch/backupfile.c 26 Nov 2013 13:19:07 - 1.21
+++ usr.bin/patch/backupfile.c 10 Oct 2020 14:36:58 -
@@ -53,21 +53,32 @@ static void invalid_arg(const char *, co
char *
find_backup_file_name(const char *file)
{
- char*dir, *base_versions;
+ char*dir, *base_versions, *tmp_file;
int highest_backup;
if (backup_type == simple)
return concat(file, simple_backup_suffix);
- base_versions = concat(basename(file), ".~");
+ tmp_file = strdup(file);
+ if (tmp_file == NULL)
+ return NULL;
+ base_versions = concat(basename(tmp_file), ".~");
+ free(tmp_file);
if (base_versions == NULL)
return NULL;
- dir = dirname(file);
+ tmp_file = strdup(file);
+ if (tmp_file == NULL) {
+ free(base_versions);
+ return NULL;
+ }
+ dir = dirname(tmp_file);
if (dir == NULL) {
free(base_versions);
+ free(tmp_file);
return NULL;
}
highest_backup = max_backup_version(base_versions, dir);
free(base_versions);
+ free(tmp_file);
if (backup_type == numbered_existing && highest_backup == 0)
return concat(file, simple_backup_suffix);
return make_version_name(file, highest_backup + 1);
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: ssh-keygen: generate ed25519 keys by default
On 2020-10-08, Eldritch wrote: > With the recent change to prefer ed25519 keys on the server side [1] > (unless I misunderstood what the change does), I think generating This only changed the client's order of preference for the various server key types. If a server doesn't offer an Ed25519 key, the client will transparently fall back to one of the other types. > ed25519 keys by default with ssh-keygen makes sense at this point. > > Is there a reason not to do this? I am curious if so, as there's no > discussion on this matter that I could find. Those are mostly user keys. What happens if you upload an Ed25519 public user key to a server that doesn't handle this key type? It won't work. And you're not likely to be presented with a helpful error message. It just doesn't work. At this point, I don't know how many SSH servers are still out there that don't handle Ed25519. I still have an ECDSA key somewhere that I use to log into a machine that still runs... "OpenSSH_6.0p1 Debian-4+deb7u7, OpenSSL 1.0.1t 3 May 2016". There is a lot of networking equipment that allows uploading of a user key for SSH login but may include a comically obsolete version of OpenSSH or some alternative implementation that doesn't do Ed25519. So... is it the right time yet? I don't know, and it's certainly not my decision, but I think that's the background. > --- ssh-keygen.c 9 Sep 2020 03:08:01 - 1.420 > +++ ssh-keygen.c 8 Oct 2020 08:21:37 - That's at least missing a corresponding change to the man page. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: basename(3) should have non-const arg, says POSIX
Todd C. Miller:
> This is probably the right thing to do but we should fix the warnings
> it generates. In this new world order, passing a const char * to
> basename() or dirname() is unsafe.
FWIW, here's the list:
/usr/src/lib/libkvm/kvm.c:684:16: warning: passing 'const char *' to parameter
of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
uf = basename(uf);
^~
/usr/src/bin/chio/parse.y:449:23: warning: passing 'const char *' to parameter
of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
if (strcmp(basename(changer), p->name) == 0) {
^~~
/usr/src/sbin/pfctl/pfctl.c:2250:15: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
n = dirname(anchor);
^~
/usr/src/sbin/pfctl/pfctl.c:2253:16: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
n = basename(anchor);
^~
/usr/src/usr.bin/compress/main.c:537:19: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
name = basename(in);
^~
/usr/src/usr.bin/cvs/checkout.c:398:14: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
d = dirname(repo);
^~~~
/usr/src/usr.bin/cvs/checkout.c:399:15: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
f = basename(repo);
^~~~
/usr/src/usr.bin/ftp/fetch.c:219:24: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
savefile = basename(path);
^~~~
/usr/src/usr.bin/ftp/util.c:785:23: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
filename = basename(filename);
^~~~
/usr/src/usr.bin/patch/backupfile.c:61:34: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
base_versions = concat(basename(file), ".~");
^~~~
/usr/src/usr.bin/patch/backupfile.c:64:16: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
dir = dirname(file);
^~~~
/usr/src/usr.bin/rcs/rlog.c:367:27: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
if ((workfile = basename(fname)) == NULL)
^
/usr/src/usr.bin/sed/main.c:401:16: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
dirname(fname));
^
/usr/src/usr.sbin/hotplugd/hotplugd.c:166:24: warning: passing 'const char *'
to parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
execl(file, basename(file), strclass, name, (char *)NULL);
^~~~
/usr/src/usr.sbin/vmd/vioqcow2.c:189:15: warning: passing 'const char *' to
parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
s = dirname(dpath);
^
/usr/src/usr.sbin/vmctl/../vmd/vioqcow2.c:189:15: warning: passing 'const char
*' to parameter of type 'char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
s = dirname(dpath);
^
--
Christian "naddy" Weisgerber na...@mips.inka.de
basename(3) should have non-const arg, says POSIX
Our basename(3) and dirname(3) take a const argument:
char*basename(const char *);
char*dirname(const char *);
POSIX says otherwise...
char *basename(char *path);
char *dirname(char *path);
... and explicitly says the functions may modify the input string.
Our functions were const-ified in 1999 by espie@:
proper const semantics for dirname & basename.
(this follows FreeBSD and Linux. Single Unix 2 is still illogical)
Well, four years ago, FreeBSD finally switched to the POSIX prototypes
https://svnweb.freebsd.org/base/head/include/libgen.h?revision=303451=markup
and in fact now has an implementation that modifies the string.
Linux (GNU libc) has a bizarro solution where you get a const basename()
and no dirname() if you just include , but POSIX basename()
and dirname() if you instead or also include .
A make build with the patch below succeeds, but gains some new
warnings "passing 'const char *' to parameter of type 'char *'
discards qualifiers".
This is a portability trap. Code written on OpenBSD may not be
prepared for basename() or dirname() to splat a '\0' into the input
string, despite the warning in the man page.
I'm in favor of moving to the POSIX prototypes, but I don't know
if there are any hidden pitfalls I may be missing. Opinions,
comments?
Index: include/libgen.h
===
RCS file: /cvs/src/include/libgen.h,v
retrieving revision 1.9
diff -u -p -r1.9 libgen.h
--- include/libgen.h25 Jan 2019 00:19:25 - 1.9
+++ include/libgen.h11 Sep 2020 20:41:34 -
@@ -22,8 +22,8 @@
#include
__BEGIN_DECLS
-char *basename(const char *);
-char *dirname(const char *);
+char *basename(char *);
+char *dirname(char *);
__END_DECLS
#endif /* _LIBGEN_H_ */
Index: lib/libc/gen/basename.3
===
RCS file: /cvs/src/lib/libc/gen/basename.3,v
retrieving revision 1.24
diff -u -p -r1.24 basename.3
--- lib/libc/gen/basename.3 25 Jan 2019 00:19:25 - 1.24
+++ lib/libc/gen/basename.3 11 Sep 2020 20:46:30 -
@@ -23,7 +23,7 @@
.Sh SYNOPSIS
.In libgen.h
.Ft char *
-.Fn basename "const char *path"
+.Fn basename "char *path"
.Sh DESCRIPTION
The
.Fn basename
Index: lib/libc/gen/basename.c
===
RCS file: /cvs/src/lib/libc/gen/basename.c,v
retrieving revision 1.16
diff -u -p -r1.16 basename.c
--- lib/libc/gen/basename.c 25 Jan 2019 00:19:25 - 1.16
+++ lib/libc/gen/basename.c 11 Sep 2020 20:43:13 -
@@ -22,7 +22,7 @@
#include
char *
-basename(const char *path)
+basename(char *path)
{
static char bname[PATH_MAX];
size_t len;
Index: lib/libc/gen/dirname.3
===
RCS file: /cvs/src/lib/libc/gen/dirname.3,v
retrieving revision 1.23
diff -u -p -r1.23 dirname.3
--- lib/libc/gen/dirname.3 8 Mar 2019 17:33:23 - 1.23
+++ lib/libc/gen/dirname.3 11 Sep 2020 20:47:08 -
@@ -23,7 +23,7 @@
.Sh SYNOPSIS
.In libgen.h
.Ft char *
-.Fn dirname "const char *path"
+.Fn dirname "char *path"
.Sh DESCRIPTION
The
.Fn dirname
Index: lib/libc/gen/dirname.c
===
RCS file: /cvs/src/lib/libc/gen/dirname.c,v
retrieving revision 1.16
diff -u -p -r1.16 dirname.c
--- lib/libc/gen/dirname.c 25 Jan 2019 00:19:25 - 1.16
+++ lib/libc/gen/dirname.c 11 Sep 2020 20:43:34 -
@@ -24,7 +24,7 @@
/* A slightly modified copy of this file exists in libexec/ld.so */
char *
-dirname(const char *path)
+dirname(char *path)
{
static char dname[PATH_MAX];
size_t len;
--
Christian "naddy" Weisgerber na...@mips.inka.de
Format string check for dprintf(3)
Add format string checking annotations for dprintf(3) and vdprintf(3). This was apparently forgotten when the functions were added. It is required so the compiler can warn t.c:25:25: warning: format string is not a string literal (potentially insecure) [-Wformat-security] dprintf(STDOUT_FILENO, msg); ^~~ Absent -Werror, I do not expect any fallout from this, but I ran a successful amd64 make build with it anyway. ok? Index: include/stdio.h === RCS file: /cvs/src/include/stdio.h,v retrieving revision 1.53 diff -u -p -r1.53 stdio.h --- include/stdio.h 9 Sep 2016 18:12:37 - 1.53 +++ include/stdio.h 10 Sep 2020 15:07:08 - @@ -204,7 +204,9 @@ __END_DECLS __BEGIN_DECLS voidclearerr(FILE *); #if __POSIX_VISIBLE >= 200809 -int dprintf(int, const char * __restrict, ...); +int dprintf(int, const char * __restrict, ...) + __attribute__((__format__ (printf, 2, 3))) + __attribute__((__nonnull__ (2))); #endif int fclose(FILE *); int feof(FILE *); @@ -266,7 +268,9 @@ int vfprintf(FILE *, const char *, __va int vprintf(const char *, __va_list); int vsprintf(char *, const char *, __va_list); #if __POSIX_VISIBLE >= 200809 -int vdprintf(int, const char * __restrict, __va_list); +int vdprintf(int, const char * __restrict, __va_list) + __attribute__((__format__ (printf, 2, 0))) + __attribute__((__nonnull__ (2))); #endif #if __ISO_C_VISIBLE >= 1999 || __XPG_VISIBLE >= 500 || __BSD_VISIBLE -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: timekeep: fixing large skews on amd64 with RDTSCP
Scott Cheloha: > This "it might slow down the network stack" thing keeps coming up, and > yet nobody can point to (a) who expressed this concern or (b) what the > penalty is in practice. It was kettenis@ who simply raised the question and asked for comments from the network people. I think we should just go ahead and use rdtsc_lfence() in tsc_get_timecount(). It is *correct*. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: man find -exec -- a little bit more hand-holding
On 2020-08-14, Jason McIntyre wrote:
> - i cannot work out what is with the \! examples. i know we try to make
> entries work for both csh and sh style shells, but stuff like this
> works without escaping:
>
> $ find . ! -type f
Going through the CVS and SCCS history, I see that the examples
came with a rewrite of find(1) at Berkeley 30 years ago.
csh's behavior that "for convenience, a `!' is passed unchanged
when it is followed by a blank, tab, newline, `=' or `('" has been
documented as such at least since the start of the CSRG repository
in 1985.
bash, whose history substitution was modeled on csh, also shares
this behavior.
I think it was never necessary to escape the '!' and the man page
examples were written with an abundance of caution and a lack of
understanding of csh's exact replacement mechanism.
--
Christian "naddy" Weisgerber na...@mips.inka.de
mountd: Avoid reading one byte before buffer
>From CheriBSD, via FreeBSD:
| Avoid reading one byte before the path buffer.
|
| This happens when there's only one component (e.g. "/foo"). This
| (mostly-harmless) bug has been present since June 1990 when it was
| commited to mountd.c SCCS version 5.9.
|
| Note: the bug is on the second changed line, the first line is changed
| for visual consistency.
https://svnweb.freebsd.org/base?view=revision=363435
You need to look at the surrounding loop to see the problem.
OK?
Index: sbin/mountd/mountd.c
===
RCS file: /cvs/src/sbin/mountd/mountd.c,v
retrieving revision 1.88
diff -u -p -r1.88 mountd.c
--- sbin/mountd/mountd.c24 Jan 2020 18:51:45 - 1.88
+++ sbin/mountd/mountd.c6 Aug 2020 14:41:16 -
@@ -2021,9 +2021,9 @@ do_mount(struct exportlist *ep, struct g
#endif
}
/* back up over the last component */
- while (*cp == '/' && cp > dirp)
+ while (cp > dirp && *cp == '/')
cp--;
- while (*(cp - 1) != '/' && cp > dirp)
+ while (cp > dirp && *(cp - 1) != '/')
cp--;
if (cp == dirp) {
if (debug)
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: timekeep: fixing large skews on amd64 with RDTSCP
Scott Cheloha:
> --- lib/libc/arch/amd64/gen/usertc.c 8 Jul 2020 09:17:48 - 1.2
> +++ lib/libc/arch/amd64/gen/usertc.c 25 Jul 2020 17:50:38 -
> @@ -21,9 +21,12 @@
> static inline u_int
> rdtsc(void)
> {
> - uint32_t hi, lo;
> - asm volatile("rdtsc" : "=a"(lo), "=d"(hi));
> - return ((uint64_t)lo)|(((uint64_t)hi)<<32);
> + uint32_t lo;
> +
> + asm volatile("lfence");
> + asm volatile("rdtsc" : "=a"(lo) : : "rdx");
Is there a guarantee that two separate asm()s will not be reordered?
> +
> + return lo;
> }
>
> static int
> --- sys/arch/amd64/amd64/tsc.c6 Jul 2020 13:33:06 - 1.19
> +++ sys/arch/amd64/amd64/tsc.c25 Jul 2020 17:50:38 -
> @@ -211,7 +211,12 @@ cpu_recalibrate_tsc(struct timecounter *
> u_int
> tsc_get_timecount(struct timecounter *tc)
> {
> - return rdtsc() + curcpu()->ci_tsc_skew;
> + uint32_t lo;
> +
> + asm volatile("lfence");
> + asm volatile("rdtsc" : "=a"(lo) : : "rdx");
> +
> + return lo + curcpu()->ci_tsc_skew;
> }
>
> void
>
I'd just do s/rdtsc/rdtsc_lfence/, which would agree well with the
rest of the file.
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: timekeep: fixing large skews on amd64 with RDTSCP
Scott Cheloha:
> Can we add the missing LFENCE instructions to userspace and the
> kernel? And can we excise the upper 32 bits?
> + uint32_t lo;
> +
> + asm volatile("lfence");
> + asm volatile("rdtsc" : "=a"(lo));
That's wrong. rtdsc will clobber %rdx, whether you use that value
or not. You need a corresponding constraint:
asm volatile("rdtsc" : "=a"(lo) : : "rdx");
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: armv7: tweak timercounter mask
Mark Kettenis: > > There is the strong suspicion that the 0x7fff mask in the various > > armv7 timecounters was simply copied from powerpc, and that these really > > are full 32-bit counters. > > > > I wanted to verify this from the data sheets, but I'm insufficiently > > familiar with the ARM ecosystem to locate those. > > The counter is described in the ARM Architecture Reference Manual. It > was introduced later so you need to look at revision C or later. Found it. That's agtimer. amptimer is the global timer in the ARM Cortex-A9 MPCore Technical Reference Manual. It's a 64-bit counter. For gptimer, I've found the OMAP4460 Technical Reference Manual. It's a 32-bit counter. So they should be all fine with 0x. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: powerpc(64): tweak timecounter mask
Mark Kettenis: > > Date: Sun, 12 Jul 2020 18:12:39 +0200 > > From: Christian Weisgerber > > > > The PowerPC/Power ISA Time Base is a 64-bit register. We can use > > the full lower 32 bits. > > > > OK? > > Sure, but this needs to be coordinated with the userland diff. No. tc_update_timekeep() copies the counter mask into the timekeep structure and the userland picks it up from there. -- Christian "naddy" Weisgerber na...@mips.inka.de
armv7: tweak timercounter mask
There is the strong suspicion that the 0x7fff mask in the various
armv7 timecounters was simply copied from powerpc, and that these really
are full 32-bit counters.
I wanted to verify this from the data sheets, but I'm insufficiently
familiar with the ARM ecosystem to locate those.
Back in September 2017, Artturi Alm proposed the very same change
here but failed to make himself heard.
Index: arch/arm/cortex/agtimer.c
===
RCS file: /cvs/src/sys/arch/arm/cortex/agtimer.c,v
retrieving revision 1.9
diff -u -p -r1.9 agtimer.c
--- arch/arm/cortex/agtimer.c 11 Aug 2018 10:42:42 - 1.9
+++ arch/arm/cortex/agtimer.c 12 Jul 2020 16:13:22 -
@@ -46,7 +46,7 @@ int32_t agtimer_frequency = TIMER_FREQUE
u_int agtimer_get_timecount(struct timecounter *);
static struct timecounter agtimer_timecounter = {
- agtimer_get_timecount, NULL, 0x7fff, 0, "agtimer", 0, NULL
+ agtimer_get_timecount, NULL, 0x, 0, "agtimer", 0, NULL
};
struct agtimer_pcpu_softc {
Index: arch/arm/cortex/amptimer.c
===
RCS file: /cvs/src/sys/arch/arm/cortex/amptimer.c,v
retrieving revision 1.7
diff -u -p -r1.7 amptimer.c
--- arch/arm/cortex/amptimer.c 6 Jul 2020 13:33:06 - 1.7
+++ arch/arm/cortex/amptimer.c 12 Jul 2020 16:13:37 -
@@ -67,7 +67,7 @@ int32_t amptimer_frequency = TIMER_FREQU
u_int amptimer_get_timecount(struct timecounter *);
static struct timecounter amptimer_timecounter = {
- amptimer_get_timecount, NULL, 0x7fff, 0, "amptimer", 0, NULL, 0
+ amptimer_get_timecount, NULL, 0x, 0, "amptimer", 0, NULL, 0
};
#define MAX_ARM_CPUS 8
Index: arch/armv7/omap/gptimer.c
===
RCS file: /cvs/src/sys/arch/armv7/omap/gptimer.c,v
retrieving revision 1.8
diff -u -p -r1.8 gptimer.c
--- arch/armv7/omap/gptimer.c 6 Jul 2020 13:33:07 - 1.8
+++ arch/armv7/omap/gptimer.c 12 Jul 2020 15:53:06 -
@@ -117,7 +117,7 @@ int gptimer_irq = 0;
u_int gptimer_get_timecount(struct timecounter *);
static struct timecounter gptimer_timecounter = {
- gptimer_get_timecount, NULL, 0x7fff, 0, "gptimer", 0, NULL, 0
+ gptimer_get_timecount, NULL, 0x, 0, "gptimer", 0, NULL, 0
};
volatile u_int32_t nexttickevent;
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: powerpc(64): tweak timecounter mask
Christian Weisgerber: > - tb_get_timecount, NULL, 0x7fff, 0, "tb", 0, NULL, 0 > + tb_get_timecount, NULL, 0x, 0, "tb", 0, NULL, 0 PS: Do we prefer ~0u over 0x? -- Christian "naddy" Weisgerber na...@mips.inka.de
powerpc(64): tweak timecounter mask
The PowerPC/Power ISA Time Base is a 64-bit register. We can use
the full lower 32 bits.
OK?
Index: arch/macppc/macppc/clock.c
===
RCS file: /cvs/src/sys/arch/macppc/macppc/clock.c,v
retrieving revision 1.44
diff -u -p -r1.44 clock.c
--- arch/macppc/macppc/clock.c 6 Jul 2020 13:33:08 - 1.44
+++ arch/macppc/macppc/clock.c 12 Jul 2020 15:17:48 -
@@ -57,7 +57,7 @@ u_int32_t ns_per_tick = 320;
static int32_t ticks_per_intr;
static struct timecounter tb_timecounter = {
- tb_get_timecount, NULL, 0x7fff, 0, "tb", 0, NULL, 0
+ tb_get_timecount, NULL, 0x, 0, "tb", 0, NULL, 0
};
/* calibrate the timecounter frequency for the listed models */
Index: arch/powerpc64/powerpc64/clock.c
===
RCS file: /cvs/src/sys/arch/powerpc64/powerpc64/clock.c,v
retrieving revision 1.1
diff -u -p -r1.1 clock.c
--- arch/powerpc64/powerpc64/clock.c10 Jun 2020 19:06:53 - 1.1
+++ arch/powerpc64/powerpc64/clock.c12 Jul 2020 15:18:02 -
@@ -37,7 +37,7 @@ struct evcount stat_count;
u_int tb_get_timecount(struct timecounter *);
static struct timecounter tb_timecounter = {
- tb_get_timecount, NULL, 0x7fff, 0, "tb", 0, NULL
+ tb_get_timecount, NULL, 0x, 0, "tb", 0, NULL
};
void cpu_startclock(void);
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: arm64 usertc
Mark Kettenis:
> Nevertheless, here is a different take on the problem. Since the
> timecounter only uses the low 32 bits we don't need the double read.
> This version also changes the timecounter mask from 0x7fff to
> 0x. That must be ok, since the counter has 64 bits and we are
> already using 0x as a mask on amd64 and sparc64.
>
> ok?
Yes, but don't forget the part in sys/arch/arm64/include/timetc.h.
Also, if I may ask, ...
> Index: sys/arch/arm64/dev/agtimer.c
> ===
> RCS file: /cvs/src/sys/arch/arm64/dev/agtimer.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 agtimer.c
> --- sys/arch/arm64/dev/agtimer.c 11 Jul 2020 15:22:44 - 1.14
> +++ sys/arch/arm64/dev/agtimer.c 11 Jul 2020 18:35:12 -
> @@ -43,7 +43,8 @@ int32_t agtimer_frequency = TIMER_FREQUE
> u_int agtimer_get_timecount(struct timecounter *);
>
> static struct timecounter agtimer_timecounter = {
> - agtimer_get_timecount, NULL, 0x7fff, 0, "agtimer", 0, NULL, 0
> + agtimer_get_timecount, NULL, 0x, 0, "agtimer", 0, NULL,
> + TC_AGTIMER
> };
>
> struct agtimer_pcpu_softc {
> @@ -191,7 +192,15 @@ agtimer_attach(struct device *parent, st
> u_int
> agtimer_get_timecount(struct timecounter *tc)
> {
> - return agtimer_readcnt64();
> + uint64_t val;
> +
> + /*
> + * No need to work around Cortex-A73 errata 858921 since we
> + * only look at the low 32 bits here.
> + */
> + __asm volatile("isb" ::: "memory");
> + __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val));
> + return (val & 0x);
Is there any point, stylistically I mean, to explicitly masking
this over the truncation implicit in the types? I'm pretty sure
we do, say, "intvar = int64var" all the time.
> }
>
> int
> Index: lib/libc/arch/aarch64/gen/usertc.c
> ===
> RCS file: /cvs/src/lib/libc/arch/aarch64/gen/usertc.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 usertc.c
> --- lib/libc/arch/aarch64/gen/usertc.c6 Jul 2020 13:33:05 -
> 1.1
> +++ lib/libc/arch/aarch64/gen/usertc.c11 Jul 2020 18:35:12 -
> @@ -1,6 +1,6 @@
> -/* $OpenBSD: usertc.c,v 1.1 2020/07/06 13:33:05 pirofti Exp $ */
> +/* $OpenBSD$ */
> /*
> - * Copyright (c) 2020 Paul Irofti
> + * Copyright (c) 2020 Mark Kettenis
> *
> * Permission to use, copy, modify, and distribute this software for any
> * purpose with or without fee is hereby granted, provided that the above
> @@ -18,4 +18,30 @@
> #include
> #include
>
> -int (*const _tc_get_timecount)(struct timekeep *, u_int *) = NULL;
> +static inline u_int
> +agtimer_get_timecount(struct timecounter *tc)
> +{
> + uint64_t val;
> +
> + /*
> + * No need to work around Cortex-A73 errata 858921 since we
> + * only look at the low 32 bits here.
> + */
> + __asm volatile("isb" ::: "memory");
> + __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val));
> + return (val & 0x);
> +}
> +
> +static int
> +tc_get_timecount(struct timekeep *tk, u_int *tc)
> +{
> + switch (tk->tk_user) {
> + case TC_AGTIMER:
> + *tc = agtimer_get_timecount(NULL);
> + return 0;
> + }
> +
> + return -1;
> +}
> +
> +int (*const _tc_get_timecount)(struct timekeep *, u_int *) =
> tc_get_timecount;
>
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
On 2020-06-26, George Koehler wrote: > Here's macppc again. My macppc isn't using your newest diff but does > now need to define TC_TB in . Crucial question: How confident are we that TB is in sync on multiprocessor machines? -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: user tc for alpha
Paul Irofti:
> > Userland gettime support for alpha.
> > Alas, completely untested since I don't have access to that arch.
>
> Never had an alpha. Reads OK to me (if you make the function static like
> kettenis@ said).
For the archives, here's a version that looks like kettenis@'s
style.
Unfortunately, as Theo has discovered, on multi-socket machines the
PCCs are not in sync. Fixing this will require kernel work...
so consider this diff withdrawn.
Index: lib/libc/arch/alpha/gen/usertc.c
===
RCS file: /cvs/src/lib/libc/arch/alpha/gen/usertc.c,v
retrieving revision 1.1
diff -u -p -r1.1 usertc.c
--- lib/libc/arch/alpha/gen/usertc.c6 Jul 2020 13:33:05 - 1.1
+++ lib/libc/arch/alpha/gen/usertc.c8 Jul 2020 11:39:37 -
@@ -18,4 +18,25 @@
#include
#include
-int (*const _tc_get_timecount)(struct timekeep *, u_int *) = NULL;
+static inline u_int
+rpcc_get_timecount(void)
+{
+ unsigned long val;
+
+ __asm volatile("rpcc %0" : "=r" (val));
+ return val;
+}
+
+static int
+tc_get_timecount(struct timekeep *tk, u_int *tc)
+{
+ switch (tk->tk_user) {
+ case TC_RPCC:
+ *tc = rpcc_get_timecount();
+ return 0;
+ }
+
+ return -1;
+}
+
+int (*const _tc_get_timecount)(struct timekeep *, u_int *) = tc_get_timecount;
Index: sys/arch/alpha/alpha/clock.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/clock.c,v
retrieving revision 1.24
diff -u -p -r1.24 clock.c
--- sys/arch/alpha/alpha/clock.c6 Jul 2020 13:33:06 - 1.24
+++ sys/arch/alpha/alpha/clock.c7 Jul 2020 20:29:47 -
@@ -64,7 +64,7 @@ int clk_irq = 0;
u_int rpcc_get_timecount(struct timecounter *);
struct timecounter rpcc_timecounter = {
- rpcc_get_timecount, NULL, ~0u, 0, "rpcc", 0, NULL, 0
+ rpcc_get_timecount, NULL, ~0u, 0, "rpcc", 0, NULL, TC_RPCC
};
extern todr_chip_handle_t todr_handle;
Index: sys/arch/alpha/include/timetc.h
===
RCS file: /cvs/src/sys/arch/alpha/include/timetc.h,v
retrieving revision 1.1
diff -u -p -r1.1 timetc.h
--- sys/arch/alpha/include/timetc.h 6 Jul 2020 13:33:06 - 1.1
+++ sys/arch/alpha/include/timetc.h 8 Jul 2020 11:30:17 -
@@ -18,6 +18,6 @@
#ifndef _MACHINE_TIMETC_H_
#define _MACHINE_TIMETC_H_
-#defineTC_LAST 0
+#define TC_RPCC1
#endif /* _MACHINE_TIMETC_H_ */
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: user tc for alpha
Mark Kettenis: > > --- lib/libc/arch/alpha/gen/usertc.c6 Jul 2020 13:33:05 - > > 1.1 > > +++ lib/libc/arch/alpha/gen/usertc.c7 Jul 2020 20:40:37 - > > +int > > +tc_get_timecount(struct timekeep *tk, u_int *tc) > > Need to make this function static to avoid namespace pollution. Then this needs to be fixed in amd64/gen/usertc.c pronto, before everybody copies it from there. -- Christian "naddy" Weisgerber na...@mips.inka.de
user tc for alpha
Userland gettime support for alpha.
Alas, completely untested since I don't have access to that arch.
Index: lib/libc/arch/alpha/gen/usertc.c
===
RCS file: /cvs/src/lib/libc/arch/alpha/gen/usertc.c,v
retrieving revision 1.1
diff -u -p -r1.1 usertc.c
--- lib/libc/arch/alpha/gen/usertc.c6 Jul 2020 13:33:05 - 1.1
+++ lib/libc/arch/alpha/gen/usertc.c7 Jul 2020 20:40:37 -
@@ -18,4 +18,18 @@
#include
#include
-int (*const _tc_get_timecount)(struct timekeep *, u_int *) = NULL;
+int
+tc_get_timecount(struct timekeep *tk, u_int *tc)
+{
+ unsigned long val;
+
+ if (tk->tk_user != TC_RPCC)
+ return -1;
+
+ __asm volatile("rpcc %0" : "=r" (val));
+ *tc = val;
+ return 0;
+}
+
+int (*const _tc_get_timecount)(struct timekeep *, u_int *)
+ = tc_get_timecount;
Index: sys/arch/alpha/alpha/clock.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/clock.c,v
retrieving revision 1.24
diff -u -p -r1.24 clock.c
--- sys/arch/alpha/alpha/clock.c6 Jul 2020 13:33:06 - 1.24
+++ sys/arch/alpha/alpha/clock.c7 Jul 2020 20:29:47 -
@@ -64,7 +64,7 @@ int clk_irq = 0;
u_int rpcc_get_timecount(struct timecounter *);
struct timecounter rpcc_timecounter = {
- rpcc_get_timecount, NULL, ~0u, 0, "rpcc", 0, NULL, 0
+ rpcc_get_timecount, NULL, ~0u, 0, "rpcc", 0, NULL, TC_RPCC
};
extern todr_chip_handle_t todr_handle;
Index: sys/arch/alpha/include/timetc.h
===
RCS file: /cvs/src/sys/arch/alpha/include/timetc.h,v
retrieving revision 1.1
diff -u -p -r1.1 timetc.h
--- sys/arch/alpha/include/timetc.h 6 Jul 2020 13:33:06 - 1.1
+++ sys/arch/alpha/include/timetc.h 7 Jul 2020 20:42:53 -
@@ -18,6 +18,7 @@
#ifndef _MACHINE_TIMETC_H_
#define _MACHINE_TIMETC_H_
-#defineTC_LAST 0
+#defineTC_RPCC 1
+#defineTC_LAST 2
#endif /* _MACHINE_TIMETC_H_ */
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
On 2020-07-03, Scott Cheloha wrote: > Are we doing powerpc, arm64, and sparc64 separately? hppa can de done as well, but somebody with access to a machine needs to investigate/ensure that the S bit in the processor status register is appropriately set to allow userland access to the Interval Timer register. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
On 2020-07-03, Mark Kettenis wrote: > Are the issue that naddy@ saw solved? Yes, they were understood and fixed. I'll defer to you and Scott regarding the TSC synchronization issues; aside from that I'm okaying the diff. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: powerpc64: 64-bit-ize memmove.S
Christian Weisgerber: > Well, I suggested it, so here's my attempt to switch powerpc64's > libc memmove.S over to 64 bits: Actually, on second thought: That function simply copies as many (double)words plus a tail of bytes as the length argument specifies. Neither source nor destination are checked for alignment, so this will happily run a loop of unaligned accesses, which doesn't sound very optimal. I'm also intrigued by this aside in the PowerPC ISA documentation: | Moreover, Load with Update instructions may take longer to execute | in some implementations than the corresponding pair of a non-update | Load instruction and an Add instruction. What does clang generate? I think we should consider dropping this "optimized" memmove.S on both powerpc and powerpc64. -- Christian "naddy" Weisgerber na...@mips.inka.de
powerpc64: 64-bit-ize memmove.S
Well, I suggested it, so here's my attempt to switch powerpc64's libc memmove.S over to 64 bits: * Treat length parameter as 64 bits (size_t) instead of 32 (u_int). * Set up the main loop to copy 64-bit doublewords instead of 32-bit words. This definitely needs double and triple checking. Things I wonder about, but didn't touch: * Why is the memcpy entry point commented out? * END... STRONG? WEAK? BUILTIN? Index: memmove.S === RCS file: /cvs/src/lib/libc/arch/powerpc64/string/memmove.S,v retrieving revision 1.1 diff -u -p -r1.1 memmove.S --- memmove.S 25 Jun 2020 02:34:22 - 1.1 +++ memmove.S 26 Jun 2020 22:22:51 - @@ -39,7 +39,7 @@ * == */ -#include "SYS.h" +#include "DEFS.h" .text @@ -64,45 +64,45 @@ ENTRY(memmove) /* start of dest*/ fwd: - addi%r4, %r4, -4/* Back up src and dst pointers */ - addi%r8, %r8, -4/* due to auto-update of 'load' */ + addi%r4, %r4, -8/* Back up src and dst pointers */ + addi%r8, %r8, -8/* due to auto-update of 'load' */ - srwi. %r9,%r5,2 /* How many words in total cnt */ - beq-last1 /* Handle byte by byte if < 4 */ + srdi. %r9,%r5,3 /* Doublewords in total count */ + beq-last1 /* Handle byte by byte if < 8 */ /* bytes total */ - mtctr %r9 /* Count of words for loop */ - lwzu%r7, 4(%r4) /* Preload first word */ + mtctr %r9 /* Count of dwords for loop */ + ldu %r7, 8(%r4) /* Preload first doubleword */ b g1 g0:/* Main loop*/ - lwzu%r7, 4(%r4) /* Load a new word */ - stwu%r6, 4(%r8) /* Store previous word */ + ldu %r7, 8(%r4) /* Load a new doubleword*/ + stdu%r6, 8(%r8) /* Store previous doubleword*/ g1: bdz-last/* Dec cnt, and branch if just */ - /* one word to store*/ - lwzu%r6, 4(%r4) /* Load another word*/ - stwu%r7, 4(%r8) /* Store previous word */ + /* one doubleword to store */ + ldu %r6, 8(%r4) /* Load another doubleword */ + stdu%r7, 8(%r8) /* Store previous doubleword*/ bdnz+ g0 /* Dec cnt, and loop again if */ - /* more words */ - mr %r7, %r6/* If word count -> 0, then... */ + /* more doublewords */ + mr %r7, %r6/* If dword count -> 0, then... */ last: - stwu%r7, 4(%r8) /* ... store last word */ + stdu%r7, 8(%r8) /* ... store last doubleword*/ last1: /* Byte-by-byte copy*/ - clrlwi. %r5,%r5,30 /* If count -> 0, then ... */ + clrldi. %r5,%r5,61 /* If count -> 0, then ... */ beqlr /* we're done */ mtctr %r5 /* else load count for loop */ - lbzu%r6, 4(%r4) /* 1st byte: update addr by 4 */ - stbu%r6, 4(%r8) /* since we pre-adjusted by 4 */ + lbzu%r6, 8(%r4) /* 1st byte: update addr by 8 */ + stbu%r6, 8(%r8) /* since we pre-adjusted by 8 */ bdzlr- /* in anticipation of main loop */ last2: @@ -120,40 +120,40 @@ reverse: add %r4, %r4, %r5 /* Work from end to beginning */ add %r8, %r8, %r5 /* so add count to string ptrs */ - srwi. %r9,%r5,2 /* Words in total count */ - beq-rlast1 /* Handle byte by byte if < 4 */ + srdi. %r9,%r5,3 /* Doublewords in total count */ + beq-rlast1 /* Handle byte by byte if < 8 */ /* bytes total */ - mtctr %r9 /* Count of words for loop */ + mtctr %r9 /* Count of dwords for loop */ - lwzu%r7, -4(%r4)/* Preload first word */ +
Re: ffs(3): libc arch versions, regress
Trying again, this time with powerpc64 added: This adds the optimized ffs(3) versions on aarch64, powerpc, and powerpc64 to libc. Also add a brief regression test. Index: lib/libc/arch/aarch64/string/Makefile.inc === RCS file: /cvs/src/lib/libc/arch/aarch64/string/Makefile.inc,v retrieving revision 1.1 diff -u -p -r1.1 Makefile.inc --- lib/libc/arch/aarch64/string/Makefile.inc 11 Jan 2017 18:09:24 - 1.1 +++ lib/libc/arch/aarch64/string/Makefile.inc 11 Jun 2020 20:30:34 - @@ -2,7 +2,7 @@ SRCS+= bcopy.c memcpy.c memmove.c \ strchr.c strrchr.c \ - bcmp.c bzero.c ffs.c memchr.c memcmp.c memset.c \ + bcmp.c bzero.c ffs.S memchr.c memcmp.c memset.c \ strcmp.c strncmp.c \ strcat.c strcpy.c strcspn.c strlen.c strlcat.c strlcpy.c \ strncat.c strncpy.c strpbrk.c strsep.c \ Index: lib/libc/arch/aarch64/string/ffs.S === RCS file: lib/libc/arch/aarch64/string/ffs.S diff -N lib/libc/arch/aarch64/string/ffs.S --- /dev/null 1 Jan 1970 00:00:00 - +++ lib/libc/arch/aarch64/string/ffs.S 11 Jun 2020 20:31:19 - @@ -0,0 +1,18 @@ +/* $OpenBSD$ */ +/* + * Written by Christian Weisgerber . + * Public domain. + */ + +#include "DEFS.h" + +ENTRY(ffs) + RETGUARD_SETUP(ffs, x15) + rbitw1, w0 + clz w1, w1 + cmp w0, wzr + csinc w0, wzr, w1, eq + RETGUARD_CHECK(ffs, x15) + ret +END(ffs) +.protected Index: lib/libc/arch/powerpc/string/Makefile.inc === RCS file: /cvs/src/lib/libc/arch/powerpc/string/Makefile.inc,v retrieving revision 1.6 diff -u -p -r1.6 Makefile.inc --- lib/libc/arch/powerpc/string/Makefile.inc 15 May 2015 22:29:37 - 1.6 +++ lib/libc/arch/powerpc/string/Makefile.inc 11 Jun 2020 20:33:04 - @@ -2,7 +2,7 @@ SRCS+= memcpy.c memmove.S \ strchr.c strrchr.c \ - bcmp.c bzero.c ffs.c memchr.c memcmp.c memset.c strcat.c \ + bcmp.c bzero.c ffs.S memchr.c memcmp.c memset.c strcat.c \ strcmp.c strcpy.c strcspn.c strlen.c strlcat.c strlcpy.c \ strncat.c strncmp.c strncpy.c strpbrk.c strsep.c \ strspn.c strstr.c swab.c Index: lib/libc/arch/powerpc/string/ffs.S === RCS file: lib/libc/arch/powerpc/string/ffs.S diff -N lib/libc/arch/powerpc/string/ffs.S --- /dev/null 1 Jan 1970 00:00:00 - +++ lib/libc/arch/powerpc/string/ffs.S 11 Jun 2020 20:33:19 - @@ -0,0 +1,16 @@ +/* $OpenBSD$ */ +/* + * Written by Christian Weisgerber . + * Public domain. + */ + +#include "SYS.h" + +ENTRY(ffs) + neg %r4, %r3 + and %r3, %r3, %r4 + cntlzw %r3, %r3 + subfic %r3, %r3, 32 + blr +END(ffs) +.protected Index: lib/libc/arch/powerpc64/string/Makefile.inc === RCS file: /cvs/src/lib/libc/arch/powerpc64/string/Makefile.inc,v retrieving revision 1.1 diff -u -p -r1.1 Makefile.inc --- lib/libc/arch/powerpc64/string/Makefile.inc 25 Jun 2020 02:34:22 - 1.1 +++ lib/libc/arch/powerpc64/string/Makefile.inc 25 Jun 2020 20:53:42 - @@ -2,7 +2,7 @@ SRCS+= memcpy.c memmove.S \ strchr.c strrchr.c \ - bcmp.c bzero.c ffs.c memchr.c memcmp.c memset.c strcat.c \ + bcmp.c bzero.c ffs.S memchr.c memcmp.c memset.c strcat.c \ strcmp.c strcpy.c strcspn.c strlen.c strlcat.c strlcpy.c \ strncat.c strncmp.c strncpy.c strpbrk.c strsep.c \ strspn.c strstr.c swab.c Index: lib/libc/arch/powerpc64/string/ffs.S === RCS file: lib/libc/arch/powerpc64/string/ffs.S diff -N lib/libc/arch/powerpc64/string/ffs.S --- /dev/null 1 Jan 1970 00:00:00 - +++ lib/libc/arch/powerpc64/string/ffs.S25 Jun 2020 20:57:16 - @@ -0,0 +1,15 @@ +/* $OpenBSD$ */ +/* + * Written by Christian Weisgerber . + * Public domain. + */ + +#include "DEFS.h" + +ENTRY(ffs) + neg %r4, %r3 + and %r3, %r3, %r4 + cntlzw %r3, %r3 + subfic %r3, %r3, 32 + blr +END_BUILTIN(ffs) Index: regress/lib/libc/ffs/Makefile === RCS file: regress/lib/libc/ffs/Makefile diff -N regress/lib/libc/ffs/Makefile --- /dev/null 1 Jan 1970 00:00:00 - +++ regress/lib/libc/ffs/Makefile 20 Jun 2020 15:26:51 - @@ -0,0 +1,6 @@ +PROG= ffs_test + +# prevent constant folding and inlining of __builtin_ffs() +CFLAGS+= -ffreestanding + +.include Index: regress/lib/libc/ffs/ffs_test.c === RCS file: regress/lib/libc/ffs/ffs_test.c diff -N regress/lib/libc/ffs/ffs_test.c --- /dev/null 1 Jan 1970 00:00:00 - +++ regress/lib/libc/
RIP, freedb (cddb service)
The freedb.org CD track database is dead. Its shutdown had already
been announced for March, and it finally disappeared.
gnudb.org, whoever they are, offers the last working alternative
that still supports the CDDB protocol. Actually, the port was dead
yesterday, but they fixed it today. I suggest we switch the default
of cdio(1)'s cddb command to gnudb.
I think we can also retire cddb/888 from /etc/services. Literally
nothing uses this any longer. gnudb uses the "cddbp-alt" port of
8880, but I don't think we need a services(5) entry for a single
site. If anything in ports uses getservbyname("cddb", ...), it's
already broken anyway.
OK?
PS:
Clearly the NSA does not consider the unencrypted transmission of
compact disc identifiers a significant source of intelligence, or
they would sponsor a better server...
Index: etc/services
===
RCS file: /cvs/src/etc/services,v
retrieving revision 1.96
diff -u -p -r1.96 services
--- etc/services27 Jan 2019 20:35:06 - 1.96
+++ etc/services24 Jun 2020 22:27:44 -
@@ -182,7 +182,6 @@ kerberos-adm749/udp #
Kerberos 5 kad
domain-s 853/tcp # DNS query-response protocol
run over TLS/DTLS
domain-s 853/udp # DNS query-response protocol
run over TLS/DTLS
rsync 873/tcp # rsync server
-cddb 888/tcp cddbp # Audio CD Database
imaps 993/tcp # imap4 protocol over TLS/SSL
imaps 993/udp # imap4 protocol over TLS/SSL
pop3s 995/tcp spop3 # pop3 protocol over TLS/SSL
Index: usr.bin/cdio/cddb.c
===
RCS file: /cvs/src/usr.bin/cdio/cddb.c,v
retrieving revision 1.22
diff -u -p -r1.22 cddb.c
--- usr.bin/cdio/cddb.c 7 Dec 2017 02:08:44 - 1.22
+++ usr.bin/cdio/cddb.c 24 Jun 2020 22:25:58 -
@@ -254,7 +254,7 @@ cddb(const char *host_port, int n, struc
int i;
const char *errstr;
- s = parse_connect_to(host_port, "cddb");
+ s = parse_connect_to(host_port, "8880");
if (s == -1)
goto end;
s2 = dup(s);
Index: usr.bin/cdio/cdio.1
===
RCS file: /cvs/src/usr.bin/cdio/cdio.1,v
retrieving revision 1.65
diff -u -p -r1.65 cdio.1
--- usr.bin/cdio/cdio.1 22 Apr 2020 05:37:00 - 1.65
+++ usr.bin/cdio/cdio.1 24 Jun 2020 22:22:34 -
@@ -58,7 +58,7 @@ The options are as follows:
.Ar host : Ns Ar port
.Xc
Specifies a CDDB host
-.Bq default: freedb.freedb.org:cddb .
+.Bq default: gnudb.gnudb.org:8880 .
.It Fl f Ar device
Specifies the name of the CD device, such as
.Pa /dev/rcd0c .
Index: usr.bin/cdio/cdio.c
===
RCS file: /cvs/src/usr.bin/cdio/cdio.c,v
retrieving revision 1.78
diff -u -p -r1.78 cdio.c
--- usr.bin/cdio/cdio.c 3 Jul 2019 03:24:02 - 1.78
+++ usr.bin/cdio/cdio.c 24 Jun 2020 22:25:19 -
@@ -239,7 +239,7 @@ main(int argc, char **argv)
cddb_host = getenv("CDDB");
if (!cddb_host)
- cddb_host = "freedb.freedb.org";
+ cddb_host = "gnudb.gnudb.org";
while ((ch = getopt(argc, argv, "svd:f:")) != -1)
switch (ch) {
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Christian Weisgerber: > If I move > > vaddr_t ps_timekeep;/* User pointer to timekeep */ > > up into the zeroed area, I get a properly randomized _timekeep in > userland. Also note that exec_sigcode_map() has this pr->ps_sigcode = 0; /* no hint */ uao_reference(e->e_sigobject); if (uvm_map(>ps_vmspace->vm_map, >ps_sigcode, round_page(sz), I don't know if we want to * explicitly set ps_timekeep to 0 in exec_timekeep_map(), or * move it into the zeroed area, which we should also do with ps_sigcode then. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Paul Irofti:
> 683 /* map the process's timekeep page */
> 684 if (exec_timekeep_map(pr))
> 685 goto free_pack_abort;
> 686 /* setup new registers and do misc. setup. */
> 687 if (pack.ep_emul->e_fixup != NULL) {
> 688 if ((*pack.ep_emul->e_fixup)(p, ) != 0)
> 689 goto free_pack_abort;
> 690 }
Yes, with this init(8) gets a proper _timekeep instead of 0x0.
For randomization of the userland page...
+ if (uvm_map(>ps_vmspace->vm_map, >ps_timekeep,
round_page(timekeep_sz),
... ps_timekeep need to be 0 here. At the moment, it inherits the
value from the parent process in fork().
In struct process in sys/proc.h, there is this:
/* The following fields are all zeroed upon creation in process_new. */
...
/* End area that is zeroed on creation. */
If I move
vaddr_t ps_timekeep;/* User pointer to timekeep */
up into the zeroed area, I get a properly randomized _timekeep in
userland.
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Christian Weisgerber: > I tweaked the patch locally to make _timekeep a visible global > symbol in libc. > > Printing its value has revealed two issues: > > * The timekeep page is mapped to the same address for every process. > It changes across reboots, but once running, it's always the same. > kettenis suggested > - vaddr_t va; > + vaddr_t va = 0; > in exec_timekeep_map(), but that doesn't make a difference. But that's the kernel mapping, and my observation concerns the userland mapping. So based on this, I moved ps_timekeep up into the fields of struct process that are zeroed on creation. With that, _timekeep is always 0 for all processes. :-/ -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Paul Irofti: [Unrelated, just to mark where we're at] > Right. Just reproduced it here. This moves the check at the top so that > each CPU checks its own skew and disables tc_user if necessary. I tweaked the patch locally to make _timekeep a visible global symbol in libc. Printing its value has revealed two issues: * The timekeep page is mapped to the same address for every process. It changes across reboots, but once running, it's always the same. kettenis suggested - vaddr_t va; + vaddr_t va = 0; in exec_timekeep_map(), but that doesn't make a difference. * I'm indeed seeing init(8) with _timekeep == NULL. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Paul Irofti:
> > b) Revert _timekeep init (breaks naddy@'s machine)
>
> Robert helped properly track down this issue to a silly null-ref.
If that was indeed the problem...
> --- lib/libc/dlfcn/init.c
> +++ lib/libc/dlfcn/init.c
> @@ -105,6 +107,14 @@ _libc_preinit(int argc, char **argv, char **envp,
> dl_cb_cb *cb)
> phnum = aux->au_v;
> break;
> #endif /* !PIC */
> + case AUX_openbsd_timekeep:
> + if (_tc_get_timecount) {
> + _timekeep = (void *)aux->au_v;
> + if (_timekeep &&
> + _timekeep->tk_version != TK_VERSION)
> + _timekeep = NULL;
> + }
> + break;
> }
> }
>
... how could aux->au_v be NULL here?
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: Retire
On 2020-06-20, Christian Weisgerber wrote: >> Well... they something in ports might still look at them in >> >> >> Can someone from ports speak about this? > > I have started an amd64 bulk build without . There were no build failures attributable to this. The header can be removed. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Paul Irofti: > Can't test right now, but if you enable the TSC_DEBUG in cpu.c or if you > put a printf in the CPU_INFO_FOREACH you will probably see the correct > skew values. It's worse: CPU_INFO_FOREACH() only sees cpu0. The others aren't attached yet. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Paul Irofti:
> This also handles negative skew values that my prevoius diff did not.
> --- sys/arch/amd64/amd64/tsc.c
> +++ sys/arch/amd64/amd64/tsc.c
> @@ -216,6 +217,8 @@ tsc_get_timecount(struct timecounter *tc)
> void
> tsc_timecounter_init(struct cpu_info *ci, uint64_t cpufreq)
> {
> + CPU_INFO_ITERATOR cii;
> +
> #ifdef TSC_DEBUG
> printf("%s: TSC skew=%lld observed drift=%lld\n", __func__,
> (long long)ci->ci_tsc_skew, (long long)tsc_drift_observed);
> @@ -244,8 +247,16 @@ tsc_timecounter_init(struct cpu_info *ci, uint64_t
> cpufreq)
> printf("ERROR: %lld cycle TSC drift observed\n",
> (long long)tsc_drift_observed);
> tsc_timecounter.tc_quality = -1000;
> + tsc_timecounter.tc_user = 0;
> tsc_is_invariant = 0;
> }
> + CPU_INFO_FOREACH(cii, ci) {
> + if (ci->ci_tsc_skew < -TSC_SKEW_MAX ||
> + ci->ci_tsc_skew > TSC_SKEW_MAX) {
> + tsc_timecounter.tc_user = 0;
> + break;
> + }
> + }
>
> tc_init(_timecounter);
> }
If the output order from TSC_DEBUG in dmesg reflects the actual
execution order, then the relative call order is this:
cpu0 tsc_timecounter_init
cpu1 cpu_start_secondary
cpu1 tsc_timecounter_init
cpu2 cpu_start_secondary
cpu2 tsc_timecounter_init
cpu3 cpu_start_secondary
cpu3 tsc_timecounter_init
That CPU_INFO_FOREACH() loop would execute in the very first cpu0
tsc_timecounter_init() call, _before_ the skews of the other CPUs
are determined in the subsequent cpu_start_secondary() calls.
So, instead, I think the skew check needs to move to the top of
tsc_timecounter_init, where each secondary CPU checks its own skew
value and knocks out tsc_timecounter.tc_user if there is a problem.
Unless I'm misunderstanding the whole thing.
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
On 2020-06-20, Christian Weisgerber wrote: > I can't get this revision of the diff to work on amd64: > * patch source > * build and install kernel, reboot > * make build > * reboot -> "Process (pid 1) got signal 11" > > I'm at a loss. As part of the "make build", the new libc is installed > and dynamically linked programs should already be using the userland > gettime calls. Clearly this works. So why does init fail on the > next reboot? I can recover by extracting ./sbin/init from a snapshot in the installer. After that, the system comes up fine in multiuser mode. Nothing else appears to be affected, apart from init. For a while, I had a reproducible situation. When you call init(8) as a normal user in multiuser mode, it will just exit with "init: Operation not permitted". Instead it would segfault! I kept tweaking lib/libc/dlfcn/init.c, rebuilding and reinstalling libc.a, rebuilding init, and watching it segfault. None of the debug write(2)s I inserted would produce any output, it seemed to die before ever reaching _libc_preinit(). I finally ktraced it: 12420 ktrace RET ktrace 0 12420 ktrace CALL execve(0x7f7ec412,0x7f7ec298,0x7f7ec2a8) 12420 ktrace NAMI "./obj/init" 12420 ktrace ARGS [0] = "./obj/init" 12420 init RET execve 0 12420 init PSIG SIGSEGV SIG_DFL code SEGV_MAPERR<1> addr=0x0 trapno=6 12420 init NAMI "init.core" There's not even a kbind(2) there. Then I removed the clearly useless debug write()s... and since then I have a hard time reproducing the problem. It doesn't make any sense. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
On 2020-06-19, Paul Irofti wrote: > I have addressed your comments bellow, except for the CPU skew one. That > code disables TSC for all CPUs, not just for PRIMARY. Would you like to > walk and add code for every CPU to check the drift and then disable the > TSC? It seems a little too much... > > [diff] I can't get this revision of the diff to work on amd64: * patch source * build and install kernel, reboot * make build * reboot -> "Process (pid 1) got signal 11" I'm at a loss. As part of the "make build", the new libc is installed and dynamically linked programs should already be using the userland gettime calls. Clearly this works. So why does init fail on the next reboot? -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: Retire
On 2020-06-19, "Theo de Raadt" wrote: > Well... they something in ports might still look at them in > > Can someone from ports speak about this? I have started an amd64 bulk build without . -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
On 2020-06-19, Mark Kettenis wrote:
> I'm talking about *skew*, not drift. If there is a significant drift
> you already knock out the TSC.
>
> What's needed is:
>
> 1. A bit of research of what an acceptable skew is. My hypothesis is
>that on many machines with a single socket the TSCs are actually in
>synch. But the way we measure the skew isn't 100% accurate so we
>still get a small skew. If we sample these values on a couple of
>machines across a couple of reboots we can probably tell what the
>uncertainty in the measurement of the skew is and define a cutoff
>based on that.
So we need amd64 snapshots to enable TSC_DEBUG, maybe a bit prettier
like below, and then reports:
cpu0: Intel(R) Xeon(R) CPU E3-1225 v3 @ 3.20GHz, 3392.69 MHz, 06-3c-03
cpu0: TSC skew=0 observed drift=0
cpu1: TSC skew=-24 observed drift=0
cpu2: TSC skew=-27 observed drift=0
cpu3: TSC skew=-25 observed drift=0
cpu0: TSC skew=0 observed drift=0
cpu1: TSC skew=-1 observed drift=0
cpu2: TSC skew=0 observed drift=0
cpu3: TSC skew=25 observed drift=0
cpu0: TSC skew=0 observed drift=0
cpu1: TSC skew=-30 observed drift=0
cpu2: TSC skew=-39 observed drift=0
cpu3: TSC skew=-41 observed drift=0
cpu0: TSC skew=0 observed drift=0
cpu1: TSC skew=-31 observed drift=0
cpu2: TSC skew=-37 observed drift=0
cpu3: TSC skew=-39 observed drift=0
cpu0: TSC skew=0 observed drift=0
cpu1: TSC skew=-31 observed drift=0
cpu2: TSC skew=-34 observed drift=0
cpu3: TSC skew=-23 observed drift=0
Index: sys/arch/amd64/amd64/tsc.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/tsc.c,v
retrieving revision 1.16
diff -u -p -r1.16 tsc.c
--- sys/arch/amd64/amd64/tsc.c 6 Apr 2020 00:01:08 - 1.16
+++ sys/arch/amd64/amd64/tsc.c 19 Jun 2020 23:49:06 -
@@ -217,7 +217,7 @@ void
tsc_timecounter_init(struct cpu_info *ci, uint64_t cpufreq)
{
#ifdef TSC_DEBUG
- printf("%s: TSC skew=%lld observed drift=%lld\n", __func__,
+ printf("%s: TSC skew=%lld observed drift=%lld\n", ci->ci_dev->dv_xname,
(long long)ci->ci_tsc_skew, (long long)tsc_drift_observed);
#endif
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: improve pkg_add bandwidth usage with some mirrors
On 2020-06-18, Marc Espie wrote: > What pkg_add does internally is a pipeline: > > ftp | signify|internal gunzip > > closing the end file handle should kill the whole chain. > So I need to figure out where it goes wrong, what's the > part that doesn't die "instantly". That's ftp(1). Our SSL people are sitting on a patch to libtls^H^H^Hssl. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
George Koehler:
> --- lib/libc/arch/powerpc/gen/usertc.c.before Sat Jun 13 21:28:50 2020
> +++ lib/libc/arch/powerpc/gen/usertc.cSat Jun 13 21:38:52 2020
> @@ -18,4 +18,19 @@
> #include
> #include
>
> -int (*const _tc_get_timecount)(struct timekeep *, uint64_t *) = NULL;
> +int
> +tc_get_timecount(struct timekeep *tk, uint64_t *tc)
> +{
> + uint64_t tb;
> + uint32_t scratch;
> +
> + if (tk->tk_user != 1)
> + return -1;
> +
> + __asm volatile ("1: mftbu %0; mftb %L0; mftbu %1;"
> + " cmpw 0,%0,%1; bne 1b" : "=r"(tb), "=r"(scratch));
You only need the lower register. Compare the kernel timecounter:
u_int
tb_get_timecount(struct timecounter *tc)
{
return ppc_mftbl();
}
As I mentioned before, the declaration of the timecounter value as
uint64_t is confusing and should be changed.
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: symmetric toeplitz hashing
On 2020-06-13, Theo Buehler wrote:
> Others have pointed out off-list that one can use __builtin_popcount(),
> but __builtin_parity() is exactly what I want. Is it available on all
> architectures?
The standard implementation will be perfectly fine, no need to resort
to magic compiler built-ins.
int
popcount(uint16_t x)
{
x = (x & 0x) + ((x & 0x) >> 1);
x = (x & 0x) + ((x & 0x) >> 2);
x = (x & 0x0F0F) + ((x & 0xF0F0) >> 4);
x = (x & 0x00FF) + ((x & 0xFF00) >> 8);
return (x);
}
... which immediately lends itself to:
int
parity(uint16_t x)
{
x = (x & 0x) ^ ((x & 0x) >> 1);
x = (x & 0x) ^ ((x & 0x) >> 2);
x = (x & 0x0F0F) ^ ((x & 0xF0F0) >> 4);
x = (x & 0x00FF) ^ ((x & 0xFF00) >> 8);
return (x);
}
--
Christian "naddy" Weisgerber na...@mips.inka.de
cpu_rnd_messybits() for hppa
Here's a cpu_rnd_messybits() for hppa. This just steals from
itmr_get_timecount(). If we want to use the mfctl() macro, we need
to include into cpu.h. I don't know if we want
that, so I went with the explicit asm() instead.
Completely untested.
Index: sys/arch/hppa/hppa/machdep.c
===
RCS file: /cvs/src/sys/arch/hppa/hppa/machdep.c,v
retrieving revision 1.259
diff -u -p -r1.259 machdep.c
--- sys/arch/hppa/hppa/machdep.c31 May 2020 06:23:57 - 1.259
+++ sys/arch/hppa/hppa/machdep.c5 Jun 2020 15:17:23 -
@@ -1496,12 +1496,3 @@ blink_led_timeout(void *vsc)
t = (((averunnable.ldavg[0] + FSCALE) * hz) >> (FSHIFT + 1));
timeout_add(>bls_to, t);
}
-
-unsigned int
-cpu_rnd_messybits(void)
-{
- struct timespec ts;
-
- nanotime();
- return (ts.tv_nsec ^ (ts.tv_sec << 20));
-}
Index: sys/arch/hppa/include/cpu.h
===
RCS file: /cvs/src/sys/arch/hppa/include/cpu.h,v
retrieving revision 1.92
diff -u -p -r1.92 cpu.h
--- sys/arch/hppa/include/cpu.h 31 May 2020 06:23:57 - 1.92
+++ sys/arch/hppa/include/cpu.h 5 Jun 2020 15:17:23 -
@@ -54,6 +54,7 @@
#ifdef _KERNEL
#include
#include
+#include
#endif /* _KERNEL */
/*
@@ -237,7 +238,16 @@ intcopy_on_fault(void);
void switch_trampoline(void);
intcpu_dumpsize(void);
intcpu_dump(void);
-unsigned int cpu_rnd_messybits(void);
+
+static inline unsigned int
+cpu_rnd_messybits(void)
+{
+unsigned int __itmr;
+
+ __asm volatile("mfctl %1,%0": "=r" (__itmr) : "i" (CR_ITMR));
+
+return (__itmr);
+}
#ifdef MULTIPROCESSOR
void cpu_boot_secondary_processors(void);
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: ffs(3): libc arch versions, regress
Christian Weisgerber:
> This adds the optimized ffs(3) versions on aarch64 and powerpc to
> libc, for completeness' sake.
>
> Also add a brief regression test.
>
> OK?
Index: lib/libc/arch/aarch64/string/Makefile.inc
===
RCS file: /cvs/src/lib/libc/arch/aarch64/string/Makefile.inc,v
retrieving revision 1.1
diff -u -p -r1.1 Makefile.inc
--- lib/libc/arch/aarch64/string/Makefile.inc 11 Jan 2017 18:09:24 -
1.1
+++ lib/libc/arch/aarch64/string/Makefile.inc 11 Jun 2020 20:30:34 -
@@ -2,7 +2,7 @@
SRCS+= bcopy.c memcpy.c memmove.c \
strchr.c strrchr.c \
- bcmp.c bzero.c ffs.c memchr.c memcmp.c memset.c \
+ bcmp.c bzero.c ffs.S memchr.c memcmp.c memset.c \
strcmp.c strncmp.c \
strcat.c strcpy.c strcspn.c strlen.c strlcat.c strlcpy.c \
strncat.c strncpy.c strpbrk.c strsep.c \
Index: lib/libc/arch/aarch64/string/ffs.S
===
RCS file: lib/libc/arch/aarch64/string/ffs.S
diff -N lib/libc/arch/aarch64/string/ffs.S
--- /dev/null 1 Jan 1970 00:00:00 -
+++ lib/libc/arch/aarch64/string/ffs.S 11 Jun 2020 20:31:19 -
@@ -0,0 +1,18 @@
+/* $OpenBSD$ */
+/*
+ * Written by Christian Weisgerber .
+ * Public domain.
+ */
+
+#include "DEFS.h"
+
+ENTRY(ffs)
+ RETGUARD_SETUP(ffs, x15)
+ rbitw1, w0
+ clz w1, w1
+ cmp w0, wzr
+ csinc w0, wzr, w1, eq
+ RETGUARD_CHECK(ffs, x15)
+ ret
+END(ffs)
+.protected
Index: lib/libc/arch/powerpc/string/Makefile.inc
===
RCS file: /cvs/src/lib/libc/arch/powerpc/string/Makefile.inc,v
retrieving revision 1.6
diff -u -p -r1.6 Makefile.inc
--- lib/libc/arch/powerpc/string/Makefile.inc 15 May 2015 22:29:37 -
1.6
+++ lib/libc/arch/powerpc/string/Makefile.inc 11 Jun 2020 20:33:04 -
@@ -2,7 +2,7 @@
SRCS+= memcpy.c memmove.S \
strchr.c strrchr.c \
- bcmp.c bzero.c ffs.c memchr.c memcmp.c memset.c strcat.c \
+ bcmp.c bzero.c ffs.S memchr.c memcmp.c memset.c strcat.c \
strcmp.c strcpy.c strcspn.c strlen.c strlcat.c strlcpy.c \
strncat.c strncmp.c strncpy.c strpbrk.c strsep.c \
strspn.c strstr.c swab.c
Index: lib/libc/arch/powerpc/string/ffs.S
===
RCS file: lib/libc/arch/powerpc/string/ffs.S
diff -N lib/libc/arch/powerpc/string/ffs.S
--- /dev/null 1 Jan 1970 00:00:00 -
+++ lib/libc/arch/powerpc/string/ffs.S 11 Jun 2020 20:33:19 -
@@ -0,0 +1,16 @@
+/* $OpenBSD$ */
+/*
+ * Written by Christian Weisgerber .
+ * Public domain.
+ */
+
+#include "SYS.h"
+
+ENTRY(ffs)
+ neg %r4, %r3
+ and %r3, %r3, %r4
+ cntlzw %r3, %r3
+ subfic %r3, %r3, 32
+ blr
+END(ffs)
+.protected
Index: regress/lib/libc/ffs/Makefile
===
RCS file: regress/lib/libc/ffs/Makefile
diff -N regress/lib/libc/ffs/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ regress/lib/libc/ffs/Makefile 12 Jun 2020 08:43:43 -
@@ -0,0 +1,6 @@
+PROG= ffs_test
+
+# prevent inlining of __builtin_ffs()
+CFLAGS+= -ffreestanding
+
+.include
Index: regress/lib/libc/ffs/ffs_test.c
===
RCS file: regress/lib/libc/ffs/ffs_test.c
diff -N regress/lib/libc/ffs/ffs_test.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ regress/lib/libc/ffs/ffs_test.c 12 Jun 2020 08:43:19 -
@@ -0,0 +1,18 @@
+/* $OpenBSD$ */
+/*
+ * Written by Christian Weisgerber .
+ * Public domain.
+ */
+
+#include
+#include
+#include
+
+int
+main(void)
+{
+ assert(ffs(0) == 0);
+ assert(ffs(0x8080) == 8);
+ assert(ffs(INT32_MIN) == 32);
+ return (0);
+}
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Theo de Raadt:
> The diff is growing complexity to support a future which wouldn't
> exist if attempts at *supporting all* architectures received priority.
Adding support for more archs is very simple, since you just need
to copy the corresponding get_timecounter function from the kernel.
Here's arm64. I'm running a kernel and libc with this.
I can also provide alpha, powerpc, and sparc64, but I don't have
such machines.
--- ./lib/libc/arch/aarch64/gen/usertc.c.orig Thu Jun 11 19:07:39 2020
+++ ./lib/libc/arch/aarch64/gen/usertc.cThu Jun 11 19:08:01 2020
@@ -18,4 +18,32 @@
#include
#include
-int (*const _tc_get_timecount)(struct timekeep *, uint64_t *) = NULL;
+static uint64_t
+readcnt64(void)
+{
+ uint64_t val0, val1;
+
+ /*
+* Work around Cortex-A73 errata 858921, where there is a
+* one-cycle window where the read might return the old value
+* for the low 32 bits and the new value for the high 32 bits
+* upon roll-over of the low 32 bits.
+*/
+ __asm volatile("isb" : : : "memory");
+ __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val0));
+ __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val1));
+ return ((val0 ^ val1) & 0x1ULL) ? val0 : val1;
+}
+
+int
+tc_get_timecount(struct timekeep *tk, uint64_t *tc)
+{
+ if (tc == NULL || tk->tk_user != 1)
+ return -1;
+
+ *tc = readcnt64();
+ return 0;
+}
+
+int (*const _tc_get_timecount)(struct timekeep *, uint64_t *)
+ = tc_get_timecount;
--- ./sys/arch/arm64/arm64/machdep.c.orig Thu Jun 11 17:46:54 2020
+++ ./sys/arch/arm64/arm64/machdep.cThu Jun 11 17:46:59 2020
@@ -91,7 +91,7 @@
charmachine[] = MACHINE;/* from */
/* timekeep number of user accesible clocks */
-int tk_nclocks = 0;
+int tk_nclocks = 1;
int safepri = 0;
--- ./sys/arch/arm64/dev/agtimer.c.orig Thu Jun 11 17:47:23 2020
+++ ./sys/arch/arm64/dev/agtimer.c Thu Jun 11 17:47:27 2020
@@ -43,7 +43,7 @@
u_int agtimer_get_timecount(struct timecounter *);
static struct timecounter agtimer_timecounter = {
- agtimer_get_timecount, NULL, 0x7fff, 0, "agtimer", 0, NULL, 0
+ agtimer_get_timecount, NULL, 0x7fff, 0, "agtimer", 0, NULL, 1
};
struct agtimer_pcpu_softc {
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Paul Irofti: > > Paul, that tk_nclocks addition isn't useful. You need to do the > > bounds checking against the number of clocks you have implemented in > > libc. How many clocks the kernel has implemented doesn't matter. > > What you are saying is that we could be in a situation where the kernel > might expose 3 clocks but we only have 2 entries in libc? Why would we get > to that point? When someone changes the clock in the kernel, that means it > is also changed in libc. I don't think we can decouple the two parts. Right? But we do: make kernel; install kernel; reboot; make build. To cross from nclocks to nclocks+1, you need to run the new nclocks+1 kernel with an nclocks userland. I keep coming back to the idea that we need an header with #define TC_FOO 1 #define TC_BAR 2 #define TC_NUM 3/* or TC_LAST or whatever */ Mark may have a better idea how to name this. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Paul Irofti:
[lib/libc/arch/*/gen/usertc.c]
> +int (*const _tc_get_timecount)(struct timekeep *, uint64_t *) = NULL;
[lib/libc/sys/microtime.c]
> +static inline int
> +tc_delta(struct timekeep *tk, u_int *delta)
> +{
> + uint64_t tc;
> + if (delta == NULL || _tc_get_timecount(tk, ))
This use of uint64_t for the timecounter return value confused me.
All the kernel code uses u_int as the return value of the timecounter_get
functions. The userland code should do the same (or uint32_t or
whatever the preferred alias is).
A central idea is that we can just copy the MD timecounter_get
functions from the kernel to the userland code. They all return
only 32 bits. (If the hardware register has more bits, like TSC
or TICK, the value is truncated.)
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Paul Irofti: > > Additionally, it changes struct timekeep in an incompatible way. ;-) > > A userland built before the addition of tk_nclocks is very unhappy > > with a kernel built afterwards. > > I have not seen this problem and have not built a snapshot to update or go > back. What do you mean by very unhappy? init(8) can't successfully spawn a shell and you're stuck at a prompt "Enter pathname of shell or RETURN for sh: " > Can you show me the exact steps you have done? Well, I had a system running with an earlier version of the diff, then I updated the source to the newer diff, built and installed a kernel, rebooted, and boom!, incompatible kernel and userland. Why is this controversial? A member tk_nclocks was inserted in the middle of struct timekeep, changing the layout of the struct. The new kernel uses the new layout, the userland the old layout. Of course they're incompatible. I should have noticed this while reading the new diff... -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Christian Weisgerber:
> Should we already bump major while the diff matures?
Oh, we can't quite bump yet. tk_major und tk_minor are only set
in exec_timekeep_map(), but aren't checked anywhere.
+ timekeep->tk_major = 0;
+ timekeep->tk_minor = 0;
Those shouldn't be magic numbers but #defines in sys/timetc.h.
We only need to check those once at process startup. In
libc/dlfcn/init.c, somewhere here:
+ case AUX_openbsd_timekeep:
+ if (_tc_get_timecount)
+ _timekeep = (void *)aux->au_v;
+ break;
Something like this I think:
case AUX_openbsd_timekeep:
if (_tc_get_timecount) {
struct timekeep *tk = (void *)aux->au_v;
if ((tk != NULL) &&
(tk->tk_major == TK_MAJOR) &&
(tk->tk_minor >= TK_MINOR))
_timekeep = tk;
}
break;
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Paul Irofti: > This iteration of the diff adds bounds checking for tk_user and moves > the usertc.c stub to every arch in libc as recommanded by deraadt@. > It also fixes a gettimeofday issue reported by cheloha@ and tb@. Additionally, it changes struct timekeep in an incompatible way. ;-) A userland built before the addition of tk_nclocks is very unhappy with a kernel built afterwards. There is no way to compile across this. You have to (U)pgrade from boot media to install a ftp.openbsd.org userland, and then you can re-compile with the new diff. Should we already bump major while the diff matures? -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Paul Irofti:
> > This iteration of the diff adds bounds checking for tk_user and moves
> > the usertc.c stub to every arch in libc as recommanded by deraadt@.
> > It also fixes a gettimeofday issue reported by cheloha@ and tb@.
>
> Forgot to add armv7 tk_nclock entries. Noticed by benno@, thanks!
One blemish I see is that tk_user is a magic number.
For example, sparc64 will have two timecounters: tick and stick.
They will be assigned magic numbers 1 and 2...
struct timecounter tick_timecounter = {
tick_get_timecount, NULL, ~0u, 0, "tick", 0, NULL, 1
};
struct timecounter stick_timecounter = {
stick_get_timecount, NULL, ~0u, 0, "stick", 1000, NULL, 2
};
... and sparc64 usertc.c will need the corresponding magic array order:
static uint64_t (*get_tc[])(void) =
{
rdtick,
rdstick,
};
I don't know if we want to go through the effort to make this
prettier. We would need an MD header, say,
that gets picked up by , with something like
#define TC_TICK 1
#define TC_STICK2
The symbolic values could then be used in the kernel timecounter
definitions...
struct timecounter tick_timecounter = {
tick_get_timecount, NULL, ~0u, 0, "tick", 0, NULL, TC_TICK
};
struct timecounter stick_timecounter = {
stick_get_timecount, NULL, ~0u, 0, "stick", 1000, NULL, TC_STICK
};
... and in libc usertc.c:
static uint64_t (*get_tc[])(void) =
{
[TC_TICK] = rdtick,
[TC_STICK] = rdstick,
};
...
*tc = (*get_tc[tk_user])();
The cost would be yet another header file per arch.
Thoughts?
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: libkern: ffs() for arm64, powerpc, powerpc64
Next try. Optimized versions for kernel ffs(3) on arm64, powerpc, powerpc64. I have tested arm64; cwen@ has tested powerpc in userland. powerpc64 is copied from powerpc. ok? Index: lib/libkern/arch/arm64/ffs.S === RCS file: lib/libkern/arch/arm64/ffs.S diff -N lib/libkern/arch/arm64/ffs.S --- /dev/null 1 Jan 1970 00:00:00 - +++ lib/libkern/arch/arm64/ffs.S10 Jun 2020 17:38:50 - @@ -0,0 +1,17 @@ +/* $OpenBSD$ */ +/* + * Written by Christian Weisgerber . + * Public domain. + */ + +#include + +ENTRY(ffs) + RETGUARD_SETUP(ffs, x15) + rbitw1, w0 + clz w1, w1 + cmp w0, wzr + csinc w0, wzr, w1, eq + RETGUARD_CHECK(ffs, x15) + ret +END(ffs) Index: lib/libkern/arch/powerpc/ffs.S === RCS file: lib/libkern/arch/powerpc/ffs.S diff -N lib/libkern/arch/powerpc/ffs.S --- /dev/null 1 Jan 1970 00:00:00 - +++ lib/libkern/arch/powerpc/ffs.S 10 Jun 2020 17:39:02 - @@ -0,0 +1,15 @@ +/* $OpenBSD$ */ +/* + * Written by Christian Weisgerber . + * Public domain. + */ + +#include + +ENTRY(ffs) + neg %r4, %r3 + and %r3, %r3, %r4 + cntlzw %r3, %r3 + subfic %r3, %r3, 32 + blr +END(ffs) Index: lib/libkern/arch/powerpc64/ffs.S === RCS file: lib/libkern/arch/powerpc64/ffs.S diff -N lib/libkern/arch/powerpc64/ffs.S --- /dev/null 1 Jan 1970 00:00:00 - +++ lib/libkern/arch/powerpc64/ffs.S10 Jun 2020 17:39:06 - @@ -0,0 +1,15 @@ +/* $OpenBSD$ */ +/* + * Written by Christian Weisgerber . + * Public domain. + */ + +#include + +ENTRY(ffs) + neg %r4, %r3 + and %r3, %r3, %r4 + cntlzw %r3, %r3 + subfic %r3, %r3, 32 + blr +END(ffs) -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: libkern: ffs() for arm64, powerpc, powerpc64
Mark Kettenis:
> Unfortunately that doesn't quite work. At least in my build it
> doesn't pick up .c files in the linker/arch directories.
>
> > Index: lib/libkern/arch/arm64/ffs.c
I was certain I had checked this, but indeed it doesn't work.
The Makefile rules are generated from sys/conf/files:
...
file lib/libkern/arch/${MACHINE_ARCH}/ffs.S | lib/libkern/ffs.c
...
We could extend this by a third file. Ugly.
Alternatively I could go back to .S, sigh.
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: libkern: ffs() for arm64, powerpc, powerpc64
On 2020-06-09, Christian Weisgerber wrote: > Here are optimized ffs(3) implementations for > * arm64 (superseding the earlier ffs.S) > * powerpc > * powerpc64 > +int ffs(int x) Oops, I'm going to change that to int ffs(int x) before commit. -- Christian "naddy" Weisgerber na...@mips.inka.de
ffs(3): libc arch versions, regress
This adds the optimized ffs(3) versions on aarch64 and powerpc to
libc, for completeness' sake.
Also add a brief regression test.
OK?
Index: lib/libc/arch/aarch64/string/ffs.c
===
RCS file: lib/libc/arch/aarch64/string/ffs.c
diff -N lib/libc/arch/aarch64/string/ffs.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ lib/libc/arch/aarch64/string/ffs.c 9 Jun 2020 19:54:21 -
@@ -0,0 +1,15 @@
+/* $OpenBSD$ */
+/*
+ * Written by Christian Weisgerber .
+ * Public domain.
+ */
+
+#include
+
+int
+ffs(int x)
+{
+ x = x & -x;
+ __asm volatile("clz %w0, %w0" : "+r" (x));
+ return (32 - x);
+}
Index: lib/libc/arch/powerpc/string/ffs.c
===
RCS file: lib/libc/arch/powerpc/string/ffs.c
diff -N lib/libc/arch/powerpc/string/ffs.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ lib/libc/arch/powerpc/string/ffs.c 9 Jun 2020 19:54:41 -
@@ -0,0 +1,15 @@
+/* $OpenBSD$ */
+/*
+ * Written by Christian Weisgerber .
+ * Public domain.
+ */
+
+#include
+
+int
+ffs(int x)
+{
+ x = x & -x;
+ __asm volatile("cntlzw %0, %0" : "+r" (x));
+ return (32 - x);
+}
Index: regress/lib/libc/ffs/Makefile
===
RCS file: regress/lib/libc/ffs/Makefile
diff -N regress/lib/libc/ffs/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ regress/lib/libc/ffs/Makefile 9 Jun 2020 19:45:02 -
@@ -0,0 +1,8 @@
+# $OpenBSD$
+
+PROG= ffs_test
+
+# prevent inlining of __builtin_ffs()
+CFLAGS+= -ffreestanding
+
+.include
Index: regress/lib/libc/ffs/ffs_test.c
===
RCS file: regress/lib/libc/ffs/ffs_test.c
diff -N regress/lib/libc/ffs/ffs_test.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ regress/lib/libc/ffs/ffs_test.c 9 Jun 2020 19:40:33 -
@@ -0,0 +1,18 @@
+/* $OpenBSD$ */
+/*
+ * Written by Christian Weisgerber .
+ * Public domain.
+ */
+
+#include
+#include
+#include
+
+int
+main(void)
+{
+ assert(ffs(0) == 0);
+ assert(ffs(0x8080) == 8);
+ assert(ffs(INT32_MIN) == 32);
+ return (0);
+}
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: libkern: ffs() for arm64
On 2020-06-08, Christian Weisgerber wrote:
> More archs to come...
Style question:
Since this mostly comes down to embedding a single special instruction
in between normal C operations, I wonder whether I should just do .c
with asm() instead of .S, and leave all the boilerplate to the
compiler? It would save me from reading up on calling conventions,
more assembly syntax, etc.
E.g.:
int ffs(int x)
{
x = x & -x;
__asm volatile("clz %0, %0" : "=r" (x));
return (32 - x);
}
--
Christian "naddy" Weisgerber na...@mips.inka.de
libkern: ffs() for arm64
Here is an optimized kernel ffs(3) for arm64. I blame dlg@ for making me scrutinize the POWER7 instruction set, which led me to the clz instruction, which led me to ffs(). I wanted to add this to libc, but then realized the futility because the compiler already inlines its optimized copy of ffs(). However, this optimization is disabled for the kernel with -ffreestanding. Honestly, I'm not sure I can claim to have written this. The elegant version below is adapted from clang output, because the compiler is smarter than I am. More archs to come... Comments? OK? Index: lib/libkern/arch/arm64/ffs.S === RCS file: lib/libkern/arch/arm64/ffs.S diff -N lib/libkern/arch/arm64/ffs.S --- /dev/null 1 Jan 1970 00:00:00 - +++ lib/libkern/arch/arm64/ffs.S8 Jun 2020 20:35:01 - @@ -0,0 +1,17 @@ +/* $OpenBSD$ */ +/* + * Written by Christian Weisgerber . + * Public domain. + */ + +#include + +ENTRY(ffs) + RETGUARD_SETUP(ffs, x15) + rbitw1, w0 + clz w1, w1 + cmp w0, #0 + csinc w0, wzr, w1, eq + RETGUARD_CHECK(ffs, x15) + ret +END(ffs) -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: powerpc64: ldbrx/stdbrx for endian.h?
On 2020-06-08, Christian Weisgerber wrote: >> Did they also happen to add opcodes for doing swaps in registers? > > No. > (I haven't looked at the vector instructions yet.) PS: There's a vector permutate instruction, but AFAICT there is no way to move data between general purpose and vector registers; you have to go through memory. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: powerpc64: ldbrx/stdbrx for endian.h?
David Gwynne: > > Starting with POWER7 (Power ISA v.2.06), there are also corresponding > > 64-bit instructions. Do we want to use those on powerpc64? Or do > > we want to keep compatibility with older processors? > > I'm ok with using the instructions. I can't think of what benefit compat in > this space would actually provide. Well, if people were to extend powerpc64 down to the PowerPC G5 (= POWER4) Apple machines, then it would not be possible to use those instructions. > Did they also happen to add opcodes for doing swaps in registers? No. (I haven't looked at the vector instructions yet.) -- Christian "naddy" Weisgerber na...@mips.inka.de
powerpc64: ldbrx/stdbrx for endian.h?
powerpc has byte-swapping 16 and 32-bit load/stores and we use those
in .
Starting with POWER7 (Power ISA v.2.06), there are also corresponding
64-bit instructions. Do we want to use those on powerpc64? Or do
we want to keep compatibility with older processors?
Index: arch/powerpc64/include/endian.h
===
RCS file: /cvs/src/sys/arch/powerpc64/include/endian.h,v
retrieving revision 1.1
diff -u -p -r1.1 endian.h
--- arch/powerpc64/include/endian.h 16 May 2020 17:11:14 - 1.1
+++ arch/powerpc64/include/endian.h 8 Jun 2020 11:16:33 -
@@ -36,7 +36,7 @@ __mswap16(volatile const __uint16_t *m)
__asm("lhbrx %0, 0, %1"
: "=r" (v)
-: "r" (m), "m" (*m));
+ : "r" (m), "m" (*m));
return (v);
}
@@ -48,7 +48,7 @@ __mswap32(volatile const __uint32_t *m)
__asm("lwbrx %0, 0, %1"
: "=r" (v)
-: "r" (m), "m" (*m));
+ : "r" (m), "m" (*m));
return (v);
}
@@ -56,11 +56,11 @@ __mswap32(volatile const __uint32_t *m)
static inline __uint64_t
__mswap64(volatile const __uint64_t *m)
{
- __uint32_t *a = (__uint32_t *)m;
__uint64_t v;
- v = (__uint64_t)__mswap32(a + 1) << 32 |
- (__uint64_t)__mswap32(a);
+ __asm("ldbrx %0, 0, %1"
+ : "=r" (v)
+ : "r" (m), "m" (*m));
return (v);
}
@@ -84,10 +84,9 @@ __swapm32(volatile __uint32_t *m, __uint
static inline void
__swapm64(volatile __uint64_t *m, __uint64_t v)
{
- __uint32_t *a = (__uint32_t *)m;
-
- __swapm32(a + 1, v >> 32);
- __swapm32(a, v);
+ __asm("stdbrx %1, 0, %2"
+ : "=m" (*m)
+ : "r" (v), "r" (m));
}
#define __HAVE_MD_SWAPIO
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: cpu_rnd_messybits() for arm64
Mark Kettenis:
> > Here is a cpu_rnd_messybits() implementation for arm64.
> > It reads the virtual counter and xors it with a bit-reversed copy
> > of itself.
> >
> > The virtual counter is used by the only timecounter implementation
> > used on arm64, so I assume it is generally available.
> >
> > It's a 64-bit counter, which we reduce to 32 bits. Since there is
> > progressively less entropy in the higher bits of a counter than in
> > the lower bits, it intuitively makes sense not just to do hi^lo,
> > but to bit-reverse one half in order to extract maximal entropy,
> > and on aarch64 bit reversal is a simple instruction.
>
> Can you make that uint64_t?
> Can you use %0 and %1 instead of %x0 and %x1?
Right.
Index: arch/arm64/arm64/machdep.c
===
RCS file: /cvs/src/sys/arch/arm64/arm64/machdep.c,v
retrieving revision 1.52
diff -u -p -r1.52 machdep.c
--- arch/arm64/arm64/machdep.c 31 May 2020 06:23:57 - 1.52
+++ arch/arm64/arm64/machdep.c 5 Jun 2020 20:00:20 -
@@ -1247,12 +1247,3 @@ dumpregs(struct trapframe *frame)
printf("pc: 0x%016lx\n", frame->tf_elr);
printf("spsr: 0x%016lx\n", frame->tf_spsr);
}
-
-unsigned int
-cpu_rnd_messybits(void)
-{
- struct timespec ts;
-
- nanotime();
- return (ts.tv_nsec ^ (ts.tv_sec << 20));
-}
Index: arch/arm64/include/cpu.h
===
RCS file: /cvs/src/sys/arch/arm64/include/cpu.h,v
retrieving revision 1.17
diff -u -p -r1.17 cpu.h
--- arch/arm64/include/cpu.h31 May 2020 06:23:57 - 1.17
+++ arch/arm64/include/cpu.h5 Jun 2020 22:13:07 -
@@ -183,7 +183,16 @@ void cpu_boot_secondary_processors(void)
#define curpcb curcpu()->ci_curpcb
-unsigned int cpu_rnd_messybits(void);
+static inline unsigned int
+cpu_rnd_messybits(void)
+{
+ uint64_t val, rval;
+
+ __asm volatile("mrs %0, CNTVCT_EL0; rbit %1, %0;"
+ : "=r" (val), "=r" (rval));
+
+ return (val ^ rval);
+}
/*
* Scheduling glue
--
Christian "naddy" Weisgerber na...@mips.inka.de
cpu_rnd_messybits() for arm64
Here is a cpu_rnd_messybits() implementation for arm64.
It reads the virtual counter and xors it with a bit-reversed copy
of itself.
The virtual counter is used by the only timecounter implementation
used on arm64, so I assume it is generally available.
It's a 64-bit counter, which we reduce to 32 bits. Since there is
progressively less entropy in the higher bits of a counter than in
the lower bits, it intuitively makes sense not just to do hi^lo,
but to bit-reverse one half in order to extract maximal entropy,
and on aarch64 bit reversal is a simple instruction.
This patch I have tested!
ok?
Index: arch/arm64/arm64/machdep.c
===
RCS file: /cvs/src/sys/arch/arm64/arm64/machdep.c,v
retrieving revision 1.52
diff -u -p -r1.52 machdep.c
--- arch/arm64/arm64/machdep.c 31 May 2020 06:23:57 - 1.52
+++ arch/arm64/arm64/machdep.c 5 Jun 2020 20:00:20 -
@@ -1247,12 +1247,3 @@ dumpregs(struct trapframe *frame)
printf("pc: 0x%016lx\n", frame->tf_elr);
printf("spsr: 0x%016lx\n", frame->tf_spsr);
}
-
-unsigned int
-cpu_rnd_messybits(void)
-{
- struct timespec ts;
-
- nanotime();
- return (ts.tv_nsec ^ (ts.tv_sec << 20));
-}
Index: arch/arm64/include/cpu.h
===
RCS file: /cvs/src/sys/arch/arm64/include/cpu.h,v
retrieving revision 1.17
diff -u -p -r1.17 cpu.h
--- arch/arm64/include/cpu.h31 May 2020 06:23:57 - 1.17
+++ arch/arm64/include/cpu.h5 Jun 2020 20:32:01 -
@@ -183,7 +183,16 @@ void cpu_boot_secondary_processors(void)
#define curpcb curcpu()->ci_curpcb
-unsigned int cpu_rnd_messybits(void);
+static inline unsigned int
+cpu_rnd_messybits(void)
+{
+ u_int64_t val, rval;
+
+ __asm volatile("mrs %x0, CNTVCT_EL0; rbit %x1, %x0;"
+ : "=r" (val), "=r" (rval));
+
+ return (val ^ rval);
+}
/*
* Scheduling glue
--
Christian "naddy" Weisgerber na...@mips.inka.de
cpu_rnd_messybits() for alpha, powerpc, sparc64
Here's a proposal for implementing cpu_rnd_messybits() as a read of
the cycle counter on alpha, powerpc, and sparc64. Since I don't have
those archs, the diff is not even compile-tested.
* alpha: RPCC is a 32-bit counter (in a 64-bit register)
* powerpc: TB is a 64-bit counter split into two registers
* sparc64: TICK is a(n implementation-defined, up to) 63-bit counter
Index: sys/arch/alpha/alpha/machdep.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/machdep.c,v
retrieving revision 1.191
diff -u -p -r1.191 machdep.c
--- sys/arch/alpha/alpha/machdep.c 31 May 2020 06:23:56 - 1.191
+++ sys/arch/alpha/alpha/machdep.c 4 Jun 2020 17:57:45 -
@@ -1854,12 +1854,3 @@ alpha_XXX_dmamap(v)
/* XXX */
return (vtophys(v) | alpha_XXX_dmamap_or); /* XXX */
} /* XXX */
/* XXX XXX END XXX XXX */
-
-unsigned int
-cpu_rnd_messybits(void)
-{
- struct timespec ts;
-
- nanotime();
- return (ts.tv_nsec ^ (ts.tv_sec << 20));
-}
Index: sys/arch/alpha/include/cpu.h
===
RCS file: /cvs/src/sys/arch/alpha/include/cpu.h,v
retrieving revision 1.62
diff -u -p -r1.62 cpu.h
--- sys/arch/alpha/include/cpu.h31 May 2020 06:23:56 - 1.62
+++ sys/arch/alpha/include/cpu.h4 Jun 2020 17:59:25 -
@@ -288,7 +288,11 @@ do {
\
*/
#definecpu_number()alpha_pal_whami()
-unsigned int cpu_rnd_messybits(void);
+static inline unsigned int
+cpu_rnd_messybits(void)
+{
+ return alpha_rpcc();
+}
/*
* Arguments to hardclock and gatherstats encapsulate the previous
Index: sys/arch/macppc/macppc/machdep.c
===
RCS file: /cvs/src/sys/arch/macppc/macppc/machdep.c,v
retrieving revision 1.191
diff -u -p -r1.191 machdep.c
--- sys/arch/macppc/macppc/machdep.c31 May 2020 06:23:57 - 1.191
+++ sys/arch/macppc/macppc/machdep.c4 Jun 2020 18:07:31 -
@@ -913,12 +913,3 @@ cpu_switchto(struct proc *oldproc, struc
cpu_switchto_asm(oldproc, newproc);
}
-
-unsigned int
-cpu_rnd_messybits(void)
-{
- struct timespec ts;
-
- nanotime();
- return (ts.tv_nsec ^ (ts.tv_sec << 20));
-}
Index: sys/arch/powerpc/include/cpu.h
===
RCS file: /cvs/src/sys/arch/powerpc/include/cpu.h,v
retrieving revision 1.67
diff -u -p -r1.67 cpu.h
--- sys/arch/powerpc/include/cpu.h 31 May 2020 06:23:58 - 1.67
+++ sys/arch/powerpc/include/cpu.h 4 Jun 2020 18:13:07 -
@@ -161,7 +161,15 @@ extern int ppc_nobat;
void cpu_bootstrap(void);
-unsigned int cpu_rnd_messybits(void);
+static inline unsigned int
+cpu_rnd_messybits(void)
+{
+ unsigned int hi, lo;
+
+ __asm volatile("mftbu %0; mftb %1" : "=r" (hi), "=r" (lo));
+
+ return (hi ^ lo);
+}
/*
* This is used during profiling to integrate system time.
Index: sys/arch/sparc64/include/cpu.h
===
RCS file: /cvs/src/sys/arch/sparc64/include/cpu.h,v
retrieving revision 1.94
diff -u -p -r1.94 cpu.h
--- sys/arch/sparc64/include/cpu.h 31 May 2020 06:23:58 - 1.94
+++ sys/arch/sparc64/include/cpu.h 4 Jun 2020 18:05:18 -
@@ -211,7 +211,15 @@ void cpu_unidle(struct cpu_info *);
#define curpcb __curcpu->ci_cpcb
#define fpproc __curcpu->ci_fpproc
-unsigned int cpu_rnd_messybits(void);
+static inline unsigned int
+cpu_rnd_messybits(void)
+{
+ u_int64_t tick;
+
+ __asm volatile("rd %%tick, %0" : "=r" (tick) :);
+
+ return ((tick >> 32) ^ tick);
+}
/*
* On processors with multiple threads we force a thread switch.
Index: sys/arch/sparc64/sparc64/machdep.c
===
RCS file: /cvs/src/sys/arch/sparc64/sparc64/machdep.c,v
retrieving revision 1.196
diff -u -p -r1.196 machdep.c
--- sys/arch/sparc64/sparc64/machdep.c 31 May 2020 06:23:58 - 1.196
+++ sys/arch/sparc64/sparc64/machdep.c 4 Jun 2020 18:01:16 -
@@ -2114,12 +2114,3 @@ blink_led_timeout(void *vsc)
t = (((averunnable.ldavg[0] + FSCALE) * hz) >> (FSHIFT + 1));
timeout_add(>bls_to, t);
}
-
-unsigned int
-cpu_rnd_messybits(void)
-{
- struct timespec ts;
-
- nanotime();
- return (ts.tv_nsec ^ (ts.tv_sec << 20));
-}
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: userland clock_gettime proof of concept
Theo de Raadt: > > Then you still need to check on every call whether the clockid has > > changed (because the kern.timecounter.hardware sysctl was changed) > > and refetch the function pointer in that case. > > Then really, we should remove that sysctl support. > > Because otherwise I don't see how it can work. Aren't there deadlock > or spinning conditions? Or at minimum, situtions where time won't flow > linearly. The patch as-is works fine when kern.timecounter.hardware is toggled back and forth between tsc with userland gettime support and, say, acpihpet0 without. tk_user marks whether the currently selected timecounter has userland support, the wrapper checks that and falls back to the system call. Reading the code, I don't see a reason why this shouldn't work fine, and experimentally it does work. Changing kern.timecounter.hardware is transparent. kettenis@'s suggestion of extending this from on/off to clockid1/clockid2/.../off makes sense to me. Most architectures will likely only have one timecounter for which userland support is possible, but sparc64 has indeed two (tick, stick). -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: Xwindows keymap weirdness
Marc Espie: > If I type 'c > > I get a ç in programs like firefox or chrome, BUT I get > ć in xterm ? > > why are things different ? which program is right ? GTK comes with its own compose rules that are occasionally different from the default X11 ones. Personally, I disable GTK's compose processing with GTK_IM_MODULE=xim. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: official ports vs DEBUG_PACKAGES
Marc Espie: > There are about 3 solutions to that: > - change the bulk machines to /usr/ports/pobj > - change the ports default to /usr/obj/ports I don't think it's reasonable to expect everybody to use the same path here. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: random(4): use arc4random_ctx_buf() for large device reads
Sebastien Marie:
> > For large reads from /dev/random, use the arc4random_ctx_*() functions
> > instead of hand-rolling the same code to set up a temporary ChaCha
> > instance.
>
> Eventually, I would get ride of myctx, initialize lctx to NULL, and use
> (lctx == NULL) to replace (myctx == 0).
Right. Here we go:
Index: rnd.c
===
RCS file: /cvs/src/sys/dev/rnd.c,v
retrieving revision 1.213
diff -u -p -r1.213 rnd.c
--- rnd.c 18 May 2020 15:00:16 - 1.213
+++ rnd.c 25 May 2020 14:58:43 -
@@ -589,6 +589,9 @@ arc4random_ctx_free(struct arc4random_ct
void
arc4random_ctx_buf(struct arc4random_ctx *ctx, void *buf, size_t n)
{
+#ifndef KEYSTREAM_ONLY
+ memset(buf, 0, n);
+#endif
chacha_encrypt_bytes((chacha_ctx *)ctx, buf, buf, n);
}
@@ -701,40 +704,31 @@ randomclose(dev_t dev, int flag, int mod
int
randomread(dev_t dev, struct uio *uio, int ioflag)
{
- u_char lbuf[KEYSZ+IVSZ];
- chacha_ctx lctx;
+ struct arc4random_ctx *lctx = NULL;
size_t total = uio->uio_resid;
u_char *buf;
- int myctx = 0, ret = 0;
+ int ret = 0;
if (uio->uio_resid == 0)
return 0;
buf = malloc(POOLBYTES, M_TEMP, M_WAITOK);
- if (total > RND_MAIN_MAX_BYTES) {
- arc4random_buf(lbuf, sizeof(lbuf));
- chacha_keysetup(, lbuf, KEYSZ * 8);
- chacha_ivsetup(, lbuf + KEYSZ, NULL);
- explicit_bzero(lbuf, sizeof(lbuf));
- myctx = 1;
- }
+ if (total > RND_MAIN_MAX_BYTES)
+ lctx = arc4random_ctx_new();
while (ret == 0 && uio->uio_resid > 0) {
size_t n = ulmin(POOLBYTES, uio->uio_resid);
- if (myctx) {
-#ifndef KEYSTREAM_ONLY
- memset(buf, 0, n);
-#endif
- chacha_encrypt_bytes(, buf, buf, n);
- } else
+ if (lctx != NULL)
+ arc4random_ctx_buf(lctx, buf, n);
+ else
arc4random_buf(buf, n);
ret = uiomove(buf, n, uio);
if (ret == 0 && uio->uio_resid > 0)
yield();
}
- if (myctx)
- explicit_bzero(, sizeof(lctx));
+ if (lctx != NULL)
+ arc4random_ctx_free(lctx);
explicit_bzero(buf, POOLBYTES);
free(buf, M_TEMP, POOLBYTES);
return ret;
--
Christian "naddy" Weisgerber na...@mips.inka.de
random(4): use arc4random_ctx_buf() for large device reads
(This is in a different part of the file from Theo's current efforts.)
For large reads from /dev/random, use the arc4random_ctx_*() functions
instead of hand-rolling the same code to set up a temporary ChaCha
instance.
ok?
Index: rnd.c
===
RCS file: /cvs/src/sys/dev/rnd.c,v
retrieving revision 1.213
diff -u -p -r1.213 rnd.c
--- rnd.c 18 May 2020 15:00:16 - 1.213
+++ rnd.c 24 May 2020 15:54:00 -
@@ -589,6 +589,9 @@ arc4random_ctx_free(struct arc4random_ct
void
arc4random_ctx_buf(struct arc4random_ctx *ctx, void *buf, size_t n)
{
+#ifndef KEYSTREAM_ONLY
+ memset(buf, 0, n);
+#endif
chacha_encrypt_bytes((chacha_ctx *)ctx, buf, buf, n);
}
@@ -701,8 +704,7 @@ randomclose(dev_t dev, int flag, int mod
int
randomread(dev_t dev, struct uio *uio, int ioflag)
{
- u_char lbuf[KEYSZ+IVSZ];
- chacha_ctx lctx;
+ struct arc4random_ctx *lctx;
size_t total = uio->uio_resid;
u_char *buf;
int myctx = 0, ret = 0;
@@ -712,29 +714,23 @@ randomread(dev_t dev, struct uio *uio, i
buf = malloc(POOLBYTES, M_TEMP, M_WAITOK);
if (total > RND_MAIN_MAX_BYTES) {
- arc4random_buf(lbuf, sizeof(lbuf));
- chacha_keysetup(, lbuf, KEYSZ * 8);
- chacha_ivsetup(, lbuf + KEYSZ, NULL);
- explicit_bzero(lbuf, sizeof(lbuf));
+ lctx = arc4random_ctx_new();
myctx = 1;
}
while (ret == 0 && uio->uio_resid > 0) {
size_t n = ulmin(POOLBYTES, uio->uio_resid);
- if (myctx) {
-#ifndef KEYSTREAM_ONLY
- memset(buf, 0, n);
-#endif
- chacha_encrypt_bytes(, buf, buf, n);
- } else
+ if (myctx)
+ arc4random_ctx_buf(lctx, buf, n);
+ else
arc4random_buf(buf, n);
ret = uiomove(buf, n, uio);
if (ret == 0 && uio->uio_resid > 0)
yield();
}
if (myctx)
- explicit_bzero(, sizeof(lctx));
+ arc4random_ctx_free(lctx);
explicit_bzero(buf, POOLBYTES);
free(buf, M_TEMP, POOLBYTES);
return ret;
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: Block device poll(2) vs kqueue(2)
On 2020-05-20, Martin Pieuchot wrote: > - FreeBSD doesn't allow opening block devices in devfs_open(), so > only character devices seem to support poll(2) and kqueue(2). FreeBSD doesn't have block devices anymore. -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: iked(8): AES_GCM ciphers for IKE
Tobias Heider: > currently iked(8) supports AES-GCM only for ESP. > The diff below adds the ENCR_AES_GCM_16 and ENCR_AES_GCM_12 variants for IKE. > (for more information see [1] and [2]). > Both variants support the 128, 196, and 256 bit key lengths. > > The new new ciphers can be configured with: > - aes-128-gcm, aes-196-gcm and aes-256-gcm for ENCR_AES_GCM_16 > - aes-128-gcm-12, aes-196-gcm-12 and aes-256-gcm-12 for ENCR_AES_GCM_12 Is there a compelling reason to implement the GCM_12 variants? I remember that truncating integrity tags is problematic for GCM. That probably doesn't matter for small IKE exchanges, but then again four extra bytes per packet don't matter either. According to RFC5282, full length tags MUST be supported anyway, truncated ones are optional. RFC5282 also says that AES-192 is NOT RECOMMENDED. So I think only aes-128-gcm and aes-256-gcm should be added. While adding the other variants is simple, there is no value in supporting them; they just add more configuration buttons. -- Christian "naddy" Weisgerber na...@mips.inka.de
