Opened 3 months ago

Closed 12 days ago

#23966 closed enhancement (implemented)

Refactor node_has_curve25519_onion_key() to use node_get_curve25519_onion_key()

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, refactor, review-group-28
Cc: Actual Points:
Parent ID: Points: 0.2
Reviewer: teor Sponsor:

Description

In #23577, we will create node_get_curve25519_onion_key().
We should use it to implement node_has_curve25519_onion_key(), so they give consistent results.

Child Tickets

Change History (24)

comment:1 Changed 2 months ago by teor

Keywords: prop224 tor-hs single-onion ipv6 removed
Parent ID: #23577

Remove the parent and spurious keywords.

comment:2 Changed 7 weeks ago by aruna1234

could you give more information?

comment:3 in reply to:  2 ; Changed 7 weeks ago by teor

Replying to aruna1234:

could you give more information?

These functions have code that's almost identical.
We can implement node_has… by calling node_get… and checking the result.
The result is valid if it passes the existing check in node_has….

This might be a good ticket for you to focus on, because it is a small first-time change for you to learn C and learn our processes.

comment:4 Changed 7 weeks ago by aruna1234

But I still didn't understand what does the bug demand?

comment:5 Changed 7 weeks ago by teor

Maybe this is not the best bug for you then,

comment:6 in reply to:  3 Changed 7 weeks ago by aruna1234

Replying to teor:

Replying to aruna1234:

could you give more information?

These functions have code that's almost identical.
We can implement node_has… by calling node_get… and checking the result.
The result is valid if it passes the existing check in node_has….

This might be a good ticket for you to focus on, because it is a small first-time change for you to learn C and learn our processes.


Does that mean that we can replace all calls to function node_has... by calling node_get... without any ambiguity i.e if it passes all tests.

comment:7 in reply to:  description Changed 7 weeks ago by aruna1234

Replying to teor:

In #23577, we will create node_get_curve25519_onion_key().
We should use it to implement node_has_curve25519_onion_key(), so they give consistent results.

Or as this says, we should create node_get_curve25519_onion_key() and then call node_has_curve25519_onion_key() from this function so they give consistent results?

comment:8 Changed 7 weeks ago by teor

Both functions already exist.
This ticket is about rewriting node_has… so that it uses node_get… and a comparison,

comment:9 Changed 6 weeks ago by aruna1234

So, have thought of something:

All the calls to node_has.. will be replaced by node_get.. .Instead we will call node_get..() and check if the value that is returned is NULL or not. If it is not NULL then we'll call routerinfo_has_curve25519_onion_key() in routerlist.c, otherwise its zero.

comment:10 Changed 6 weeks ago by teor

Not quite, but you're very close.

When I read node_get_curve25519_onion_key() and node_has_curve25519_onion_key(), I see that they both have a similar pattern:

  • check if something exists, and return something
  • check if something else exists, and return something
  • check if something else exists, and return something

But we don't like duplicate code like this: it makes it easy to make mistakes.

So, what we want to do instead is:

  • leave node_get_curve25519_onion_key() as it is
  • make node_has_curve25519_onion_key() call node_get_curve25519_onion_key(), and check if the result is not NULL.

That way, we remove the duplicate code. And we only need to change one function, node_has_curve25519_onion_key().

comment:11 Changed 6 weeks ago by teor

Status: newneeds_revision

That's not quite what we wanted - we want node_has_curve25519_onion_key() to become a very simple function.

I'll walk you through the process.

We start with two functions that look like this:

/** Return true iff <b>node</b> has a curve25519 onion key. */
int
node_has_curve25519_onion_key(const node_t *node)
{
  if (!node)
    return 0;

  if (node->ri)
    return routerinfo_has_curve25519_onion_key(node->ri);
  else if (node->md)
    return microdesc_has_curve25519_onion_key(node->md);
  else
    return 0;
}

/** Return the curve25519 key of <b>node</b>, or NULL if none. */
const curve25519_public_key_t *
node_get_curve25519_onion_key(const node_t *node)
{
  if (node->ri)
    return node->ri->onion_curve25519_pkey;
  else if (node->md)
    return node->md->onion_curve25519_pkey;
  else
    return NULL;
}

And we want to end up with node_has_curve25519_onion_key() calling node_get_curve25519_onion_key(), and checking if the result is NULL:

/** Return true iff <b>node</b> has a curve25519 onion key. */
int
node_has_curve25519_onion_key(const node_t *node)
{
  return node_get_curve25519_onion_key(node) != NULL;
}

/** Return the curve25519 key of <b>node</b>, or NULL if none. */
const curve25519_public_key_t *
node_get_curve25519_onion_key(const node_t *node)
{
  if (node->ri)
    return node->ri->onion_curve25519_pkey;
  else if (node->md)
    return node->md->onion_curve25519_pkey;
  else
    return NULL;
}

But there were some good checks in node_has_curve25519_onion_key(), so we want to move them to node_get_curve25519_onion_key():

/** Return true iff <b>node</b> has a curve25519 onion key. */
int
node_has_curve25519_onion_key(const node_t *node)
{
  return node_get_curve25519_onion_key(node) != NULL;
}

/** Return the curve25519 key of <b>node</b>, or NULL if none. */
const curve25519_public_key_t *
node_get_curve25519_onion_key(const node_t *node)
{
  if (!node)
    return NULL;

  if (routerinfo_has_curve25519_onion_key(node->ri))
    return node->ri->onion_curve25519_pkey;
  else if (microdesc_has_curve25519_onion_key(node->md))
    return node->md->onion_curve25519_pkey;
  else
    return NULL;
}

Can you test that the code works (we use the "make check" command) and then turn it into a patch?

Edit: oops, I mixed up the last two steps.

Last edited 6 weeks ago by teor (previous) (diff)

comment:12 Changed 6 weeks ago by teor

Did you compile this and run the unit tests using "make check"?

Please fix the indenting here - we indent code blocks, so this will confuse people:

   if(!node)
   return NULL;

comment:13 Changed 6 weeks ago by aruna1234

I did run the make check command. It is failing at line 1638 that is the one with the first if condition that returns NULL. I tried running without the if condition, but it's still failing at the same line.

Changed 6 weeks ago by aruna1234

comment:14 Changed 4 weeks ago by teor

Hi, please provide one patch file that includes all your changes.

comment:15 Changed 4 weeks ago by teor

Hi, please provide one patch file that applies to the Tor master branch. This one doesn't.

comment:16 Changed 3 weeks ago by aruna1234

Hey! I added the changes in a new branch with only one commit->
https://github.com/ArunaMaurya221B/tor/commit/01efbe3bd3d7e7d3bd372c04531ea2825ac9f706

comment:17 Changed 3 weeks ago by teor

Reviewer: teor
Status: needs_revisionmerge_ready
Type: defectenhancement

Thanks for creating a branch on master. That's much easier for us!
This patch is ready to go, just one more final review before we merge.

I added a changes file to aruna1234's patch. Please see my branch Bug-23966 on https://github.com/teor2345/tor.git

comment:18 Changed 3 weeks ago by aruna1234

Thanks for the patience. Took quite some time:)

comment:19 Changed 3 weeks ago by nickm

Keywords: review-group-28 added

comment:20 Changed 12 days ago by nickm

Resolution: implemented
Status: merge_readyclosed

merging!

Note: See TracTickets for help on using tickets.