Yes, the add_iacs() approach, like the remove_iacs() code that was there, gets PO'd when the buffers are full, which could happen in a very busy streaming environment. That whole area could use a rewrite. (That business where it re-sets the rdidx/wridx vars to zero when the counts are 0, in order to avoid buffer wraparound conditions, is a huge red flag. Ick.) My code is bad if you feed it a full buffer to start with, no doubt.
The pesky last/lost byte business is part of the same thing, really. -- Jim -----Original Message----- From: Denys Vlasenko [mailto:[EMAIL PROTECTED] Sent: Wednesday, November 12, 2008 2:15 PM To: Cathey, Jim Cc: [email protected] Subject: Re: outstanding telnetd bug fix, and two new features On Wednesday 12 November 2008 20:35, Cathey, Jim wrote: > Both approaches seem to work just fine. The add_iacs() > patch I did is smaller, and I believe fits better into > the existing structure of telnetd, such as it is: > > $ size networking/telnetd.o > text data bss dec hex filename > 3752 16 16 3784 ec8 networking/telnetd.o > $ # Replace add_iacs() patch with iac_safe_write() patch & rebuild... > $ size networking/telnetd.o > text data bss dec hex filename > 3792 16 16 3824 ef0 networking/telnetd.o > $ > > (These are mips64 numbers.) > > I had to apply iac_safe_write() code as a patch to what we > had, replacing my patch. (For comparison purposes.) I couldn't > get the current svn to fully build for me, and I've got nearly > zero time to spend on this. I believe the comparative results > are still valid, however. > > Essentially both patches each apply one new subroutine, which > are listed here for easy comparison: > > static void > add_iacs(struct tsession *ts, int *pnum_fromtty) > { > unsigned char *ptr = TS_BUF2 + ts->rdidx2; > unsigned char *end = ptr + MIN(BUFSIZE - ts->rdidx2, *pnum_fromtty); Let's assume there are full "BUFSIZE - ts->rdidx2" bytes in the buffer, and the very first byte is 0xff. > while (ptr < end) { > if (*ptr == IAC) { > memmove(ptr+1, ptr, end - ptr);// Results in dupe IAC! Here we would store one byte past the buffer's end. > if (end < TS_BUF2 + BUFSIZE) > end++; ...else - just forget that pesky last byte, who cares? > (*pnum_fromtty)++; > ptr++; // Don't process the dupe next time either. > } > ptr++; > } > } > > *** Versus *** > > static size_t iac_safe_write(int fd, const char *buf, size_t count) > { > const char *oxff; > size_t wr, rc, total; > > total = 0; > while (1) { > if (count == 0) > return total; > if (*buf == (char)0xff) { > rc = safe_write(fd, "\xff\xff", 2); > if (rc != 2) > break; > buf++; > total++; > count--; > continue; > } > /* count != 0, *buf != 0xff */ > oxff = memchr(buf, 0xff, count); > wr = count; > if (oxff) > wr = oxff - buf; > rc = safe_write(fd, buf, wr); > if (rc != wr) > break; > buf += rc; > total += rc; > count -= rc; > } > /* here: rc - result of last short write */ > if ((ssize_t)rc < 0) { /* error? */ > if (total == 0) > return rc; > rc = 0; > } > return total + rc; > } > > I happen to think that mine is simpler; it's for certain smaller! I am all for smaller, as long as it does not introduce bugs. -- vda _______________________________________________ busybox mailing list [email protected] http://busybox.net/cgi-bin/mailman/listinfo/busybox
