Opened 4 years ago

Closed 4 years ago

#19563 closed task (implemented)

Test: add unit test for tor_htonll() and tor_ntohll()

Reported by: dgoulet Owned by:
Priority: Low Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: test easy
Cc: Actual Points:
Parent ID: Points: 0.2
Reviewer: dgoulet Sponsor:


Added in commit 9744a40f.

Child Tickets

Change History (7)

comment:1 Changed 4 years ago by icanhasaccount

Heya - I added an attempt at this on the following branch:

One thing I noticed - in test_util_format.c there is a macro to define htonll:

#if !defined(HAVE_HTONLL) && !defined(htonll)
#define htonll(x) (x)
static uint64_t
htonll(uint64_t a)

return htonl((uint32_t)(a>>32)) | (((uint64_t)htonl((uint32_t)a))<<32);


Should the test case using the above be changed to use tor_htonll? If so I can do this in a separate patch.

Thank you in advance for your time in reading this comment.

comment:2 Changed 4 years ago by dgoulet

Milestone: Tor: 0.2.???Tor: 0.3.0.x-final
Reviewer: dgoulet
Status: newneeds_revision

Yes actually! That test file should definitely use tor_htonll(). Feel free to fix that in a separate commit.

About, your patch, looks good! I would simply test the edge cases that is the UINT64 max and 0. Also, I would define the expected "little endian" and "big endian" value as const variables instead of just "n" and then copy 4 times the expected value of converted n. It will just make things clearer and avoid us to typo anything.

Moving it to 030 milestone. Also, next time, set the patch to needs_review, will be easier for us to spot that it actually needs review :). Big thanks for this!

comment:3 Changed 4 years ago by icanhasaccount

Status: needs_revisionneeds_review

Thank you - I've updated the branch re the feedback. I'll open another bug (with a patch) for test_util_format.c.

Thanks again!

comment:4 Changed 4 years ago by dgoulet

Oh it's fine that you do the cleanup with this branch! :) I'll look at it asap.

comment:5 Changed 4 years ago by icanhasaccount

Heya - thanks, I've put the fix for test_util_format into the same branch:)

comment:6 Changed 4 years ago by dgoulet

Status: needs_reviewmerge_ready

lgtm and works for me! (well only tested on Little Endian :S). Thanks!

comment:7 Changed 4 years ago by nickm

Resolution: implemented
Status: merge_readyclosed


Note: See TracTickets for help on using tickets.