[Stegotorus] Strange comparison in chop_circuit_t::send_targeted(conn, blocksize)

Hey Zack, Please take a look at this comparison in chop_circuit_t::send_targeted(conn, blocksize): if (avail > blocksize - lo) avail = blocksize - lo; else if (avail > SECTION_LEN) avail = SECTION_LEN; else if (upstream_eof && !sent_fin) // this block will carry the last byte of real data to be sent in // this direction; mark it as such op = op_FIN; Assume SECTION_LEN = 65536 suppose avail = 67000 blocksize - lo = 66500 => avail = 66500 avail = 67000 blocksize - lo = 67500 => avail = 65536 it's like that if avail is very big then the it is OK if it's bigger than SECTION_LEN but if it's moderately big then it shouldn't be bigger than SECTION_LEN, which seems very arbitrary. On the other hand if blocksize - lo < SECTION_LEN again it doesn't make sense, because then it never reaches to (avail > SECTION_LEN). So it's like if SECTION_LEN < blocksize - lo the behavior is random and if it's SECTION_LEN > blocksize - lo is useless. Moreover, when blocksize - lo > avail > SECTION_LEN I'm getting assertion error in the other overload of send_targeted. Everything makes sense and the assertion failure goes away if I replace the code with this: if ((avail > blocksize - lo) || (avail > SECTION_LEN)) { avail = min(blocksize - lo, SECTION_LEN); } else if (upstream_eof && !sent_fin) // this block will carry the last byte of real data to be sent in // this direction; mark it as such op = op_FIN; Please tell me what you think and if the second chunk is the correct replacement? Bests, vmon

On 2012-08-10 8:11 AM, vmon wrote:
Hey Zack,
Please take a look at this comparison in chop_circuit_t::send_targeted(conn, blocksize):
if (avail > blocksize - lo) avail = blocksize - lo; else if (avail > SECTION_LEN) avail = SECTION_LEN; else if (upstream_eof && !sent_fin) // this block will carry the last byte of real data to be sent in // this direction; mark it as such op = op_FIN;
Assume SECTION_LEN = 65536 suppose avail = 67000 blocksize - lo = 66500 => avail = 66500 avail = 67000 blocksize - lo = 67500 => avail = 65536
it's like that if avail is very big then the it is OK if it's bigger than SECTION_LEN but if it's moderately big then it shouldn't be bigger than SECTION_LEN, which seems very arbitrary.
The *intent* here is to clamp 'avail' to min(blocksize - lo, SECTION_LEN) because: we can't fit more than SECTION_LEN bytes of data in a block under any circumstances, and we were asked to provide a block which is no larger than 'blocksize' but we have 'lo' bytes of overhead to cope with. So your rewrite is correct. I think maybe the control flow here could do with some going-over for clarity but that needn't happen right now.
if ((avail > blocksize - lo) || (avail > SECTION_LEN))
The nested parentheses are unnecessary in this case.
{ avail = min(blocksize - lo, SECTION_LEN); }
No curly braces around one-statement blocks. zw
participants (2)
-
vmon
-
Zack Weinberg