undefined (#11792)

Trac:
Keywords: tor-relay dir zlib oom deleted, oom, dir, zlib, tor-relay, 026-triaged-1 added

My branch bug11792_1 should do this stuff. Could use some preliminary review.

Trac:
Status: new to needs_review

Prelim review.

conn_get_buffer_age:

You can probably drop the age2 from the conn->outbuf conditional block, now that this logic is in its own function

  if (conn->outbuf) {
-    age2 = buf_get_oldest_chunk_timestamp(conn->outbuf, now);
+    age = buf_get_oldest_chunk_timestamp(conn->outbuf, now);
-    if (age2 > age)
-      age = age2;
  }

tor_zlib_state_size_precalc:

Without knowing the preexisting code and structs it's is difficult to understand. Reading legacy/trac#11791 (moved) helps. Some of the numbers are obvious, but adding a descriptive comment to the return statement explaining the components would also help a lot.

Other than that, this looks sane.

Begin code review:

ec59167c:
- This looks fine to me, I think. I presume you've checked that we're not at risk of multiple-counting by also freeing the linked conn later and adjusting for its memory usage again?
c5260f5b78c434a223dc82fd3153cd9c8a4f63bf:
- I think this is okay.
55556568283d44219e8439d0dea7d01bb182623d:
- I think this looks reasonable, but conn_get_buffer_age() defined the age of a circuit with nothing buffered in either direction as zero isn't totally intuitive to me. It leads to reasonable behavior, but maybe a sentence explicitly stating that it does that in its comment is in order?
58020ed079af3794a0869f4a6bfa34ccaecc4b61:
- This looks fine to me.

End code review.

Replying to andrea:

Begin code review:

ec59167c:

This looks fine to me, I think. I presume you've checked that we're not at risk of multiple-counting by also freeing the linked conn later and adjusting for its memory usage again?

The linked conn can't be another stream ... and we don't consider linked directory connections later on, only the linked ones. So I think it can't be double-counted.

c5260f5b78c434a223dc82fd3153cd9c8a4f63bf:

I think this is okay.

55556568283d44219e8439d0dea7d01bb182623d:

I think this looks reasonable, but conn_get_buffer_age() defined the age of a circuit with nothing buffered in either direction as zero isn't totally intuitive to me. It leads to reasonable behavior, but maybe a sentence explicitly stating that it does that in its comment is in order?

Will do.

58020ed079af3794a0869f4a6bfa34ccaecc4b61:

This looks fine to me.

End code review.

Thanks!

Pushed two extra commits to bug11792_1. Squashed the branch as bug11792_1_squashed, then merged it into master as 59e11483 (conflicts resolved, I believe).

Trac:
Status: needs_review to closed
Resolution: N/A to implemented
Type: defect to enhancement

Activity