Opened 11 months ago

Closed 7 months 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:

Description

Added in commit 9744a40f.

Child Tickets

Change History (7)

comment:1 Changed 7 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 7 months ago by dgoulet

  • Milestone changed from Tor: 0.2.??? to Tor: 0.3.0.x-final
  • Reviewer set to dgoulet
  • Status changed from new to needs_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 7 months ago by icanhasaccount

  • Status changed from needs_revision to needs_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 7 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 7 months ago by icanhasaccount

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

comment:6 Changed 7 months ago by dgoulet

  • Status changed from needs_review to merge_ready

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

comment:7 Changed 7 months ago by nickm

  • Resolution set to implemented
  • Status changed from merge_ready to closed

merged!

Note: See TracTickets for help on using tickets.