Re: relayd interrupted transfers
Hi! Your version works for me. Rivo On 24/08/2017, 17:10, "Alexander Bluhm" wrote: On Thu, Aug 24, 2017 at 12:44:27PM +, Rivo Nurges wrote: > This will fix my problem and regress still passes. Yes this also works for me. I think the variable name "dst" is not good. Normally it refers to someting on the cre->dst side. As the buffer is on our side, I think EVBUFFER_LENGTH(EVBUFFER_OUTPUT(bev)) is better. I was also wondering whether it is correct to skip the bufferevent_enable(cre->dst->bev, EV_READ) further down. Would your current diff create pumped dataflow behavior as the output buffer must be completely empty before new data is read? The relay_splice() checks for the buffer length itself and disables reading if the buffers are not empty. This does not work due to the bufferevent_enable(cre->dst->bev, EV_READ) which was added later. So I think the logic should look like this. Does this work for you? bluhm Index: usr.sbin/relayd/relay.c === RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/relayd/relay.c,v retrieving revision 1.225 diff -u -p -r1.225 relay.c --- usr.sbin/relayd/relay.c 9 Aug 2017 21:29:17 - 1.225 +++ usr.sbin/relayd/relay.c 24 Aug 2017 14:02:38 - @@ -791,12 +791,12 @@ relay_write(struct bufferevent *bev, voi getmonotime(&con->se_tv_last); - if (con->se_done) + if (con->se_done && EVBUFFER_LENGTH(EVBUFFER_OUTPUT(bev)) == 0) goto done; - if (relay_splice(cre->dst) == -1) - goto fail; if (cre->dst->bev) bufferevent_enable(cre->dst->bev, EV_READ); + if (relay_splice(cre->dst) == -1) + goto fail; return; done:
Re: ksh: history file is corrupt
On 2017/08/24 17:42, Jeremie Courreges-Anglas wrote: > On Wed, Aug 23 2017, Stuart Henderson wrote: > > On 2017/08/15 11:57, Jeremie Courreges-Anglas wrote: > >> CVSROOT: /cvs > >> Module name: src > >> Changes by:j...@cvs.openbsd.org2017/08/15 11:57:57 > >> > >> Modified files: > >>bin/ksh: alloc.c > >> > >> Log message: > >> Remove expensive pointer check in afree() > >> > >> The check added in rev 1.8 walks the whole freelist to catch cases where > >> an unknown pointer is passed to afree(); but it can't catch cases > >> whether the struct link has been corrupted by an invalid memory write. > >> And it becomes very expensive when you have lots of items in an area > >> (for example with a huge HISTSIZE). > >> > >> Discussed with & ok millert@ tb@ > >> > > > > I'm not certain that it's a result of this change, but I updated to a > > snapshot past this change yesterday and today in one of my open shells > > I started getting "ksh: history file is corrupt" showing up when trying > > to run any commands (the command is then not executed). Opening a new > > terminal I get a hang (interruptable with ^C): > > > > load: 0.38 cmd: ksh 2244 [lockf] 0.02u 0.02s 0% 243k > > > > My histfile is currently 1197 lines: > > > > $ wc .ksh_history > > 1197 12866 274756 .ksh_history > > > > Some of them are fairly long lines (e.g. 4096, 8465, 8482, 8345 chars) > > and I probably haven't had quite such long lines before since the switch > > to the plain history file, so if it's connected with that, it could also > > have been a problem prior to the recent change and I just didn't run into > > it before. > > An overlong (>= 4096) line will result in the "ksh: history file is > corrupt" warning, cvs says that this behavior was introduced with the > current plaintext history file support. But it shouldn't result in hung > interactive shell processes. Could you please track this down to > a short histfile that you could share? I've been trying to re-trigger the hang this evening, I got it once with the untrimmed history file that I'd backed up, but haven't repeated it with any trimmed one, or the untrimmed one again now. Given the wchan "lockf" I think this must have been in the history_lock() loop? Perhaps there was a broken ksh process holding onto the file in a terminal that I've since closed.. These long command lines aren't *all* that unusual if you've pasted a cc/linker command into fc and are fiddling with parameters trying to figure out how to build a recalcitrant port :)
Re: libc: string: bcopy: get rid of unneeded goto
just noticed, memmove.c does that too: https://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/src/lib/libc/string/memmove.c?rev=1.2&content-type=text/plain
libc: string: bcopy: get rid of unneeded goto
Index: bcopy.c === RCS file: /cvs/src/lib/libc/string/bcopy.c,v retrieving revision 1.7 diff -u -p -r1.7 bcopy.c --- bcopy.c 31 Aug 2015 02:53:57 - 1.7 +++ bcopy.c 24 Aug 2017 19:16:30 - @@ -53,7 +53,7 @@ bcopy(const void *src0, void *dst0, size size_t t; if (length == 0 || dst == src) /* nothing to do */ - goto done; + return; /* * Macros: loop-t-times; and loop-t-times, t>0 @@ -107,7 +107,5 @@ bcopy(const void *src0, void *dst0, size t = length & wmask; TLOOP(*--dst = *--src); } -done: - return; } DEF_WEAK(bcopy);
ksh: history file is corrupt
On Wed, Aug 23 2017, Stuart Henderson wrote: > On 2017/08/15 11:57, Jeremie Courreges-Anglas wrote: >> CVSROOT: /cvs >> Module name: src >> Changes by: j...@cvs.openbsd.org2017/08/15 11:57:57 >> >> Modified files: >> bin/ksh: alloc.c >> >> Log message: >> Remove expensive pointer check in afree() >> >> The check added in rev 1.8 walks the whole freelist to catch cases where >> an unknown pointer is passed to afree(); but it can't catch cases >> whether the struct link has been corrupted by an invalid memory write. >> And it becomes very expensive when you have lots of items in an area >> (for example with a huge HISTSIZE). >> >> Discussed with & ok millert@ tb@ >> > > I'm not certain that it's a result of this change, but I updated to a > snapshot past this change yesterday and today in one of my open shells > I started getting "ksh: history file is corrupt" showing up when trying > to run any commands (the command is then not executed). Opening a new > terminal I get a hang (interruptable with ^C): > > load: 0.38 cmd: ksh 2244 [lockf] 0.02u 0.02s 0% 243k > > My histfile is currently 1197 lines: > > $ wc .ksh_history > 1197 12866 274756 .ksh_history > > Some of them are fairly long lines (e.g. 4096, 8465, 8482, 8345 chars) > and I probably haven't had quite such long lines before since the switch > to the plain history file, so if it's connected with that, it could also > have been a problem prior to the recent change and I just didn't run into > it before. An overlong (>= 4096) line will result in the "ksh: history file is corrupt" warning, cvs says that this behavior was introduced with the current plaintext history file support. But it shouldn't result in hung interactive shell processes. Could you please track this down to a short histfile that you could share? -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: relayd interrupted transfers
Hi!
This will fix my problem and regress still passes.
Index: usr.sbin/relayd/relay.c
===
RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.225
diff -u -p -r1.225 relay.c
--- usr.sbin/relayd/relay.c 9 Aug 2017 21:29:17 - 1.225
+++ usr.sbin/relayd/relay.c 24 Aug 2017 12:41:55 -
@@ -788,9 +788,12 @@ relay_write(struct bufferevent *bev, voi
{
struct ctl_relay_event *cre = arg;
struct rsession *con = cre->con;
+ struct evbuffer *dst = EVBUFFER_OUTPUT(bev);
getmonotime(&con->se_tv_last);
+ if (EVBUFFER_LENGTH(dst))
+ return;
if (con->se_done)
goto done;
if (relay_splice(cre->dst) == -1)
Rivo
On 23/08/2017, 14:42, "Alexander Bluhm" wrote:
On Tue, Aug 22, 2017 at 05:31:17PM +, Rivo Nurges wrote:
> relay_error() sets se_done even if write buffer is not drained and
> relay_write() will close the connection if se_done is set
I have seen a sporadic fail with chunked encoding in the daily
regression test run. So something might be wrong. Your idea of
not closing the relay if there is data in the buffer, makes sense.
Unfortunately the regression tests in /usr/src/regress/usr.sbin/relayd
fail with your patch. It hangs as the EOF is not properly propagated.
I think the change in relay_error() affects too much.
bluhm
Re: relayd interrupted transfers
On Thu, Aug 24, 2017 at 12:44:27PM +, Rivo Nurges wrote: > This will fix my problem and regress still passes. Yes this also works for me. I think the variable name "dst" is not good. Normally it refers to someting on the cre->dst side. As the buffer is on our side, I think EVBUFFER_LENGTH(EVBUFFER_OUTPUT(bev)) is better. I was also wondering whether it is correct to skip the bufferevent_enable(cre->dst->bev, EV_READ) further down. Would your current diff create pumped dataflow behavior as the output buffer must be completely empty before new data is read? The relay_splice() checks for the buffer length itself and disables reading if the buffers are not empty. This does not work due to the bufferevent_enable(cre->dst->bev, EV_READ) which was added later. So I think the logic should look like this. Does this work for you? bluhm Index: usr.sbin/relayd/relay.c === RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/relayd/relay.c,v retrieving revision 1.225 diff -u -p -r1.225 relay.c --- usr.sbin/relayd/relay.c 9 Aug 2017 21:29:17 - 1.225 +++ usr.sbin/relayd/relay.c 24 Aug 2017 14:02:38 - @@ -791,12 +791,12 @@ relay_write(struct bufferevent *bev, voi getmonotime(&con->se_tv_last); - if (con->se_done) + if (con->se_done && EVBUFFER_LENGTH(EVBUFFER_OUTPUT(bev)) == 0) goto done; - if (relay_splice(cre->dst) == -1) - goto fail; if (cre->dst->bev) bufferevent_enable(cre->dst->bev, EV_READ); + if (relay_splice(cre->dst) == -1) + goto fail; return; done:
Re: freezero(NULL, 0)
On Thu, Aug 24, 2017 at 11:34:52AM +1000, Damien Miller wrote: > Hi, > > memset(NULL, 0, 0) is (strictly speaking) undefined behaviour, but > there's no reason that freezero(3) needs to follow suit. > > This mentions that freezero(NULL, 0) is valid in the manpage, so that > anyone who copies this API should get it right too. Isn't this overkill? The man page already states that calling freezero() with a NULL pointer is a no-op: "If ptr is NULL, no action occurs." -Otto > > ok? > > Index: malloc.3 > === > RCS file: /cvs/src/lib/libc/stdlib/malloc.3,v > retrieving revision 1.115 > diff -u -p -r1.115 malloc.3 > --- malloc.3 15 May 2017 18:05:34 - 1.115 > +++ malloc.3 24 Aug 2017 01:31:52 - > @@ -210,6 +210,12 @@ argument must be equal or smaller than t > that returned > .Fa ptr . > .Fn freezero > +may be called with a > +.Dv NULL > +pointer argument if the > +.Fa size > +argument is zero. > +.Fn freezero > guarantees the memory range starting at > .Fa ptr > with length
