Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various minor cleanups in net_crypto. #932

Merged
merged 1 commit into from
Jun 28, 2018
Merged

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jun 23, 2018

  • Consistently use num_packets_array to get the packet count in the
    packet buffer.
  • Use const in more places.
  • Typo fix: begginning.
  • Rewrite length < 1 for unsigned int to length == 0.
  • Limit scope of some loop variables by using for-init-decl.
  • Use early return in error paths to reduce indentation and for clarity.
  • Use net_unpack_* instead of manual ntohs.
  • Fix an uninitialised stack variable copy.
  • Fix a potential null pointer dereference.
  • Consistently use get_crypto_connection. It was inlined in some
    places. I de-inlined it now.

This change is Reviewable

@iphydf iphydf added this to the v0.2.3 milestone Jun 23, 2018
@iphydf iphydf force-pushed the cleanups branch 5 times, most recently from e80481a to 233f321 Compare June 23, 2018 17:34
@iphydf iphydf changed the title Various minor cleanups in net_crypto. WIP: Various minor cleanups in net_crypto. Jun 23, 2018
@iphydf iphydf force-pushed the cleanups branch 4 times, most recently from 94c3666 to b832686 Compare June 24, 2018 20:15
@iphydf iphydf modified the milestones: v0.2.3, v0.2.x Jun 24, 2018
@robinlinden
Copy link
Member

:lgtm_strong:


Reviewed 8 of 8 files at r2.
Review status: 0 of 1 LGTMs obtained (waiting on @sudden6)


Comments from Reviewable

@iphydf iphydf force-pushed the cleanups branch 4 times, most recently from 48f20df to e7fd866 Compare June 25, 2018 21:19
@iphydf iphydf changed the title WIP: Various minor cleanups in net_crypto. Various minor cleanups in net_crypto. Jun 25, 2018
@sudden6
Copy link

sudden6 commented Jun 26, 2018

Reviewed 1 of 18 files at r1, 1 of 8 files at r2, 1 of 1 files at r3.
Review status: 0 of 1 LGTMs obtained (waiting on @sudden6)


toxcore/net_crypto.c, line 1106 at r3 (raw file):

    /* If last packet send failed, try to send packet again.
       If sending it fails we won't be able to send the new packet. */
    if (conn->maximum_speed_reached) {

Invert condition and do early return? or do you want to keep the success case at the end of the function?


toxcore/net_crypto.c, line 1111 at r3 (raw file):

        const int ret = get_data_pointer(c->log, &conn->send_array, &dt, packet_num);

        uint8_t send_failed = 0;

This variable can be removed by moving the return into the if


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Jun 28, 2018

Review status: 0 of 1 LGTMs obtained (waiting on @sudden6)


toxcore/net_crypto.c, line 1106 at r3 (raw file):

Previously, sudden6 wrote…

Invert condition and do early return? or do you want to keep the success case at the end of the function?

Yes, I'd rather keep success at the end.


toxcore/net_crypto.c, line 1111 at r3 (raw file):

Previously, sudden6 wrote…

This variable can be removed by moving the return into the if

Done.


Comments from Reviewable

* Consistently use `num_packets_array` to get the packet count in the
  packet buffer.
* Use `const` in more places.
* Typo fix: begginning.
* Rewrite `length < 1` for unsigned int to `length == 0`.
* Limit scope of some loop variables by using for-init-decl.
* Use early return in error paths to reduce indentation and for clarity.
* Use `net_unpack_*` instead of manual `ntohs`.
* Fix an uninitialised stack variable copy.
* Fix a potential null pointer dereference.
* Consistently use `get_crypto_connection`. It was inlined in some
  places. I de-inlined it now.
* Add Loggers to some functions in preparation for adding log statements.
@sudden6
Copy link

sudden6 commented Jun 28, 2018

:lgtm_strong:


Reviewed 1 of 1 files at r4.
Review status: :shipit: complete! 1 of 1 LGTMs obtained


toxcore/net_crypto.c, line 1106 at r3 (raw file):

Previously, iphydf wrote…

Yes, I'd rather keep success at the end.

Ok, can live with that


Comments from Reviewable

@iphydf iphydf merged commit 7c4b971 into TokTok:master Jun 28, 2018
@iphydf iphydf deleted the cleanups branch June 28, 2018 21:24
@iphydf iphydf modified the milestones: v0.2.x, v0.2.4 Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants