#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:

Description

Added in commit 9744a40f.

Child Tickets

Change History (7)

comment:1 Changed 13 months ago by icanhasaccount

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

https://github.com/mintytoast/tor-stuff/tree/bug_19563

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

#if !defined(HAVE_HTONLL) && !defined(htonll)
#ifdef WORDS_BIGENDIAN
#define htonll(x) (x)
#else
static uint64_t
htonll(uint64_t a)
{

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

}
#endif
#endif

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 13 months 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 13 months 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 13 months ago by dgoulet

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

comment:5 Changed 13 months ago by icanhasaccount

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

comment:6 Changed 13 months ago by dgoulet

Status: needs_reviewmerge_ready

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

comment:7 Changed 13 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.